Simplify `break_query_cycles` by zetanumbers · Pull Request #153185 · rust-lang/rust

zetanumbers · 2026-02-27T13:40:26Z

This is break_query_cycles implementation from #149849 (f466828) modified to be non-deterministic to match its current implementation and remove TreeNodeIndex plumbing code.

I thought I could be a bit more brave with changes.

rustbot · 2026-02-27T13:40:31Z

r? @madsmtm

rustbot has assigned @madsmtm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler, query-system
compiler, query-system expanded to 68 candidates
Random selection from 15 candidates

zetanumbers · 2026-02-27T14:08:02Z

r? @nnethercote

Since you have already looked at #149849 I thought you could also take a look at this.

zetanumbers · 2026-02-27T17:01:20Z

cc @Zoxc

Zoxc · 2026-02-27T19:34:53Z

compiler/rustc_middle/src/query/job.rs

 pub struct QueryWaiter<'tcx> {
    pub query: Option<QueryJobId>,
    pub condvar: Condvar,
-    pub span: Span,


The removal of spans here doesn't seem great.

Leaving unused struct fields doesn't seem like a good long-term approach either. I can split its removal into a separate commit for a fast revert if necessary.

I mean it should still be used. Each query call has an associated span and that should be propagated to the cycle error.

But the span is currently unused, correct?

Done in 6c56188

Zoxc · 2026-02-27T19:36:06Z

compiler/rustc_query_impl/src/job.rs

