-
Notifications
You must be signed in to change notification settings - Fork 153
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
Categorize perf runs when posting to GitHub #881
Conversation
712661f
to
58e127a
Compare
58e127a
to
7027c45
Compare
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.
Happy to merge with one comment resolved (feel free to do so yourself).
Benchmarking this pull request likely means that it is \ | ||
perf-sensitive, so we're automatically marking it as not fit \ | ||
for rolling up. Please note that if the perf results are \ |
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.
Just an idea: if the perf results don't return any significant perf changes, maybe this rollup=never
isn't necessary?
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.
A perf run is likely done when a change to a perf sensitive part of the compiler is done. It may not result in any significant perf changes when the perf run is done, but due to interaction with other PR's it may still have a perf change when actually landing.
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.
This categorization is also way too insensitive IMO. A perf run with improvements up to 0.6% and a couple of regressions up to 0.8% was marked as not having any significant changes. Many changes >0.2% across multiple benchmarks are unlikely to be noise. rust-lang/rust#86404 (comment)
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.
I do think we can fine-tune sensitivity, but until we're even more confident I agree that we shouldn't drop the rollup=never. Our queue doesn't suffer much from it today anyway, to my knowledge.
When the triage bot posts the results of running a performance run, it now provides a summary of the performance run. It should look something like this:
Finished benchmarking try commit (ab345): comparison url.
Summary: This change led to significant regressions 😿 in compiler performance.
full
builds ofwg-grammar-check
)