Merge mir query analysis invocations #140856

oli-obk · 2025-05-09T15:43:40Z

same thing as #140854 just a different set of queries

Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.

oli-obk · 2025-05-09T15:43:50Z

@bors try @rust-timer queue

Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries

bors · 2025-05-09T15:45:01Z

⌛ Trying commit d63b394 with merge bddede8...

bors · 2025-05-09T17:51:01Z

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

rust-timer · 2025-05-09T19:21:27Z

Finished benchmarking commit (bddede8): comparison URL.

Overall result: ❌ regressions - 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.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.9%, secondary -2.2%)

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.7%	[0.4%, 1.1%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-2.7%, -0.5%]	20
Improvements ✅ (secondary)	-2.2%	[-4.0%, -0.4%]	2
All ❌✅ (primary)	-0.9%	[-2.7%, 1.1%]	23

Cycles

Results (primary 1.5%, secondary 0.2%)

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.8%	[0.4%, 3.9%]	26
Regressions ❌ (secondary)	4.5%	[3.2%, 5.4%]	10
Improvements ✅ (primary)	-0.8%	[-1.3%, -0.5%]	3
Improvements ✅ (secondary)	-7.0%	[-7.8%, -6.3%]	6
All ❌✅ (primary)	1.5%	[-1.3%, 3.9%]	29

Binary size

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

Bootstrap: 773.559s -> 774.531s (0.13%)
Artifact size: 365.71 MiB -> 365.58 MiB (-0.03%)

oli-obk · 2025-05-09T19:40:11Z

@Zoxc do you think changes like this are detrimental to parallel rustc?

Zoxc · 2025-05-09T20:52:09Z

Merging parallel sections is generally good, but actual measurements are also nice.

oli-obk · 2025-05-10T07:39:39Z

Right. I'll figure out how to do that locally. But until then

r? @nnethercote

oli-obk · 2025-05-10T07:40:26Z

Ah no I forgot the regression only shows in cycles, not in instructions...

oli-obk · 2025-05-10T07:42:50Z

Branch misses and cache misses are also really badly regressed. So my original analysis still stands, even if wall time itself is not really affected

nnethercote · 2025-05-14T05:56:07Z

compiler/rustc_hir_analysis/src/lib.rs

@@ -233,6 +222,17 @@ pub fn check_crate(tcx: TyCtxt<'_>) {
        }
    });

