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

Add CMake config to build benchmarks and fix some minor issues in existing tests #2582

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

andrewhop
Copy link
Contributor

@andrewhop andrewhop commented Feb 12, 2021

Resolved issues:

This is a step towards addressing #1324

Description of changes:

s2n's benchmarks were only build-able using the included Makefiles. I like using Clion as my IDE which only supports CMake. This change adds the required CMake config to build the benchmarks though that. Once I have this I will add more targetted benchmarks.

Call-outs:

Address any potentially confusing code. Is there code added that needs to be cleaned up later? Is there code that is missing because it’s still in development?

It appears nothing is building the benchmarks in the CI and the existing PEM and base64 tests have drifted and are not compatible with the latest s2n changes. I updated the tests when possible but had to change the method signature of s2n_ensure_memcpy_trace to be able to compile with a C++ compiler.

Testing:

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=$INSTALL_DIR -DBENCHMARK=1 ../
...
ninja
...
desktop git:(benchmark-fix)✗ > ./bin/s2n_pem_parse_benchmark
2021-02-11T16:16:53-08:00
Running ./bin/s2n_pem_parse_benchmark
Run on (12 X 4800 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x6)
  L1 Instruction 32 KiB (x6)
  L2 Unified 256 KiB (x6)
  L3 Unified 12288 KiB (x1)
Load Average: 1.65, 1.83, 2.23
***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
-----------------------------------------------------------------------------------
Benchmark                                         Time             CPU   Iterations
-----------------------------------------------------------------------------------
ValidPemTestFixture/Load_Valid_Pem/0           3.99 ns         3.99 ns    175994087
ValidPemTestFixture/Load_Valid_Pem/1           3.97 ns         3.97 ns    176162953
ValidPemTestFixture/Load_Valid_Pem/2           4.07 ns         4.07 ns    176377315
ValidPemTestFixture/Load_Valid_Pem/3           4.06 ns         4.06 ns    172058162
ValidPemTestFixture/Load_Valid_Pem/4           4.01 ns         4.01 ns    172203191
ValidPemTestFixture/Load_Valid_Pem/5           3.97 ns         3.97 ns    176619955
ValidPemTestFixture/Load_Valid_Pem/6           3.98 ns         3.98 ns    176642775
ValidPemTestFixture/Load_Valid_Pem/7           3.97 ns         3.97 ns    176531530
ValidPemTestFixture/Load_Valid_Pem/8           3.98 ns         3.98 ns    174472028
ValidPemTestFixture/Load_Valid_Pem/9           3.98 ns         3.98 ns    175676921
ValidPemTestFixture/Load_Valid_Pem/10          3.99 ns         3.99 ns    172734165
ValidPemTestFixture/Load_Valid_Pem/11          3.97 ns         3.97 ns    175941642
ValidPemTestFixture/Load_Valid_Pem/12          3.97 ns         3.97 ns    176477595
ValidPemTestFixture/Load_Valid_Pem/13          3.97 ns         3.97 ns    176315857
ValidPemTestFixture/Load_Valid_Pem/14          3.97 ns         3.97 ns    176418837
ValidPemTestFixture/Load_Valid_Pem/15          3.97 ns         3.97 ns    176378295
InvalidPemTestFixture/Load_Invalid_Pem/0       4.18 ns         4.18 ns    167130693
InvalidPemTestFixture/Load_Invalid_Pem/1       4.18 ns         4.18 ns    167399371
InvalidPemTestFixture/Load_Invalid_Pem/2       4.26 ns         4.26 ns    167742782
InvalidPemTestFixture/Load_Invalid_Pem/3       4.18 ns         4.18 ns    167772082
InvalidPemTestFixture/Load_Invalid_Pem/4       4.17 ns         4.17 ns    167773674
InvalidPemTestFixture/Load_Invalid_Pem/5       4.18 ns         4.18 ns    167635773

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

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #2582 (70e4716) into main (dca4aa1) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2582      +/-   ##
==========================================
+ Coverage   82.08%   82.16%   +0.08%     
==========================================
  Files         272      272              
  Lines       19188    19168      -20     
==========================================
- Hits        15750    15749       -1     
+ Misses       3438     3419      -19     

Impacted file tree graph

utils/s2n_ensure.h Outdated Show resolved Hide resolved
Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

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

Thanks for making the change! Looks great.

@andrewhop andrewhop merged commit c84aae8 into aws:main Feb 16, 2021
@dougch dougch mentioned this pull request Apr 13, 2021
13 tasks
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.

3 participants