Stabilize bisection logic to ease toolchain reuse #372
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Makes cargo-rustc-bisect iterate toolchains in well-defined order that is largely independent(with a huge asterisk) of bisection bounds.
Motivation
Currently the bisection logic is very straightforward and very sensitive to bisection boundaries, making
--preserve
flag not particularly useful.To illustrate, suppose we want a bisect a regression that happened on 2020-10-05. We run bisector with
--start 2020-10-01 --end 2020-10-30
, and we end up checking the following nightlies, in order:2020-10-01
,2020-10-30
,2020-10-15
,2020-10-07
,2020-10-03
,2020-10-05
, <- 6 toolchains totalThis is fine and reasonable. But if we then try to run the same bisection again but with very slightly different bounds of
--start 2020-10-02 --end 2020-10-30
, then we'd be checking:2020-10-02
,2020-10-30
(r),2020-10-16
,2020-10-08
,2020-10-04
,2020-10-06
,2020-10-05
(r) <- 7 toolchains total, of which only two are reused (the starting point and the regression point)So we end up doing a whole lot of toolchain needlessly, which is mildly annoying since waiting for toolchains to download is in my experience the most time-intensive part of the whole bisection process.
It would be nice if the order of the tried nightlies was somehow "fixed" and "same" regardless of what bisection bounds we use. (Even though it obviously needs to depend on the bounds since testing toolchain out of bounds is pointless).
Design
(This is both a slight oversimplification, and overly poetic. I seem to struggle expressing this idea in words well, since i'm not quite sure how universal my mental image of a binary tree is)
Imagine every nightly nightly since the beginning of time laid out in a line sequentially. Now convert that sequential line into a (complete, left-heavy) binary search tree, with the nightly#1 and nightly#3 being children of nightly#2, nightly#2 and nightly#6 being children of nightly#4 etc.
With this binary tree structure at our disposal we can start every bisection at the root, reusing that one, and then moving down to either its left or right child, until we narrow down the regression at the leaf (all leaves are odd-numbered nightlies, so it is guaranteed that the leaf is either a first-regressed or last-unregressed). And when we do a different bisection tomorrow, we'll start at the root again, which we no longer need to download, and then depending on the test result at the root we have a 50/50 chance that we have the next toolchain already downloaded as well. And so forth. Which is neat!
Now, of course in practice we don't want to start every bisection at the tree root, since testing tree root is pointless when it's outside of the known regression range.
But we can easily adapt the core idea and check the lowest depth node in range instead. In effect that would give us the exact same binary search order that we have today, but after the initial 1-2 steps that would align us with the overall tree structure, if that makes sense.
Implementation
Two steps.
First: replace
next = (rm_no + lm_yes) / 2;
withnext = midpoint_stable(rm_no, lm_yes)
whereThis ensures that the given
&[Toolchain]
slice is iterated in "binary search tree order"Second: since the
&[Toolchain]
slice given toleast_satisfying
only holds the potentially interesting toolchains instead of all of them, we need to pass on the information of the placement of the given&[Toolchain]
sub-slice in the (semi-hypothetical) "all toolchains" array. Otherwise we are still exactly as sensitive to the--start
date as when we started.So add an extra
start_offset
argument toleast_satisfying
with its value being "how many days has nightlies has there been since the first nightly ever released until the first toolchain in the given&[Toolchain]
slice". (This is not applicable to CI artifact toolchains)Testing
midpoint_stable
does what we want it to do--start
/--end
boundaries slightly and not needing to redownload a bunch of stuff. That also proved extremely useful for triaging regression evolution (i.e. understanding when an ICE becomes a different unrelated ICE)Drawbacks
--preserve
flag passedI considered adding an extra flag to control this behavior, but ultimately decided that it complicates the code a bit too much for very minimal gains. I would be happy to change it if someone has strong opinions on the subject.