Don't allocate on SimplifyCfg/Locals/Const on every MIR pass #110477

miguelraz · 2023-04-18T03:21:43Z

Hey! 👋🏾 This is a first PR attempt to see if I could speed up some rustc internals.

Thought process:

pub struct SimplifyCfg {
    label: String,
}

in compiler/src/rustc_mir_transform/simplify.rs fires multiple times per MIR analysis. This means that a likely string allocation is happening in each of these runs, which may add up, as they are not being lazily allocated or cached in between the different passes.

...yes, I know that adding a global static array is probably not the future-proof solution, but I wanted to lob this now as a proof of concept to see if it's worth shaving off a few cycles and then making more robust.

rustbot · 2023-04-18T03:21:50Z

r? @compiler-errors

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

rustbot · 2023-04-18T03:21:52Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2023-04-18T03:28:18Z

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.

jyn514 · 2023-04-18T03:28:22Z

@bors try @rust-timer queue

bors · 2023-04-18T03:28:31Z

⌛ Trying commit ccc7a0c1af6f58b3081024583d3cfdbcfb4f3434 with merge b89ba5d787d15245a9be6ac6f1619c153b23ea97...

compiler/rustc_mir_transform/src/simplify.rs

bors · 2023-04-18T05:12:30Z

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

rust-timer · 2023-04-18T06:29:02Z

Finished benchmarking commit (b89ba5d787d15245a9be6ac6f1619c153b23ea97): 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

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.7%	[1.7%, 1.7%]	1
Regressions ❌ (secondary)	2.3%	[2.0%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-4.1%, -4.1%]	1
All ❌✅ (primary)	1.7%	[1.7%, 1.7%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-4.7%, -2.7%]	3
All ❌✅ (primary)	-	-	0

compiler-errors · 2023-04-18T06:45:29Z

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?)

workingjubilee · 2023-04-18T07:06:04Z

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

compiler-errors · 2023-04-18T07:12:28Z

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",
    }
}

JakobDegen · 2023-04-18T07:40:49Z

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

workingjubilee · 2023-04-18T07:54:41Z

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.

Noratrieb · 2023-04-18T08:26:52Z

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!

Noratrieb · 2023-04-18T09:06:11Z

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

lqd · 2023-04-18T09:11:48Z

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.

cjgillot · 2023-04-18T17:39:47Z

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?

miguelraz · 2023-04-18T18:24:37Z

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

compiler-errors

@miguelraz can you squash this into one commit? Other than that, this PR looks good to go.

jyn514 · 2023-04-18T18:38:04Z

@bors r=compiler-errors

bors · 2023-04-18T18:38:06Z

📌 Commit fc27ae1 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-04-19T02:57:22Z

⌛ Testing commit fc27ae1 with merge 9e7f72c...

JakobDegen · 2023-04-19T04:04:43Z

compiler/rustc_mir_transform/src/simplify_branches.rs

