Skip to content

add Sign1Message benchmarks#54

Merged
shizhMSFT merged 3 commits intoveraison:mainfrom
qmuntal:bench-sign1
Apr 22, 2022
Merged

add Sign1Message benchmarks#54
shizhMSFT merged 3 commits intoveraison:mainfrom
qmuntal:bench-sign1

Conversation

@qmuntal
Copy link
Copy Markdown
Member

@qmuntal qmuntal commented Apr 21, 2022

This PR adds benchmarks for Sign1Message methods. These benchmarks will be useful later one to do memory and cpu profiling.

These are the results on my Windows laptop (use only for reference):

goos: windows
goarch: amd64
pkg: github.com/veraison/go-cose
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkSign1Message_MarshalCBOR-12              666874              1791 ns/op             692 B/op         17 allocs/op
BenchmarkSign1Message_UnmarshalCBOR-12            368671              3303 ns/op            1592 B/op         28 allocs/op
BenchmarkSign1Message_Sign-12                     631834              1739 ns/op             668 B/op         16 allocs/op
BenchmarkSign1Message_Verify-12                   585108              1910 ns/op             668 B/op         16 allocs/op

For #52

@qmuntal
add Sign1Message benchmarks
9b458e3 
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
thomas-fossati
thomas-fossati reviewed Apr 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (and useful) to me, thanks!

I have inlined a small editorial comment.

bench_test.go Outdated
Comment on lines +25 to +32
type zeroReader struct{}

func (zeroReader) Read(b []byte) (int, error) {
for i := range b {
b[i] = 0
}
return len(b), nil
}
Copy link
Copy Markdown
Contributor

@thomas-fossati thomas-fossati Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this a dup of zeroSource in conformance_test.go?

Copy link
Copy Markdown
Member Author

@qmuntal qmuntal Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@qmuntal
dedup zeroSource
b293959 
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@yogeshbdeshpande
Copy link
Copy Markdown
Contributor

yogeshbdeshpande commented Apr 22, 2022 •
edited
Loading

Few General comments:
Are you planning to have bench test for sign in other file, then please rename this file to bench_test_sign1.go
Perhaps for future, makes more sense to move the bench test files to a new folder as benchmark and add all the tests there?

@qmuntal
Copy link
Copy Markdown
Member Author

qmuntal commented Apr 22, 2022

Few General comments: Are you planning to have bench test for sign in other file, then please rename this file to bench_test_sign1.go Perhaps for future, makes more sense to move the bench test files to a new folder as benchmark and add all the tests there?

I plan to stick with bench_test.go for new benchmarks, as I don't expect that those grow too much.
Also, it is common and idiomatic to keep benchmark tests in the same directory as the code being benchmarked.

shizhMSFT
shizhMSFT approved these changes Apr 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qmuntal
combine noSigner with noVerifier
9b59e6b 
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
shizhMSFT
shizhMSFT approved these changes Apr 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shizhMSFT shizhMSFT merged commit 08be6c0 into veraison:main Apr 22, 2022
@qmuntal qmuntal deleted the bench-sign1 branch April 22, 2022 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomas-fossati thomas-fossati thomas-fossati approved these changes

@yogeshbdeshpande yogeshbdeshpande yogeshbdeshpande approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@qmuntal @yogeshbdeshpande @thomas-fossati @shizhMSFT