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

Support generating PRs for rollup-merged perf-testing #701

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jul 21, 2020

This adds two new commands to rust-timer, both of which support being listed multiple times per comment, and can be made anywhere:

  • @rust-timer make-pr-for <sha>
  • @rust-timer update-branch-for <sha>

Both commands should be passed a rollup merge SHA, e.g., https://github.com/rust-lang/rust/commit/105cd4955425de2613ac2ae6c2d2d1baf17ebd2b (with or without a full https URL prefix, though rust-lang/rust@SHA is not supported).

We create a pull request on rust-lang/rust and immediately invoke try on it to gather perf statistics. The pull request is created such that it contains two commits:

  • A revert of current master back to master at the time of rollup merge creation. This is not perfect -- ideally it would be the previous commit of master when the rollup merged -- but finding which commit that was isn't trivial, so we go with this. It should be pretty close, as rollup PRs are generally retry'd into top of queue and are not long-lived.
  • The merge commit of the rolled-up PR's branch into that old master.

This should have the effect of mostly faithfully representing what it would look like to merge the PR into that old master by itself (rather than in the rollup), though it isn't absolutely perfect -- there's a number of things that could go wrong to make this infeasible.

One of the easiest ways for this to be problematic is if there have been CI-fixing changes landing in master since the creation of the rollup. Currently, those changes would likely mean that the pull request we've created is essentially useless, as it'll lack those changes and that means it cannot be easily run on its own. In such a scenario though nothing should stop the human operator from manually pushing some commits to the created PR.

An alternative to this is to generate a revert of the merge of just the commit passed onto current master, and benchmark that. The results would then be considered an "inverse" (i.e., good means the original PR was bad). This is much less prone to problems caused by external changes but is a less faithful and is somewhat harder to interpret the results of.

The second is basically solely for the edge case of something landing in master in the (short) amount of time the first one takes to work; this should be quite rare in practice. Eventually we may automatically detect this case, but the current PR doesn't do so as this is relatively hard.

@Mark-Simulacrum Mark-Simulacrum merged commit 366a107 into rust-lang:master Jul 21, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the auto-rollup-pr branch July 21, 2020 14:10
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.

1 participant