Split overlapping_{inherent,trait}_impls #147502

camsteffen · 2025-10-09T00:04:40Z

This yielded some perf improvement for me. Reduces some calls to impl_trait_header query. But I think the llvm optimization is more relevant.

camsteffen · 2025-10-09T01:00:16Z

@bors2 try @rust-timer queue

rust-bors · 2025-10-09T01:00:19Z

@camsteffen: 🔑 Insufficient privileges: not in try users

chenyukang · 2025-10-09T03:10:40Z

@bors try @rust-timer queue

Split overlapping_{inherent,trait}_impls

rust-bors · 2025-10-09T05:27:10Z

☀️ Try build successful (CI)
Build commit: 9074fa7 (9074fa71ccb33edde162958971faaeefd2f6e3cf, parent: 61efd190243db101d1e47c82c7ec543565c25f62)

rust-timer · 2025-10-09T06:37:24Z

Finished benchmarking commit (9074fa7): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@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.2%	[-0.4%, -0.1%]	17
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	3
All ❌✅ (primary)	-0.2%	[-0.4%, -0.1%]	17

Max RSS (memory usage)

Results (primary -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.6%	[1.6%, 1.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.2%	[-4.2%, -4.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-4.2%, 1.6%]	2

Cycles

Results (secondary -3.3%)

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

Binary size

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

Bootstrap: 473.034s -> 471.072s (-0.41%)
Artifact size: 388.41 MiB -> 388.40 MiB (-0.00%)

fmease · 2025-10-11T12:03:10Z

compiler/rustc_trait_selection/src/traits/coherence.rs

+fn fresh_impl_header<'tcx>(
+    infcx: &InferCtxt<'tcx>,
+    impl_def_id: DefId,
+    is_of_trait: bool,


Have you perchance tried to pass the impl trait refs through to this function all the way from overlapping_trait_impls? Meaning having a signature of

Suggested change

is_of_trait: bool,

impl_trait_ref: Option<ty::EarlyBinder<'tcx, ty::TraitRef<'tcx>>>,

to avoid having to reacquire it down below (thus avoiding the .unwrap()). I'm not asking this from a perf perspective (I presume the green query call overhead is close to negligible) but from a code cleanliness one (tho arguably, my suggestion could be much worse when it comes from to legibility and so on).

If it doesn't inconvenience you too much, could you try that or otherwise assess if that potentially makes things nicer? Idk.

Sure, I think that's a good idea. Even if it's a bit more code. Pushed a commit. Maybe another perf run is in order.

fmease · 2025-10-11T12:07:32Z

Very nice, thanks! I have one question. Once answered, r=me

r? fmease @rustbot author

rustbot · 2025-10-11T12:36:44Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

fmease · 2025-10-11T12:37:10Z

@bors delegate+

bors · 2025-10-11T12:37:13Z

✌️ @camsteffen, you can now approve this pull request!

If @fmease told you to "r=me" after making some further change, please make that change, then do @bors r=@fmease

fmease · 2025-10-11T16:59:35Z

compiler/rustc_trait_selection/src/traits/coherence.rs

+                impl2_def_id,
+                impl1_def_id,
+                trait_refs,


Here the trait refs need to be swapped in accordance with the DefIds above. That's why you see the CI failure.

Maybe instead of passing the two trait refs zipped Option<(_, _)> in a few places, pass them next to the respective DefId. Sth. akin to impl1: (DefId, Option<…>) / (impl1_def_id, impl1_trait_ref): (…, …) or impl1_def_id: DefId, impl1_trait_ref: Option<…>.

I admit this is getting hairier. At any time we can revert back to the bool approach but it might still be interesting to perf the Option<_> approach.

doh! pushed again.

camsteffen · 2025-10-13T04:43:52Z

@bors r=fmease

bors · 2025-10-13T04:43:54Z

📌 Commit 735ddcc has been approved by fmease

It is now in the queue for this repository.

camsteffen · 2025-10-13T04:55:36Z

@bors ping

bors · 2025-10-13T04:55:39Z

😪 I'm awake I'm awake

camsteffen · 2025-10-13T04:58:04Z

@bors p=1 (the queue seems stuck)

camsteffen · 2025-10-13T04:59:06Z

That didn't help

@bors p=0

Split overlapping_{inherent,trait}_impls This yielded some perf improvement for me. Reduces some calls to `impl_trait_header` query. But I think the llvm optimization is more relevant.

bors · 2025-10-13T04:59:15Z

⌛ Testing commit 735ddcc with merge 8b957a3...

camsteffen · 2025-10-13T05:00:01Z

Ope yes it did

rust-log-analyzer · 2025-10-13T05:09:48Z

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

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

27 files already formatted
checking C++ file formatting
spellchecking files
building external tool typos from package [email protected]
error: failed to get `cfg-if` as a dependency of package `ahash v0.8.12`
    ... which satisfies dependency `ahash = "^0.8"` (locked to 0.8.12) of package `typos-cli v1.34.0`

Caused by:
  download of cf/g-/cfg-if failed

Caused by:
  failed to download from `https://index.crates.io/cf/g-/cfg-if`

Caused by:
  [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
rerun tidy with `--extra-checks=spellcheck --bless` to fix typos
tidy [extra_checks]: IO error: cargo install failed
tidy [extra_checks]: FAIL
tidy: The following check failed: extra_checks
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools-bin/rust-tidy /checkout /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo /checkout/obj/build 4 /node/bin/npm --extra-checks=py,cpp,js,spellcheck` failed with exit code 1
Created at: src/bootstrap/src/core/build_steps/tool.rs:1549:23
Executed at: src/bootstrap/src/core/build_steps/test.rs:1280:29

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:03:37
  local time: Mon Oct 13 05:09:35 UTC 2025
  network time: Mon, 13 Oct 2025 05:09:35 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

bors · 2025-10-13T05:10:25Z

💔 Test failed - checks-actions

camsteffen · 2025-10-13T05:27:51Z

@bors retry

Split overlapping_{inherent,trait}_impls This yielded some perf improvement for me. Reduces some calls to `impl_trait_header` query. But I think the llvm optimization is more relevant.

bors · 2025-10-13T09:04:27Z

⌛ Testing commit 735ddcc with merge 20a49a6...

Zalathar · 2025-10-13T09:04:27Z

@bors retry (bors seems to be stuck)

bors · 2025-10-13T09:05:39Z

⌛ Testing commit 735ddcc with merge 956f47c...

bors · 2025-10-13T12:11:58Z

☀️ Test successful - checks-actions
Approved by: fmease
Pushing 956f47c to master...

github-actions · 2025-10-13T12:15:26Z

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 3545698 (parent) -> 956f47c (this PR)

Test differences

Show 5 test diffs

Stage 2

[run-make] tests/run-make/compressed-debuginfo-zstd: ignore (ignored if LLVM wasn't build with zstd for ELF section compression or LLVM is not the default codegen backend) -> pass (J0)

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

Job group index

J0: x86_64-gnu-nopt

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 956f47c32f1bd97b22cd702d7ccf78f0f0d42c34 --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

dist-android: 1094.2s -> 1502.5s (37.3%)
pr-check-1: 2104.9s -> 1441.5s (-31.5%)
dist-aarch64-apple: 8195.6s -> 5878.1s (-28.3%)
x86_64-gnu-tools: 4264.0s -> 3280.3s (-23.1%)
aarch64-apple: 7709.4s -> 9363.2s (21.5%)
x86_64-rust-for-linux: 3601.5s -> 2850.5s (-20.9%)
x86_64-gnu-miri: 5464.7s -> 4331.3s (-20.7%)
arm-android: 7190.9s -> 6024.5s (-16.2%)
dist-x86_64-apple: 7136.2s -> 8213.0s (15.1%)
x86_64-gnu-nopt: 8452.9s -> 7226.5s (-14.5%)

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-10-13T13:24:10Z

Finished benchmarking commit (956f47c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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.2%	[-0.4%, -0.1%]	15
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	4
All ❌✅ (primary)	-0.2%	[-0.4%, -0.1%]	15

Max RSS (memory usage)

Results (primary 1.5%)

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

	mean	range	count
Regressions ❌ (primary)	9.3%	[9.3%, 9.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.4%	[-6.4%, -6.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[-6.4%, 9.3%]	2

Cycles

Results (primary 3.5%)

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

	mean	range	count
Regressions ❌ (primary)	3.5%	[2.2%, 4.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.5%	[2.2%, 4.9%]	2

Binary size

Results (secondary 0.0%)

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

Bootstrap: 474.753s -> 474.655s (-0.02%)
Artifact size: 388.13 MiB -> 388.12 MiB (-0.00%)

Split overlapping_{inherent,trait}_impls

735ddcc

rustbot assigned SparrowLii Oct 9, 2025

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

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 9, 2025

Auto merge of #147502 - camsteffen:split-overlapping-impls, r=<try>

9074fa7

Split overlapping_{inherent,trait}_impls

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 9, 2025

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

camsteffen mentioned this pull request Oct 9, 2025

Limit impl_trait_header query to only trait impls #144607

Merged

fmease approved these changes Oct 11, 2025

View reviewed changes

rustbot assigned fmease and unassigned SparrowLii Oct 11, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2025

camsteffen force-pushed the split-overlapping-impls branch from 3592437 to a1adf25 Compare October 11, 2025 16:10

This comment has been minimized.

Sign in to view

fmease reviewed Oct 11, 2025

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 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 Oct 13, 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 Oct 13, 2025

fmease removed the perf-regression Performance regression. label Oct 13, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2025

bors merged commit 956f47c into rust-lang:master Oct 13, 2025
22 checks passed

rustbot added this to the 1.92.0 milestone Oct 13, 2025

camsteffen deleted the split-overlapping-impls branch October 17, 2025 12:39

	is_of_trait: bool,
	impl_trait_ref: Option<ty::EarlyBinder<'tcx, ty::TraitRef<'tcx>>>,

Split overlapping_{inherent,trait}_impls #147502

Split overlapping_{inherent,trait}_impls #147502

Conversation

camsteffen commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

camsteffen commented Oct 9, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Oct 9, 2025

Uh oh!

chenyukang commented Oct 9, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Oct 9, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Oct 9, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

fmease Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 11, 2025

Uh oh!

fmease commented Oct 11, 2025

Uh oh!

bors commented Oct 11, 2025

Uh oh!

This comment has been minimized.

fmease Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

rust-log-analyzer commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

Zalathar commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

bors commented Oct 13, 2025

Uh oh!

Uh oh!

github-actions bot commented Oct 13, 2025

Test differences

Stage 2

camsteffen commented Oct 9, 2025 •

edited

Loading

fmease Oct 11, 2025 •

edited

Loading

fmease commented Oct 11, 2025 •

edited

Loading

fmease Oct 11, 2025 •

edited

Loading