Only disable cache if predicate has opaques within it #132625

compiler-errors · 2024-11-05T03:17:29Z

This is an alternative to #132075.

This refines the check implemented in #126024 to only disable the global cache if the predicate being considered has opaques in it. This is still theoretically unsound, since goals can indirectly rely on opaques in the defining scope, but we're much less likely to hit it.

It doesn't totally fix #132064: for example, lemmy goes from 1:29 (on rust 1.81) to 9:53 (on nightly) to 4:07 (after this PR). But I think it's at least more sound than a total revert :/

r? lcnr

compiler-errors · 2024-11-05T03:25:44Z

@bors try @rust-timer queue

bors · 2024-11-05T03:26:54Z

⌛ Trying commit e6a1578 with merge 26e9516...

…, r=<try> Only disable cache if predicate has opaques within it This is an alternative to rust-lang#132075. It doesn't totally fix the issue, for example: `lemmy` goes from 1:29 (on rust 1.81) to 9:53 (on nightly) to 4:07 (after this PR). But I think it's at least *more* sound than a total revert :/ vibes??? r? lcnr

bors · 2024-11-05T05:21:42Z

☀️ Try build successful - checks-actions
Build commit: 26e9516 (26e9516df6889da22da9d6b0ba840be75724b5c4)

rust-timer · 2024-11-05T06:42:39Z

Finished benchmarking commit (26e9516): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.4%	[0.4%, 0.4%]	2
Improvements ✅ (primary)	-3.1%	[-5.0%, -1.5%]	6
Improvements ✅ (secondary)	-3.3%	[-5.0%, -0.4%]	9
All ❌✅ (primary)	-3.1%	[-5.0%, -1.5%]	6

Max RSS (memory usage)

Results (primary 1.2%, 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)	1.2%	[0.9%, 1.4%]	2
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	1.2%	[0.9%, 1.4%]	2

Cycles

