-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Opt-in to FulfillmentError
generation to avoid doing extra work in the new solver
#125864
Conversation
Some changes occurred in engine.rs, potentially modifying the public API of Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt HIR ty lowering was modified cc @fmease |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
I don't expect this to be much of an optimization in the old solver, but worth checking I guess :^) |
…ng, r=<try> Opt-in to `FulfillmentError` generation to avoid doing extra work in the new solver In the new solver, we do additional trait solving in order to generate fulfillment errors, because all we have is the root obligation. This is problematic because there are many cases where we don't need the full error information, and instead are just calling `ObligationCtxt::select_all_or_error` to probe whether a predicate holds or not. This is also problematic because we use `ObligationCtxt`s within the error reporting machinery itself, and so we can definitely cause stack overflows: https://github.com/rust-lang/rust/blob/a94483a5f2bae907bc898fc7a8d9cc87db47b693/compiler/rustc_trait_selection/src/solve/inspect/analyse.rs#L75-L84 So instead, make `TraitEngine` and `ObligationCtxt` generic over `E: FulfillmentErrorLike<'tcx>`, and introduce a new `ScrubbedTraitError` which only stores whether the failure was due to a "true error" or an ambiguity. Then, introduce `ObligationCtxt::new_with_diagnostics` for the callsites that actually inspect their `FulfillmentError`s. r? `@lcnr`
dee3d9a
to
bb84313
Compare
Last try-build has never finished. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
ty @fmease |
…ng, r=<try> Opt-in to `FulfillmentError` generation to avoid doing extra work in the new solver In the new solver, we do additional trait solving in order to generate fulfillment errors, because all we have is the root obligation. This is problematic because there are many cases where we don't need the full error information, and instead are just calling `ObligationCtxt::select_all_or_error` to probe whether a predicate holds or not. This is also problematic because we use `ObligationCtxt`s within the error reporting machinery itself, and so we can definitely cause stack overflows: https://github.com/rust-lang/rust/blob/a94483a5f2bae907bc898fc7a8d9cc87db47b693/compiler/rustc_trait_selection/src/solve/inspect/analyse.rs#L75-L84 So instead, make `TraitEngine` and `ObligationCtxt` generic over `E: FulfillmentErrorLike<'tcx>`, and introduce a new `ScrubbedTraitError` which only stores whether the failure was due to a "true error" or an ambiguity. Then, introduce `ObligationCtxt::new_with_diagnostics` for the callsites that actually inspect their `FulfillmentError`s. r? `@lcnr`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
e8c972e
to
e8fe56c
Compare
Finished benchmarking commit (6c3f703): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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. @bors rollup=never 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 (secondary 2.1%)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.121s -> 673.78s (0.70%) |
@@ -1347,7 +1347,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { | |||
return; | |||
}; | |||
// Try to find predicates on *generic params* that would allow copying `ty` | |||
let ocx = ObligationCtxt::new(self.infcx); | |||
let ocx = ObligationCtxt::new_with_diagnostics(self.infcx); |
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.
we use new_with_diagnostics
because we care about leaves instead of the root goal?
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.
Yes, I believe so.
&self, | ||
inference_vars: CanonicalVarValues<'tcx>, | ||
answer: T, | ||
fulfill_cx: &mut dyn TraitEngine<'tcx>, | ||
fulfill_cx: &mut dyn TraitEngine<'tcx, E>, |
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.
shouldn't this always use scrubbed errors? we never emit errors inside of a canonical query after all 🤔
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.
This was removed in a follow-up I believe
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.
Wait no, it's because ScrubbedTraitError
is defined in a downstream crate (rustc_trait_selection
). And pulling it up to rustc_infer
is a bit annoying, because then the impl FromFulfillmentError for OldSolverError
is foreign because that type technically comes from rustc_data_structures
, but I guess I could do it.
pub trait FulfillmentErrorLike<'tcx>: Debug + 'tcx { | ||
fn is_true_error(&self) -> bool; | ||
} |
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.
do we actually need this, i'd expect every user of is_true_error
to use the concrete "non-diagnostics` errors
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.
We use is_true_error
for both scrubbed and real errors, since it's useful to avoid needing to call select_where_possible
then select_all_or_error
.
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.
We could not do that, but it would mean there are far more places where we need to call new_with_diagnostics
for little benefit.
compiler/rustc_trait_selection/src/traits/query/type_op/implied_outlives_bounds.rs
Outdated
Show resolved
Hide resolved
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 like this change 😊 sorry for all the formatting nits xx
r=me after nits
…to new_with_diagnostics
e8fe56c
to
157b041
Compare
This comment has been minimized.
This comment has been minimized.
157b041
to
a41c44f
Compare
@bors r=lcnr |
☀️ Test successful - checks-actions |
Finished benchmarking commit (eb5e244): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 3.7%, secondary 2.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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 667.912s -> 672.681s (0.71%) |
In the new solver, we do additional trait solving in order to generate fulfillment errors, because all we have is the root obligation. This is problematic because there are many cases where we don't need the full error information, and instead are just calling
ObligationCtxt::select_all_or_error
to probe whether a predicate holds or not. This is also problematic because we useObligationCtxt
s within the error reporting machinery itself, and so we can definitely cause stack overflows:rust/compiler/rustc_trait_selection/src/solve/inspect/analyse.rs
Lines 75 to 84 in a94483a
So instead, make
TraitEngine
andObligationCtxt
generic overE: FulfillmentErrorLike<'tcx>
, and introduce a newScrubbedTraitError
which only stores whether the failure was due to a "true error" or an ambiguity. Then, introduceObligationCtxt::new_with_diagnostics
for the callsites that actually inspect theirFulfillmentError
s.r? @lcnr
Number-wise, there are:
calls to each
ObligationCtxt
constructor, which suggests that there are indeed a lot of callsites that don't care about diagnostics.