-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Move fast paths around to gain a bit of new-solver perf #158249
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
378f1af
move overflow check
jdonszelmann 88ee7e4
move fast path into evaluate_goal
jdonszelmann 2386145
remove fastpath from fullfilment loop
jdonszelmann d54b2ab
experiment with better evaluate_goal_step
jdonszelmann 15d7b5b
return stalled info from fast path
jdonszelmann 47b6070
move goal fast path into evaluate_goal_cold
jdonszelmann f806e9b
shuffle fastpaths around to before creating an evalctxt
jdonszelmann 203832a
fast path when adding goals
jdonszelmann 1cb6d00
fix bug when fast paths aren't rerun when opaques change
jdonszelmann a572787
review comments (nits)
jdonszelmann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
162 changes: 162 additions & 0 deletions
162
compiler/rustc_next_trait_solver/src/solve/eval_ctxt/fast_path.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| //! This file contains a number of standalone functions useful for taking _fast paths_ in the trait | ||
| //! solver. The exact place where we check for these fast paths changes, and matters a lot for | ||
| //! performance. Ideally we'd only check them in `evaluate_goal`, but when evaluating root goals | ||
| //! we can check them earlier and save some time creating an `EvalCtxt` in the first place. | ||
| //! | ||
| //! For debugging, fast paths can be disabled using `-Zdisable-fast-paths`. | ||
|
|
||
| use rustc_type_ir::inherent::*; | ||
| use rustc_type_ir::solve::{ | ||
| Certainty, ComputeGoalFastPathOutcome, Goal, GoalStalledOn, GoalStalledOnOpaques, | ||
| SucceededInErased, | ||
| }; | ||
| use rustc_type_ir::{InferCtxtLike, Interner}; | ||
|
|
||
| use crate::delegate::SolverDelegate; | ||
| use crate::solve::eval_ctxt::{RerunDecision, should_rerun_after_erased_canonicalization}; | ||
| use crate::solve::{GoalEvaluation, HasChanged}; | ||
|
|
||
| #[derive(Debug, Clone, Copy)] | ||
| pub(super) enum RerunStalled { | ||
| WontMakeProgress(Certainty), | ||
| MayMakeProgress, | ||
| } | ||
|
|
||
| /// If we have run a goal before, and it was stalled, check that any of the goal's | ||
| /// args have changed. This is a cheap way to determine that if we were to rerun this goal now, | ||
| /// it will remain stalled since it'll canonicalize the same way and evaluation is pure. | ||
| /// Therefore, we can skip this rerun | ||
| #[inline] | ||
| pub(super) fn rerunning_stalled_goal_may_make_progress<D, I>( | ||
| delegate: &D, | ||
| stalled_on: Option<&GoalStalledOn<I>>, | ||
| ) -> RerunStalled | ||
| where | ||
| D: SolverDelegate<Interner = I>, | ||
| I: Interner, | ||
| { | ||
| use RerunStalled::*; | ||
|
|
||
| // If fast paths are turned off, then we assume all goals can always make progress | ||
| if delegate.disable_trait_solver_fast_paths() { | ||
| return MayMakeProgress; | ||
| } | ||
|
|
||
| // If the goal isn't stalled, we should definitely run it. | ||
| let Some(&GoalStalledOn { ref opaques, ref stalled_vars, ref sub_roots, stalled_certainty }) = | ||
| stalled_on | ||
| else { | ||
| return MayMakeProgress; | ||
| }; | ||
|
|
||
| // If any of the stalled goal's generic arguments changed, | ||
| // rerunning might make progress so we should rerun. | ||
| if stalled_vars.iter().any(|value| delegate.is_changed_arg(*value)) { | ||
| return MayMakeProgress; | ||
| } | ||
|
|
||
| // If some inference took place in any of the sub roots, | ||
| // rerunning might make progress so we should rerun. | ||
| if sub_roots.iter().any(|&vid| delegate.sub_unification_table_root_var(vid) != vid) { | ||
| return MayMakeProgress; | ||
| } | ||
|
|
||
| match opaques { | ||
| GoalStalledOnOpaques::No => {} | ||
| &GoalStalledOnOpaques::Yes { | ||
| num_opaques_in_storage, | ||
| ref previously_succeeded_in_erased, | ||
| } => { | ||
| // If any opaques changed in the opaque type storage, | ||
| // rerunning might make progress so we should rerun. | ||
| if delegate | ||
| .opaque_types_storage_num_entries() | ||
| .needs_reevaluation(num_opaques_in_storage) | ||
| { | ||
| // Unless this goal previously succeeded in erased mode. | ||
| // If the stalled goal successfully evaluated while erasing opaque types, | ||
| // and the current state of the opaque type storage is not different in a way that is | ||
| // relevant, this stalled goal cannot make any progress and we set this variable to true. | ||
| let mut previous_erased_run_is_still_valid = false; | ||
|
|
||
| if let &SucceededInErased::Yes { accessed_opaques } = previously_succeeded_in_erased | ||
| { | ||
| match should_rerun_after_erased_canonicalization( | ||
| accessed_opaques, | ||
| delegate.typing_mode_raw(), | ||
| &delegate.clone_opaque_types_lookup_table(), | ||
| ) { | ||
| RerunDecision::Yes => {} | ||
| RerunDecision::EagerlyPropagateToParent => { | ||
| unreachable!("we never retry stalled queries if the parent was erased") | ||
| } | ||
| RerunDecision::No => { | ||
| previous_erased_run_is_still_valid = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !previous_erased_run_is_still_valid { | ||
| return MayMakeProgress; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, we can be sure that this stalled goal cannot make any progress | ||
| // and we can exit early. | ||
| WontMakeProgress(stalled_certainty) | ||
| } | ||
|
|
||
| /// `compute_goal_fast_path` is complicated enough that outling helps, so it gets optimized | ||
| /// separately from the caller. `compute_goal_fast_path` is the inlined version, | ||
| /// and most call sites (when adding goals) use it. However, when entering the root | ||
| /// we also want to check the fast path, and there the outlining matters. | ||
| /// | ||
| /// FIXME(perf) cold might not be worth it here, given that we shuffled some things around since it | ||
| /// mattered. | ||
| #[cold] | ||
|
jdonszelmann marked this conversation as resolved.
|
||
| #[inline(never)] | ||
| pub(super) fn compute_goal_fast_path_cold<D, I>( | ||
| delegate: &D, | ||
| goal: Goal<I, I::Predicate>, | ||
| origin_span: I::Span, | ||
| ) -> Option<GoalEvaluation<I>> | ||
| where | ||
| D: SolverDelegate<Interner = I>, | ||
| I: Interner, | ||
| { | ||
| compute_goal_fast_path(delegate, goal, origin_span) | ||
| } | ||
|
|
||
| /// This is a fast path optimization: | ||
| /// See the docs on [`ComputeGoalFastPathOutcome`] | ||
| pub fn compute_goal_fast_path<D, I>( | ||
| delegate: &D, | ||
| goal: Goal<I, I::Predicate>, | ||
| origin_span: I::Span, | ||
| ) -> Option<GoalEvaluation<I>> | ||
| where | ||
| D: SolverDelegate<Interner = I>, | ||
| I: Interner, | ||
| { | ||
| if delegate.disable_trait_solver_fast_paths() { | ||
| return None; | ||
| } | ||
|
|
||
| match delegate.compute_goal_fast_path(goal, origin_span) { | ||
| ComputeGoalFastPathOutcome::NoFastPath => None, | ||
| ComputeGoalFastPathOutcome::TriviallyHolds => Some(GoalEvaluation { | ||
| goal, | ||
| certainty: Certainty::Yes, | ||
| has_changed: HasChanged::No, | ||
| stalled_on: None, | ||
| }), | ||
| ComputeGoalFastPathOutcome::TriviallyStalled { stalled_on } => Some(GoalEvaluation { | ||
| goal, | ||
| certainty: Certainty::AMBIGUOUS, | ||
| has_changed: HasChanged::No, | ||
| stalled_on: Some(stalled_on), | ||
| }), | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we stall on a goal while there are 2 opaque types in the storage, then add a third which doesn't matter for our erased run, we'll now alwasy check
should_rerun_after_erased_canonicalization🤔I am not fully confident, but expect that updating
num_opaques_in_storageto the current value if we getRerunDecision::Nowould be correct. Need to look atshould_rerun_after_erased_canonicalizationView changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll file a follow up for that