2021-07-06.md

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

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

Regressions

Rollup of 8 pull requests #86588

• Moderate regression in instruction counts (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 and checking spans to see if Rust 2021 closure capturing should be used. 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: rust-lang/rust#86588 (comment)

Improve debug symbol names to avoid ambiguity and work better with MSVC's debugger #85269

• Moderate regression in instruction counts (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 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: rust-lang/rust#85269 (comment)

Improvements

Derive Copy for VarianceDiagInfo #86670

• Moderate improvement in instruction counts (up to -4.8% on full builds of wg-grammar-check)

Add inflate to pgo #86697

• Moderate improvement in instruction counts (up to -3.4% on full builds of keccak-debug)

Fix const-generics ICE related to binding #86795

• Moderate improvement in instruction counts (up to -1.8% on full builds of inflate-check)

Mixed

Include terminators in instance size estimate #86777

• Moderate regression in instruction counts (up to 4.4% on incr-unchanged builds of deeply-nested-async-check)
• Moderate improvement in instruction counts (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: rust-lang/rust#86777 (comment)

Inline Iterator as IntoIterator. #84560

• Large regression in instruction counts (up to 6.2% on incr-patched: println builds of webrender-opt)
• Moderate improvement in instruction counts (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: rust-lang/rust#84560 (comment)

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 has not been addressed.