Only run changed benchmarks during triage - #2553
Conversation
514a8a9 to
a656503
Compare
Kobzol
left a comment
Kobzol
left a comment
There was a problem hiding this comment.
Looks simple enough, though the PR fetching should be unnecessary.
| let comparison = calculate_metric_comparison( | ||
| ctxt, | ||
| &QueuedCommit { | ||
| sha: rollup.head.sha, |
There was a problem hiding this comment.
This won't really work, because the PR's HEAD SHA is not the merge SHA that was merged and benchmarked. We could query the PR from the benchmark_request DB table, we even have a unique index on (pr, commit_type), but adding a new query is a bit annoying, just for this. We could query the last N completed requests and try to find the PR amongst them, which would work in practice, but is also a bit meh.
Maybe the easiest would be to use ctxt.get_master_commits(), iterate the master commits to find one with the same rollup PR number, and then get its SHA.
There was a problem hiding this comment.
Good catch, I totally didn't consider that.
I indeed went for get_master_commits was that is by far the easiest and should work for 99% of usecases. I can always add a new query in the future if this becomes a problem.
|
Thanks for the review, will fix the PR later today, gotta get some work done for my job which is not yet rustc :) |
a656503 to
f7cc92f
Compare
f7cc92f to
5697eac
Compare
5697eac to
536289b
Compare
Kobzol
left a comment
Kobzol
left a comment
There was a problem hiding this comment.
Looks great! Left a few remaining nits.
| .find(|commit| commit.pr.is_some_and(|pr| pr == rollup)) | ||
| .cloned() | ||
| else { | ||
| bail!("The `@rust-timer triage` command can only be executed in rollups. If this is a rollup, it might be too old.") |
There was a problem hiding this comment.
| bail!("The `@rust-timer triage` command can only be executed in rollups. If this is a rollup, it might be too old.") | |
| bail!("The `@rust-timer triage` command can only be executed in merged rollups. If this is a merged rollup, it might be too old.") |
| .is_relevant() | ||
| .then_some(c.test_case.benchmark.to_string()) | ||
| }); | ||
| let newly_failed_benchmarks = comparison.newly_failed_benchmarks.into_keys(); |
There was a problem hiding this comment.
What's the reasoning for including failed benchmarks? To figure out which PRs broke them? If yes, could you please add a comment here that explains that?
| for benchmark in &benchmarks_to_run { | ||
| writeln!(&mut result, "* {benchmark}").unwrap(); | ||
| } | ||
| writeln!(&mut result, "</details>").unwrap(); |
There was a problem hiding this comment.
| writeln!(&mut result, "</details>").unwrap(); | |
| writeln!(&mut result, "</details>\n").unwrap(); |
2 participants
This PR is based on #2552, and because stacked PRs aren't working cross-fork also contains it's commits :c
I recommend reviewing commit by commit. Every commit has a single purpose, compiles and passes tests.
This implements step 3 of #t-compiler/performance > Proposal for
@rust-timer triagecommandr? @Kobzol