Skip to content

Add triage 2021-07-06 #892

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

Merged
merged 1 commit into from
Jul 6, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions triage/2021-07-06.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# 2021-07-06 Triage Log

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.

Triage done by **@rylev**.
Revision range: [5a7834050f3a0ebcd117b4ddf0bc1e8459594309..9a27044f42ace9eb652781b53f598e25d4e7e918](https://perf.rust-lang.org/?start=5a7834050f3a0ebcd117b4ddf0bc1e8459594309&end=9a27044f42ace9eb652781b53f598e25d4e7e918&absolute=false&stat=instructions%3Au)

2 Regressions, 3 Improvements, 2 Mixed
1 of them in rollups

#### Regressions

Rollup of 8 pull requests [#86588](https://github.com/rust-lang/rust/issues/86588)
- 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`)
- The regressions are worse in `deeply-nested-async`.
- 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.
- Follow-up comment: https://github.com/rust-lang/rust/pull/86588#issuecomment-874773229


Improve debug symbol names to avoid ambiguity and work better with MSVC's debugger [#85269](https://github.com/rust-lang/rust/issues/85269)
- 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`)
- This might be the case of simply doing more work (including allocations) where there were comparatively few before.
- 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.
- @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.
-Follow-up comment: https://github.com/rust-lang/rust/pull/85269#issuecomment-874776341


#### Improvements

Derive `Copy` for `VarianceDiagInfo` [#86670](https://github.com/rust-lang/rust/issues/86670)
- 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`)


Add inflate to pgo [#86697](https://github.com/rust-lang/rust/issues/86697)
- 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`)


Fix const-generics ICE related to binding [#86795](https://github.com/rust-lang/rust/issues/86795)
- 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`)


#### Mixed

Include terminators in instance size estimate [#86777](https://github.com/rust-lang/rust/issues/86777)
- 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`)
- 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`)
- 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.
- This seemed to impact the `deeply-nested-async` benchmark which has the tendency to be more sensitive to changes like this.
- Follow-up comment: https://github.com/rust-lang/rust/pull/86777#issuecomment-874779995


Inline Iterator as IntoIterator. [#84560](https://github.com/rust-lang/rust/issues/84560)
- 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`)
- 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`)
- 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.
- Follow-up comment: https://github.com/rust-lang/rust/pull/84560#issuecomment-874781386


#### Nags requiring follow up

- Now that we are adding labels to performance regressions, it should hopefully be easier to follow up.
- 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.