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

oli-obk · 2024-06-05T10:01:20Z

fixes #119272

This is certainly a crude way to make the cache sound wrt opaque types, but since perf lets us get away with this, let's do it in the old solver and let the new solver fix this correctly once and for all.

cc #122192 (comment)

oli-obk · 2024-06-05T10:01:30Z

@bors try @rust-timer queue

…ound_yay, r=<try> Do not use global cache for selection candidates if opaque types can be constrained fixes rust-lang#105787 r? `@ghost` This is certainly the crudest way to make the cache sound wrt opaque types, but if perf lets us get away with this, let's do it in the old solver and let the new solver fix this correctly once and for all. If perf is prohibitively bad, I'll look into alternatives (using canonical queries, checking whether any opaque types got constrained or whether decisions based on the availability of opaque types were made, ..) cc rust-lang#122192 (comment)

bors · 2024-06-05T10:02:40Z

⌛ Trying commit 0714168 with merge 22162a8...

bors · 2024-06-05T11:39:59Z

☀️ Try build successful - checks-actions
Build commit: 22162a8 (22162a8d2d0d3c15a3c43166a998f04a7d83769b)

rust-timer · 2024-06-05T13:11:38Z

Finished benchmarking commit (22162a8): comparison URL.

Overall result: ❌ regressions - 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.

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.5%, 1.5%]	6
Regressions ❌ (secondary)	6.3%	[0.6%, 14.5%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[0.5%, 1.5%]	6

Max RSS (memory usage)

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

Cycles

Results (primary -0.7%, secondary 3.7%)

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)	11.7%	[9.5%, 13.5%]	6
Improvements ✅ (primary)	-0.7%	[-0.8%, -0.7%]	2
Improvements ✅ (secondary)	-2.2%	[-3.1%, -1.2%]	8
All ❌✅ (primary)	-0.7%	[-0.8%, -0.7%]	2

Binary size

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

Bootstrap: 667.402s -> 668.179s (0.12%)
Artifact size: 318.98 MiB -> 318.90 MiB (-0.02%)

oli-obk · 2024-06-05T15:39:19Z

@bors try @rust-timer queue

…ound_yay, r=<try> Do not use global cache for selection candidates if opaque types can be constrained fixes rust-lang#105787 r? `@ghost` This is certainly the crudest way to make the cache sound wrt opaque types, but if perf lets us get away with this, let's do it in the old solver and let the new solver fix this correctly once and for all. If perf is prohibitively bad, I'll look into alternatives (using canonical queries, checking whether any opaque types got constrained or whether decisions based on the availability of opaque types were made, still using the cache for things that can't possibly constrain opaque types (probably sound, famous last words), ..) cc rust-lang#122192 (comment) * [ ] check if this actually fixes rust-lang#105787 or if it just fixes the opaque type reproducer

bors · 2024-06-05T15:40:31Z

⌛ Trying commit 319843a with merge 56d23c2...

bors · 2024-06-05T17:18:31Z

☀️ Try build successful - checks-actions
Build commit: 56d23c2 (56d23c24d45f6d6a5af9529a6d829f8b01b83abd)

rust-timer · 2024-06-05T18:34:55Z

Finished benchmarking commit (56d23c2): 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 a highly reliable metric that was 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.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.5%)

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.5%	[-2.6%, -2.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.6%, -2.4%]	2

Cycles

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

Binary size

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

Bootstrap: 666.65s -> 668.05s (0.21%)
Artifact size: 318.96 MiB -> 319.00 MiB (0.01%)

lcnr · 2024-06-06T10:38:36Z

this is unfortunately still not tracking the current hidden type for the opaque afaict: Projection(<T as Trait>::Assoc, impl Foo) should have a different result on the hidden type of impl Foo.

Have you tried entirely disabling the global cache, only using the one local to the current infcx if we're defining at least one opaque types?

oli-obk · 2024-06-06T11:22:05Z

That would require duplicating the cache data structures, but i'll give it a shot. Just disabling caching entirely was too bad to land #126024 (comment)

oli-obk · 2024-06-06T11:23:49Z

this is unfortunately still not tracking the current hidden type for the opaque afaict: Projection(<T as Trait>::Assoc, impl Foo) should have a different result on the hidden type of impl Foo.

Huh? only in reveal all mode, where it doesn't matter anyway. We don't use the hidden type in projections in the old solver iirc

