perf(vk): eliminate rec_timeline write-back stalls and capture deadlock #2458

github-sync · 2026-11-11T20:07:20Z

Erfboom commented

2026-11-11 20:07:20 +00:00

(Migrated from github.com)

Problem

When vk_timeline_wait(v) waits for a write-back fence value v stamped on pages during burst recording execution, it submits a wait on dev.timeline (via VkSemaphoreWaitInfo). If an earlier submission in the queue is currently stopped at an event wait or synchronization point, dev.timeline does not advance to v, resulting in a pipeline stall or capture deadlock and timing out after 250 ms.

Additionally:

submit_recording() was recording sequence numbers using std::map::emplace(v, seq), which silently does nothing if v was already present from an earlier pass, leaving stale sequence numbers in value_rec.
wait_recorded() had a redundant check on per_queue_fences_off(), preventing timeline synchronization when per-queue fences were disabled even though rec_timeline was initialized.

Solution

In vk_timeline_wait(), first query wait_recorded(v, 250000000ull, &ok) to wait on rec_timeline where the burst recording actually signalled completion. If the recording sequence is known, wait on that timeline semaphore directly and return early, bypassing stalls on dev.timeline.
In submit_recording(), use direct subscript assignment value_rec[v] = seq; and value_rec[r.value] = seq; to ensure values are mapped to their most up-to-date recording sequence.
In wait_recorded(), remove the per_queue_fences_off() gate so valid rec_timeline waits are honored.

Impact

Eliminates write-back timeline stalls and capture deadlocks during intensive burst recordings.
Prevents 250ms timeouts in GPU replay timeline synchronization.

## Problem When `vk_timeline_wait(v)` waits for a write-back fence value `v` stamped on pages during burst recording execution, it submits a wait on `dev.timeline` (via `VkSemaphoreWaitInfo`). If an earlier submission in the queue is currently stopped at an event wait or synchronization point, `dev.timeline` does not advance to `v`, resulting in a pipeline stall or capture deadlock and timing out after 250 ms. Additionally: - `submit_recording()` was recording sequence numbers using `std::map::emplace(v, seq)`, which silently does nothing if `v` was already present from an earlier pass, leaving stale sequence numbers in `value_rec`. - `wait_recorded()` had a redundant check on `per_queue_fences_off()`, preventing timeline synchronization when per-queue fences were disabled even though `rec_timeline` was initialized. ## Solution 1. In `vk_timeline_wait()`, first query `wait_recorded(v, 250000000ull, &ok)` to wait on `rec_timeline` where the burst recording actually signalled completion. If the recording sequence is known, wait on that timeline semaphore directly and return early, bypassing stalls on `dev.timeline`. 2. In `submit_recording()`, use direct subscript assignment `value_rec[v] = seq;` and `value_rec[r.value] = seq;` to ensure values are mapped to their most up-to-date recording sequence. 3. In `wait_recorded()`, remove the `per_queue_fences_off()` gate so valid `rec_timeline` waits are honored. ## Impact - Eliminates write-back timeline stalls and capture deadlocks during intensive burst recordings. - Prevents 250ms timeouts in GPU replay timeline synchronization.

Joey-Magnus (Migrated from github.com) reviewed

2026-11-11 21:24:48 +00:00

Joey-Magnus (Migrated from github.com) left a comment

The value_rec fix is already on main via #2419 (finished_in keeps the latest part's sequence number), which is the conflict. At this head, test_draw_stream_visibility.py fails because the test's multimap stub has no operator[] (vk_replay.cpp:389). Also:

Removing the per_queue_fences_off() gate (:2146) breaks the documented RX_VK_PER_QUEUE_FENCES=0 escape hatch.
Routing vk_timeline_wait through wait_recorded (:273) changes how capture write-backs wait on game threads, with no log or repro of the deadlock.

I'd rebase down to the vk_timeline_wait change with evidence, or close.

The `value_rec` fix is already on main via #2419 (`finished_in` keeps the latest part's sequence number), which is the conflict. At this head, `test_draw_stream_visibility.py` fails because the test's `multimap` stub has no `operator[]` (`vk_replay.cpp:389`). Also: - Removing the `per_queue_fences_off()` gate (`:2146`) breaks the documented `RX_VK_PER_QUEUE_FENCES=0` escape hatch. - Routing `vk_timeline_wait` through `wait_recorded` (`:273`) changes how capture write-backs wait on game threads, with no log or repro of the deadlock. I'd rebase down to the `vk_timeline_wait` change with evidence, or close.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 22:44:40 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for this. I merged main into the branch (f426c25a). The only conflict was in submit_recording: #2419 had already made value_rec keep each value's latest recording, so the merge keeps main's version and drops your value_rec[v] = seq hunk. That also clears the test_draw_stream_visibility.py failure @Joey-Magnus found. What's left is two hunks: the wait_recorded path in vk_timeline_wait, and the removed gate.

Is it still needed? There's no deadlock to fix. vk_timeline_wait already gives up after 250 ms (ea78039, before this branch was cut). The case that's still left is a stall plus possibly stale data. finish_recording only signals timeline up to oldest_unfinished() - 1. So while an older submission on another queue is stopped at an event wait, a capture waiting for a write-back whose recording has already gone to the GPU waits the full 250 ms, then copies its late pages anyway. Waiting on that recording's number on rec_timeline is the right fix. It's what invalidate's readback wait already does on main. rec_seq is handed out under replay_mu in submit order on the one queue, value_rec keeps the last part's number since #2419, and wait_recorded sends the held batch before it waits. So I think the change is correct.

