Don't allocate on SimplifyCfg/Locals/Const on every MIR pass by miguelraz · Pull...

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Do you have any evidence to suggest that these are expensive operations? I don't think this code is really worth the extra complication and possibly introducing new panic edges to the compiler just to avoid some string formatting.

Trying commit ccc7a0c with merge b89ba5d...

Try build successful - checks-actions
Build commit: b89ba5d (b89ba5d787d15245a9be6ac6f1619c153b23ea97)

Finished benchmarking commit (b89ba5d): comparison URL.

Overall result: improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions (primary)	-	-	0
Regressions (secondary)	-	-	0
Improvements (primary)	-0.7%	[-2.8%, -0.2%]	19
Improvements (secondary)	-2.4%	[-7.5%, -0.2%]	17
All (primary)	-0.7%	[-2.8%, -0.2%]	19

Max RSS (memory usage)

Results

Cycles

Results

Apparently keccak and codegen-cranelift have been particularly noisy recently, so I've been advised to ignore those.

As for the other positive perf results, I'll have to take a closer look at them to see if they're legit or just noise as well. (Or maybe I'll queue another perf run in the morning and see if these perf results stick?)

comment worth noting from a sidebar conversation about (current behavior, as it relates to this PR):

[11:57 PM] compilererrors: there are 273521 calls to format! [editor's note: when constructing mir passes] to bootstrap stdlib

Anyways, @miguelraz, I did some thinking. I think the right approach for this would be to make SimplifyCfg/etc's constructors instead take some enum that actually makes the matches you constructed above exhaustive. We can probably store those enums in the mir pass structs instead of a &'static str, then match on them in fn name to turn them into a &'static str.

Something like:

enum SimplifyCfgPassName {
    Initial,
    PromoteConsts,
    ...
}

impl SimplifyCfg {
  fn new(e: SimplifyCfgPassName) -> Self {
    SimplifyCfg { e }
  }

  fn name(&self) -> &'static str {
    match self.e {
      SimplifyCfgPassName::Initial => "SimplifyCfg-initial",
    }
}

The improvement is probably legit. I've seen major regressions in the past resulting from over-calling name and creating string allocations for it

We can probably store those enums in the mir pass structs instead of a &'static str, then match on them in fn name to turn them into a &'static str.

This would also reduce the size from the size of the string ref (pointer and length) to the size of the enum, which will save about 7~15 bytes per instance of this struct that is in memory at any given moment. Not the most important win compared to allocation overhead, but y'know, everything counts in large amounts.

cachegrind results from libc Debug Full:

1,260,425  ???:<rustc_passes::dead::MarkSymbolVisitor as rustc_hir::intravisit::Visitor>::visit_qpath
  888,494  ???:<core::cell::once::OnceCell<bool>>::get_or_try_init::<<core::cell::once::OnceCell<bool>>::get_or_init<<rustc_middle::mir::basic_blocks::BasicBlocks>::is_cfg_cyclic::{closure#0}>::{closure#0}, !>
 -790,524  library/core/src/fmt/mod.rs:core::fmt::write
 -710,531  ???:<rustc_data_structures::graph::iterate::TriColorDepthFirstSearch<rustc_middle::mir::basic_blocks::BasicBlocks>>::run_from_start::<rustc_data_structures::graph::iterate::CycleDetector>
 -659,804  obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/jemalloc-sys-5bd9bcfdf2a83955/out/build/src/arena.c:_rjem_je_arena_ralloc
 -638,356  ???:<rustc_passes::dead::MarkSymbolVisitor>::check_def_id
 -634,657  ./string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_avx_unaligned_erms
 -564,660  library/core/src/fmt/mod.rs:<&mut W as core::fmt::Write>::write_str
 -527,874  ???:<rustc_passes::dead::MarkSymbolVisitor as rustc_hir::intravisit::Visitor>::visit_ty
 -513,184  obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/jemalloc-sys-5bd9bcfdf2a83955/out/build/src/jemalloc.c:do_rallocx
 -508,194  library/core/src/fmt/mod.rs:core::fmt::Formatter::pad
 -435,018  ???:rustc_ast::mut_visit::noop_visit_fn_decl::<rustc_expand::expand::InvocationCollector>
 -417,494  ???:rustc_passes::reachable::has_custom_linkage
 -399,892  ???:<rustc_middle::ty::context::TyCtxt>::def_kind::<rustc_span::def_id::LocalDefId>
 -395,262  library/core/src/fmt/mod.rs:alloc::fmt::format::format_inner

lots of noise, but there are some memcpys and core::fmt, so this looks like a legit improvement. Awesome!

I quickly profiled a few other allocation sites inside simplify and found a few interesting results, it may be worth it to SmallVec a bunch of these: https://hackmd.io/3ViTm3u5QDST-c6mUIyjXg

keccak and cranelift-codegen are indeed noisy right now unfortunately.

Some of the other wins look related to formatting so probably legit.

There shouldn't be many instances of these structs in-flight at the same time, so maybe we wouldn't really see size reduction benefits (nor exhaustiveness for such debugging info), and can e.g. take the name as &'static str and do the Simplify*-$pass concatenations at the 10 or so call-sites instead.

I think the right approach for this would be to make SimplifyCfg/etc's constructors instead take some enum that actually makes the matches you constructed above exhaustive. We can probably store those enums in the mir pass structs instead of a &'static str, then match on them in fn name to turn them into a &'static str.

Even simpler: we can make SimplifyCfg itself an enum, and have fn name match on self?

@cjgillot yes, I just realized that and push that very change, thanks for the tip!

@bors r=compiler-errors

Commit fc27ae1 has been approved by compiler-errors

It is now in the queue for this repository.

Testing commit fc27ae1 with merge 9e7f72c...

Test successful - checks-actions
Approved by: compiler-errors
Pushing 9e7f72c to master...

Finished benchmarking commit (9e7f72c): comparison URL.

Overall result: improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions (primary)	-	-	0
Regressions (secondary)	-	-	0
Improvements (primary)	-0.5%	[-0.7%, -0.3%]	10
Improvements (secondary)	-0.6%	[-0.7%, -0.4%]	9
All (primary)	-0.5%	[-0.7%, -0.3%]	10

Max RSS (memory usage)

Results

Cycles

Results

For the record, this PR was a revived attempt of #108026.