+    let mut root_query = None;
+    for (&query, info) in &query_map.map {
+        if info.job.parent.is_none() {
+            assert!(root_query.is_none(), "found multiple threads without start");


There can be multiple roots if queries are called in parallel from outside a query.

How would queries ever be called in parallel from outside a query? If I would relax this requirement I would also like to know the context.

Say a program join(|| a(), || b()) where a() and b() are queries. It's trivial to do.

Well, this is not what I've meant. But I see you are saying I shouldn't assume anything more than that.

Done in 2e0fbb9.

Zoxc · 2026-02-28T21:15:29Z

Under the assumption that #149849 doesn't land, what's the motivation for these changes? Code size?

nnethercote · 2026-03-01T05:19:12Z

I don't know much about the cycle breaking / waiter / latch stuff, so I will need to spend some time reading closely over that code before I can do a sensible review here. @zetanumbers, can you explain in some more detail what this PR is doing and why? Feel free to assume I don't know anything and make your explanation as detailed as you like :) Thank you.

zetanumbers · 2026-03-02T08:56:20Z

Under the assumption that #149849 doesn't land, what's the motivation for these changes? Code size?

Reduction of code complexity. We don't need 8 functions do to this after considering two theorems I've written down mid break_query_cycles.

zetanumbers · 2026-03-02T09:35:22Z

And I had draft of a code removing "rustc query cycle handler" thread from deadlock_handler for this break_query_cycles implementation. I wrote it for reasons described in #149849 (comment) but later opted for a minimal change from try_lock to lock there instead:

rust/compiler/rustc_query_impl/src/job.rs

Lines 133 to 134 in d7a2352

    
           // Latch mutexes should be at least about to unlock as we do not hold it anywhere too long 
        
           let lock = latch.info.lock();

nnethercote · 2026-03-02T10:21:19Z

I started looking over the existing break_query_cycles implementation today and my head is spinning. This PR removes 150 lines of code and multiple functions to I'm positively inclined towards it based on that alone.

@zetanumbers: you said this matches the current implementation. Is it exactly the same? You haven't given the expanded explanation I requested yet so I'm still guessing. As a newcomer to this code both the old code and the new code in this PR are very difficult to follow.

nnethercote · 2026-03-03T04:51:32Z

compiler/rustc_query_impl/src/job.rs

+#[allow(rustc::potential_query_instability)]
+fn find_cycle_in_graph<'tcx>(
+    query_map: &QueryJobMap<'tcx>,
+) -> (QueryJobId, usize, CycleError<QueryStackDeferred<'tcx>>) {


The function comment should explain the return type, especially the usize. Alternatively, you could introduce a new struct and use that instead of the tuple.

nnethercote · 2026-03-03T04:52:49Z

compiler/rustc_query_impl/src/job.rs

+        .map
+        .iter()
+        .find(|(_, info)| info.job.parent.is_none())
+        .expect("no root query was found");


If you use values() instead of iter() you won't need to filter out the keys.

nnethercote · 2026-03-03T04:54:06Z

compiler/rustc_query_impl/src/job.rs

+        id: QueryJobId,
+        span: Span,
+        /// Waiter index or `usize::MAX` if subquery was executed
+        waiter_idx: usize,


Can you change to Option<usize> instead of using MAX for the exceptional case?

nnethercote · 2026-03-03T05:05:27Z

compiler/rustc_query_impl/src/job.rs


-        true
+    // Per statement above we should have wait at either of two occurrences of the duplicate query
+    let waiter_idx = if last.waiter_idx != usize::MAX {


If last.waiter_idx is an Option<usize> you can use unwrap_or_else here.

nnethercote · 2026-03-03T05:06:20Z

compiler/rustc_query_impl/src/job.rs

-    for _ in 0..wakelist.len() {
-        rustc_thread_pool::mark_unblocked(registry);
-    }
+    // And so this `Vec::remove` shouldn't cause a panic


This comment reads strangely, like it's referring to an earlier comment.

nnethercote · 2026-03-03T05:07:08Z

compiler/rustc_query_impl/src/job.rs

-/// uses a query latch and then resuming that waiter.
-/// There may be multiple cycles involved in a deadlock, so this searches
-/// all active queries for cycles before finally resuming all the waiters at once.
 pub fn break_query_cycles<'tcx>(


This function should have at least a short doc comment.

nnethercote · 2026-03-03T05:11:04Z

A few minor comments above but I think this generally looks good. I don't understand every detail but it's a lot shorter and easier to follow that the existing code. I would still appreciate it if you modified the PR description to include some more details about what this PR does, and why.

nnethercote · 2026-03-03T05:17:51Z

Also, I guess the PR title should now say "two functions"?

Zoxc · 2026-03-03T06:47:04Z

This PR seems highly questionable to me. My initial concerns suggest @zetanumbers does not fully understand the problem, so I question the correctness of the rest of the code too.

We're trying to find cycles in the directed graph of active query jobs. The straightforward solution is a DFS search for cycles in that graph, which is what my code does. I have no clue what this PR is doing.

zetanumbers · 2026-03-03T07:21:39Z

We're trying to find cycles in the directed graph of active query jobs. The straightforward solution is a DFS search for cycles in that graph, which is what my code does. I have no clue what this PR is doing.

I will write a detailed description later, but this is also a DFS search for cycles in that graph.

Replace beack_query_cycles implementation with new

d7a2352

rustbot assigned madsmtm Feb 27, 2026

zetanumbers mentioned this pull request Feb 27, 2026

Deterministic query cycles for parallel front-end #149849

Open

rustbot assigned nnethercote and unassigned madsmtm Feb 27, 2026

zetanumbers changed the title ~~Simplify break_query_cycles~~ Simplify break_query_cycles into a single function Feb 27, 2026

Write down theorems new cycle breaking depends on

f7862e4

This comment has been minimized.

Sign in to view

Fix typo

9a3400d

Zoxc reviewed Feb 27, 2026

View reviewed changes

Restore QueryWaiter::span field

253b3c1

zetanumbers added 2 commits March 2, 2026 13:15

Remove whitespace

6d5996a

Do not assume there's only one root query

2e0fbb9

zetanumbers added 3 commits March 2, 2026 16:36

Use QueryWaiter's span

6c56188

Split cycle breaking into find_cycle_in_graph

52c016b

Debug assert find_cycle_in_graph is consistent with find_cycle_in_stack

11b79d6

nnethercote reviewed Mar 3, 2026

View reviewed changes

nnethercote added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2026

zetanumbers changed the title ~~Simplify break_query_cycles into a single function~~ Simplify break_query_cycles Mar 3, 2026

Uh oh!

Conversation

zetanumbers commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 27, 2026

Uh oh!

zetanumbers commented Feb 27, 2026

Uh oh!

zetanumbers commented Feb 27, 2026

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zetanumbers Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zoxc commented Feb 28, 2026

Uh oh!

nnethercote commented Mar 1, 2026

Uh oh!

zetanumbers commented Mar 2, 2026

Uh oh!

zetanumbers commented Mar 2, 2026

Uh oh!

nnethercote commented Mar 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Mar 3, 2026

Uh oh!

nnethercote commented Mar 3, 2026

Uh oh!

Zoxc commented Mar 3, 2026

Uh oh!

zetanumbers commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zetanumbers commented Feb 27, 2026 •

edited

Loading

zetanumbers Feb 27, 2026 •

edited

Loading