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

Update speed.cc to use the same jitter function as rand.c #2100

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

andrewhop
Copy link
Contributor

Description of changes:

AWS-LC uses jent_read_entropy which should be reflected in the benchmark. Also turn on Jitter as the entropy source so the CI actually builds/runs this code path and ensures it continues to work.

Call-outs:

This doesn't affect the performance results, before:

./tool/bssl speed -filter Jitter
Did 253 Jitter (16 bytes) operations in 1081577us (233.9 ops/sec): 0.0 MB/s
Did 30 Jitter (256 bytes) operations in 1021729us (29.4 ops/sec): 0.0 MB/s
Did 6 Jitter (1350 bytes) operations in 1097450us (5.5 ops/sec): 0.0 MB/s
Did 1 Jitter (8192 bytes) operations in 1087014us (0.9 ops/sec): 0.0 MB/s
Did 1 Jitter (16384 bytes) operations in 2187891us (0.5 ops/sec): 0.0 MB/s

After:

./tool/bssl speed -filter Jitter
Did 230 Jitter (16 bytes) operations in 1013691us (226.9 ops/sec): 0.0 MB/s
Did 30 Jitter (256 bytes) operations in 1034730us (29.0 ops/sec): 0.0 MB/s
Did 6 Jitter (1350 bytes) operations in 1114960us (5.4 ops/sec): 0.0 MB/s
Did 1 Jitter (8192 bytes) operations in 1095828us (0.9 ops/sec): 0.0 MB/s
Did 1 Jitter (16384 bytes) operations in 2198877us (0.5 ops/sec): 0.0 MB/s

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@andrewhop andrewhop requested a review from a team as a code owner January 8, 2025 00:01
@andrewhop andrewhop requested a review from smittals2 January 8, 2025 00:01
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.76%. Comparing base (c826c21) to head (1092ead).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2100   +/-   ##
=======================================
  Coverage   78.76%   78.76%           
=======================================
  Files         598      598           
  Lines      103656   103656           
  Branches    14720    14720           
=======================================
+ Hits        81641    81645    +4     
+ Misses      21363    21359    -4     
  Partials      652      652           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -26,7 +26,8 @@ function build_aws_lc_fips {
-DENABLE_DILITHIUM=ON \
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DBUILD_SHARED_LIBS=1 \
-DBUILD_TESTING=OFF
-DBUILD_TESTING=OFF \
-DENABLE_FIPS_ENTROPY_CPU_JITTER=1
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: Will this negatively affect any of our benchmarks? Should we separately benchmark builds both w/ and w/o CPU jitter entropy?

Copy link
Contributor Author

@andrewhop andrewhop Jan 10, 2025

Choose a reason for hiding this comment

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

This is just a test build to make sure it can build, the actually canary doesn't run with this flag. But yeah, any other benchmark that also needs randomness (such as the key gen tests) it could have an impact.

@andrewhop andrewhop merged commit 0e78c22 into aws:main Jan 10, 2025
124 of 125 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants