|
| 1 | +# 2021-07-06 Triage Log |
| 2 | + |
| 3 | +A fairly mixed week with improvements and regressions mostly balancing themselves out. The highlight of this week is the new performance triage process which will now label PRs that introduce performance regressions with the `perf-regression` label. Authors and/or reviewers are expected to justify their performance regression either by a short summary of why the change is worth it despite the regression or by creating an issue to follow-up on the regression. |
| 4 | + |
| 5 | +Triage done by **@rylev**. |
| 6 | +Revision range: [5a7834050f3a0ebcd117b4ddf0bc1e8459594309..9a27044f42ace9eb652781b53f598e25d4e7e918](https://perf.rust-lang.org/?start=5a7834050f3a0ebcd117b4ddf0bc1e8459594309&end=9a27044f42ace9eb652781b53f598e25d4e7e918&absolute=false&stat=instructions%3Au) |
| 7 | + |
| 8 | +2 Regressions, 3 Improvements, 2 Mixed |
| 9 | +1 of them in rollups |
| 10 | + |
| 11 | +#### Regressions |
| 12 | + |
| 13 | +Rollup of 8 pull requests [#86588](https://github.com/rust-lang/rust/issues/86588) |
| 14 | +- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=f1e691da2e640bb45fe18f8a5bd8f7afa65ce21d&end=964a81eb37db6ee33b8fc107582618bf2befe02d&stat=instructions:u) (up to 1.9% on `full` builds of `deeply-nested-async-check`) |
| 15 | +- The regressions are worse in `deeply-nested-async`. |
| 16 | +- Most of the rollup is documentation or tooling changes. The only real changes in code were in [MIR pretty printing](https://github.com/rust-lang/rust/pull/86566) and [checking spans to see if Rust 2021 closure capturing should be used](https://github.com/rust-lang/rust/pull/86536). Both seem rather benign. However, given the performance regression is async code (which may take more advantage of closures), perhaps the closure capture change should be investigated first. |
| 17 | +- Follow-up comment: https://github.com/rust-lang/rust/pull/86588#issuecomment-874773229 |
| 18 | + |
| 19 | + |
| 20 | +Improve debug symbol names to avoid ambiguity and work better with MSVC's debugger [#85269](https://github.com/rust-lang/rust/issues/85269) |
| 21 | +- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=851c82e88ade86bfe3b4ee785d5e5ab1d954b61c&end=2545459bff0aae43288e2e17bff0d332c49a6353&stat=instructions:u) (up to 1.5% on `incr-unchanged` builds of `unify-linearly-debug`) |
| 22 | +- This might be the case of simply doing more work (including allocations) where there were comparatively few before. |
| 23 | +- Unfortunately a perf run was not run before merging (due to the somewhat complication nature of it landing). This is another example where we'll probably want to invest more in ensuring our performance triage process does not lose track of such changes. |
| 24 | +- @michaelwoerister already opened [#86431](https://github.com/rust-lang/rust/issues/86431) to investigate this area of the code. Given the regression isn't very bad, I suggest we let this change slide and try to address the performance of debug info generation wholistically. |
| 25 | +-Follow-up comment: https://github.com/rust-lang/rust/pull/85269#issuecomment-874776341 |
| 26 | + |
| 27 | + |
| 28 | +#### Improvements |
| 29 | + |
| 30 | +Derive `Copy` for `VarianceDiagInfo` [#86670](https://github.com/rust-lang/rust/issues/86670) |
| 31 | +- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=fecc65a19763364b8dafbbf1d23be562268bd387&end=47b2f15bba4544170e6e748910e7c01da467c897&stat=instructions:u) (up to -4.8% on `full` builds of `wg-grammar-check`) |
| 32 | + |
| 33 | + |
| 34 | +Add inflate to pgo [#86697](https://github.com/rust-lang/rust/issues/86697) |
| 35 | +- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=47b2f15bba4544170e6e748910e7c01da467c897&end=f12d91f9da9b06813b3bc0c31aa6133070ada9ab&stat=instructions:u) (up to -3.4% on `full` builds of `keccak-debug`) |
| 36 | + |
| 37 | + |
| 38 | +Fix const-generics ICE related to binding [#86795](https://github.com/rust-lang/rust/issues/86795) |
| 39 | +- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=798baebde1fe77e5a660490ec64e727a5d79970d&end=cd48e61c5d8f83fbfbfc28db0b0bce1354d0ced1&stat=instructions:u) (up to -1.8% on `full` builds of `inflate-check`) |
| 40 | + |
| 41 | + |
| 42 | +#### Mixed |
| 43 | + |
| 44 | +Include terminators in instance size estimate [#86777](https://github.com/rust-lang/rust/issues/86777) |
| 45 | +- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=56dee7c49ecdec4c2c9eccc6ff966cf58847bda6&end=7a9ff746fe20a38a3adc0ac65e1789f6e4b099ad&stat=instructions:u) (up to 4.4% on `incr-unchanged` builds of `deeply-nested-async-check`) |
| 46 | +- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=56dee7c49ecdec4c2c9eccc6ff966cf58847bda6&end=7a9ff746fe20a38a3adc0ac65e1789f6e4b099ad&stat=instructions:u) (up to -1.9% on `full` builds of `ripgrep-opt`) |
| 47 | +- This was identified as potentially being performance sensitive since it leads to changes in CGU partitioning, but unfortunately, @bors has already been invoked on the PR. Arguably, we should have run a performance test anyway. |
| 48 | +- This seemed to impact the `deeply-nested-async` benchmark which has the tendency to be more sensitive to changes like this. |
| 49 | +- Follow-up comment: https://github.com/rust-lang/rust/pull/86777#issuecomment-874779995 |
| 50 | + |
| 51 | + |
| 52 | +Inline Iterator as IntoIterator. [#84560](https://github.com/rust-lang/rust/issues/84560) |
| 53 | +- Large regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=5249414809d40fe22eca0c36105a2f71b9006e04&end=6e9b3696d494a32d493585f96f0671123066cd58&stat=instructions:u) (up to 6.2% on `incr-patched: println` builds of `webrender-opt`) |
| 54 | +- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=5249414809d40fe22eca0c36105a2f71b9006e04&end=6e9b3696d494a32d493585f96f0671123066cd58&stat=instructions:u) (up to -3.2% on `full` builds of `deeply-nested-opt`) |
| 55 | +- Performance run was run on the change which looks similar to results here. Given that this led to fairly significant regressions in some benchmarks, there should probably be some justification as to why the performance regressions are acceptable. |
| 56 | +- Follow-up comment: https://github.com/rust-lang/rust/pull/84560#issuecomment-874781386 |
| 57 | + |
| 58 | + |
| 59 | +#### Nags requiring follow up |
| 60 | + |
| 61 | +- Now that we are adding labels to performance regressions, it should hopefully be easier to follow up. |
| 62 | +- Last week's follow up on max-rss regression in [#86034](https://github.com/rust-lang/rust/pull/86034#issuecomment-871488586) has not been addressed. |
0 commit comments