T-compiler Meeting Agenda 2022-06-09
Announcements
- Tomorrow time:2022-06-10T15:00:00+02:00 Types Team: Planning/Deep-Dive, calendar link
- Tomorrow time:2022-06-10T16:00:00+02:00 T-compiler steering:
path sanitisation changes rfc#31272022 Q2 P-high review compiler-team#517 , calendar link - The steering meeting regarding rfc#3127 will now be a week later; see Zulip message for why dates were swapped.
- Reminder: if you see a PR/issue that seems like there might be legal implications due to copyright/IP/etc, please let the Core team know (or at least message @pnkfelix or @Wesley Wiser so we can pass it along).
Other WG meetings (calendar link)
- time:2022-06-13T17:00:00+02:00 wg-rls-2.0 weekly sync-up calendar link
- time:2022-06-13T22:00:00+02:00 [wg-traits] GATs Sync calendar link
- time:2022-06-15T15:00:00+02:00 [Types team] Hack session: Advanced subtyping calendar link
MCPs/FCPs
- New MCPs (take a look, see if you like them!)
- No new proposals this time.
- Old MCPs (not seconded, take a look)
- “Tier 3 target proposal: riscv64gc-linux-android (Android target for
riscv64gc
)” compiler-team#472 (last review activity: 5 months ago)- :loudspeaker: Stale MCP: will be closed next week
- Zulip discussion
- MCP author replied to all questions, so it seems no visible concern to be addressed
- “
-Dwarnings
to cover all warnings” compiler-team#473 (last review activity: 5 months ago)- :loudspeaker: Stale MCP: will be closed next week
- Zulip discussion
- Comments seem to underline possible conflicts with
cargo
linting system.
- “Dealing with type/const ambiguities” compiler-team#480 (last review activity: 4 months ago)
- “Removing codegen logic for
nvptx-nvidia-cuda
(32-bit target)” compiler-team#496 (last review activity: 2 months ago) - “Arbitrary annotations in compiletest” compiler-team#513 (last review activity: about 24 days ago)
- “Tier 3 target proposal: riscv64gc-linux-android (Android target for
- Pending FCP requests (check your boxes!)
- “Stabilize
-Zgcc-ld=lld
as-Clink-self-contained=linker -Clinker-flavor=gcc-lld
” compiler-team#510 - “Increase the minimum linux-gnu versions” rust#95026
- Note: T-libs discussed and approved this change (comment)
- “Stabilize
- Things in FCP (make sure you’re good with it)
- No FCP requests this time.
- Accepted MCPs
- “Build-time execution sandboxing” compiler-team#475
- “Change compiletest declarations parsing” compiler-team#512
- Finalized FCPs (disposition merge)
- “Tracking Issue for
-Z terminal-width
” rust#84673 - “Remove migrate borrowck mode” rust#95565
- “Stabilize the
bundle
native library modifier” rust#95818 - “Modify MIR building to drop repeat expressions with length zero” rust#95953
- “Remove label/lifetime shadowing warnings” rust#96296
- “Lang: Stabilize usage of rustc_nonnull_optimization_guaranteed on -1” rust#97122
- “Tracking Issue for
WG checkins
- @_WG-traits Traits (generic work of the WG) by @nikomatsakis and @Jack Huey (previous checkin)
Officially renamed to types team. Steady progress on various initiatives. Biggest update is migrate mode removal. Over this month, we’re going to do triage during deep dives.
- @_WG-mir-opt MIR Optimizations by @oli (previous checkin)
Checkin text
Backport nominations
T-compiler beta / T-compiler stable
- :beta: “Update LLVM submodule” rust#97690
- Fixes rust#97428,
P-critical
unsoundness bug - author @Nikita Popov mentions that LLVM upgrade should be safe (comment)
- Fixes rust#97428,
- :beta: “Fail gracefully when encountering an HRTB in APIT. " rust#97683
- Fixes rust#96954, a
P-medium
ICE that should be an error (like in stable) - @nagisa nominated (comment)
- Fixes rust#96954, a
- :beta: “Aarch64 call abi does not zeroext (and one cannot assume it does so)” rust#97800
- Fixes a
P-critical
unsoundness bug on tier 1 platform - patch author and backport nomination by @pnkfelix
- Fixes a
- :stable: “Aarch64 call abi does not zeroext (and one cannot assume it does so)” rust#97800
- also stable backport, comment clarifies the nomination: if we had to do a point release for other reasons, should this be included?
T-rustdoc beta / T-rustdoc stable
- No backport nominations for
T-rustdoc
this time.
:back: / :shrug: / :hand:
PRs S-waiting-on-team
- No PRs waiting on
T-compiler
this time.- (4 PRs omitted because either drafts or waiting on other teams / RFC process)
Oldest PRs waiting for review
- “reduce RPC overhead for common proc_macro operations” rust#86822
- nominated by @nnethercote (comment)
- PR is also
I-compiler-nominated
- “Add option to pass environment variables” rust#94387 (last review activity: about 33 days ago)
- cc latest reviewers: @Esteban Küber and @bjorn3
- “
Clone
suggestions” rust#95115 (last review activity: about 26 days ago)- assigned reviewer @Esteban Küber, suggests an additional reviewer (comment)
- also cc: @oli and @Jak{e,ob} Degen (as they left comments, too)
- any actionable for the PR author at this time?
- “Add Finalize statement to make deaggregation “reversible” by storing all information in MIR” rust#96043 (last review activity: about 25 days ago)
- unsure about status: maybe a comment on latest perf run results?
- cc: @Wesley Wiser since assigned reviewer
- “Print type of every call in a method call chain” rust#96918 (last review activity: about 20 days ago)
- unsure about current status
- cc: @Michael Goulet (compiler-errors) maybe to comment on latest notes left by @Esteban Küber?
- “Only compile #[used] as llvm.compiler.used for ELF targets” rust#93718 (last review activity: about 55 days ago)
- seems ready for another round of review
- cc @pnkfelix
Issues of Note
Short Summary
- 2 T-compiler P-critical issues
- 62 T-compiler P-high issues
- 0 P-critical, 0 P-high, 5 P-medium, 0 P-low regression-from-stable-to-beta
- 0 P-critical, 0 P-high, 2 P-medium, 0 P-low regression-from-stable-to-nightly
- 1 P-critical, 40 P-high, 78 P-medium, 9 P-low regression-from-stable-to-stable
P-critical
- “Infinite recursion optimized away with
opt-level=z
” rust#97428- Fixed by upgrading to LLVM/14.x (#97690)
- #97690 incorporates fix by @Nikita Popov comment
- “Wrong cast of u16 to usize on aarch64” rust#97463
- Fixed by #97800
- No
P-critical
issues forT-rustdoc
this time.
P-high regressions
- No
P-high
beta regressions this time.
Unassigned P-high nightly regressions
- No unassigned
P-high
nightly regressions this time.
Performance logs
A busy week in compiler performance, but fortunately improvements outweighed regressions. The biggest improvements came from @nnethercote’s work on making the decoding of SourceFile::lines
lazy which significantly cuts the costs of decoding crate metadata. The biggest regressions came from the removal of json handling in rustc_serialize
which has been a multi-month effort to improve the maintainability of json (de-)serialization in the compiler.
Triage done by @rylev. Revision range: 0a43923a..bb55bd
Summary:
mean | max | count | |
---|---|---|---|
Regressions (primary) | 0.5% | 3.2% | 36 |
Regressions (secondary) | 0.3% | 0.9% | 15 |
Improvements (primary) | -1.3% | -15.1% | 124 |
Improvements (secondary) | -2.7% | -13.5% | 182 |
All (primary) | -0.9% | -15.1% | 160 |
2 Regression, 6 Improvements, 5 Mixed; 4 of them in rollups 48 artifact comparisons made in total
Regressions
rewrite error handling for unresolved inference vars #89862 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | 0.2% | 0.3% | 7 |
Regressions (secondary) | 0.4% | 1.0% | 23 |
Improvements (primary) | N/A | N/A | 0 |
Improvements (secondary) | N/A | N/A | 0 |
All (primary) | 0.2% | 0.3% | 7 |
- Ran cache grind diff and saw that
ObligationForest::process_obligations
is getting called a lot more. - I’m very unfamiliar with trait resolution, so I’m unsure if this is a red herring or not.
- In any case, here is the full diff.
- Left a comment as such here
Remove all json handling from rustc_serialize #85993 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | 0.5% | 1.3% | 68 |
Regressions (secondary) | 0.9% | 3.5% | 40 |
Improvements (primary) | -0.4% | -0.6% | 3 |
Improvements (secondary) | -0.5% | -1.0% | 24 |
All (primary) | 0.5% | 1.3% | 71 |
- @nnethercote was not able to reproduce the regressions in a local build, and it seems the consensus is that the regressions are worth the hit.
Improvements
Make params be SmallVec as originally was #97670 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | N/A | N/A | 0 |
Improvements (primary) | -0.2% | -0.2% | 3 |
Improvements (secondary) | N/A | N/A | 0 |
All (primary) | -0.2% | -0.2% | 3 |
Add PID to LLVM PGO profile path #97137 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | N/A | N/A | 0 |
Improvements (primary) | -0.6% | -0.8% | 20 |
Improvements (secondary) | -0.7% | -1.0% | 8 |
All (primary) | -0.6% | -0.8% | 20 |
Rollup of 6 pull requests #97742 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | N/A | N/A | 0 |
Improvements (primary) | N/A | N/A | 0 |
Improvements (secondary) | -1.9% | -2.6% | 12 |
All (primary) | N/A | N/A | 0 |
interpret: better control over whether we read data with provenance #97684 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | N/A | N/A | 0 |
Improvements (primary) | -0.7% | -1.9% | 8 |
Improvements (secondary) | -5.5% | -10.5% | 12 |
All (primary) | -0.7% | -1.9% | 8 |
Remove migrate borrowck mode #95565 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | N/A | N/A | 0 |
Improvements (primary) | -0.5% | -1.9% | 47 |
Improvements (secondary) | -0.5% | -1.4% | 21 |
All (primary) | -0.5% | -1.9% | 47 |
Lazify SourceFile::lines
. #97575 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | 0.4% | 0.5% | 6 |
Improvements (primary) | -1.8% | -15.3% | 52 |
Improvements (secondary) | -2.9% | -13.8% | 124 |
All (primary) | -1.8% | -15.3% | 52 |
Mixed
Add #[inline]
to Vec
’s Deref/DerefMut
#97553 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | 0.5% | 0.8% | 6 |
Regressions (secondary) | 0.4% | 0.7% | 31 |
Improvements (primary) | -1.2% | -1.7% | 10 |
Improvements (secondary) | -1.1% | -1.9% | 10 |
All (primary) | -0.5% | -1.7% | 16 |
- As with any chance to inlining, performance is expected to change and to not always have a positive impact.
- The improvements outweigh the regressions (especially in primary benchmarks), and so it doesn’t seem worth it to dig too much more into this.
Rollup of 6 pull requests #97644 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | 0.6% | 0.7% | 5 |
Improvements (primary) | -0.2% | -0.3% | 2 |
Improvements (secondary) | -0.3% | -0.5% | 11 |
All (primary) | -0.2% | -0.3% | 2 |
- The improvements outweigh the regressions (which are pretty small and contained in secondary benchmarks).
- Given it’s in a rollup, it’s not worth the effort to investigate.
Rollup of 5 pull requests #97654 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | N/A | N/A | 0 |
Regressions (secondary) | 0.6% | 0.6% | 1 |
Improvements (primary) | N/A | N/A | 0 |
Improvements (secondary) | -0.6% | -0.8% | 8 |
All (primary) | N/A | N/A | 0 |
- The improvements outweigh the regressions (which are pretty small and contained in secondary benchmarks).
- Given it’s in a rollup, it’s not worth the effort to investigate.
Rollup of 3 pull requests #97694 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | 0.2% | 0.2% | 1 |
Regressions (secondary) | 0.3% | 0.5% | 5 |
Improvements (primary) | -0.4% | -0.6% | 12 |
Improvements (secondary) | -0.3% | -0.5% | 13 |
All (primary) | -0.4% | -0.6% | 13 |
- The improvements outweigh the regressions (which are pretty small and almost completely contained in secondary benchmarks).
- Given it’s in a rollup, it’s not worth the effort to investigate.
Inline bridge::Buffer
methods. #97604 (Comparison Link)
mean | max | count | |
---|---|---|---|
Regressions (primary) | 3.8% | 3.8% | 1 |
Regressions (secondary) | N/A | N/A | 0 |
Improvements (primary) | -0.3% | -0.3% | 1 |
Improvements (secondary) | -2.1% | -2.1% | 3 |
All (primary) | 1.8% | 3.8% | 2 |
- I’m a bit confused as this PR was opened to address a performance regression, but it seems to be itself a partial perf regression (at least in instruction counts).
- This causes a near 4% perf regression in serde_derive-1.0.136. Granted that particular benchmark has been somewhat noisy but not nearly to the level seen here.
- Add a comment seeking justification
Nominated Issues
- “reduce RPC overhead for common proc_macro operations” rust#86822
- nominated by @nnethercote (comment)
- points out better perf results and that the PR is ready for review
- “sess: stabilize
--terminal-width
” rust#95635- nominated by @oli for a mention for T-compiler
- “Update Mac Catalyst support for Clang 13” rust#96392
- “New rustc and Cargo options to allow path sanitisation by default” rfcs#3127
- @Andy Wang asked for T-compiler to sign-off the changes (comment)
- @bjorn3 followed with a review
- related T-compiler meeting compiler-team#516
Next week’s WG checkins
- @_wg-polymorphization Polymorphization by @davidtwco (previous checkin)
- @_WG-rls2.0 RLS 2.0 by @matklad (previous checkin)