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

Set up regression benchmark for scalar performance #4649

Merged
merged 17 commits into from
Jul 26, 2024

Conversation

kaukabrizvi
Copy link
Contributor

@kaukabrizvi kaukabrizvi commented Jul 15, 2024

Resolved issues:

N/A

Description of changes:

To integrate performance benchmarking into the CI pipeline for s2n-tls, this PR introduces the structure for regression harnesses that benchmark essential TLS connection components. This PR specifically includes the following:

  • Config Creation (test_set_config): Builds a new s2n-tls config with a security policy, host callback and certs
  • RSA Handshake (test_rsa_handshake): Measures the performance of performing a handshake with a pre-set config.

Crabgrind is used to call the Valgrind client request interface, enabling selective benchmarking only for the desired code regions within each harness.

Call-outs:

  • Reverting some changes in PR #4623: Certain structs defined in bindings/rust/s2n-tls/testing are reverted to be public by enabling the unstable-testing feature. This allows the regression module to import these with the feature enabled. This struct facilitates setting up connections like the RSA handshake harness.
  • Recursive cargo test: To run the tests via valgrind the testing library calls on each test recursively by setting an environment variable flag. This enables running tests without performance capturing enabled via cargo test and with performance capturing enabled with VALGRIND = true cargo test
  • Separate functions: To measure fine-grained parts of typical TLS connections, these harnesses are separated into distinct functions despite some overlap. Cachegrind's limitations in dumping stats at specified points necessitate creating a new config in test_rsa_handshake rather than combining everything into a single harness.

Testing:

This addition does not change any existing functionality.

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

@jmayclin jmayclin requested review from jmayclin and goatgoose July 15, 2024 22:35
bindings/rust/s2n-tls/Cargo.toml Outdated Show resolved Hide resolved
tests/regression/README.md Outdated Show resolved Hide resolved
tests/regression/run_harnesses.sh Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/README.md Outdated Show resolved Hide resolved
tests/regression/README.md Outdated Show resolved Hide resolved
tests/regression/src/bin/config_configure.rs Outdated Show resolved Hide resolved
tests/regression/src/bin/config_create.rs Outdated Show resolved Hide resolved
@kaukabrizvi kaukabrizvi marked this pull request as ready for review July 16, 2024 20:43
@kaukabrizvi kaukabrizvi requested a review from jmayclin July 19, 2024 16:17
bindings/rust/s2n-tls/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 14 to 15
[profile.release]
debug = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? I'd prefer to have this crate in the existing workspace.

Copy link
Contributor Author

@kaukabrizvi kaukabrizvi Jul 19, 2024

Choose a reason for hiding this comment

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

Yes, it works and enabling debug information is necessary for Cachegrind to accurately identify where instructions come from using the symbols. Setting debug in profile.release ensures that we retain the necessary debug information for detailed analysis while also tracking optimization in release. We can do that here or in the existing workspace but there is more room for discussion about where the crate should live.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I should have been more specific. I thought you could only specify profile overrides at the workspace root, not individual crates in the workspace. If this isn't in the main workspace it needs a

[workspace]
members = ["."]

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said... I would prefer that it is in the main workspace so these tests are run as part of the main test suite. Otherwise they get a bit neglected and people only find out they're broken in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, it can run as part of the regular pre-PR workflow for devs once diffing is enabled by comparing local to the HEAD-1 commit. I'll make these changes in the next revision.

Copy link
Contributor Author

@kaukabrizvi kaukabrizvi Jul 23, 2024

Choose a reason for hiding this comment

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

In trying to add these tests to the existing workspace I ran into an issue where crabgrind can't compile due to an unresolved import 'std::ffi::c_char'. This type was added to std in Rust 1.64.0. Since the bindings/rust workspace requires the 1.63.0 rust-toolchain, this import doesn't resolve. We can either:

  1. Bump the rust-toolchain version: Local development might miss breaking changes specific to the underlying Rust 1.63.0 bindings.

  2. Include the regression test in a separate crate: Regression tests will be isolated from the main test suites.

If there are any other potential solutions I might have missed, please let me know. However, given these options, I believe it makes sense to develop the regression tests in a separate crate to avoid that sort of disruption to the bindings development process.

tests/regression/run_harnesses.sh Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
@kaukabrizvi kaukabrizvi requested a review from camshaft July 23, 2024 17:47
@kaukabrizvi kaukabrizvi changed the title Set up regression benchmark and config harnesses Set up regression benchmark for scalar performance Jul 23, 2024
tests/regression/Cargo.toml Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
@kaukabrizvi kaukabrizvi requested a review from jmayclin July 24, 2024 23:29
@jmayclin jmayclin enabled auto-merge (squash) July 26, 2024 16:14
@jmayclin jmayclin merged commit fff7b15 into aws:main Jul 26, 2024
34 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.

3 participants