- pub fn new(label: &str) -> Self {
- SimplifyConstCondition { label: format!("SimplifyConstCondition-{}", label) }
- }
+pub enum SimplifyConstConditionPassName {


(nit) no need to block the PR on this, but if you touch this code again in the future could you rename this to just SimplifyConstCondition? That would make it consistent with all the other ones

Thanks for this, finally got around to this fix.
#110657

bors · 2023-04-19T05:08:47Z

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

rust-timer · 2023-04-19T06:49:25Z

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

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.5%	[1.9%, 3.1%]	2
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	0.5%	[-3.6%, 3.1%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.3%	[2.7%, 3.6%]	4
Improvements ✅ (primary)	-2.9%	[-4.6%, -1.5%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.9%	[-4.6%, -1.5%]	8

miguelraz · 2023-04-21T21:27:12Z

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

…ctor, r=compiler-errors nit: consistent naming for SimplifyConstCondition Fixing a small naming inconsistency that `@JakobDegen` brought up in rust-lang#110477 (comment). Please signal for rollup.

rustbot assigned compiler-errors Apr 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2023

miguelraz changed the title ~~try interning SimplifyCfg strings~~ Don't allocate on SimplifyCfg/Locals/Const on every MIR pass Apr 18, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 18, 2023

Noratrieb reviewed Apr 18, 2023

View reviewed changes

compiler/rustc_mir_transform/src/simplify.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Apr 18, 2023

View reviewed changes

compiler/rustc_mir_transform/src/simplify.rs Outdated Show resolved Hide resolved

compiler/rustc_mir_transform/src/simplify.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 18, 2023

compiler-errors approved these changes Apr 18, 2023

View reviewed changes

refactor SimlifyCfg and friends - no globals, just enums

fc27ae1

miguelraz force-pushed the canoodling2-electric-boogaloo branch from 36b3f70 to fc27ae1 Compare April 18, 2023 18:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2023

JakobDegen reviewed Apr 19, 2023

View reviewed changes

miguelraz mentioned this pull request Apr 19, 2023

simplify.rs smallvecs #110524

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 19, 2023

bors merged commit 9e7f72c into rust-lang:master Apr 19, 2023

rustbot added this to the 1.71.0 milestone Apr 19, 2023

miguelraz deleted the canoodling2-electric-boogaloo branch April 21, 2023 21:27

miguelraz mentioned this pull request Apr 21, 2023

nit: consistent naming for SimplifyConstCondition #110657

Merged

matthiaskrgr mentioned this pull request Nov 20, 2023

ICE: broken MIR in DefId: index of non-array #118111

Closed

matthiaskrgr mentioned this pull request Feb 27, 2024

ICE: assertion failed: Binder == Binder #121688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't allocate on SimplifyCfg/Locals/Const on every MIR pass #110477

Don't allocate on SimplifyCfg/Locals/Const on every MIR pass #110477

miguelraz commented Apr 18, 2023 •

edited

Loading

rustbot commented Apr 18, 2023

rustbot commented Apr 18, 2023

compiler-errors commented Apr 18, 2023 •

edited

Loading

jyn514 commented Apr 18, 2023

This comment has been minimized.

bors commented Apr 18, 2023

bors commented Apr 18, 2023

This comment has been minimized.

rust-timer commented Apr 18, 2023

compiler-errors commented Apr 18, 2023

workingjubilee commented Apr 18, 2023

compiler-errors commented Apr 18, 2023 •

edited

Loading

JakobDegen commented Apr 18, 2023

workingjubilee commented Apr 18, 2023 •

edited

Loading

Noratrieb commented Apr 18, 2023

Noratrieb commented Apr 18, 2023

lqd commented Apr 18, 2023

cjgillot commented Apr 18, 2023

miguelraz commented Apr 18, 2023

compiler-errors left a comment

jyn514 commented Apr 18, 2023

bors commented Apr 18, 2023

bors commented Apr 19, 2023

JakobDegen Apr 19, 2023

miguelraz Apr 21, 2023

bors commented Apr 19, 2023

rust-timer commented Apr 19, 2023

miguelraz commented Apr 21, 2023

Don't allocate on SimplifyCfg/Locals/Const on every MIR pass #110477

Don't allocate on SimplifyCfg/Locals/Const on every MIR pass #110477

Conversation

miguelraz commented Apr 18, 2023 • edited Loading

rustbot commented Apr 18, 2023

rustbot commented Apr 18, 2023

compiler-errors commented Apr 18, 2023 • edited Loading

jyn514 commented Apr 18, 2023

This comment has been minimized.

bors commented Apr 18, 2023

bors commented Apr 18, 2023

This comment has been minimized.

rust-timer commented Apr 18, 2023

Overall result: ✅ improvements - no action needed

compiler-errors commented Apr 18, 2023

workingjubilee commented Apr 18, 2023

compiler-errors commented Apr 18, 2023 • edited Loading

JakobDegen commented Apr 18, 2023

workingjubilee commented Apr 18, 2023 • edited Loading

Noratrieb commented Apr 18, 2023

Noratrieb commented Apr 18, 2023

lqd commented Apr 18, 2023

cjgillot commented Apr 18, 2023

miguelraz commented Apr 18, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

jyn514 commented Apr 18, 2023

bors commented Apr 18, 2023

bors commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 19, 2023

rust-timer commented Apr 19, 2023

Overall result: ✅ improvements - no action needed

miguelraz commented Apr 21, 2023

miguelraz commented Apr 18, 2023 •

edited

Loading

compiler-errors commented Apr 18, 2023 •

edited

Loading

compiler-errors commented Apr 18, 2023 •

edited

Loading

workingjubilee commented Apr 18, 2023 •

edited

Loading