mir_build: Avoid some useless work when visiting "primary" bindings #137465

Zalathar · 2025-02-23T07:17:38Z

While looking over visit_primary_bindings, I noticed that it does a bunch of extra work to build up a collection of “user-type projections”, even though 2/3 of its call sites don't even use them. Those callers can get the same result via thir::Pat::walk_always.

(And it turns out that doing so also avoids creating some redundant user-type entries in MIR for some binding constructs.)

I also noticed that even when the user-type projections are used, the process of building them ends up eagerly cloning some nested vectors at every recursion step, even in cases where they won't be used because the current subpattern has no bindings. To avoid this, the visit method now assembles a linked list on the stack containing the information that would be needed to create projections, and only creates the concrete projections as needed when a primary binding is encountered.

Some relevant prior PRs:

There should be no user-visible change in compiler output.

rustbot · 2025-02-23T07:17:46Z

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Zalathar · 2025-02-23T07:34:03Z

My earlier drafts didn't have any measurable perf effect, but let's try the real thing just in case:

@bors try @rust-timer queue

bors · 2025-02-23T07:35:14Z

⌛ Trying commit 1d8a6e7 with merge cde7127...

mir_build: Avoid some useless work when visiting "primary" bindings While looking over `visit_primary_bindings`, I noticed that it does a bunch of extra work to build up a collection of “user-type projections”, even though 2/3 of its call sites don't even use them. Those callers can get the same result via `thir::Pat::walk_always`. (And it turns out that doing so also avoids creating some redundant user-type entries in MIR for some binding constructs.) I also noticed that even when the user-type projections *are* used, the process of building them ends up eagerly cloning some nested vectors at every recursion step, even in cases where they won't be used because the current subpattern has no bindings. To avoid this, the visit method now assembles a linked list on the stack containing the information that *would* be needed to create projections, and only creates the concrete projections as needed when a primary binding is encountered. Some relevant prior PRs: - rust-lang#55274 - rust-lang@0bfe184 in rust-lang#55937

bors · 2025-02-23T09:35:55Z

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

rust-timer · 2025-02-23T10:51:38Z

