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

Enabling differential performance benchmarking #4667

Merged
merged 20 commits into from
Aug 8, 2024

Conversation

kaukabrizvi
Copy link
Contributor

@kaukabrizvi kaukabrizvi commented Jul 26, 2024

Resolved issues:

N/A

Description of changes:

This PR builds on #4649 to introduce differential benchmarking. Specifically, it adds a suffix flag to toggle between versions of s2n-tls to compare them and a diff_mode flag to control scalar vs differential performance analysis. The actual diff functionality relies on cg_annotate --diff which diffs two cachegrind output files to pinpoint the sources of performance difference.

Call-outs:

  • Modularizing test functions: The run_valgrind_test function is split up into smaller components for readability and since run_diff_mode relies on similar functionality. There are no major functional changes to scalar performance benchmarking from Set up regression benchmark for scalar performance #4649, however components of this function are broken up into smaller modules.
  • Output file storage: Cachegrind annotated output files are now stored in a subdirectory within target/perf_outputs based on the version attributed to them, curr, prev, or diff. This makes it easier to access the output files instead of having to skim file names between test versions, taking into consideration the workflow of embedding them into CI and downloading them through the GitHub Actions workflow.

Testing:

This addition does not change any existing functionality. The benchmarking functionality was tested by following the developer workflow outlined in the README for differential performance.

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

@kaukabrizvi kaukabrizvi marked this pull request as ready for review July 26, 2024 23:36
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
Comment on lines 78 to 89
let suffix = get_test_suffix();
if !is_running_under_valgrind() {
let ctrl = InstrumentationControl;
test_body(&ctrl)
if is_diff_mode() {
run_diff_test(test_name);
Ok(())
} else {
let ctrl = InstrumentationControl;
test_body(&ctrl)
}
} else {
run_valgrind_test(test_name);
run_valgrind_test(test_name, &suffix);
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are a great example of why it's a good idea to consolidate/abstract how the tests are run from the harness definitions themselves. If we didn't have this abstraction, you'd have to go update every harness (and hopefully you consistently applied the change :))

@kaukabrizvi
Copy link
Contributor Author

kaukabrizvi commented Jul 31, 2024

In the newest revision, I have included changes to the way DIFF_MODE runs. Instead of specifying curr/prev through a suffix, the developer runs scalar performance on both versions, the performance information is now stored by the commit hash associated with that particular version. Then, when diff_mode is run it auto-detects the older of the two performance files and sets that as the previous version for comparison so that the performance difference represents current - previous.

tests/regression/README.md 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
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
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
tests/regression/src/lib.rs Show resolved Hide resolved
tests/regression/src/lib.rs Show resolved Hide resolved
tests/regression/src/lib.rs Outdated Show resolved Hide resolved
@jmayclin jmayclin merged commit bb3509c into aws:main Aug 8, 2024
35 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