T-compiler Meeting Agenda 2022-04-21

Announcements

• Tomorrow time:2022-04-22T13:00:00:00+02 Types Team: Deep-dive on Formality subtyping rules calendar link
  • Topic of the meeting: wg-traits#39
• Tomorrow time:2022-04-22T16:00:00:00+02 Compiler Team: Compiler Leadership and Succession (calendar link)
  • Topic of the meeting compiler-team#506 and (if time allows) compiler-team#484
• 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).

MCPs/FCPs

• New MCPs (take a look, see if you like them!)
  • "Add attribute to run specific tests in an isolated process" compiler-team#508
  • "Stabilize -Zgcc-ld=lld" compiler-team#510
• Old MCPs (not seconded, take a look)
  • "Accept pc in place of unknown and unknown in place of pc for x86_64 and i?86 targets" compiler-team#441 (last review activity: 9 months ago)
  • "Make -Z binary-dep-depinfo the default behavior" compiler-team#464 (last review activity: 6 months ago)
  • "Tier 3 target proposal: riscv64gc-linux-android (Android target for riscv64gc)" compiler-team#472 (last review activity: 4 months ago)
  • "-Dwarnings to cover all warnings" compiler-team#473 (last review activity: 4 months ago)
  • "Build-time execution sandboxing" compiler-team#475 (last review activity: 3 months ago)
  • "Dealing with type/const ambiguities" compiler-team#480 (last review activity: 3 months ago)
  • "Removing codegen logic for nvptx-nvidia-cuda (32-bit target)" compiler-team#496 (last review activity: about 34 days ago)
  • "Add yeet experimentally" compiler-team#501 (last review activity: about 20 days ago)
• Pending FCP requests (check your boxes!)
  • "Tracking issue for Consistent no-prelude attribute (RFC 501)" rust#20561
  • "Tracking Issue for -Z terminal-width" rust#84673
  • "Increase the minimum linux-gnu versions" rust#95026
  • "Remove mutable_borrow_reservation_conflict lint and allow the code pattern" rust#96268
