Skip to content

Simplify MaybeRequiresStorage - #160055

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
nnethercote:simplify-MaybeRequiresStorage
Jul 29, 2026
Merged

Simplify MaybeRequiresStorage#160055
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
nnethercote:simplify-MaybeRequiresStorage

Conversation

@nnethercote

@nnethercote nnethercote commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

It currently contains a MaybeBorrowedLocals cursor. The cursor is within a RefCell because cursor traversal requires mutation. The cursor is used in the MaybeRequiresStorage::check_for_move operation.

Instead of using this cursor within MaybeRequiresStorage it's possible to do a pre-traversal of MaybeBorrowedLocals to extract the necessary information and then give that (immutably) to MaybeRequiresStorage.

This commit makes that change. The extracted information is in the new KillableLocals type, which is computed by the KillableLocalsVisitor type within MaybeRequiresStorage::new. MaybeRequiresStorage no longer needs lifetimes. MoveVisitor is no longer needed. And the complicated locals_live_across_suspend_points gets a little simpler.

r? @cjgillot

@rustbot

rustbot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot 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 Jul 28, 2026
@nnethercote

nnethercote commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Unlikely to affect perf, but let's check:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 28, 2026
rust-bors Bot pushed a commit that referenced this pull request Jul 28, 2026
@bors
Auto merge of #160055 - nnethercote:simplify-MaybeRequiresStorage, r=…
865fdae 
…<try>

Simplify `MaybeRequiresStorage`
@rust-bors

rust-bors Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 865fdae (865fdae870aacc207f236ab7493331f9ebb7e663)
Base parent: bf9944f (bf9944f0b8006b152ef4d5f408ae75a0dde3d044)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (865fdae): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.4% [-0.6%, -0.2%] 7
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -0.4%, secondary 2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 4.4% [3.2%, 5.5%] 2
Regressions ❌
(secondary)		 5.8% [5.8%, 5.8%] 1
Improvements ✅
(primary)		 -10.0% [-10.0%, -10.0%] 1
Improvements ✅
(secondary)		 -1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) -0.4% [-10.0%, 5.5%] 3

Cycles

