Implement goal caching with the new solver

[compiler-errors]

Maybe it's wrong, idk. Opening mostly for first impressions before I go to sleep.

r? @lcnr, cc @cjgillot

[lcnr]

we currently move all cycle participants to the global cache but should only move the head of the cycle. The other goals still in the provisional cache should get dropped

[lcnr]

we should add a comment here, which should state:

• this is for global caching to correctly track dependencies
• that we only cache the cycle root and why that should still be fairly performant

can write some of that myself or can chat about it in sync it to correctly summarize the results of https://rust-lang.zulipchat.com/#narrow/stream/364551-t-types.2Ftrait-system-refactor/topic/global.20cache/near/324056794

[lcnr]

@rustbot author

[compiler-errors]

Is this the proper way of checking the head of the cycle? Just the first entry in this drain iterator?

(modulo the type error that currently exists, will fix)

[lcnr]

jup 👍 though it's probably nicer to first drain all other cycle participants. SO something like

            // If the current goal is the head of a cycle, we drop all other
            // cycle participants without moving them to the global cache.
            let other_cycle_participants = provisional_entry_index.index() + 1;
            for (i, entry) in cache.entries.drain_enumerated(other_cycle_participants..) {
                let actual_index = cache.lookup_table.remove(&entry.goal);
                debug_assert_eq!(Some(i), actual_index);
                debug_assert!(entry.depth == depth);
            }
            
            let current_goal = cache.entries.pop().unwrap();
            let actual_index = cache.lookup_table.remove(&entry.goal);
            debug_assert_eq!(Some(i), provisional_entry_index.index());
            debug_assert!(entry.depth == depth);
            try_to_global_cache

can we somehow move the pop entries and fix lookup_table into a function 🤔 it's a bit scary that we may forget to do one of them somewhere

[compiler-errors]

can we somehow move the pop entries and fix lookup_table into a function

Hm... I can't see how that works with the drain_enumerated, unless we'd write a wrapper around that or something?

[bors]

☔ The latest upstream changes (presumably #107965) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

Still needs a comment for why we can only cache the root goal 🤔

[rustbot]

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

[cjgillot]

The DepGraph is a DAG, so we cannot give it cycles. Therefore, we need a way to represent this cycle (= a SCC in the evaluation graph) as a tree of DepNodes. Those DepNodes must contain as transitive dependencies all the queries that are run while evaluating this SCC.

Later, when replaying an execution, the DepGraph marks the dependencies green/red in their order of apparition. This is important to avoid ICEs. For instance, if the original execution called def_kind to check that a definition was not a closure before calling fn_sig, the replay must realise that def_kind has changed before trying to force fn_sig.

So an SCC's DepNodes must contain as transitive dependencies all the queries that are run while evaluating this SCC, in chronological order.

Having a single DepNode to represent the SCC root (= first goal to have been pushed on the stack) is ok.

Consider a cycle A -> B -> A that we cached as a single DepNode for the cycle root A. The SCC root DepNode represents A.

When replaying, we know that evaluating A makes sense. However, we do not know that the A -> B edge nor B makes sense from just that information, we need to start re-playing the dependencies to know that.

Conversely, we cannot use the same SCC root DepNode to cache B. When re-playing, the invoking code would know B, and the DepGraph would start replaying the execution from A. But the edge B -> A may not exist any more and A may not even make sense.

[cjgillot]

In terms of code organisation, it may be interesting to have a single function that does all the global cache manipulation (looking at the cached goals, calling the dep-graph and containing the code in try_move_finished_goal_to_global_cache), and eventually generic on the closure that you pass to with_anon_task.

    fn with_new_goal(
        &mut self,
        tcx: TyCtxt<'tcx>,
        canonical_goal: CanonicalGoal<'tcx>,
        evaluate: impl FnMut(&mut Self) -> QueryResult<'tcx>,
    ) -> QueryResult<'tcx> {
        if let Some(result) = tcx.new_solver_evaluation_cache.get(&canonical_goal, tcx) {
            return result;
        }

        match self.try_push_stack(tcx, canonical_goal) { ... }

        let (result, dep_node) = tcx.dep_graph.with_anon_task(tcx, DepKind::TraitSelect, evaluate);

        /* inline try_move_finished_goal_to_global_cache */

        result
    }

[compiler-errors]

Ended up inlining try_move_finished_goal_to_global_cache here. Also attempted to distill that comment about why we can't cache non-root goals, but not sure if I totally wrap my head around it despite a thorough explanation 😅

[bors]

☔ The latest upstream changes (presumably #108707) made this pull request unmergeable. Please resolve the merge conflicts.

[lcnr]

r=me after extending the comment a bit

not sure if @cjgillot still wants to take another look

[lcnr]

Suggested change

	// dependencies, our non-root goal may no longer appear as child of the root goal.
	// dependencies, our non-root goal may no longer appear as child of the root goal.
	//
	// See https://github.com/rust-lang/rust/pull/108071 for some additional
	// context.

[compiler-errors]

@bors r=lcnr

gonna approve this for now, happy to do any documentation or refactoring follow-ups later.

[bors]

📌 Commit d21e4d8 has been approved by lcnr

It is now in the queue for this repository.