rustc-perf icon indicating copy to clipboard operation
rustc-perf copied to clipboard

rust-timer generated PRs include more than specific PR

Open Mark-Simulacrum opened this issue 5 years ago • 2 comments

For example, https://github.com/rust-lang/rust/pull/81207 included more than just the requested PR, instead also including the PRs in the "head" of the rollup. This is likely because we're reverting up to master before the rollup, but should instead revert to the parent commit of the merge commit for a specific PR in the rollup.

Mark-Simulacrum avatar Jan 26 '21 22:01 Mark-Simulacrum

This tests were also affected:

rust-lang/rust#81420 rust-lang/rust#81933

I don't think the potential regressions were looked into once it was noticed that this bug was occurring. I just want to make sure there's follow up on those when this issue is fixed, or manual follow up prior to that.

tgnottingham avatar Feb 15 '21 20:02 tgnottingham

I think the current approach is not at all testing what's intended, beyond the issue here. Since we use @rust-timer queue on the generated PR, we're comparing the performance of the try commit versus its master parent. But look at their contents:

Try commit content

The tip of the generated PR always has the exact content of the "Rollup merge" (well, whatever commit is given to @rust-timer make-pr-for, but that's what's expected I think) of the requested PR within the rollup PR, because that's the tree object we specify for its last commit. The try commit itself has the exact same content (barring a race condition where master gets more commits between the time the PR's commits are generated and the time the try build starts). The preceding revert commit we create has no effect, AFAICT.

That "Rollup merge" commit, and therefore the try commit, contains the requested PR, as well as all the PRs that were included in the rollup before it, and nothing that has merged to master since.

Try commit parent content

On the other hand, the try's master parent is the tip of master at the time of the try build, which contains the entire rollup, along with everything that has merged to master since the rollup PR was created.

So, not only is this testing way more changes than we'd like, it's also not testing the requested PR, since both sides of the comparison contain it.


I'm not sure exactly how we'd like to test the requested PR, but any solution that compares to the latest master has the problem of master already including the PR. So, if we're using the latest master as a comparison point, the only way to do it is to do a revert comparion by doing a proper git-revert of the commits in the requested PR. That can result in conflicts and require manual intervention, however. But maybe that's good enough.

A solution that would never result in git conflicts is to test the tip of the original PR against the master commit upon which it was based. Also, the tip of the original PR should almost always pass CI tests, since the rollup did.

I'm not familiar enough with the build infrastructure to know how much work this would be to integrate though.

tgnottingham avatar Aug 14 '21 21:08 tgnottingham