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

Changing sha2 benchmarks to refer to the faster Hacl_SHA2_Streaming versions #362

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

karthikbhargavan
Copy link
Collaborator

No description provided.

@karthikbhargavan karthikbhargavan requested a review from a team as a code owner February 19, 2023 13:38
@cla-bot cla-bot bot added the cla-signed label Feb 19, 2023
@karthikbhargavan karthikbhargavan changed the title Changing sha2 benchmars to refer to the faster Hacl_SHA2_Streaming versions Changing sha2 benchmarks to refer to the faster Hacl_SHA2_Streaming versions Feb 19, 2023
@coveralls
Copy link

coveralls commented Feb 19, 2023

Pull Request Test Coverage Report for Build 4252472757

  • 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.441%

Totals Coverage Status
Change from base Build 4252436588: 0.0%
Covered Lines: 30398
Relevant Lines: 56881

💛 - Coveralls


template<class... Args>
void
HACL_Sha2_new_oneshot(benchmark::State& state, Args&&... args)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this function isn't used?
Please remove or use.

@@ -159,7 +174,7 @@ BENCHMARK_CAPTURE(HACL_Sha2_oneshot,
sha2_224,
HACL_HASH_SHA2_224_DIGEST_LENGTH,
expected_digest_sha2_224,
Hacl_Hash_SHA2_hash_224)
Hacl_Streaming_SHA2_sha224)
Copy link
Member

Choose a reason for hiding this comment

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

This makes the benchmark pretend like the oneshot function exposed by the library have this performance. Which they don't
I agree that this should be used but a few things need to happen as well

  • remove the slow API
  • update the documentation to point consumers to the right API

I'm fine with doing this in follow-ups. But it either needs to be filed as follow-ups to tackle after this (maybe @duesee or @pnmadelaine can take care of this), or done here.

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.

3 participants