Properly stall coroutine witnesses in new solver #138845
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor changes to |
| @@ -65,13 +65,13 @@ pub enum TypingMode<I: Interner> { | |||
| /// let x: <() as Assoc>::Output = true; | |||
| /// } | |||
| /// ``` | |||
| Analysis { defining_opaque_types: I::DefiningOpaqueTypes }, | |||
| Analysis { defining_opaque_types: I::LocalDefIds, stalled_generators: I::LocalDefIds }, | |||
There was a problem hiding this comment.
Probably could squash this into one list.
| @@ -162,7 +162,7 @@ where | |||
| self.select(selcx) | |||
| } | |||
|
|
|||
| fn drain_unstalled_obligations( | |||
| fn drain_stall_obligations_for_coroutines( | |||
There was a problem hiding this comment.
| fn drain_stall_obligations_for_coroutines( | |
| fn drain_stalled_obligations_for_coroutines( |
| ScrubbedTraitError<'tcx>, | ||
| >(self.at, ct, vec![None; ct.outer_exclusive_binder().as_usize()]) | ||
| { | ||
| Ok((value, _)) => value, |
There was a problem hiding this comment.
We throw away ambiguous preds here b/c we may be using this folder on types that really are just not fully normalizable.
There was a problem hiding this comment.
The preds list should just contain stalled coroutine obligations, after all.
| /// entered before passing `value` to the function. This is currently needed for | ||
| /// `normalize_erasing_regions`, which skips binders as it walks through a type. | ||
| /// | ||
| /// TODO: doc |
There was a problem hiding this comment.
I need to explain that this doesn't return all ambiguous preds, just the ones that are stalled on coroutines.
| struct StalledOnCoroutines<'tcx> { | ||
| stalled_generators: &'tcx ty::List<LocalDefId>, | ||
| span: Span, | ||
| // TODO: Cache |
There was a problem hiding this comment.
Cache would be nice since visiting everything 128318913 times to look for coroutines is probably expensive.
| @@ -189,6 +189,20 @@ where | |||
| debug_assert!(ecx.opaque_type_is_rigid(opaque_ty.def_id)); | |||
| } | |||
|
|
|||
| if let ty::CoroutineWitness(def_id, _) = goal.predicate.self_ty().kind() { | |||
There was a problem hiding this comment.
I should pull this out into a helper, b/c I think I need to also apply this hack to copy/clone. I think those are it tho.
| // Increase this limit if necessary, but do try to keep the size low if possible | ||
| #[cfg(target_pointer_width = "64")] | ||
| const _: () = { | ||
| if size_of::<Key<'static>>() > 88 { | ||
| if size_of::<Key<'static>>() > 96 { |
This comment has been minimized.
This comment has been minimized.
Tweaks to writeback and `Obligation -> Goal` conversion Each of these commits are self-contained, but are prerequisites that I'd like to land before rust-lang#138845, which still needs some cleaning. The ""most controversial"" one is probably [Explicitly don't fold coroutine obligations in writeback](rust-lang@e7d27ba), which I prefer because I think using `fold_predicate` to control against not normalizing predicates seems... easy to mess up 🤔, and we could have *other things* that we don't want to normalize. Explicitly noting whether we want `resolve` to normalize is a lot clearer (and currently in writeback is limited to resolving stalled coroutine obligations), since we can attach it to a comment that explains *why*.
Rollup merge of rust-lang#138846 - compiler-errors:stall-prereqs, r=lcnr Tweaks to writeback and `Obligation -> Goal` conversion Each of these commits are self-contained, but are prerequisites that I'd like to land before rust-lang#138845, which still needs some cleaning. The ""most controversial"" one is probably [Explicitly don't fold coroutine obligations in writeback](rust-lang@e7d27ba), which I prefer because I think using `fold_predicate` to control against not normalizing predicates seems... easy to mess up 🤔, and we could have *other things* that we don't want to normalize. Explicitly noting whether we want `resolve` to normalize is a lot clearer (and currently in writeback is limited to resolving stalled coroutine obligations), since we can attach it to a comment that explains *why*.
|
☔ The latest upstream changes (presumably #138873) made this pull request unmergeable. Please resolve the merge conflicts. |
| } | ||
|
|
||
| fn pending_obligations(&self) -> PredicateObligations<'tcx> { | ||
| self.obligations.clone_pending() | ||
| } | ||
|
|
||
| fn drain_unstalled_obligations(&mut self, _: &InferCtxt<'tcx>) -> PredicateObligations<'tcx> { | ||
| self.obligations.take_pending() | ||
| fn drain_stall_obligations_for_coroutines( |
There was a problem hiding this comment.
| fn drain_stall_obligations_for_coroutines( | |
| fn drain_stalled_obligations_for_coroutines( |
| fn visit_goal(&mut self, inspect_goal: &super::inspect::InspectGoal<'_, 'tcx>) -> Self::Result { | ||
| inspect_goal.goal().predicate.visit_with(self)?; | ||
|
|
||
| if let Some(candidate) = inspect_goal.unique_applicable_candidate() { |
There was a problem hiding this comment.
this type visitor feels somewhat fragile and I expect unique_applicable_candidate and the limited recursion depth to cause us to fail to stall obligations in very rare cases. otoh I don't think this is a problem though
so my understanding here is:
- for correctness it doesn't matter how many obligations we stall
- for diagnostics (and perf) we want to stall as few obligations as possible
- failing to stall causes unexpected ambiguity errors
Please add this as a comment somewhere, prolly the stalled_coroutine_obligations field of the typeck results
There was a problem hiding this comment.
Yep, that's my understanding. We could perhaps stall obligations if we find coroutines in the predicate or if we hit the recursion limit, but idk if we have a facility to detect when we hit the recursion limit here. Shouldn't be too hard to fix, but I'd rather leave that to when we need it.
|
Let's see how bad the perf is from making items larger. @bors try @rust-timer queue |
compiler-errors
force-pushed
the
stall-generators
branch
from
fb7257b to
b6f1961
Compare
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…<try> Properly stall coroutine witnesses in new solver TODO: write description r? lcnr
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5443aaa): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking 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 @bors rollup=never Instruction countThis 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.
Max RSS (memory usage)Results (primary 1.2%, secondary -1.8%)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.
CyclesResults (secondary -1.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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.999s -> 780.062s (0.27%) |
rustbot
added
perf-regression
|
Let me try putting coroutines into the same list as the opaques 🤔 |
|
alternatively, intern |
compiler-errors
force-pushed
the
stall-generators
branch
from
0a666a2 to
3439c11
Compare
rustbot
added
A-attributes
|
changes to the core type system Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in src/tools/clippy cc @rust-lang/clippy HIR ty lowering was modified cc @fmease Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_sanitizers cc @rcvalle Some changes occurred in compiler/rustc_monomorphize/src/partitioning/autodiff.rs cc @ZuseZ4 Some changes occurred to the CTFE machinery Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_passes/src/check_attr.rs The Miri subtree was changed cc @rust-lang/miri Some changes occurred in match checking cc @Nadrieril Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
|
Sorry for pings lmao |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
compiler-errors
force-pushed
the
stall-generators
branch
from
3439c11 to
9a64c73
Compare
|
@bors try |
…<try> Properly stall coroutine witnesses in new solver TODO: write description r? lcnr
|
⌛ Trying commit 9a64c73 with merge ad3921840b47e97d39de47d555811ce56e2b5642... |
compiler-errors
force-pushed
the
stall-generators
branch
from
9a64c73 to
37f42e0
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…<try> Properly stall coroutine witnesses in new solver TODO: write description r? lcnr
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
[DO NOT MERGE] bootstrap with `-Znext-solver=globally` A revival of rust-lang#124812. Current status: ~~`./x.py b --stage 2` passes 🎉~~ `try` builds succeed 🎉 🎉 🎉 [top 100 most downloaded crates on crates.io compile](rust-lang#133502 (comment)) [first perf run](rust-lang#133502 (comment)) 👻 ### in-flight changes - rust-lang#124852, unsure whether I actually want to land this PR for now - rust-lang#139587 - https://github.com/lcnr/rust/tree/opaque-type-method-call - rust-lang#138845 - rust-lang#139762 - double the available recursion depth in the new solver ☠️ r? `@ghost`
|
Finished benchmarking commit (db86470): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countThis 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.
Max RSS (memory usage)Results (primary 0.5%, secondary 0.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.
CyclesResults (primary 0.4%, secondary -4.0%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 775.5s -> 774.98s (-0.07%) |
rustbot
removed
the
S-waiting-on-perf
TODO: write description
r? lcnr