Pass list of defineable opaque types into canonical queries #122077

oli-obk · 2024-03-06T10:26:23Z

This eliminates DefiningAnchor::Bubble for good and brings the old solver closer to the new one wrt cycles and nested obligations. At that point the difference between DefiningAnchor::Bind([]) and DefiningAnchor::Error was academic. We only used the difference for some sanity checks, which actually had to be worked around in places, so I just removed DefiningAnchor entirely and just stored the list of opaques that may be defined.

fixes #108498
fixes #116877

run crater
- Pass list of defineable opaque types into canonical queries #122077 (comment)

Pass list of defineable opaque types into canonical queries based on rust-lang#121796 This eliminates `DefiningAnchor::Bubble` for good and brings the old solver closer to the new one wrt cycles and nested obligations. r? `@ghost`

oli-obk · 2024-03-06T10:34:16Z

compiler/rustc_trait_selection/src/solve/mod.rs

@@ -320,5 +320,6 @@ fn response_no_constraints_raw<'tcx>(
            external_constraints: tcx.mk_external_constraints(ExternalConstraintsData::default()),
            certainty,
        },
+        defining_anchor: Default::default(),


This one is fishy, we can probably always pass in the anchor from the InferCtxt

oli-obk · 2024-03-06T10:37:21Z

compiler/rustc_trait_selection/src/infer.rs

-        let (infcx, key, canonical_inference_vars) = self
-            .with_opaque_type_inference(DefiningAnchor::Bubble)
-            .build_with_canonical(DUMMY_SP, canonical_key);
+        let (infcx, key, canonical_inference_vars) =
+            self.build_with_canonical(DUMMY_SP, canonical_key);


This is the core change of this PR. Canonical queries now always use the anchor of their caller

compiler-errors · 2024-03-06T15:25:35Z

compiler/rustc_middle/src/query/plumbing.rs

