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

Hook up icount benchmarks to CI #1431

Merged
merged 2 commits into from
Aug 30, 2023
Merged

Hook up icount benchmarks to CI #1431

merged 2 commits into from
Aug 30, 2023

Conversation

aochagavia
Copy link
Contributor

@aochagavia aochagavia commented Aug 30, 2023

This PR adds a GitHub actions job that does the following:

  • Benchmark the PR's target branch and store the results in a temp file.
  • Benchmark the PR itself and store the results in a temp file.
  • Compare the two runs and set the job summary to display the pretty-printed results.
  • Let the job fail if there is a noteworthy difference in instruction counts, hinting you should look at the job summary to see the details.

You can already see it in action by looking at the new job triggered by this very PR 😉. It is failing, because the icounts have increased a lot (I discovered we need to run the resumed handshakes about 30x to filter out noise, instead of 3x).

Relevant context:

  • It's not possible to post the benchmark results as a comment to the PR directly from the GitHub Actions job. This happens because the job runner has read-only access to the issues when the PR has been created from a fork (see the docs). Codecov manages to do this by running as a separate application, with its own permissions.
  • I'm using the term "noteworthy" instead of "significant" to avoid giving the impression we are doing anything fancy with statistics here.

@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #1431 (71633e6) into main (676df24) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1431   +/-   ##
=======================================
  Coverage   96.49%   96.49%           
=======================================
  Files          71       71           
  Lines       15169    15169           
=======================================
  Hits        14637    14637           
  Misses        532      532           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Looks good, just one nit to flag.

.github/workflows/icount-bench.yml Outdated Show resolved Hide resolved
@cpu
Copy link
Member

cpu commented Aug 30, 2023

I think we can go ahead and merge this and iterate if there's any additional feedback.

@cpu cpu added this pull request to the merge queue Aug 30, 2023
Merged via the queue into rustls:main with commit 95cffd6 Aug 30, 2023
21 of 22 checks passed
@aochagavia aochagavia deleted the ci-bench-2 branch August 30, 2023 15:56
@aochagavia
Copy link
Contributor Author

Thanks for the reviews!

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