Skip to content

More refactoring + Implement step 1 of triage command - #2531

Merged
Kobzol merged 5 commits into
rust-lang:mainfrom
JonathanBrouwer:triage-command2
Aug 14, 2026
Merged

More refactoring + Implement step 1 of triage command#2531
Kobzol merged 5 commits into
rust-lang:mainfrom
JonathanBrouwer:triage-command2

Conversation

@JonathanBrouwer

Copy link
Copy Markdown
Contributor

This PR implements step 1 of the triage command as described in the initial pitch #t-compiler/performance > Proposal for `@rust-timer triage` command @ 💬.

It consists of 4 commits, which I recommend to review individually:

  1. Simplify enqueue_sha function -> Simplifies the enqueue_sha by pushing the comment posting logic out of the function & removing to option to provide multiple commits. This PR benefits from "hide whitespace" during review again.
  2. Factor enqueue_sha_build into its own function -> Factor part of the @rust-timer build command into its own function, which can be re-used by the triage command.
  3. Add command parsing for triage command
  4. Implement triage command

r? @Kobzol

@Kobzol Kobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! Could you please also document the new command in the website, at https://perf.rust-lang.org/help.html?

Comment thread site/src/github.rs Outdated
Comment thread site/src/github.rs Outdated

{
let conn = ctxt.conn().await;
record_try_benchmark_request_without_artifacts(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note (pre-existing): this is a bit sketchy, we record a request without artifacts, and then immediately attach the artifacts. We could just do it in one step for build. Though I suspect that would actually make the surrounding code more complicated, so it's not worth it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can take a look in a follow-up PR if this is worth it, but I think indeed it would just make everything more complicated

Comment thread site/src/request_handlers/github.rs Outdated
Comment thread site/src/request_handlers/github.rs
Comment thread site/src/request_handlers/github.rs Outdated
Comment thread site/src/request_handlers/github.rs
@JonathanBrouwer

Copy link
Copy Markdown
Contributor Author

@Kobzol I think I handled all the review comments

@Kobzol Kobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, let's try this!

@Kobzol
Kobzol added this pull request to the merge queue Aug 14, 2026
Merged via the queue into rust-lang:main with commit 0e0a997 Aug 14, 2026
15 checks passed
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