-
Notifications
You must be signed in to change notification settings - Fork 712
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
Conversation
tests/regression/Cargo.toml
Outdated
[profile.release] | ||
debug = true |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = ["."]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
-
Bump the rust-toolchain version: Local development might miss breaking changes specific to the underlying Rust 1.63.0 bindings.
-
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.
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:
Crabgrind is used to call the Valgrind client request interface, enabling selective benchmarking only for the desired code regions within each harness.
Call-outs:
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.