Pass list of defineable opaque types into canonical queries by oli-obk · Pull Re...
source link: https://github.com/rust-lang/rust/pull/122077
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Pass list of defineable opaque types into canonical queries #122077
Conversation
Contributor
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.
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
This comment was marked as outdated.
This comment was marked as outdated.
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
This comment was marked as outdated.
| @@ -320,5 +320,6 @@ fn response_no_constraints_raw<'tcx>( | ||
external_constraints: tcx.mk_external_constraints(ExternalConstraintsData::default()), |
||
certainty, |
||
}, |
||
defining_anchor: Default::default(), |
Contributor
Author
This one is fishy, we can probably always pass in the anchor from the InferCtxt
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); |
Contributor
Author
This is the core change of this PR. Canonical queries now always use the anchor of their caller
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
| @@ -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?
Contributor
Author
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
Contributor
Author
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.
This comment was marked as outdated.
This comment has been minimized.
Contributor
|
☔ The latest upstream changes (presumably #122151) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
|
☔ The latest upstream changes (presumably #122182) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
Author
|
@bors try |
Contributor
Author
|
@bors r=lcnr rollup=never |
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
Contributor
|
☀️ Test successful - checks-actions |
Collaborator
|
Finished benchmarking commit (b234e44): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesResults Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.403s -> 674.046s (0.69%) |
Contributor
Author
|
hmm weird, a previous perf run was clean. I must have changed something since then. I'll investigate. |
Contributor
Author
|
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 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
No one assigned
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK