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 |
So this actually is only half the problem. Other query keys like |
compiler-errors
force-pushed
the
stall-generators
branch
from
b6f1961 to
6419a55
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…<try> Properly stall coroutine witnesses in new solver TODO: write description r? lcnr
This comment has been minimized.
This comment has been minimized.
meant interning |
|
Could try that too, but that'll be quite a bit more annoying bc unlike the typing env it's not a type localized to rustc_middle. Let's see what perf says about this approach tho so we can compare (tho i expect for it to be neutral). |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f28a368): 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 0.9%, 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 (primary -3.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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 774.376s -> 778.08s (0.48%) |
rustbot
removed
the
S-waiting-on-perf
|
Testing if perf regression is due to calls of a new query. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…<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) |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (061aa26): 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.9%)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 4.5%, secondary 3.3%)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: 774.756s -> 778.266s (0.45%) |
rustbot
removed
the
S-waiting-on-perf
TODO: write description
r? lcnr