Results (primary -2.4%, secondary -4.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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.4%	[-3.9%, -1.2%]	6
Improvements ✅ (secondary)	-4.6%	[-5.3%, -3.9%]	6
All ❌✅ (primary)	-2.4%	[-3.9%, -1.2%]	6

Binary size

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

Bootstrap: 780.214s -> 780.319s (0.01%)
Artifact size: 335.23 MiB -> 335.32 MiB (0.02%)

lcnr · 2024-11-05T15:35:57Z

while this isn't entirely correct if an opaque is not directly referenced by the obligation but ends up being used, e.g. as the return type of an FnDef or via an impl referencing a TAIT, I don't worry much about this causing issues on stable and we can look into more involved fixes (e.g. track the defined opaques in the global cache key instead) before stabilizing TAIT.

so r=me after updating the description + src to not just be HACK: hack

lqd · 2024-11-05T15:57:59Z

I don't worry much about this causing issues on stable

would reverting #126024 cause issues on stable? (I ask because of the reported 4x-10x regressions to compilation times it caused)

lcnr · 2024-11-05T16:45:06Z

I don't worry much about this causing issues on stable

would reverting #126024 cause issues on stable? (I ask because of the reported 4x-10x regressions to compilation times it caused)

in theory the incorrect caching should be triggered a lot more easily when reverting #126024 compared to the approach in this PR. It should still be fairly hard to trigger. I do have an idea on how to hopefully fully fix the regression

lcnr · 2024-11-05T16:46:07Z

compiler/rustc_trait_selection/src/traits/select/mod.rs

-            TypingMode::Analysis { defining_opaque_types } => defining_opaque_types.is_empty(),
+            // HACK: hack
+            TypingMode::Analysis { defining_opaque_types } => {
+                defining_opaque_types.is_empty() || !pred.has_opaque_types()


given that we use can_use_global_caches both on access and on write, could we change this to instead check that pred does not contain an opaque from defining_opaque_types, i.e. it only contains rigid opaque types

On stable pretty much the only time you observe opaque types in their defining scope it for recursive RPIT, so I expect the slowdown to be caused by this triggering for opaques we can't actually define rn

I actually tested that and it doesn't improve perf very much lol

yeah perf only goes from 4:07 to 4:04 when i implement a simple visitor to check if it contains an opaque in the defines list

yeah perf only goes from 4:07 to 4:04 when i implement a simple visitor to check if it contains an opaque in the defines list

that seems weird to me 🤔 what's causing the regression here, reuse between different MIR borrowck queries?

lcnr · 2024-11-05T17:37:30Z

compiler/rustc_trait_selection/src/traits/select/mod.rs

-            TypingMode::Analysis { defining_opaque_types } => defining_opaque_types.is_empty(),
+            //
+            // HACK: This is still theoretically unsound. Goals can indirectly rely
+            // on opaquesvin the defining scope, and it's easier to do so with TAIT.


Suggested change

// on opaquesvin the defining scope, and it's easier to do so with TAIT.

// on opaques in the defining scope, and it's easier to do so with TAIT.

compiler-errors · 2024-11-06T20:53:41Z

@bors r=lcnr

bors · 2024-11-06T20:53:43Z

📌 Commit 4915373 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-11-06T21:22:18Z

⌛ Testing commit 4915373 with merge c07aa1e...

bors · 2024-11-07T00:07:51Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing c07aa1e to master...

rust-timer · 2024-11-07T02:19:28Z

Finished benchmarking commit (c07aa1e): 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)	-2.7%	[-5.0%, -0.2%]	7
Improvements ✅ (secondary)	-3.0%	[-5.1%, -0.3%]	10
All ❌✅ (primary)	-2.7%	[-5.0%, -0.2%]	7

Max RSS (memory usage)

Results (primary 1.7%, secondary 4.4%)

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.7%	[1.5%, 1.9%]	2
Regressions ❌ (secondary)	4.4%	[4.4%, 4.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[1.5%, 1.9%]	2

Cycles

Results (primary -2.4%, secondary -5.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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.4%	[-3.7%, -1.0%]	7
Improvements ✅ (secondary)	-5.6%	[-7.6%, -2.5%]	14
All ❌✅ (primary)	-2.4%	[-3.7%, -1.0%]	7

Binary size

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

Bootstrap: 781.244s -> 780.618s (-0.08%)
Artifact size: 335.32 MiB -> 335.27 MiB (-0.01%)

apiraino · 2024-11-14T15:35:13Z

Beta and stable backports accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

Stable backport also approved, though we don't see this alone as a reason to open a dot release.

@rustbot label +beta-accepted +stable-accepted

[beta] backports - Use completion item indices instead of property matching rust-lang#132987, rust-lang/rust-analyzer#18503 - Reject raw lifetime followed by `'`, like regular lifetimes do rust-lang#132341 - Only disable cache if predicate has opaques within it rust-lang#132625 - rustdoc-search: case-sensitive only when capitals are used rust-lang#133043 r? cuviper

[beta] backports - Use completion item indices instead of property matching rust-lang#132987, rust-lang/rust-analyzer#18503 - Reject raw lifetime followed by `'`, like regular lifetimes do rust-lang#132341 - Only disable cache if predicate has opaques within it rust-lang#132625 - rustdoc-search: case-sensitive only when capitals are used rust-lang#133043 - (ci) Update macOS Xcode to 15 rust-lang#131570 r? cuviper

rustbot assigned lcnr Nov 5, 2024

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 Nov 5, 2024

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 Nov 5, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 5, 2024

lcnr reviewed Nov 5, 2024

View reviewed changes

compiler-errors force-pushed the cache-only-if-opaque branch from e6a1578 to 4779c16 Compare November 5, 2024 17:09

lcnr reviewed Nov 5, 2024

View reviewed changes

Only disable cache if predicate has opaques within it

4915373

compiler-errors force-pushed the cache-only-if-opaque branch from 4779c16 to 4915373 Compare November 5, 2024 17:38

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 Nov 6, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 7, 2024

bors merged commit c07aa1e into rust-lang:master Nov 7, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Nov 7, 2024

bors mentioned this pull request Nov 7, 2024

Revert #126024 "Do not use global caches if opaque types can be defined" #132075

Closed

rustbot removed the perf-regression Performance regression. label Nov 7, 2024

jieyouxu mentioned this pull request Nov 7, 2024

regression: crate compiles much slower with 1.82 #132064

Open

compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 9, 2024

apiraino added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Nov 14, 2024

rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Nov 14, 2024

cuviper mentioned this pull request Nov 15, 2024

[beta] backports #133077

Merged

cuviper modified the milestones: 1.84.0, 1.83.0 Nov 15, 2024

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only disable cache if predicate has opaques within it #132625

Only disable cache if predicate has opaques within it #132625

compiler-errors commented Nov 5, 2024 •

edited

Loading

compiler-errors commented Nov 5, 2024

This comment has been minimized.

bors commented Nov 5, 2024

bors commented Nov 5, 2024

This comment has been minimized.

rust-timer commented Nov 5, 2024

lcnr commented Nov 5, 2024 •

edited

Loading

lqd commented Nov 5, 2024 •

edited

Loading

lcnr commented Nov 5, 2024

lcnr Nov 5, 2024 •

edited

Loading

compiler-errors Nov 5, 2024

compiler-errors Nov 5, 2024

lcnr Nov 5, 2024 •

edited

Loading

lcnr Nov 5, 2024

compiler-errors Nov 5, 2024

compiler-errors commented Nov 6, 2024

bors commented Nov 6, 2024

bors commented Nov 6, 2024

bors commented Nov 7, 2024

rust-timer commented Nov 7, 2024

apiraino commented Nov 14, 2024

	// on opaquesvin the defining scope, and it's easier to do so with TAIT.
	// on opaques in the defining scope, and it's easier to do so with TAIT.

Only disable cache if predicate has opaques within it #132625

Only disable cache if predicate has opaques within it #132625

Conversation

compiler-errors commented Nov 5, 2024 • edited Loading

compiler-errors commented Nov 5, 2024

This comment has been minimized.

bors commented Nov 5, 2024

bors commented Nov 5, 2024

This comment has been minimized.

rust-timer commented Nov 5, 2024

Overall result: ❌✅ regressions and improvements - please read the text below

lcnr commented Nov 5, 2024 • edited Loading

lqd commented Nov 5, 2024 • edited Loading

lcnr commented Nov 5, 2024

lcnr Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Nov 6, 2024

bors commented Nov 6, 2024

bors commented Nov 6, 2024

bors commented Nov 7, 2024

rust-timer commented Nov 7, 2024

Overall result: ✅ improvements - no action needed

apiraino commented Nov 14, 2024

compiler-errors commented Nov 5, 2024 •

edited

Loading

lcnr commented Nov 5, 2024 •

edited

Loading

lqd commented Nov 5, 2024 •

edited

Loading

lcnr Nov 5, 2024 •

edited

Loading

lcnr Nov 5, 2024 •

edited

Loading