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

cachegrind: ignore leading path to the benchmarked rustls checkout #31

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

aochagavia
Copy link
Collaborator

For each PR benchmark we create two checkouts of the rustls repository, each one in its own directory (e.g. /tmp/.tmpZE2KDR and /tmp/.tmpq8vPPQ). As a consequence, when comparing the cachegrind output the diff tool fails to correlate the functions of both runs. The result is a totally useless diff, where all instructions from the baseline are considered to have disappeared and all instructions from the candidate to have appeared (for an example, see https://bench.rustls.dev/comparisons/371463d8129f4f0541e38063358ec2ad4428a44a:453cf7968b251b22db5bcadebefe3ea37125b630/cachegrind-diff/handshake_tickets_aws_lc_rs_1.2_rsa_aes_server)

This commit tells cachegrind to ignore the leading path (everything up to target/release/build), restoring the ability to track individual rustls functions.

For each PR benchmark we create two checkouts of the rustls repository,
each one in its own directory (e.g. `/tmp/.tmpZE2KDR` and
`/tmp/.tmpq8vPPQ`). As a consequence, when comparing the cachegrind
output the diff tool fails to correlate the functions of both runs. The
result is a totally useless diff, where all instructions from the
baseline are considered to have disappeared and all instructions from
the candidate to have appeared (for an example, see
https://bench.rustls.dev/comparisons/371463d8129f4f0541e38063358ec2ad4428a44a:453cf7968b251b22db5bcadebefe3ea37125b630/cachegrind-diff/handshake_tickets_aws_lc_rs_1.2_rsa_aes_server)

This commit tells cachegrind to ignore the leading path (everything up
to `target/release/build`), restoring the ability to track individual
rustls functions.
@aochagavia
Copy link
Collaborator Author

@cpu I can't think of an easy way for you to test this... The way I developed the solution myself was by:

  1. Manually running the icount benchmarks locally from two different paths (rustls and rustls2).
  2. Creating a comparison with the helper code from rustls/ci-bench and manually checking the diffs in the output (cargo run --release -- compare foo bar). This reproduced the issue.
  3. Iterating on a fix in the helper code.
  4. Creating a comparison with the helper code and manually checking the diffs in the output (cargo run --release -- compare foo bar). This proved that the issue was solved.
  5. Copying the fix to this repository.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

This fix makes sense to me. I trust your debugging process here and don't think I need to reproduce it from scratch. If there are further issues I know where to look to iterate on the regex :-)

Thanks!

@aochagavia aochagavia merged commit 7156fb4 into main Dec 21, 2023
9 checks passed
@aochagavia aochagavia deleted the fix-cachegrind-diffs branch December 21, 2023 15:52
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.

2 participants