rust-lang / rust-lang/rustc-perf
Significance Factor is flawed
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 733
- Forks
- 189
- Avg merge
- 10h 52m
- Merged PRs (30d)
- 43
Description
Currently there is a PR for Rust, rust-lang/rust#97293, which I suspect is experiencing significant noise due to the way rustc-perf calculates significance. After reading about how the significance factor is calculated, I believe it is severely flawed in multiple ways.
First, the relative change from a single measurement of the previous commit to a single measurement of the new commit is extremely sensitive to noise. At the moment there is no way to reliably tell whether a change in measurements between two commits is actually significant or if it's an outlier due to noise, just by looking at the raw data.
Next, since the quartiles of historical data are entirely unweighted, any prior commit that significantly affects performance will alter the quartiles and thus the significance factor in a very severe and problematic way. For example, let's say we are using the previous 100 commits as our historical data, and we are considering a very low-noise test. A new commit increases compile times by ~1% across the board. At first, there will be no change to the quartiles; then, after 25 new commits, Q3 will suddenly sharply increase, and only after 75 commits, Q1 will suddenly sharply increase to match.
- If you do any perf run before 25 commits, the quartiles will be outdated (but probably still reasonable).
- If you do any perf run between 25 and 75 commits, the Q3 will sharply increase, the IQR will be hugely inflated, and the significance factor will be underestimated. If the the new commit instead decreased compile times by ~1%, the IQR would be hugely deflated and the significance factor would be overestimated.
- Only after 75 commits, everything somewhat returns to normal.
(This effect is even worse for commits that are exactly 25 or 75 commits later, e.g. when the quartiles sharply change on the border of the merge base and the PR commit). I hope you agree that these issues are severe and need to be fixed.
Due to these issues, I would like to propose an entirely new method instead:
- Instead of generating one perf reading for each new commit, generate a number of perf readings for each nightly and use the most recent nightly as the merge base for the PR. Assume the data is normally distributed and calculate the mean/variance.
- Do a perf run for the new commit. Assume that the variance under the new commit is the same as the variance of the nightly commit.
- Find the smallest two-sided prediction interval containing the new measurement, and report the percentage as the result. (Under the given assumptions, this is equivalent to calculating the P-value of a two-tailed hypothesis test).
Pros: much much more accurate
- Uses only data from a single prior commit; completely unaffected by historical data.
- Any significant changes are reflected immediately and completely.
- Less prone to noise (thanks to a far more accurate method of quantifying variance).
- I'm not a statistician, but I believe this also has a stronger mathematical backing than the current method.
Cons:
- PRs must be rebased onto the latest nightly instead of master. (is this actually a pro?...)
- May require a higher overall number of perf runs, though this is configurable.
Side notes:
- If the sample size for each nightly is small, perhaps variance (and maybe mean as well) should be calculated using some variation of a high-alpha exponential moving average; and/or the variance should be considered "known" for the purpose of the prediction interval.
- Could also generate multiple measurements for each requested perf test, which would provide even better results but would obviously be slow/expensive.
Anyway, I am new to this project, so I would love to hear any comments and feedback!
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
The issue names no files, tests, or entry points. Start by locating the significance-factor implementation and its existing tests, then compare the current calculation with the proposed nightly-based statistical approach; done requires an agreed design and validated behavior for noisy and historically shifted measurements.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- performance
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100