Skip to content

Avoid cloning CFG-related analysis data. - #156841

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
dianqk:avoid-clone
Oct 23, 2026
Merged

Avoid cloning CFG-related analysis data.#156841
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
dianqk:avoid-clone

Conversation

@dianqk

@dianqk dianqk commented May 22, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

View all comments

For perf. We can avoid cloning CFG-related data while mutating the body.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 22, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@dianqk

dianqk commented May 22, 2026

Copy link
Copy Markdown
Member Author

@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 May 22, 2026
rust-bors Bot pushed a commit that referenced this pull request May 22, 2026
@bors
Auto merge of #156841 - dianqk:avoid-clone, r=<try>
218c186 
Avoid cloning CFG-related analysis data.
@rust-bors

rust-bors Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 218c186 (218c18693cbb9ae14f53b685355647f5dbeb1e44, parent: b52edc25bfbaa955b4b83c10f998e5224c3478b2)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented May 22, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (218c186): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@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.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 1

Max RSS (memory usage)

Results (primary -0.1%, secondary -2.1%)

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

mean range count
Regressions ❌
(primary)		 7.9% [7.9%, 7.9%] 1
Regressions ❌
(secondary)		 2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)		 -2.8% [-3.6%, -2.3%] 3
Improvements ✅
(secondary)		 -4.4% [-7.1%, -1.7%] 2
All ❌✅ (primary) -0.1% [-3.6%, 7.9%] 4

Cycles

Results (primary 2.2%, secondary -4.2%)

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

mean range count
Regressions ❌
(primary)		 2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.2% [-4.3%, -4.0%] 3
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Binary size

Results (primary 0.1%, secondary 0.1%)

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

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.1%] 51
Regressions ❌
(secondary)		 0.1% [0.0%, 0.1%] 25
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.1% [0.0%, 0.1%] 51

Bootstrap: 513.181s -> 524.394s (2.18%)
Artifact size: 400.52 MiB -> 400.58 MiB (0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 22, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@dianqk
dianqk force-pushed the avoid-clone branch 2 times, most recently from 7b31218 to 46b3903 Compare October 20, 2026 11:02
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@dianqk

dianqk commented Oct 20, 2026

Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

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 Oct 20, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Oct 20, 2026
@bors
Auto merge of #156841 - dianqk:avoid-clone, r=<try>
6e7d17c 
Avoid cloning CFG-related analysis data.
@rust-bors

rust-bors Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 6e7d17c (6e7d17c98a451b9e7b87a3ebad826f733bffcc11)
Base parent: a7c37f9 (a7c37f9f0ee5e93396106e9de20f4a86b4154e63)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (6e7d17c): comparison URL.

Overall result: no relevant changes - 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

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

Max RSS (memory usage)

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

Cycles

Results (primary -2.6%, secondary -1.5%)

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

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.9% [0.4%, 1.4%] 2
Improvements ✅
(primary)		 -2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)		 -2.5% [-6.6%, -0.5%] 5
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Binary size

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

Bootstrap: 491.717s -> 493.723s (0.41%)
Artifact size: 408.99 MiB -> 408.95 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 21, 2026
@dianqk
dianqk marked this pull request as ready for review October 21, 2026 00:47
@rustbot

rustbot commented Oct 21, 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. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2026
@rustbot

rustbot commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

r? @mu001999

rustbot has assigned @mu001999.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, mir
  • compiler, mir expanded to 77 candidates
  • Random selection from 19 candidates

mu001999
mu001999 reviewed Oct 21, 2026
View reviewed changes
Comment thread compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs Outdated
Comment thread compiler/rustc_middle/src/mir/basic_blocks.rs Outdated

impl CfgCache {
#[inline]
pub fn dominators(&self, bbs: &'_ BasicBlocks<'_>) -> &Dominators<BasicBlock> {

@mu001999 mu001999 Oct 21, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way to avoid cloning? If we expose CfgCache, we can only rely on callers to ensure that the bbs passed in is the one that cache belongs to. 🤔

View changes since the review

@dianqk dianqk Oct 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a way to keep using a mutable instance while only reading an immutable part of its data. In most cases we only have one body per pass, so I don't think we can easily make that mistake.
The alternative I can think of is to use the immutable body and collect whatever we want to modify — that's MirPatch — but it's not easy to use everywhere.

@mu001999 mu001999 Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can easily make that mistake.

Agreed.

Just to be safe, perhaps we could change the receiver to self: &Arc<Self> and add a debug_assert!(Arc::ptr_eq(self, &bbs.cache)); for these methods?

@dianqk dianqk Oct 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dianqk
dianqk force-pushed the avoid-clone branch 2 times, most recently from c363acf to 5bfb2bf Compare October 22, 2026 10:53
mu001999
mu001999 approved these changes Oct 22, 2026
View reviewed changes

@mu001999 mu001999 left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bors r+ rollup

View changes since this review

@rust-bors

rust-bors Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 222c583 has been tentatively approved by mu001999

It will be put into the queue for this repository once PR CI succeeds.

@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 Oct 22, 2026
@JonathanBrouwer JonathanBrouwer mentioned this pull request Oct 22, 2026
Merged
rust-bors Bot pushed a commit that referenced this pull request Oct 22, 2026
@bors
Auto merge of #164757 - JonathanBrouwer:rollup-QMS3ewH, r=JonathanBro…
b8fa1fb 
…uwer

Rollup of 12 pull requests

Successful merges:

 - #164458 (Avoid stdlib's `impl Hash for Path` for stable hashing)
 - #161790 (update to illumos sysroot 20210501)
 - #163087 (rmake: Add helpers for iterating symbols and objects)
 - #163586 (refactor Windows remove_dir_all)
 - #163983 (std: use `SOCK_NONBLOCK` in `connect_timeout`)
 - #164307 (Rename `PostTypeckUntilBorrowck` -> `PostTypeckThroughBorrowck`)
 - #164425 (Declare `SyntaxContext` through `rustc_index::newtype_index!`)
 - #156841 (Avoid cloning CFG-related analysis data.)
 - #164654 (Add reference annotations for unions)
 - #164717 (Allow `llm-assisted` label to be set without write access)
 - #164735 (Remove docs mentioning instability of wasm64)
 - #164737 (Update LLVM PassBuilder constructor calls to pass a context)

Failed merges:

 - #164636 (Remove `record_*` macros from the metadata encoder)
rust-bors Bot pushed a commit that referenced this pull request Oct 23, 2026
@bors
Auto merge of #164757 - JonathanBrouwer:rollup-QMS3ewH, r=JonathanBro…
eb022b1 
…uwer

Rollup of 12 pull requests

Successful merges:

 - #164458 (Avoid stdlib's `impl Hash for Path` for stable hashing)
 - #161790 (update to illumos sysroot 20210501)
 - #163087 (rmake: Add helpers for iterating symbols and objects)
 - #163586 (refactor Windows remove_dir_all)
 - #163983 (std: use `SOCK_NONBLOCK` in `connect_timeout`)
 - #164307 (Rename `PostTypeckUntilBorrowck` -> `PostTypeckThroughBorrowck`)
 - #164425 (Declare `SyntaxContext` through `rustc_index::newtype_index!`)
 - #156841 (Avoid cloning CFG-related analysis data.)
 - #164654 (Add reference annotations for unions)
 - #164717 (Allow `llm-assisted` label to be set without write access)
 - #164735 (Remove docs mentioning instability of wasm64)
 - #164737 (Update LLVM PassBuilder constructor calls to pass a context)

Failed merges:

 - #164636 (Remove `record_*` macros from the metadata encoder)
rust-bors Bot pushed a commit that referenced this pull request Oct 23, 2026
@bors
Auto merge of #164757 - JonathanBrouwer:rollup-QMS3ewH, r=JonathanBro…
0cefa27 
…uwer

Rollup of 12 pull requests

Successful merges:

 - #164458 (Avoid stdlib's `impl Hash for Path` for stable hashing)
 - #161790 (update to illumos sysroot 20210501)
 - #163087 (rmake: Add helpers for iterating symbols and objects)
 - #163586 (refactor Windows remove_dir_all)
 - #163983 (std: use `SOCK_NONBLOCK` in `connect_timeout`)
 - #164307 (Rename `PostTypeckUntilBorrowck` -> `PostTypeckThroughBorrowck`)
 - #164425 (Declare `SyntaxContext` through `rustc_index::newtype_index!`)
 - #156841 (Avoid cloning CFG-related analysis data.)
 - #164654 (Add reference annotations for unions)
 - #164717 (Allow `llm-assisted` label to be set without write access)
 - #164735 (Remove docs mentioning instability of wasm64)
 - #164737 (Update LLVM PassBuilder constructor calls to pass a context)

Failed merges:

 - #164636 (Remove `record_*` macros from the metadata encoder)
@rust-bors
rust-bors Bot merged commit 9b007cb into rust-lang:main Oct 23, 2026
14 checks passed
@rustbot rustbot added this to the 1.101.0 milestone Oct 23, 2026
rust-bors Bot pushed a commit that referenced this pull request Oct 23, 2026
@bors
Unrolled build for #156841 in rollup 164757
ced830a 
Rollup merge of #156841 - dianqk:avoid-clone, r=mu001999

Avoid cloning CFG-related analysis data.

For perf. We can avoid cloning CFG-related data while mutating the body.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mu001999 mu001999 mu001999 approved these changes

Assignees

@mu001999 mu001999

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.101.0

Development

Successfully merging this pull request may close these issues.

5 participants

@dianqk @rust-log-analyzer @rust-timer @rustbot @mu001999