+    if tcx.features().rustc_attrs() {


Why dump attrs stuff later? I'm not opposed, just there's no motivation given.

I wanted to make space there to merge the parallel blocks before and after

nnethercote · 2025-05-14T05:58:13Z

Branch misses and cache misses are also really badly regressed. So my original analysis still stands, even if wall time itself is not really affected

Can you remind me what the original analysis is? I've lost track of the motivation for this PR. Is it for code simplification, performance, both/something else? Thanks.

oli-obk · 2025-05-14T06:15:21Z

Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.

oli-obk · 2025-05-14T06:20:44Z

The reasons I'm doing it is that

merging those blocks allows for more parallelism as you don't run parallel blocks in sequence
merging blocks allows merging analysis queries shrinking the dep graph
should allow us to do more early aborting in case of errors and/or moving query calls from the analysis query into others that allow early aborting the others (and doing more tainting and stuff)

nnethercote · 2025-05-14T06:22:28Z

The reasons I'm doing it is that

Aha, that helps a lot. Please put that explanation in the commit message for the third commit, and then r=me, thanks.

The reasons I'm doing it is that * merging those blocks allows for more parallelism as you don't run parallel blocks in sequence * merging blocks allows merging analysis queries shrinking the dep graph * should allow us to do more early aborting in case of errors and/or moving query calls from the analysis query into others that allow early aborting the others (and doing more tainting and stuff)

oli-obk · 2025-05-14T08:05:39Z

@bors r=nnethercote

bors · 2025-05-14T08:05:42Z

📌 Commit e011c43 has been approved by nnethercote

It is now in the queue for this repository.

oli-obk · 2025-05-14T10:49:25Z

@bors retry

oli-obk · 2025-05-14T10:49:39Z

@bors rollup=never

Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec<QueryResult> lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.

bors · 2025-05-14T12:03:04Z

⌛ Testing commit e011c43 with merge d8ccc90...

bors · 2025-05-14T14:07:30Z

💔 Test failed - checks-actions

clubby789 · 2025-05-14T14:17:19Z

System.IO.IOException: There is not enough space on the disk - @bors retry

Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec<QueryResult> lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.

bors · 2025-05-14T20:38:02Z

⌛ Testing commit e011c43 with merge ae6350f...

rust-log-analyzer · 2025-05-14T20:48:53Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2025-05-14T20:49:59Z

💔 Test failed - checks-actions

oli-obk · 2025-05-17T12:47:04Z

@bors retry

bors · 2025-05-17T22:31:47Z

⌛ Testing commit e011c43 with merge 16244bc...

bors · 2025-05-18T01:29:42Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 16244bc to master...

github-actions · 2025-05-18T01:32:20Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 777d372 (parent) -> 16244bc (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 16244bc37a6ff610daa38afcddfc1a8bb226702e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

x86_64-apple-1: 7371.2s -> 9885.6s (34.1%)
dist-apple-various: 6014.7s -> 7659.6s (27.3%)
x86_64-apple-2: 4665.0s -> 4171.6s (-10.6%)
dist-arm-linux: 4966.9s -> 4503.5s (-9.3%)
x86_64-gnu-aux: 6386.8s -> 5905.9s (-7.5%)
x86_64-mingw-2: 6744.0s -> 7194.8s (6.7%)
x86_64-gnu-nopt: 5471.5s -> 5833.5s (6.6%)
x86_64-gnu-stable: 7103.7s -> 6676.9s (-6.0%)
x86_64-gnu-llvm-19-1: 5231.5s -> 5506.9s (5.3%)
dist-x86_64-apple: 7108.3s -> 7476.2s (5.2%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-05-18T04:25:34Z

Finished benchmarking commit (16244bc): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	5
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

Max RSS (memory usage)

Results (primary -3.1%, secondary 0.1%)

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.0%	[3.0%, 3.1%]	2
Improvements ✅ (primary)	-3.1%	[-6.8%, -1.2%]	3
Improvements ✅ (secondary)	-0.9%	[-1.8%, -0.5%]	6
All ❌✅ (primary)	-3.1%	[-6.8%, -1.2%]	3

Cycles

Results (primary 1.9%, secondary 2.6%)

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.9%	[0.9%, 3.7%]	18
Regressions ❌ (secondary)	2.6%	[0.6%, 4.8%]	18
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[0.9%, 3.7%]	18

Binary size

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

Bootstrap: 775.816s -> 777.625s (0.23%)
Artifact size: 365.47 MiB -> 365.43 MiB (-0.01%)

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 May 9, 2025

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 May 9, 2025

bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2025

Auto merge of rust-lang#140856 - oli-obk:merge-queries2, r=<try>

bddede8

Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries

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 May 9, 2025

oli-obk marked this pull request as ready for review May 10, 2025 07:39

rustbot assigned nnethercote May 10, 2025

oli-obk added 2 commits May 13, 2025 08:46

Some require_lang_item -> is_lang_item replacements

bd58700

Run rustc_attrs dumps after typeck

c86e33b

nnethercote reviewed May 14, 2025

View reviewed changes

oli-obk force-pushed the merge-queries2 branch from d63b394 to e011c43 Compare May 14, 2025 08:05

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2025

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2025

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 May 14, 2025

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

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 May 14, 2025

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

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 May 17, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2025

bors merged commit 16244bc into rust-lang:master May 18, 2025
7 checks passed

rustbot added this to the 1.89.0 milestone May 18, 2025

bors mentioned this pull request May 18, 2025

Infer nounwind and use it in MIR opts #130909

Draft

Merge mir query analysis invocations #140856

Merge mir query analysis invocations #140856

Uh oh!

Conversation

oli-obk commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented May 9, 2025

Uh oh!

This comment has been minimized.

bors commented May 9, 2025

Uh oh!

bors commented May 9, 2025

Uh oh!

This comment has been minimized.

rust-timer commented May 9, 2025

Overall result: ❌ regressions - no action needed

Uh oh!

oli-obk commented May 9, 2025

Uh oh!

Zoxc commented May 9, 2025

Uh oh!

oli-obk commented May 10, 2025

Uh oh!

oli-obk commented May 10, 2025

Uh oh!

oli-obk commented May 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented May 14, 2025

Uh oh!

oli-obk commented May 14, 2025

Uh oh!

oli-obk commented May 14, 2025

Uh oh!

nnethercote commented May 14, 2025

Uh oh!

oli-obk commented May 14, 2025

Uh oh!

bors commented May 14, 2025

Uh oh!

oli-obk commented May 14, 2025

Uh oh!

oli-obk commented May 14, 2025

Uh oh!

bors commented May 14, 2025

Uh oh!

bors commented May 14, 2025

Uh oh!

clubby789 commented May 14, 2025

Uh oh!

bors commented May 14, 2025

Uh oh!

rust-log-analyzer commented May 14, 2025

Uh oh!

bors commented May 14, 2025

Uh oh!

oli-obk commented May 17, 2025

Uh oh!

bors commented May 17, 2025

Uh oh!

bors commented May 18, 2025

Uh oh!

Uh oh!

github-actions bot commented May 18, 2025

Test differences

Job duration changes

Uh oh!

rust-timer commented May 18, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

Uh oh!

oli-obk commented May 9, 2025 •

edited

Loading