lcnr · 2024-06-06T11:27:17Z

rust/compiler/rustc_trait_selection/src/traits/project.rs

Lines 271 to 276 in 2d28b63

    
           // Need to define opaque types to support nested opaque types like `impl Fn() -> impl Trait` 
        
           match infcx.at(&obligation.cause, obligation.param_env).eq( 
        
               DefineOpaqueTypes::Yes, 
        
               normalized, 
        
               actual, 
        
           ) {

tmandry · 2024-07-24T02:50:20Z

@mk12 it's the one in build-fuchsia.sh.

Opened #128127 to bump the commit instead of using cherry picks.

This includes changes to unblock merging rust-lang#126024.

…r=<try> Bump Fuchsia This includes changes to unblock merging rust-lang#126024. try-job: x86_64-fuchsia

…r=Kobzol Bump Fuchsia This includes changes to unblock merging rust-lang#126024. try-job: x86_64-fuchsia

Kobzol · 2024-07-24T09:43:57Z

#128127 was merged.

oli-obk · 2024-07-24T10:46:02Z

@bors r=lcnr

bors · 2024-07-24T10:46:06Z

📌 Commit 61b5e11 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-07-24T11:59:14Z

⌛ Testing commit 61b5e11 with merge 2ccafed...

bors · 2024-07-24T14:24:59Z

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

rust-timer · 2024-07-24T15:51:43Z

Finished benchmarking commit (2ccafed): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.4%	[1.6%, 5.5%]	6
Regressions ❌ (secondary)	3.1%	[0.4%, 5.4%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.4%	[1.6%, 5.5%]	6

Max RSS (memory usage)

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

Cycles

Results (primary 2.6%, 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)	2.6%	[1.1%, 4.3%]	6
Regressions ❌ (secondary)	4.4%	[2.0%, 5.7%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[1.1%, 4.3%]	6

Binary size

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

Bootstrap: 771.472s -> 772.807s (0.17%)
Artifact size: 328.92 MiB -> 328.91 MiB (-0.00%)

oli-obk · 2024-07-24T16:00:23Z

Hmm... I forgot to rerun perf... But, this PR does fix a lingering soundness issue that we just don't have a reproduction for. The caches are now sound, but not used globally if an item has opaques. That's obviously a bulldozing way of solving this issue, but I'm not sure we can fix it in a robust way in the old solver.

This is to unblock an upstream change: rust-lang/rust#126024 Change-Id: I4e1c0b2ba35da81c9e2c6e3ae3e07fa8db1a48a1 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1082793 Reviewed-by: James Robinson <jamesr@google.com> Reviewed-by: Tyler Mandry <tmandry@google.com> Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com> Reviewed-by: Marie Janssen <jamuraa@google.com> Fuchsia-Auto-Submit: Julia Ryan <pineapple@google.com>

This is a follow-up to I4e1c0b2ba35da81c9e2c6e3ae3e07fa8db1a48a1 to increase the recursion limit in more places to unblock the upstream change rust-lang/rust#126024. Test: full build with custom Rust toolchain including the PR Change-Id: Ia0b9ee4d53c00a94864a50190973f48515d2f76d Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1087892 Commit-Queue: Mitchell Kember <mkember@google.com> Reviewed-by: James Robinson <jamesr@google.com> Reviewed-by: Tyler Mandry <tmandry@google.com>

This is a follow-up to I4e1c0b2ba35da81c9e2c6e3ae3e07fa8db1a48a1 to increase the recursion limit in more places to unblock the upstream change rust-lang/rust#126024. Test: full build with custom Rust toolchain including the PR Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1087892 Original-Revision: 577138b8eed9d4a9737c4d644db16f601be69362 GitOrigin-RevId: 96bea561bbfb8776e33b00a4e8c05ae38ad0775d Change-Id: I240d5f70e3663fd6690e2acf6f42e622e7450475

pnkfelix · 2024-07-30T16:03:15Z

Hmm... this PR claims that it fixes #119272 , but the ICE listed there is still occurring on nightly.

Is the idea that there is an unsoundness that is somehow being illustrated by the test case in that issue, and this PR is addressing that unsoundness (via a blunt hammer) while the ICE itself is still not addressed? (Or was the wrong issue simply linked here?)

…, r=lcnr Only disable cache if predicate has opaques within it This is an alternative to rust-lang#132075. This refines the check implemented in rust-lang#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 rust-lang#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

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

oli-obk force-pushed the candidate_key_caching_is_unsound_yay branch from 0714168 to 319843a Compare June 5, 2024 15:38

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

This comment has been minimized.

Sign in to view

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

oli-obk changed the title ~~Do not use global cache for selection candidates if opaque types can be constrained~~ Add definable opaque types to the selection cache key Jun 5, 2024

rustbot assigned lcnr Jun 5, 2024

tmandry added a commit to tmandry/rust that referenced this pull request Jul 24, 2024

Bump Fuchsia

2fd41df

This includes changes to unblock merging rust-lang#126024.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024

Auto merge of rust-lang#128127 - tmandry:you-wouldnt-bump-a-fuchsia, …

17a1345

…r=<try> Bump Fuchsia This includes changes to unblock merging rust-lang#126024. try-job: x86_64-fuchsia

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024

Auto merge of rust-lang#128127 - tmandry:you-wouldnt-bump-a-fuchsia, …

08a9ca7

…r=Kobzol Bump Fuchsia This includes changes to unblock merging rust-lang#126024. try-job: x86_64-fuchsia

Don't use global caches if opaques can be defined

61b5e11

oli-obk force-pushed the candidate_key_caching_is_unsound_yay branch from 221ae5e to 61b5e11 Compare July 24, 2024 10:45

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 Jul 24, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 24, 2024

bors merged commit 2ccafed into rust-lang:master Jul 24, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Jul 24, 2024

bors mentioned this pull request Jul 24, 2024

Do not try to reveal hidden types when trying to prove auto-traits in the defining scope #122192

Merged

rustbot added the perf-regression Performance regression. label Jul 24, 2024

oli-obk deleted the candidate_key_caching_is_unsound_yay branch July 24, 2024 15:53

GrigorenkoPV mentioned this pull request Sep 21, 2024

regression: overflow evaluating the requirement #130578

Closed

This was referenced Oct 23, 2024

regression: crate compiles much slower with 1.82 #132064

Open

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

Closed

lqd mentioned this pull request Nov 5, 2024

Only disable cache if predicate has opaques within it #132625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oli-obk commented Jun 5, 2024 •

edited

Loading

oli-obk commented Jun 5, 2024

This comment has been minimized.

bors commented Jun 5, 2024

This comment has been minimized.

bors commented Jun 5, 2024

This comment has been minimized.

rust-timer commented Jun 5, 2024

oli-obk commented Jun 5, 2024

This comment has been minimized.

bors commented Jun 5, 2024

bors commented Jun 5, 2024

This comment has been minimized.

rust-timer commented Jun 5, 2024

lcnr commented Jun 6, 2024

oli-obk commented Jun 6, 2024

oli-obk commented Jun 6, 2024

lcnr commented Jun 6, 2024

tmandry commented Jul 24, 2024

Kobzol commented Jul 24, 2024

oli-obk commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

rust-timer commented Jul 24, 2024

oli-obk commented Jul 24, 2024

pnkfelix commented Jul 30, 2024 •

edited

Loading

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

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

Conversation

oli-obk commented Jun 5, 2024 • edited Loading

oli-obk commented Jun 5, 2024

This comment has been minimized.

bors commented Jun 5, 2024

This comment has been minimized.

bors commented Jun 5, 2024

This comment has been minimized.

rust-timer commented Jun 5, 2024

Overall result: ❌ regressions - ACTION NEEDED

oli-obk commented Jun 5, 2024

This comment has been minimized.

bors commented Jun 5, 2024

bors commented Jun 5, 2024

This comment has been minimized.

rust-timer commented Jun 5, 2024

Overall result: ❌ regressions - no action needed

lcnr commented Jun 6, 2024

oli-obk commented Jun 6, 2024

oli-obk commented Jun 6, 2024

lcnr commented Jun 6, 2024

tmandry commented Jul 24, 2024

Kobzol commented Jul 24, 2024

oli-obk commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

rust-timer commented Jul 24, 2024

Overall result: ❌ regressions - ACTION NEEDED

oli-obk commented Jul 24, 2024

pnkfelix commented Jul 30, 2024 • edited Loading

oli-obk commented Jun 5, 2024 •

edited

Loading

pnkfelix commented Jul 30, 2024 •

edited

Loading