Results (primary 0.6%, secondary 4.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 3.5% [3.5%, 3.5%] 1
Regressions ❌
(secondary)		 4.4% [4.4%, 4.4%] 1
Improvements ✅
(primary)		 -2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.6% [-2.4%, 3.5%] 2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.856s -> 487.985s (0.03%)
Artifact size: 388.05 MiB -> 388.09 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 28, 2026
cjgillot
cjgillot reviewed Jul 28, 2026
View reviewed changes
Comment thread compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs
@cjgillot

cjgillot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

So much cleaner

r=me with a nit

@nnethercote
Simplify MaybeRequiresStorage
0f0b9f8 
It currently contains a `MaybeBorrowedLocals` cursor. The cursor is
within a `RefCell` because cursor traversal requires mutation. The
cursor is used in the `MaybeRequiresStorage::check_for_move` operation.

Instead of using this cursor within `MaybeRequiresStorage` it's possible
to do a pre-traversal of `MaybeBorrowedLocals` to extract the necessary
information and then give that (immutably) to `MaybeRequiresStorage`.

This commit makes that change. The extracted information is in the new
`KillableLocals` type, which is computed by the `KillableLocalsVisitor`
type within `MaybeRequiresStorage::new`. `MaybeRequiresStorage` no
longer needs lifetimes. `MoveVisitor` is no longer needed. And the
complicated `locals_live_across_suspend_points` gets a little simpler.
@nnethercote
nnethercote force-pushed the simplify-MaybeRequiresStorage branch from 176271d to 0f0b9f8 Compare July 28, 2026 20:58
@nnethercote

nnethercote commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

I added an extra comment.

@bors r=cjgillot

@rust-bors

rust-bors Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 0f0b9f8 has been approved by cjgillot

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Jul 28, 2026
@nnethercote

nnethercote commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@bors rollup=iffy (because of very small perf improvements in two secondary benchmarks)

@jhpratt jhpratt mentioned this pull request Jul 29, 2026
Merged
rust-bors Bot pushed a commit that referenced this pull request Jul 29, 2026
@bors
Auto merge of #160112 - jhpratt:rollup-6M1V5yA, r=jhpratt
c5f1c73 
Rollup of 11 pull requests

Successful merges:

 - #158168 (Added implementation on `set_permissions_nofollow` for all primary platforms)
 - #160055 (Simplify `MaybeRequiresStorage`)
 - #157226 (Partially stabilize `box_vec_non_null`)
 - #158879 (simplify `slice::Iter[Mut]::next_chunk` implementation)
 - #159413 (Enable `#[diagnostic::on_unknown]` during late res)
 - #160091 (Fix rustdoc toolbar height when title is taller than one line)
 - #158615 (fix: don't fire `explicit_outlives_requirements` on `?Sized` type params)
 - #159666 (fix(ld64.lld): route version mismatch warnings to linker_info on macOS)
 - #160032 (rustdoc-json: Make `Stability` compatible with non-self-describing serde formats)
 - #160039 (Add regression test for enum unconstrained parameter )
 - #160049 (Use assert_eq! in splat codegen tests)
rust-bors Bot pushed a commit that referenced this pull request Jul 29, 2026
@bors
Auto merge of #160112 - jhpratt:rollup-6M1V5yA, r=jhpratt
ce69831 
Rollup of 11 pull requests

Successful merges:

 - #158168 (Added implementation on `set_permissions_nofollow` for all primary platforms)
 - #160055 (Simplify `MaybeRequiresStorage`)
 - #157226 (Partially stabilize `box_vec_non_null`)
 - #158879 (simplify `slice::Iter[Mut]::next_chunk` implementation)
 - #159413 (Enable `#[diagnostic::on_unknown]` during late res)
 - #160091 (Fix rustdoc toolbar height when title is taller than one line)
 - #158615 (fix: don't fire `explicit_outlives_requirements` on `?Sized` type params)
 - #159666 (fix(ld64.lld): route version mismatch warnings to linker_info on macOS)
 - #160032 (rustdoc-json: Make `Stability` compatible with non-self-describing serde formats)
 - #160039 (Add regression test for enum unconstrained parameter )
 - #160049 (Use assert_eq! in splat codegen tests)
@rust-bors
rust-bors Bot merged commit 108fa3e into rust-lang:main Jul 29, 2026
13 checks passed
@rustbot rustbot added this to the 1.99.0 milestone Jul 29, 2026
rust-timer added a commit that referenced this pull request Jul 29, 2026
@rust-timer
Unrolled build for #160055
17d5429 
Rollup merge of #160055 - nnethercote:simplify-MaybeRequiresStorage, r=cjgillot

Simplify `MaybeRequiresStorage`

It currently contains a `MaybeBorrowedLocals` cursor. The cursor is within a `RefCell` because cursor traversal requires mutation. The cursor is used in the `MaybeRequiresStorage::check_for_move` operation.

Instead of using this cursor within `MaybeRequiresStorage` it's possible to do a pre-traversal of `MaybeBorrowedLocals` to extract the necessary information and then give that (immutably) to `MaybeRequiresStorage`.

This commit makes that change. The extracted information is in the new `KillableLocals` type, which is computed by the `KillableLocalsVisitor` type within `MaybeRequiresStorage::new`. `MaybeRequiresStorage` no longer needs lifetimes. `MoveVisitor` is no longer needed. And the complicated `locals_live_across_suspend_points` gets a little simpler.

r? @cjgillot
@nnethercote
nnethercote deleted the simplify-MaybeRequiresStorage branch July 29, 2026 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.99.0

Development

Successfully merging this pull request may close these issues.

4 participants

@nnethercote @rustbot @rust-timer @cjgillot