Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop asserts from benchmarks #343

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

franziskuskiefer
Copy link
Member

Also ensure nothing is optimised out (fixes weird p256 performance numbers).

Also ensure nothing is optimised out
@franziskuskiefer franziskuskiefer requested a review from a team as a code owner January 22, 2023 20:24
@cla-bot cla-bot bot added the cla-signed label Jan 22, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3981397350

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 53.588%

Totals Coverage Status
Change from base Build 3959834694: 0.0%
Covered Lines: 30653
Relevant Lines: 57201

💛 - Coveralls


bytes r(signature.begin(), signature.begin() + 32);
Copy link
Contributor

@duesee duesee Jan 23, 2023

Choose a reason for hiding this comment

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

Since asserts are stripped in --release it's generally good we don't use them in benchmarks. So, from all the changes, I assume only this one -- moving r, s before the loop -- could be important? It doesn't seem to change much on my machine, though:

--------------------------------------------------------------------
Benchmark                          Time             CPU   Iterations
--------------------------------------------------------------------

--- franziskus/fixup-some-benchmarks, but with r,s before loop
P256_SHA256_ECDSA_Sign        328377 ns       328279 ns         2130
P256_SHA256_ECDSA_Verify      734751 ns       734636 ns          947
P256_ECDH                     320494 ns       320433 ns         2184
Openssl_P256_ECDSA_Sign        20389 ns        20383 ns        34030
Openssl_P256_ECDSA_Verify      54693 ns        54673 ns        12692
Openssl_P256_ECDH              38962 ns        38950 ns        17927

--- franziskus/fixup-some-benchmarks
P256_SHA256_ECDSA_Sign        325494 ns       325416 ns         2150
P256_SHA256_ECDSA_Verify      730341 ns       730129 ns          952
P256_ECDH                     318984 ns       318906 ns         2192
Openssl_P256_ECDSA_Sign        20162 ns        20154 ns        34424
Openssl_P256_ECDSA_Verify      54081 ns        54060 ns        12041
Openssl_P256_ECDH              38737 ns        38727 ns        18051

Do you see a difference on your machine?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any changes in performance from this change either. It's dominated by the math.

Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

Thanks! I left a question below, but in any case this is good to go!

@franziskuskiefer franziskuskiefer merged commit 1331f11 into main Jan 23, 2023
@franziskuskiefer franziskuskiefer deleted the franziskus/fixup-some-benchmarks branch January 23, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants