Skip to content

Pre-compute MIR CFG caches for borrowck and other analyses #142540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Pre-compute MIR CFG caches for borrowck and other analyses #142540

wants to merge 3 commits into from
+15 −6

Conversation

cjgillot
Copy link
Contributor

I was puzzled that #142390 introduces additional computations of CFG traversals: borrowck computes them, right?

It turns out that borrowck clones the MIR body, so doesn't share its cache with other analyses.

This PR:

  • forces the computation of all caches in mir_promoted query;
  • modifies region renumbering to avoid dropping that cache.

@rustbot
Copy link
Collaborator

rustbot commented Jun 15, 2025

r? @fee1-dead

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

@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 Jun 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 15, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot
Copy link
Contributor Author

@bors2 try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors
Copy link

rust-bors bot commented Jun 15, 2025

⌛ Trying commit e7e8f6d with merge 18218cc…

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 15, 2025
@rust-bors
Auto merge of #142540 - cjgillot:renumber-cfg, r=<try>
18218cc 
Pre-compute MIR CFG caches for borrowck and other analyses

I was puzzled that #142390 introduces additional computations of CFG traversals: borrowck computes them, right?

It turns out that borrowck clones the MIR body, so doesn't share its cache with other analyses.

This PR:
- forces the computation of all caches in `mir_promoted` query;
- modifies region renumbering to avoid dropping that cache.

<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 15, 2025
@rust-bors
Copy link

rust-bors bot commented Jun 15, 2025

☀️ Try build successful (CI)
Build commit: 18218cc (18218cc5c664c3521f4769f46ef5e5e10128c92c, parent: 75e7cf5f85aad82331a38deff24845b63eaf30f3)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (18218cc): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.5% [0.2%, 2.9%] 68
Regressions ❌
(secondary)		 0.7% [0.2%, 1.8%] 47
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.5% [0.2%, 2.9%] 68

Max RSS (memory usage)

Results (primary 1.7%, secondary 1.3%)

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

mean range count
Regressions ❌
(primary)		 1.7% [0.9%, 2.8%] 12
Regressions ❌
(secondary)		 2.5% [2.0%, 3.0%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.2% [-1.2%, -1.2%] 1
All ❌✅ (primary) 1.7% [0.9%, 2.8%] 12

Cycles

Results (primary 1.1%)

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

mean range count
Regressions ❌
(primary)		 1.1% [0.7%, 1.4%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.1% [0.7%, 1.4%] 2

Binary size

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

Bootstrap: 756.494s -> 756.086s (-0.05%)
Artifact size: 372.15 MiB -> 372.18 MiB (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 15, 2025
tmiasko
tmiasko reviewed Jun 15, 2025
View reviewed changes
compiler/rustc_middle/src/mir/basic_blocks.rs Outdated
Comment on lines 57 to 60
let _ = self.predecessors();
let _ = self.switch_sources();
let _ = self.reverse_postorder();
let _ = self.dominators();
Copy link
Contributor

Choose a reason for hiding this comment

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

  • switch_sources seems to be only used by DestProp, DeadStoreElimination, and StateTransform.
  • In the past it was an improvement to evaluate dominators in borrowck in a lazy manner.

@cjgillot
Copy link
Contributor Author

Perf deserves investigation. I really didn't expect mir_borrowck to get slower.

@cjgillot
Copy link
Contributor Author

@bors2 try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors
Copy link

rust-bors bot commented Jun 15, 2025

⌛ Trying commit 3e77d40 with merge 98e2e6a…

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 15, 2025
@rust-bors
Auto merge of #142540 - cjgillot:renumber-cfg, r=<try>
98e2e6a 
Pre-compute MIR CFG caches for borrowck and other analyses

I was puzzled that #142390 introduces additional computations of CFG traversals: borrowck computes them, right?

It turns out that borrowck clones the MIR body, so doesn't share its cache with other analyses.

This PR:
- forces the computation of all caches in `mir_promoted` query;
- modifies region renumbering to avoid dropping that cache.

<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 15, 2025
@rust-bors
Copy link

rust-bors bot commented Jun 15, 2025

☀️ Try build successful (CI)
Build commit: 98e2e6a (98e2e6aea9abc7e12f76e27d72aced95f45b5260, parent: 586ad391f5ee4519acc7cae340e34673bae762b1)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (98e2e6a): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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)		 1.8% [1.0%, 2.6%] 4
Regressions ❌
(secondary)		 1.2% [0.6%, 1.6%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.8% [1.0%, 2.6%] 4

Max RSS (memory usage)

Results (primary 0.1%, secondary -2.5%)

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

mean range count
Regressions ❌
(primary)		 3.1% [0.5%, 6.5%] 3
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.4% [-1.8%, -0.8%] 6
Improvements ✅
(secondary)		 -2.5% [-2.7%, -2.2%] 2
All ❌✅ (primary) 0.1% [-1.8%, 6.5%] 9

Cycles

Results (secondary 1.2%)

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

Binary size

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

Bootstrap: 755.712s -> 754.846s (-0.11%)
Artifact size: 372.06 MiB -> 372.09 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmiasko tmiasko tmiasko left review comments

Assignees

@fee1-dead fee1-dead

Labels
perf-regression Performance regression. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjgillot @rustbot @rust-timer @tmiasko @fee1-dead