What it's missing is evidence that this happens. Joey asked for the same thing. Could you post a log from main showing vk: a capture's wait for write-back ... timed out, or the late-wait time in take_capture's stats, and the same numbers with this branch? Without that it's a plausible fix for a case nobody has seen yet.

Two more things:

The per_queue_fences_off() gate: both existing callers (wait and invalidate's readback wait) already check !per_queue_fences_off() themselves, so removing it from wait_recorded changes nothing for them. Its only effect is that the new capture path ignores RX_VK_PER_QUEUE_FENCES=0. That's the part of Joey's point that still applies. I'd keep the gate and guard the new call the way invalidate does (see inline).
A source check would be cheap, along the lines of tests/test_recorded_wait_flush.py: vk_timeline_wait calls wait_recorded before it waits on timeline.

Nits: the title and description still talk about a deadlock and the value_rec change. Please update them for what's left after the merge. The new timeout logs the same text as the old one, so a log can't tell you which wait gave up.

Related: #2222 (merged) bounds invalidate's fallback wait. It doesn't touch vk_timeline_wait, so there's no overlap, and the two fit together.

CI is still running on the merge. If the numbers show the 250 ms stalls on main and they go away here, with the gate kept, I'm happy to approve. If the timeout never shows up in practice, closing is fine too.

Thanks for this. I merged main into the branch (f426c25a). The only conflict was in `submit_recording`: #2419 had already made `value_rec` keep each value's latest recording, so the merge keeps main's version and drops your `value_rec[v] = seq` hunk. That also clears the `test_draw_stream_visibility.py` failure @Joey-Magnus found. What's left is two hunks: the `wait_recorded` path in `vk_timeline_wait`, and the removed gate. **Is it still needed?** There's no deadlock to fix. `vk_timeline_wait` already gives up after 250 ms (ea78039, before this branch was cut). The case that's still left is a stall plus possibly stale data. `finish_recording` only signals `timeline` up to `oldest_unfinished() - 1`. So while an older submission on another queue is stopped at an event wait, a capture waiting for a write-back whose recording has already gone to the GPU waits the full 250 ms, then copies its late pages anyway. Waiting on that recording's number on `rec_timeline` is the right fix. It's what `invalidate`'s readback wait already does on main. `rec_seq` is handed out under `replay_mu` in submit order on the one queue, `value_rec` keeps the last part's number since #2419, and `wait_recorded` sends the held batch before it waits. So I think the change is correct. What it's missing is evidence that this happens. Joey asked for the same thing. Could you post a log from main showing `vk: a capture's wait for write-back ... timed out`, or the late-wait time in take_capture's stats, and the same numbers with this branch? Without that it's a plausible fix for a case nobody has seen yet. Two more things: - The `per_queue_fences_off()` gate: both existing callers (`wait` and `invalidate`'s readback wait) already check `!per_queue_fences_off()` themselves, so removing it from `wait_recorded` changes nothing for them. Its only effect is that the new capture path ignores `RX_VK_PER_QUEUE_FENCES=0`. That's the part of Joey's point that still applies. I'd keep the gate and guard the new call the way `invalidate` does (see inline). - A source check would be cheap, along the lines of `tests/test_recorded_wait_flush.py`: `vk_timeline_wait` calls `wait_recorded` before it waits on `timeline`. Nits: the title and description still talk about a deadlock and the `value_rec` change. Please update them for what's left after the merge. The new timeout logs the same text as the old one, so a log can't tell you which wait gave up. Related: #2222 (merged) bounds `invalidate`'s fallback wait. It doesn't touch `vk_timeline_wait`, so there's no overlap, and the two fit together. CI is still running on the merge. If the numbers show the 250 ms stalls on main and they go away here, with the gate kept, I'm happy to approve. If the timeout never shows up in practice, closing is fine too.

gpu/vk/vk_replay.cpp

					
				@ -279,0 +280,4 @@

				    // (a burst recording whose write-back stamped wb_pages with burst_value does not advance `timeline` while an older queue

				    // submission is stopped at an event wait; wait on `rec_timeline` where that recording actually signalled completion)

				    int ok = 0;

				    if (wait_recorded(v, 250000000ull, &ok)) {

Suggest guarding this the way invalidate does, so RX_VK_PER_QUEUE_FENCES=0 (and async / RX_VK_SYNC_WAITS=0) still gives the old behaviour for A/B:

const bool unforced = !async_on() && sync_waits_on() && !per_queue_fences_off();
if (unforced && wait_recorded(v, 250000000ull, &ok)) {

(async_on/sync_waits_on/per_queue_fences_off would need forward declarations here, like wait_recorded.)

Suggest guarding this the way `invalidate` does, so `RX_VK_PER_QUEUE_FENCES=0` (and async / `RX_VK_SYNC_WAITS=0`) still gives the old behaviour for A/B: ```cpp const bool unforced = !async_on() && sync_waits_on() && !per_queue_fences_off(); if (unforced && wait_recorded(v, 250000000ull, &ok)) { ``` (`async_on`/`sync_waits_on`/`per_queue_fences_off` would need forward declarations here, like `wait_recorded`.)

gpu/vk/vk_replay.cpp

With the call above guarded, this check can stay. The existing callers check !per_queue_fences_off() themselves anyway, so removing it only changed the new caller.

With the call above guarded, this check can stay. The existing callers check `!per_queue_fences_off()` themselves anyway, so removing it only changed the new caller.