Deterministic query cycles for parallel front-end

[zetanumbers]

View all comments

The mechanism is similar to cycle detection in single-threaded mode. We traverse the deadlocked query graph from the top active query downwards to subqueries until we visit some query a second time, thus finding a cycle. With multi-thread front-end enabled one query may now have more than one active subqueries, aka we used one of parallel interfaces parallel!, join, par_for_each, etc. As such we have to traverse the "leftmost" active subquery to recover the sequential behavior of these parallel interfaces in single-threaded mode. New TreeNodeIndex saves implicit context information about what join (or scope) task we entered while executing a query, which we then use in break_query_cycle.

However we then have to guarantee the query stack from single-threaded mode is included in the active query graph. This is true for join function as their first task will be completed on the same thread and same will be tried for the second task unless stolen which is fine for us. scope places tasks in local queue and pops them in LIFO maner, while other worker threads could only steal from that queue in FIFO maner, thus we can guarantee the next task is either stolen or available for execution.

Fixes #153391
Fixes #142064
Fixes #142063
Fixes #127971

UPDATE: commits are sliced to the finest detail

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[rustbot]

r? @eholk

rustbot has assigned @eholk.
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

[matthiaskrgr]

@bors try @rust-timer queue

[Kivooeo]

@matthiaskrgr there is a compilation error in this PR, not sure if this can be benchmarked?

Ok this is surprising

[rust-bors]

☀️ Try build successful (CI)
Build commit: 306a768 (306a768e2f03217e62aee6655739249be1d8aa95, parent: 377656d3dd3f9c23a9c8713e163f4365a5261a84)

[rust-timer]

Finished benchmarking commit (306a768): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 1.8%]	155
Regressions ❌ (secondary)	0.7%	[0.2%, 1.9%]	138
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	0.5%	[0.2%, 1.8%]	155

Max RSS (memory usage)

Results (secondary -1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-3.5%, -0.9%]	3
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.2%, secondary 2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.0%, 2.4%]	3
Regressions ❌ (secondary)	3.6%	[2.8%, 5.3%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-4.6%, -4.6%]	1
All ❌✅ (primary)	2.2%	[2.0%, 2.4%]	3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 471.42s -> 474.532s (0.66%)
Artifact size: 389.04 MiB -> 389.08 MiB (0.01%)

[Zoxc]

It's a bit unclear to me what source of non-determinism this tries to fix. Does this alter parallel execution in any way other than picking which query in the cycle use for resumption? It looks to me like you're trying to pick the point in the cycle to break which corresponds to a single threaded execution, but I don't think that point is guaranteed to be resumable.

Currently it looks like we're not deterministically picking which query in a cycle to resume when multiple is present. That's fairly simple to improve, though I think the queries available for resumption is non-deterministic to start with.

[zetanumbers]

It's a bit unclear to me what source of non-determinism this tries to fix. Does this alter parallel execution in any way other than picking which query in the cycle use for resumption? It looks to me like you're trying to pick the point in the cycle to break which corresponds to a single threaded execution, but I don't think that point is guaranteed to be resumable.

I make an assumption that when we get a query cycle there is the "point in the cycle to break which corresponds to a single threaded execution" which currently rustc_thread_scope::scope violates due to the order of task executions (and which I plan to replace with join). If you traverse query graph in a "single-threaded manner" you will eventually find a thread waiting for already visited query to finish because graph is finite and every active query during a deadlock has a subquery (either direct or waiting on) to go to next. That last query wait closing a loop has to be resumed. I assume any query can be reached by traversing down to some subquery from the root query which I also assume is unique.

though I think the queries available for resumption is non-deterministic to start with.

That's what I am trying to make deterministic.

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[zetanumbers]

Is it possible to write tests for these issues? It would be good to have test-based evidence that the problems are fixed, and won't regress in the future.

I believe it requires a rewrite in ./x test utility to execute some ui tests sequentially to retain multi-threading behavior and repeatedly for more common bug reproduction. The latter requirement is done in #143953, while I am unaware if the former one is implemented anywhere. However the majority of parallel compiler bugs should be consistently reproducible this way.

I plan to work on parallel testsuite one way or another.

[nnethercote]

@zetanumbers: thanks for the additional commits, they have good improvements. I have marked most of my comments as resolved. There are still a small number that need action.

@Zoxc: any thoughts?

[Zoxc]

Just to clarify, my example shows that it's not possible to make cycle recovery deterministic with the way we're currently executing multiple queries on a single stack.

So we do the next most deterministic thing and resume thread 1 calling cycle hander for a() query just like with single-threaded front-end.

Resuming thread 1 in my example leads to the join internally waiting on the right task in thread 2. Thread 2 however is waiting on itself, so that means the program deadlocked. The cycle is entirely in thread 2, so the only option that breaks the cycle is resuming thread 2, thus diverging from the single threaded execution.

[zetanumbers]

Resuming thread 1 in my example leads to the join internally waiting on the right task in thread 2. Thread 2 however is waiting on itself, so that means the program deadlocked. The cycle is entirely in thread 2, so the only option that breaks the cycle is resuming thread 2, thus diverging from the single threaded execution.

Yes, my query cycle breaking code does this.

[zetanumbers]

Just as it is described here:

https://github.com/zetanumbers/rust/blob/15674b4d5a3d84e4b32699601526b25f49dbb899/compiler/rustc_query_impl/src/job.rs#L110-L142

[zetanumbers]

So I have isolated new break_query_cycles implementation into a separate PR #153185. I will rebase onto it from now on instead of rebasing onto main directly, after review comments will be resolved.