-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Optimize try_evaluate_obligations
#160479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,7 +138,6 @@ impl<'tcx, E: 'tcx> FulfillmentCtxt<'tcx, E> { | |
| } | ||
|
|
||
| fn inspect_evaluated_obligation( | ||
| &self, | ||
| infcx: &InferCtxt<'tcx>, | ||
| obligation: &PredicateObligation<'tcx>, | ||
| result: &Result<GoalEvaluation<TyCtxt<'tcx>>, NoSolution>, | ||
|
|
@@ -196,22 +195,39 @@ where | |
| fn try_evaluate_obligations(&mut self, infcx: &InferCtxt<'tcx>) -> TraitErrors<E> { | ||
| assert_eq!(self.usable_in_snapshot, infcx.num_open_snapshots()); | ||
| let mut errors = TraitErrors::NoErrors; | ||
| let delegate = <&SolverDelegate<'tcx>>::from(infcx); | ||
| loop { | ||
| let mut any_changed = false; | ||
| for (mut obligation, stalled_on) in mem::take(&mut self.obligations.pending) { | ||
| let goal = obligation.as_goal(); | ||
| let delegate = <&SolverDelegate<'tcx>>::from(infcx); | ||
| let mut overflowed = false; | ||
|
|
||
| self.obligations.pending.retain_mut(|(obligation, opt_stalled_on)| { | ||
| if overflowed { | ||
| return false; | ||
| } | ||
|
|
||
| let result = delegate.evaluate_root_goal(goal, obligation.cause.span, stalled_on); | ||
| self.inspect_evaluated_obligation(infcx, &obligation, &result); | ||
| // Common case: still stalled; keep the obligation. This path is extremely hot in | ||
| // some cases; there can be thousands of pending obligations. | ||
| if let Some(stalled_on) = opt_stalled_on | ||
| && let Some(certainty) = delegate.goal_remains_stalled(stalled_on) | ||
| && matches!(certainty, Certainty::Maybe(_)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a follow-up?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
| { | ||
| return true; | ||
| } | ||
|
|
||
| let result = delegate.evaluate_root_goal( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: the first thing evaluate_root_goal does is to check again goal_remains_stalled.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there is some repeated work: The common case is more than 100x hotter than what follows. It would be possible to refactor |
||
| obligation.as_goal(), | ||
| obligation.cause.span, | ||
| opt_stalled_on.take(), | ||
| ); | ||
| Self::inspect_evaluated_obligation(infcx, &obligation, &result); | ||
| let GoalEvaluation { goal, certainty, has_changed, stalled_on } = match result { | ||
| Ok(result) => result, | ||
| Err(NoSolution) => { | ||
| errors.push(E::from_solver_error( | ||
| infcx, | ||
| NextSolverError::TrueError(obligation), | ||
| NextSolverError::TrueError(obligation.clone()), | ||
| )); | ||
| continue; | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -229,9 +245,11 @@ where | |
| obligation.recursion_depth += 1; | ||
|
|
||
| if !infcx.tcx.recursion_limit().value_within_limit(obligation.recursion_depth) { | ||
| self.obligations.on_fulfillment_overflow(infcx); | ||
| // Only return true errors that we have accumulated while processing. | ||
| return errors; | ||
| // At this point we want to stop evaluating goals. We can't break out of | ||
| // `retain_mut`, so instead we set this flag which causes all other | ||
| // elements to be skipped. | ||
| overflowed = true; | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this drops all obligations from the pending obligations without pushing anything into
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully trust the LLM generated explanation in that issue, but this code does at least look quite sus and we should definitely assert that there's some error in the error paths :>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lcnr This isn't the true cause. The original reproducer in rust-lang/trait-system-refactor-initiative#294 (comment) reproduces with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or actually, this looks preexisting '^^ even before this PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did my usual "rewrite it to be faster without changing its behaviour" thing and my understanding of this code is shallow. Is it safe to assume that someone with a deeper knowledge than me will follow up, or do you want me to do something? |
||
| } else { | ||
| any_changed = true; | ||
| } | ||
|
|
@@ -253,11 +271,24 @@ where | |
| if infcx.in_hir_typeck | ||
| && (obligation.has_non_region_infer() || obligation.has_free_regions()) | ||
| { | ||
| infcx.push_hir_typeck_potentially_region_dependent_goal(obligation); | ||
| infcx.push_hir_typeck_potentially_region_dependent_goal( | ||
| obligation.clone(), | ||
|
jdonszelmann marked this conversation as resolved.
|
||
| ); | ||
| } | ||
| false | ||
| } | ||
| Certainty::Maybe(_) => { | ||
| // Update `opt_stalled_on` goal, for the next retain_mut, because we are | ||
| // running until a fixpoint. | ||
| *opt_stalled_on = stalled_on; | ||
|
nnethercote marked this conversation as resolved.
|
||
| true | ||
| } | ||
| Certainty::Maybe(_) => self.obligations.register(obligation, stalled_on), | ||
| } | ||
| }); | ||
| if overflowed { | ||
| self.obligations.on_fulfillment_overflow(infcx); | ||
| // Only return true errors that we have accumulated while processing. | ||
| return errors; | ||
| } | ||
|
|
||
| if !any_changed { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.