• Things in FCP (make sure you're good with it)
  • No FCP requests this time.
• Accepted MCPs
  • "Add support for MIR opt unit tests via a new -Z flag" compiler-team#502
  • "Remove -Zast-json" compiler-team#503
  • "Add TyKind::Alias to keep type alias information" compiler-team#504
  • "try to not rely on attributes from extern crates" compiler-team#505
• Finalized FCPs (disposition merge)
  • "Check if call return type is visibly uninhabited when building MIR" rust#93313
  • "Fix constants not getting dropped if part of a diverging expression" rust#94775
  • "[let_chains] Forbid let inside parentheses" rust#95008

WG checkins

• @_WG-rustc-dev-guide Rustc Dev Guide by @Santiago Pastorino and @Yuki Okushi|217081 (previous checkin):

Most notable changes

• sessiondiagnostic: translation #1333
• Add example how lints can be feature gated #1332
• update section for type system constants #1329

Most notable WIPs

• rewrite bootstrapping stages #1327
• Document ErrorGuaranteed #1316
• Edit "What the compiler does to your code" #1306
• Describe Type Alias Impl Trait (TAIT) Inference Algorithm #1297
• Added detail to codegen section #1216
• Update build instructions for rustdoc #1117
• Document inert vs active attributes #1110
• Explain the new valtree system for type level constants. #1097

• @_WG-traits (impl trait) Traits (impl trait) by @nikomatsakis @oli (previous checkin):

• type alias impl trait refactoring has landed
• Implementationd PRs for all 3 remaining soundness bugs have been opened
• next steps: review all open bugs and triage them to see if they are stabilization blockers
  T-ypes deep dive meeting next week friday: the new scheme for determining hidden types of opaque types

Backport nominations

T-compiler beta / T-compiler stable

• :beta: "Remove NodeIdHashingMode." rust#95656
  • nominated by @Wesley Wiser
  • Should fix rust#95945, a ICE on incremental compilation
• No stable nominations for T-compiler this time.

T-rustdoc beta / T-rustdoc stable

• No backport nominations for T-rustdoc this time.

/ /

PRs S-waiting-on-team

T-compiler

• No PRs waiting on T-compiler this time.

Oldest PRs waiting for review

T-compiler

• "Fix incorrect suggestion for trait bounds involving binary operators" rust#94034 (last review activity: 2 months ago)
  • previously mentioned, @oli and @Esteban Küber could follow-up on this
• "Handle generic bounds in a uniform way in HIR" rust#93803 (last review activity: about 58 days ago)
  • cc: @Esteban Küber
• "Remove all json handling from rustc_serialize" rust#85993 (last review activity: about 54 days ago)
  • update: PR depends on MCP #503, now closed
  • cc: @Aaron Hill
• "rustc_apfloat: Double::mul_add_r panic with specific values" rust#93225 (last review activity: about 47 days ago)
  • @pnkfelix and @Wesley Wiser discussed licensing issue about the the LLVM's APFloat library rust#55993
  • Wesley mentioned a follow-up with Core team / Foundation (comment)
• "Better error message for _ in function signature in impl Trait for Ty" rust#95395 (last review activity: about 23 days ago)
  • Bot assigned @Wesley Wiser as reviewer, reroll dice?
• "Add Apple WatchOS compile targets" rust#95243 (last review activity: about 23 days ago)
  • PR needs a reviewer
• "Only output DepKind in dump-dep-graph." rust#95434 (last review activity: about 22 days ago)
  • PR needs a reviewer

Issues of Note

Short Summary

• 0 T-compiler P-critical issues
  • 0 of those are unassigned
• 62 T-compiler P-high issues
  • 48 of those are unassigned
• 0 P-critical, 2 P-high, 2 P-medium, 0 P-low regression-from-stable-to-beta
• 0 P-critical, 0 P-high, 1 P-medium, 0 P-low regression-from-stable-to-nightly
• 0 P-critical, 40 P-high, 82 P-medium, 10 P-low regression-from-stable-to-stable

P-critical

T-compiler

• No P-critical issues for T-compiler this time.

T-rustdoc

• No P-critical issues for T-rustdoc this time.

P-high regressions

P-high beta regressions

• "Rust beta: "cannot infer type" when compiling bottom crate" rust#96074
  • regression reportedly breaks at least compiling one real world crate (bottom)
  • confirmed P-high?
• "regression: TLS linker issues" rust#96132
  • confirmed P-high?
  • Bisection points to rust#94373 (cc: @erikdesjardins)
  • @simulacrum suggests to revert at least for 1.61

Unassigned P-high nightly regressions

• No unassigned P-high nightly regressions this time.

Performance logs

triage logs for 2022-04-19

A rough week, if only in terms of the sheer number of PRs that were flagged as regressions. Going through 31 regressive PR's, 13 of them rollups, is not fun.
There were some nice wins from e.g. #95968 and #95981.
The main worrisome regression is a 1% compile-time from #96010 that seems like it was not expected.

Triage done by @pnkfelix.
Revision range: 4e1927db..4ca19e09

7 Regressions, 12 Improvements, 24 Mixed; 13 of them in rollups
51 artifact comparisons made in total
30 Untriaged Pull Requests

Regressions

Rollup of 7 pull requests #95966 (Comparison Link)

• left comment: Unclear cause, fairly small regression, and rollup: not worth the time to investigate – benefit/cost tradeoff is not worth it.

Rollup of 4 pull requests #95999 (Comparison Link)

• left comment: I don't see any reason why the PRs in this rollup would have anything to do with its associated regression.

Use mir constant in thir instead of ty::Const #94255 (Comparison Link)

• left comment: An earlier perf run, on I think the same commit series as what was eventually merged, showed no primary regressions at all.
• A number of secondary benchmarks regressed, but they are all stress test microbenchmarks, and should not block landing work of this nature.
• So, I'm chalking the minor amount of regression that was observed up to noise.

Update stdarch #95958 (Comparison Link)

• left comments. Only one regression but it wasn't clear why this PR would cause it.
• Subsequent attempt to cachegrind diff the two rustc's indicate that there may be some amount of noise at play here. Marked as triaged.

Rollup of 7 pull requests #96123 (Comparison Link)

• six secondary regressions, all in variants of match-stress and all on the order of 0.48%.
• left comment: narrow scope, secondary benchmark, small regression, and rollup: not worth investigating further.

Rollup of 5 pull requests #96178 (Comparison Link)

• regressed syn-opt-1.0.89 opt full by 2.5%.
• The only PR on the list in this rollup that I could imagine having any effect at all on performance is PR #96156.
• looked at graph, seems like benchmark became more noisy recently, and this is part of the variance injected by that noise.

Add slice::remainder #92287 (Comparison Link)

• regressed externs (a secondary benchmark) incr-full in debug and opt profiles by 1.2%.
• left comment; based on cachegrind output and the perf graph, this does not seem to be a persistent regression. We do not need to worry about it.

Improvements

• Rollup of 5 pull requests #95974 (Comparison Link)
• errors: lazily load fallback fluent bundle #95968 (Comparison Link)
• Optimize <SourceFile as Decodable>::decode #95981 (Comparison Link)
• Remove <mbe::TokenTree as Clone> #95928 (Comparison Link)
• Allow self-profiler to only record potentially costly arguments when argument recording is turned on #95689 (Comparison Link)
• Fix x test --doc --stage 0 library/std #95993 (Comparison Link)
• Rollup of 6 pull requests #96134 (Comparison Link)
• Speed up Vec::clear(). #96002 (Comparison Link)
• fix error handling for pthread_sigmask(3) #93530 (Comparison Link)
• Respect ranlib specified for target during LLVM build #96080 (Comparison Link)
• Require all paths passed to ShouldRun::paths to exist on disk #95906 (Comparison Link)
• show suggestion to replace generic bounds with associated types in more cases #95379 (Comparison Link)

Mixed

Skip Lazy for some metadata tables #95867 (Comparison Link)

• already triaged by the PR author.

Rollup of 4 pull requests #95987 (Comparison Link)

• the big hit here was to tt-muncher (a secondary benchmark), which regressed on 10 different configurations, by amounts varying from 1.4% up to 3.0%.
• the cachegrind diff for the 3.0% hit is here: https://gist.github.com/7674ad03c8f72705eb5bd2651202f40a
• that cachegrind diff leads pnkfelix to think that PR #95794: "parse_tt: a few more tweaks" is the cause of the regression, specifically commit 4ba609601f1a99ddf3cf0cf70f57c4a045f0f23f
• however, that PR went through multiple rounds of performance evaluation.
• in any case, looking at the graph for tt-muncher indicates that whatever performance hit was suffered, it was subsequently resolved by PR #95928.

rustdoc: Reduce allocations in a markdown function #95905 (Comparison Link)

• the primary regressions were to externs incr-full, debug and opt variants.
• from looking at the graph, there may be a faint upward trend on externs as a whole, but pnkfelix does not think this PR caused any direct regression.

Rollup of 7 pull requests #95990 (Comparison Link)

• This PR regressed diesel doc by 4.1%, and associated-items doc by 71% (!). Seems important.
• It seems likely that the rustdoc regression is due to PR #95316.
• Left comment on PR #95316; not marking rollup as triaged for now.

Remove NodeIdHashingMode. #95656 (Comparison Link)

• Already marked as triaged since it addressed significant footgun in incr-comp; see comment from mw

Rollup of 6 pull requests #96015 (Comparison Link)

• only secondary regressions, and majority are to ctfe-stress-5 incr-unchanged (on all of check,debug,opt), on the order of 1.1%.
• did local cachegrind run on check build: https://gist.github.com/4be85f17d74ee6bf2c92efcd922a6fc9
• regression seems to be blamed upon rustc_data_structures::intern::Interned<rustc_middle::mir::interpret::allocation::Allocation> as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable
• but nothing in the rollup PR seems like it could possibly have had an impact there.
• the graph of ctfe-stress-5-check does seem like there has been some gradual regression over time, starting around 2022-04-09.
• pnkfelix would not blame that on this rollup PR, though.

Update cargo #96031 (Comparison Link)

• Regressions are just to secondary benchmarks and pnkfelix thinks benefits of a cargo update outweigh the costs presented here, such as they are.

library: Move CStr to libcore, and CString to liballoc #94079 (Comparison Link)

• Report claims this Caused a 0.8% regression on primary benchmark unicode-normalization-0.1.19 (check full).
• I was not able to reproduce the regression locally; all my runs showed an improvement.
• This may be running afoul of rust-lang/rustc-perf#1299

Rollup of 11 pull requests #96087 (Comparison Link)

• Report claims this Caused a 0.8% regression on primary benchmark unicode-normalization-0.1.19 (check full).
• This, like other PR's this week, may be running afoul of rust-lang/rustc-perf#1299

Optimize RcInnerPtr::inc_strong()/inc_weak() instruction count #95224 (Comparison Link)

• The bulk of this regression is attached to secondary benchmark regression-31157 opt (incr-patched: println, incr-full, full), by up to 3.86%
• the self-profile data seems to indicate that the bulk of the cost here is spent in LLVM.
• the cachegrind output seconds that: https://gist.github.com/4fa7403ee2e812e7712c5046e9eb4d72
• This PR is motivated by an improvement to the object code itself generated by the compiler. I guess that improvement did not result in a net win for the compiler itself.
• regression-31157 (rust#31157) is encoding a case where we were seeing a 20x slowdown. So we should not be worrying about 2% or 4% performance losses there.

Only check the compiler and standard library before documenting them (take 2) #95450 (Comparison Link)

• hypothesized to be noise and pnkfelix doesn't have time to dig more deeply.
• left comment, marked as triaged.

Better method call error messages #92364 (Comparison Link)

• The main regression here was to externs, but as previously noted, there's a lot of historical noise in the data for externs, and not trustworthy.

Rollup of 9 pull requests #96108 (Comparison Link)

• This observed 1% regression on ctfe-stress-5.
• Much like PR #96015, I do not think I would put individual blame on this rollup PR for the problems we may be seeing over time in ctfe-stress-5.

Rollup of 7 pull requests #96117 (Comparison Link)

• This observed 1% regression on ctfe-stress-5.
• Much like PR #96015 and PR #96108, I do not think I would put individual blame on this rollup PR for the problems we may be seeing over time in ctfe-stress-5.

rustc_metadata: Do not encode unnecessary module children #95899 (Comparison Link)

• This observed 1% regression on ctfe-stress-5.
• Much like PR #96015, #96108, and #96117, I do not think I would put individual blame on this PR for the problems we may be seeing over time in ctfe-stress-5.

Implement core::ptr::Unique on top of NonNull #96010 (Comparison Link)

• This seems like it introduced a regression into the compilation times for a lot of important crates.
• I'm not clear on what its buying us, it seems like internal code cleanup? That doesn't justify a >=1% compile-time regression to webrender and syn.

Refactor HIR item-like traversal (part 1) #95655 (Comparison Link)

• This is an expected regression and has already been marked as triaged.

Remove last vestiges of skippng ident span hashing #96016 (Comparison Link)

• This was already marked as triaged, but at the time it was marked, the only regressions were to secondary benchmarks.
• The version that landed flagged a 2.6% regression to primary benchmark syn-1.0.89
• (self-profile says its due to extra time spent in LLVM).
• anyway the historical data makes me think this is just noise.

Report undeclared lifetimes during late resolution. #95779 (Comparison Link)

• These performance regressions were anticipated (via measurement) during review of the PR and effectively already triaged.

Fix rustdoc duplicated blanket impls #96091 (Comparison Link)

• The main hit is to externs, which I'm rejecting as ignorable this week due to extant noise.

Revert: Make TLS __getit #[inline(always)] on non-Windows #96139 (Comparison Link)

• Unfortunately this injected a 3.3% hit to syn-1.0.89, …
• … but it is also fixing a beta-regression, which takes priority here.
• anyway its already marked as triaged by simulacrum.

Make x test --stage 2 compiler/rustc_XXX faster to run #96000 (Comparison Link)

• This was flagged as injecting a 1.5% regression on syn-1.0.89.
• It seems like syn-1.0.89 opt full suddenly became much noisier around 2022-04-17, and I do not know why.

Micro-optimize ty::relate::relate_substs by avoiding match #96020 (Comparison Link)

• Already triaged by nnethercote.

Rollup of 6 pull requests #96214 (Comparison Link)

• regressed deeply-nested-multi incr-unchanged by 1% to 1.5%.
• the bulk of the regression here seems to already be blamable on PR #96020. Why is this rollup PR getting flagged separately? (filed a bug against rustc-perf for this.)

Nominated Issues

T-compiler

• "Disable unwinding for emscripten again" rust#95950
  • nominated by @nagisa (comment), suggests a write-up to take a decision and try to stick with it (comment)
  • cc: PR author @Jules Bertholet, suggests adding a feature-gate (comment)

RFC

• No I-compiler-nominated RFCs this time.