Repository navigation
add a cache to the WfPredicates visitor - #161274
Conversation
|
changes to the core type system cc @lcnr |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| use std::iter; | |||
|
|
|||
| use rustc_data_structures::fx::FxHashSet; | |||
There was a problem hiding this comment.
something something comparing this to DelayedSet 🤔
I would expect WF to mostly be for quite small types
There was a problem hiding this comment.
these UI tests you've added do compile even without this fix, don't they? They are just really slow. I am worried about using UI tests here as it's hard to deal with them again. I would instead prefer adding these as secondary benchmarks, cc @rust-lang/wg-compiler-performance
| ) -> Result<Option<I::Const>, RerunNonErased> { | ||
| if self.typing_mode().is_erased_not_coherence() { | ||
| match self.opaque_accesses.rerun_always(RerunReason::EvaluateConst)? {} | ||
| let resolved = self.resolve_vars_if_possible(alias_const); |
There was a problem hiding this comment.
@jdonszelmann mind taking this over?
We originally bailed here because we didnt want to deal with the exact behavior of evaluate_const and how it accesses the TypingMode/opaque type storage. also cc https://rust-lang.zulipchat.com/#narrow/channel/260443-project-const-generics/topic/.60evaluate_const.60.20is.20scary/near/617438696 @BoxyUwU @zedddie as we're changing evaluate const to a next-solver only method at which point handling ErasedNonCoherence correctly becomes even easier 🤔
would you mind moving this change out of this PR as the other one feels a lot easier to merge :>
|
Let's check whether this is visible on our current benchmarks. If not, then yes, it'd be nice to add new secondary one for this. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
speed up next-solver on some pathological cases
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (71c2d57): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.7%, secondary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 456.256s -> 457.184s (0.20%) |
|
If we do add a secondary benchmark we should do it with intention. We probably have multiple pathological cases for the new trait solver. It would be good to gather them into a single benchmark rather than adding, say, five new tiny benchmarks. The ctfe-stress benchmark is an example where this has been done in the past. |
|
Ooh, interesting perf. For now, I got rid of the 2 uitests and the second commit, and replaced the FxHashSet with a DelayedSet. The const eval PR is here: #161380 |
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
speed up next-solver on some pathological cases
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (bd53954): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary 2.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 456.256s -> 456.614s (0.08%) |
WfPredicates
|
@bors r+ |
WfPredicatesWfPredicates visitor
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 8536f4d (parent) -> def3ee9 (this PR) Test differencesShow 12 test diffs12 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard def3ee9e4f0fe1429574f402bb624439bbd13fc6 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (def3ee9): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.5%, secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 468.334s -> 468.743s (0.09%) |
…donszelmann only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang#161274: rust-lang#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
…donszelmann only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang#161274: rust-lang#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
…donszelmann only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang#161274: rust-lang#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
…donszelmann only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang#161274: rust-lang#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
…donszelmann only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang#161274: rust-lang#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
Rollup merge of #161380 - sjwang05:retry-consteval-less, r=jdonszelmann only rerun const eval in next-solver if the const actually references opaques Split off from #161274: #161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang/rust#161274: rust-lang/rust#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
only rerun const eval in next-solver if the const actually references opaques Split off from rust-lang/rust#161274: rust-lang/rust#161274 (comment) Previously, we would unconditionally rerun eval if we were in ErasedNotCoherence mode, even if the const's eval result wouldn't depend on the TypingMode. This caused a slowdown with large types whose nested goals contained normalization goals involving types with anon assoc consts, since normalizing needs to evaluate those consts, which can't be done in ErasedNotCoherence mode, causing those goals, and all their parent goals, to be reevaluated. For instance, if we had a large tuple with N elements in a goal, and each of the N elements had such a normalization goal, this would cause us to go quadratic, as is the case here: rust-lang/trait-system-refactor-initiative#272 (comment) With this PR, we only retry const eval if the const's generics actually depend on any opaques which ErasedNotCoherence would influence. We also bail if gce is enabled, since not doing so causes us to ICE. next-solver is still 5x-ish slower than old solver on rust-lang/trait-system-refactor-initiative#272 (comment) due to some other ✨ Hidden Quadratics somewhere, but this PR improves perf on that specific reproducer by about 3x. cc @lcnr, @jdonszelmann
View all comments
Part of speeding up compiling
ReShellwith-Znext-solver, see rust-lang/trait-system-refactor-initiative#272 and #t-types/trait-system-refactor > more `reshell` slowness.The PR changes the
WfPredicatesvisitor to only collect new bounds for unique types. That is, if we have a type like:etc., the visitor used to end up collecting one WF obligation for each path from
T_Nto itsWhitespaceleaves, even though WF of a type (I believe) doesn't depend on the path the visitor took to get there, which allows us to deduplicate by Ty. Not deduplicating caused us to go O(2^N) here.next-solver is still about ~5x slower than the old solver on the third reproducer due to some other hidden quadratics, and fixing that seems to be more involved, but I think this PR will still be ✨ An Improvement.
r? lcnr