@@ -340,10 +340,10 @@ macro_rules! define_callbacks {
                    <$($K)* as keys::Key>::CacheSelector as CacheSelector<'tcx, Erase<$V>>
                >::Cache;

-                // Ensure that keys grow no larger than 64 bytes
+                // Ensure that keys grow no larger than 72 bytes


Is this not a hard limit? Why was this added in the first place?

probably perf, but I'm trying to resolve it anyway before going out of draft. I looked up the original PR, and it was just for not accidentally growing

So I'm a bit stuck on CanonicalTypeOpAscribeUserTypeGoal. It's 8 bytes too large now, but there's also nothing that stands out as "this should be interned". Just moving something onto a TyCtxt arena doesn't really seem right to me, that's just avoiding the above size check without adhering to its spirit.

bors · 2024-03-08T02:03:14Z

☔ The latest upstream changes (presumably #122151) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-03-08T12:56:58Z

☔ The latest upstream changes (presumably #122182) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2024-03-11T17:25:23Z

@bors try

bors · 2024-03-11T17:26:33Z

⌛ Trying commit cb7c75b with merge e34498b...

…of waiting until borrowck is done

…ion_obligations

…ing scope" is actually needed

…nding issue

…is PR

…formation about the defining anchor

oli-obk · 2024-04-08T15:16:52Z

@bors r=lcnr rollup=never

bors · 2024-04-08T15:16:55Z

📌 Commit dc97b1e has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-04-08T23:01:54Z

⌛ Testing commit dc97b1e with merge b234e44...

bors · 2024-04-09T01:01:21Z

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

rust-timer · 2024-04-09T02:18:40Z

Finished benchmarking commit (b234e44): 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)	1.9%	[0.2%, 5.4%]	101
Regressions ❌ (secondary)	2.3%	[0.3%, 4.7%]	77
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-0.6%, 5.4%]	102

Max RSS (memory usage)

Results

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)	-1.7%	[-2.5%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.7%	[-2.5%, -0.9%]	2

Cycles

Results

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.5%	[1.1%, 5.2%]	55
Regressions ❌ (secondary)	3.0%	[1.1%, 5.2%]	41
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[1.1%, 5.2%]	55

Binary size

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

Bootstrap: 669.403s -> 674.046s (0.69%)
Artifact size: 318.50 MiB -> 318.46 MiB (-0.01%)

oli-obk · 2024-04-09T04:52:58Z

hmm weird, a previous perf run was clean. I must have changed something since then. I'll investigate.

oli-obk · 2024-04-09T05:03:12Z

Ah... dd72bf9

Scraping regions is expensive.

I'll implement an alternative. Let's not revert this PR, it's an important bugfix and blocker of a lot of follow up work

Avoid a scrape_region_constraints and instead register the region constraints directly Should fix the perf regression from rust-lang#122077 (comment)

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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Mar 6, 2024

oli-obk force-pushed the eager_opaque_checks4 branch from 6946681 to 119a935 Compare March 6, 2024 10:31

This comment was marked as outdated.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2024

This comment was marked as outdated.

Sign in to view

oli-obk commented Mar 6, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

compiler-errors reviewed Mar 6, 2024

View reviewed changes

This comment was marked as outdated.

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

oli-obk mentioned this pull request Mar 7, 2024

TAIT decision on "may define implies must define" #117861

Closed

oli-obk force-pushed the eager_opaque_checks4 branch 3 times, most recently from c9357a9 to 05ee8ae Compare March 7, 2024 21:35

This comment has been minimized.

Sign in to view

oli-obk force-pushed the eager_opaque_checks4 branch from 05ee8ae to e3291ea Compare March 8, 2024 06:27

oli-obk mentioned this pull request Mar 8, 2024

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

Merged

oli-obk force-pushed the eager_opaque_checks4 branch from e3291ea to cb7c75b Compare March 11, 2024 17:24

oli-obk added 11 commits April 8, 2024 15:00

Pass list of defineable opaque types into canonical queries

19bd91d

Eagerly check for accidentally registered region constraints instead …

b2b5fa8

…of waiting until borrowck is done

Adjust a comment to the new status quo

08e25e4

Scrape extraneous regions from instantiate_nll_query_response_and_reg…

dd72bf9

…ion_obligations

Eliminate DefiningAnchor now that is just a single-variant enum

2f2350e

Shrink the size of ClosureTypeInfo to fit into 64 bytes again

ba5f041

Avoid fetching the opaque type origin when only "is this in the defin…

7cfa521

…ing scope" is actually needed

Mark some tests as known-bugs and add the test case from the correspo…

cd9453c

…nding issue

Add a FIXME for something that requires touching too much code for th…

cd5ca3c

…is PR

bless mir-opt tests

c8dfb59

Ensure the canonical_param_env_cache does not contain inconsistent in…

dc97b1e

…formation about the defining anchor

oli-obk force-pushed the eager_opaque_checks4 branch from 52ab386 to dc97b1e Compare April 8, 2024 15:08

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 Apr 8, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 9, 2024

bors merged commit b234e44 into rust-lang:master Apr 9, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 9, 2024

bors mentioned this pull request Apr 9, 2024

Unconditionally show update nightly hint on ICE #122200

Merged

oli-obk deleted the eager_opaque_checks4 branch April 9, 2024 04:52

oli-obk mentioned this pull request Apr 9, 2024

Avoid a scrape_region_constraints and instead register the region constraints directly #123669

Open

cushionbadak mentioned this pull request Apr 10, 2024

ICE: two delayed bugs: {OpaqueTypeKey { def_id: ...} and error performing ParamEnvAnd {...} #123707

Closed

apiraino removed the to-announce Announce this issue on triage meeting label Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass list of defineable opaque types into canonical queries #122077

Pass list of defineable opaque types into canonical queries #122077

oli-obk commented Mar 6, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

oli-obk Mar 6, 2024

oli-obk Mar 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

compiler-errors Mar 6, 2024

oli-obk Mar 6, 2024 •

edited

Loading

oli-obk Mar 7, 2024

This comment was marked as outdated.

This comment has been minimized.

bors commented Mar 8, 2024

bors commented Mar 8, 2024

oli-obk commented Mar 11, 2024

bors commented Mar 11, 2024

oli-obk commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 9, 2024

rust-timer commented Apr 9, 2024

oli-obk commented Apr 9, 2024

oli-obk commented Apr 9, 2024

Pass list of defineable opaque types into canonical queries #122077

Pass list of defineable opaque types into canonical queries #122077

Conversation

oli-obk commented Mar 6, 2024 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

Choose a reason for hiding this comment

oli-obk Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment has been minimized.

bors commented Mar 8, 2024

bors commented Mar 8, 2024

oli-obk commented Mar 11, 2024

bors commented Mar 11, 2024

oli-obk commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 9, 2024

rust-timer commented Apr 9, 2024

Overall result: ❌ regressions - ACTION NEEDED

oli-obk commented Apr 9, 2024

oli-obk commented Apr 9, 2024

oli-obk commented Mar 6, 2024 •

edited

Loading

oli-obk Mar 6, 2024 •

edited

Loading