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

Benchmark pull requests against main #304

Merged
merged 14 commits into from
Dec 14, 2022
Merged

Conversation

pnmadelaine
Copy link
Collaborator

@pnmadelaine pnmadelaine commented Dec 12, 2022

This PR adds the --compare flag to the ./mach benchmark subcommand, allowing to benchmark against another revision of hacl-packages.
It also adds a github action that will use that command on PRs and hacl-star branches, comparing them with main.

@pnmadelaine pnmadelaine requested a review from a team as a code owner December 12, 2022 12:39
@cla-bot cla-bot bot added the cla-signed label Dec 12, 2022
Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

Thanks, @pnmadelaine! This looks all very good to me. I have a few (non-blocking) suggestions and a more general question.

The more general question is: I'm sure you tested this locally, but it's difficult for me to see if everything works as expected. Do you think we could extract the benchmarking step into a standalone script that we can use to test locally?

.github/workflows/benchmark_pull_request.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark_pull_request.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark_pull_request.yml Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 12, 2022

Pull Request Test Coverage Report for Build 3694501943

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 55.966%

Totals Coverage Status
Change from base Build 3687385504: 0.0%
Covered Lines: 33754
Relevant Lines: 60312

💛 - Coveralls

@pnmadelaine
Copy link
Collaborator Author

pnmadelaine commented Dec 12, 2022

@duesee we could definitely extract the script, maybe even put it in mach?

@duesee
Copy link
Contributor

duesee commented Dec 12, 2022

@duesee we could definitely extract the script, maybe even put it in mach?

Good idea 👍

@duesee duesee self-requested a review December 13, 2022 08:51
Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

As discussed offline, this is good to go, thanks! I think it would be worth it to try to get rid of the extra download of gbenchmark, though. But it should be fine to leave that as is should there be unexpected complications when changing that.

@pnmadelaine
Copy link
Collaborator Author

pnmadelaine commented Dec 14, 2022

Since CI was blocking and I was done moving the script to mach I decided to push everything at once.
The benchmark_pull_request github workflow is now failing because it tries to use the --compare flag from the main branch...
EDIT: I fixed this by changing the behaviour of the --compare flag, it is now meant to be run from the new revision, instead of the old one. This seems more logical anyway.

Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

I'll finish the pending review with "Comment". Feel free to re-request a review when this is ready!

@duesee duesee self-requested a review December 14, 2022 15:07
Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

Thank you for the extra effort! This is easier to review now. I checked that mach benchmark --compare works as expected and detects performance regressions. mach returns an error code in this case so that GitHub Actions will (hopefully) fail. Nice work!

@duesee
Copy link
Contributor

duesee commented Dec 14, 2022

@franziskuskiefer, do you want to have another look? In any case we should not forget to squash before merge :-)

@franziskuskiefer
Copy link
Member

Thanks, lgtm!

@franziskuskiefer franziskuskiefer merged commit 797e5f8 into main Dec 14, 2022
@franziskuskiefer franziskuskiefer deleted the pnmadelaine-benchmarks branch December 14, 2022 18:30
@franziskuskiefer franziskuskiefer mentioned this pull request Dec 19, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants