new solver: remove provisional cache #115843
Conversation
rustbot
added
S-waiting-on-review
lcnr
force-pushed
the
bb-provisional-cache
branch
3 times, most recently
from
77aaab2
to
90a69ea
Compare
| entry.cycle_root_depth = root_depth; | ||
| root.cycle_participants.insert(entry.input); | ||
| #[allow(rustc::potential_query_instability)] | ||
| root.cycle_participants.extend(entry.cycle_participants.drain()); |
There was a problem hiding this comment.
we also have to add cycle participants from previous roots to the new root, this was previously incorrect.
Writing a regression test here is awful 😅 I am going to take the risk that this regresses in the future to be theoretically unsound again.
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
lcnr
force-pushed
the
bb-provisional-cache
branch
from
87ff2bf
to
d71501f
Compare
| // `root` goal of this cycle. | ||
| // | ||
| // See tests/ui/traits/new-solver/cycles/fixpoint-rerun-all-cycle-heads.rs | ||
| // for an example. | ||
| self.stack[stack_depth].has_been_used = true; |
There was a problem hiding this comment.
stack_depth was previously the depth of the root, resulting in tests/ui/traits/new-solver/cycles/fixpoint-rerun-all-cycle-heads.rs.
|
☔ The latest upstream changes (presumably #115929) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnr
force-pushed
the
bb-provisional-cache
branch
from
d71501f
to
e8b8ddd
Compare
| @@ -47,7 +50,7 @@ pub(super) struct SearchGraph<'tcx> { | |||
| /// | |||
| /// An element is *deeper* in the stack if its index is *lower*. | |||
| stack: IndexVec<StackDepth, StackEntry<'tcx>>, | |||
| provisional_cache: ProvisionalCache<'tcx>, | |||
| stack_entries: FxHashMap<CanonicalInput<'tcx>, StackDepth>, | |||
There was a problem hiding this comment.
side-note: maybe we should use an FxIndexMap to prevent that query instability lint.
|
very cool and awesome comments @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (60bb519): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression 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)ResultsThis 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.
CyclesResultsThis 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: 630.657s -> 630.048s (-0.10%) |
…iler-errors next solver: provisional cache this adds the cache removed in rust-lang#115843. However, it should now correctly track whether a provisional result depends on an inductive or coinductive stack. While working on this, I was using the following doc: https://hackmd.io/VsQPjW3wSTGUSlmgwrDKOA. I don't think it's too helpful to understanding this, but am somewhat hopeful that the inline comments are more useful. There are quite a few future perf improvements here. Given that this is already very involved I don't believe it is worth it (for now). While working on this PR one of my few attempts to significantly improve perf ended up being unsound again because I was not careful enough ✨ r? `@compiler-errors`
The provisional cache is a performance optimization if there are large, interleaving cycles. Such cycles generally do not exist. It is incredibly complex and unsound in all trait solvers which have one: the old solver, chalk, and the new solver (link).
Given the assumption that it is not perf-critical and also incredibly complex, remove it from the new solver, only checking whether a goal is on the stack. While writing this, I uncovered two additional soundness bugs, see the inline comments for them.
r? @compiler-errors