Make the retained dep graph deterministic under the parallel frontend

[xmakro]

Part of #154314.

The in-memory dep graph kept for -Zquery-dep-graph is built by GraphEncoder::record, which pushed each node using try_lock and silently dropped the node when the lock was contended. Single-threaded the only contention is a query forced re-entrantly from within with_retained_dep_graph, so dropping was harmless. Under the parallel frontend several encoder threads record nodes at the same time, so a contended try_lock dropped nodes and edges at random, leaving the retained graph nondeterministic. The dep-graph ui tests, which assert on its contents, then failed intermittently.

The reentrancy came from check_paths emitting its diagnostics while holding the retained graph: the error path calls def_path_str, which can create new dep nodes and re-enter the encoder. This reads the path results into owned values while the lock is held and emits the diagnostics afterwards, so record never re-enters while the lock is held. record can then block on the lock unconditionally instead of using try_lock, so concurrent encoders never drop a node or edge.

The retained graph has only one other reader, dump_graph (-Zdump-dep-graph), which also holds the lock while it formats nodes. Formatting a DepNode goes through its Debug impl, which uses def_path_debug_str; that path is deliberately query-free (it reads the untracked definitions and crate store), so it never records a node and cannot re-enter record. Concurrent encoder threads blocking on the lock is ordinary contention rather than a deadlock, since a thread holding the lock never waits on another thread to record.

This only affects compilation with -Zquery-dep-graph; with the flag off the retained graph is absent and nothing changes.

The 7 dep-graph ui tests are re-enabled under the parallel frontend. Validated with the directives neutralized: all 7 fail at --parallel-frontend-threads=16 --iteration-count=30; with the fix they pass at 300 iterations, single-threaded exact comparison, and across tests/incremental.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kivooeo (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 17 candidates

[zetanumbers]

You see the problem is that we are trying to keep dep graph's reflexive-transitive closure confluent. As such for parallel frontend we need to emit errors while holding a mutex lock. However looking at your PR made me think of a different way of keeping this property. You can sort the result vector to have an optimal operation order.

[Kivooeo]

I'm not a good reviewer for that

r? nnethercote

[xmakro]

You see the problem is that we are trying to keep dep graph's reflexive-transitive closure confluent. As such for parallel frontend we need to emit errors while holding a mutex lock. However looking at your PR made me think of a different way of keeping this property. You can sort the result vector to have an optimal operation order.

Thanks for the comment. My understanding is that assert_dep_graph is single-threaded. It runs in save_dep_graph before the par_join, and after all queries have finished, so nothing else is recording into the retained graph by then. But what could happen is reentrancy where check_paths holds the lock, emits an error, that calls def_path_str, which creates a dep node and re-enters record on the same thread, which then blocks on the lock it's already holding. So I collect the path results under the lock and emit afterwards. I don't think the emit order matters for the closure here, since every has_path is decided under the lock before anything is printed and nothing reads reachability afterwards, is this correct? The other option I see to make this more clear is to move the graph out of the lock, then we can avoid the lock during this phase, and keep the emit inside the for loop.

The important part is that the record() is called concurrently during analysis and codegen, and on a failed try_lock, the old code skipped the push, and then dropped that node/edge from the retained graph. So I think the change to lock() is needed. And then the other change to store items in the results vector becomes the follow up to make sure we don't deadlock on reentrancy.