Finished benchmarking commit (cde7127): 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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.3%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -4.0%)

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)	-4.0%	[-4.0%, -4.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.304s -> 770.384s (0.01%)
Artifact size: 359.67 MiB -> 359.64 MiB (-0.01%)

Zalathar · 2025-02-23T11:25:29Z

@bors rollup=maybe

Zalathar · 2025-03-01T01:43:12Z

(Rebased; no changes.)

Zalathar · 2025-03-01T01:47:31Z

compiler/rustc_mir_build/src/builder/matches/mod.rs

+    /// onto `canonical_user_type_annotations`, so that they end up in MIR
+    /// even if they aren't associated with any bindings.
+    #[instrument(level = "debug", skip(self, f))]
+    fn visit_primary_bindings_special(


I couldn't come up with a good name for this method, so I punted and went with something that signals “please read the docs for this weird method instead of just calling it”.

One of the problems with the old name (visit_primary_bindings) was that it sounded like a very normal visitor method, when in fact it was also doing some non-trivial extra stuff, and I think that's how we ended up in a situation where it was being called in other places that didn't need the extra stuff.

rustbot · 2025-03-14T10:02:52Z

Some changes occurred in match lowering

cc @Nadrieril

bors · 2025-03-15T12:30:23Z

☔ The latest upstream changes (presumably #138464) made this pull request unmergeable. Please resolve the merge conflicts.

This avoids the need to unwrap an option after ensuring that it is some.

The existing method does some non-obvious extra work to collect user types and build user-type projections, which is specifically needed by `declare_bindings` and not by the other two callers.

Zalathar · 2025-03-16T01:17:44Z

Rebased to resolve trivial import conflict.

compiler/rustc_mir_build/src/builder/matches/mod.rs

oli-obk · 2025-03-17T09:58:16Z

compiler/rustc_mir_build/src/builder/matches/user_ty.rs

+        let ops_reversed = self.iter_ops_reversed().cloned().collect::<Vec<_>>();
+        // The "first" op should always be `PushUserType`.
+        // Other projections are only added if there is at least one user type.
+        assert_matches!(ops_reversed.last(), Some(ProjectedUserTypesOp::PushUserType { .. }));
+
+        let mut projections = vec![];
+        for op in ops_reversed.into_iter().rev() {


you could avoid this rev-collect-rev by just asserting that projections are not empty when pushing projections on them

Hmm, I don't think that's right.

The ops need to be traversed in “forwards” order because each projection needs to be applied to all types that have already been pushed, but not to any types that haven't been pushed yet.

ah right, we can't iterate the other direction unless we use a recursive function to walk the linked list, which has its own problems

(This is somewhat academic, because currently I don't think it's possible for surface syntax to give us a pattern binding within multiple nested user-type-ascriptions. But the existing code handles the fully-general case of multiple ascriptions.)

oli-obk · 2025-03-17T10:17:24Z

@bors r+

bors · 2025-03-17T10:17:28Z

📌 Commit 5434242 has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133870 (Stabilize `asm_goto` feature gate) - rust-lang#137449 (Denote `ControlFlow` as `#[must_use]`) - rust-lang#137465 (mir_build: Avoid some useless work when visiting "primary" bindings) - rust-lang#138349 (Emit function declarations for functions with `#[linkage="extern_weak"]`) - rust-lang#138412 (Install licenses into `share/doc/rust/licenses`) - rust-lang#138577 (rustdoc-json: Don't also include `#[deprecated]` in `Item::attrs`) - rust-lang#138588 (Avoid double lowering of idents) Failed merges: - rust-lang#138321 ([bootstrap] Distribute split debuginfo if present) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#137465 - Zalathar:visit-primary, r=oli-obk mir_build: Avoid some useless work when visiting "primary" bindings While looking over `visit_primary_bindings`, I noticed that it does a bunch of extra work to build up a collection of “user-type projections”, even though 2/3 of its call sites don't even use them. Those callers can get the same result via `thir::Pat::walk_always`. (And it turns out that doing so also avoids creating some redundant user-type entries in MIR for some binding constructs.) I also noticed that even when the user-type projections *are* used, the process of building them ends up eagerly cloning some nested vectors at every recursion step, even in cases where they won't be used because the current subpattern has no bindings. To avoid this, the visit method now assembles a linked list on the stack containing the information that *would* be needed to create projections, and only creates the concrete projections as needed when a primary binding is encountered. Some relevant prior PRs: - rust-lang#55274 - rust-lang@0bfe184 in rust-lang#55937 --- There should be no user-visible change in compiler output.

rustbot assigned cjgillot Feb 23, 2025

rustbot added S-waiting-on-review T-compiler labels Feb 23, 2025

Zalathar force-pushed the visit-primary branch from e7489a4 to 1d8a6e7 Compare February 23, 2025 07:18

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Feb 23, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf label Feb 23, 2025

Zalathar force-pushed the visit-primary branch from 1d8a6e7 to 2abd47e Compare March 1, 2025 01:43

Zalathar commented Mar 1, 2025

View reviewed changes

Zalathar force-pushed the visit-primary branch from 2abd47e to b12eb54 Compare March 14, 2025 10:02

Zalathar added 4 commits March 16, 2025 12:10

Add a mir-opt test that demonstrates user type annotations

bca5f56

Simplify handling of visibility_scope in declare_bindings

977106a

This avoids the need to unwrap an option after ensuring that it is some.

Split visit_primary_bindings into two variants

7805b46

The existing method does some non-obvious extra work to collect user types and build user-type projections, which is specifically needed by `declare_bindings` and not by the other two callers.

Build UserTypeProjections lazily when visiting bindings

5434242

Zalathar force-pushed the visit-primary branch from b12eb54 to 5434242 Compare March 16, 2025 01:17

oli-obk reviewed Mar 17, 2025

View reviewed changes

compiler/rustc_mir_build/src/builder/matches/mod.rs Show resolved Hide resolved

oli-obk reviewed Mar 17, 2025

View reviewed changes

oli-obk self-assigned this Mar 17, 2025

oli-obk unassigned cjgillot Mar 17, 2025

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 17, 2025

matthiaskrgr mentioned this pull request Mar 17, 2025

Rollup of 7 pull requests #138611

Merged

bors merged commit fd4ad33 into rust-lang:master Mar 17, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 17, 2025

Zalathar deleted the visit-primary branch March 17, 2025 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mir_build: Avoid some useless work when visiting "primary" bindings #137465

mir_build: Avoid some useless work when visiting "primary" bindings #137465

Zalathar commented Feb 23, 2025 •

edited

Loading

rustbot commented Feb 23, 2025

Zalathar commented Feb 23, 2025

This comment has been minimized.

bors commented Feb 23, 2025

bors commented Feb 23, 2025

This comment has been minimized.

rust-timer commented Feb 23, 2025

Zalathar commented Feb 23, 2025

Zalathar commented Mar 1, 2025

Zalathar Mar 1, 2025

rustbot commented Mar 14, 2025

bors commented Mar 15, 2025

Zalathar commented Mar 16, 2025

oli-obk Mar 17, 2025

Zalathar Mar 17, 2025

oli-obk Mar 17, 2025

Zalathar Mar 17, 2025

oli-obk commented Mar 17, 2025

bors commented Mar 17, 2025

mir_build: Avoid some useless work when visiting "primary" bindings #137465

mir_build: Avoid some useless work when visiting "primary" bindings #137465

Conversation

Zalathar commented Feb 23, 2025 • edited Loading

rustbot commented Feb 23, 2025

Zalathar commented Feb 23, 2025

This comment has been minimized.

bors commented Feb 23, 2025

bors commented Feb 23, 2025

This comment has been minimized.

rust-timer commented Feb 23, 2025

Overall result: ✅ improvements - no action needed

Zalathar commented Feb 23, 2025

Zalathar commented Mar 1, 2025

Choose a reason for hiding this comment

rustbot commented Mar 14, 2025

bors commented Mar 15, 2025

Zalathar commented Mar 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 17, 2025

bors commented Mar 17, 2025

Zalathar commented Feb 23, 2025 •

edited

Loading