Repository navigation
perf(vk): eliminate rec_timeline write-back stalls and capture deadlock - #2458
Conversation
When vk_timeline_wait waits on a write-back fence value produced by burst recordings, waiting on dev.timeline deadlocks if an earlier queue submission is stopped at an event wait. Wait on rec_timeline via wait_recorded() where the burst recording actually signalled completion. Additionally, replace std::map::emplace with direct subscript assignment in submit_recording to ensure burst values map to their latest sequence number, and remove the redundant per_queue_fences_off() gate in wait_recorded(). Co-Authored-By: erfboom <gabrielh.ltd@gmail.com>
Joey-Magnus
left a comment
There was a problem hiding this 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 documentedRX_VK_PER_QUEUE_FENCES=0escape hatch. - Routing
vk_timeline_waitthroughwait_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.
vk_replay.cpp: main (NotRexed#2419) already keeps each value's latest recording in value_rec; keep main's version of that hunk.
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for this. I merged main into the branch (f426c25). 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 (waitandinvalidate's readback wait) already check!per_queue_fences_off()themselves, so removing it fromwait_recordedchanges nothing for them. Its only effect is that the new capture path ignoresRX_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 wayinvalidatedoes (see inline). - A source check would be cheap, along the lines of
tests/test_recorded_wait_flush.py:vk_timeline_waitcallswait_recordedbefore it waits ontimeline.
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.
| // (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)) { |
There was a problem hiding this comment.
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.)
| /// timeline: true when that recording is known (the result in *ok), false when it is not (f not finished yet) | ||
| bool wait_recorded(uint64_t f, uint64_t timeout_ns, int* ok) { | ||
| if (!rec_timeline || per_queue_fences_off()) return false; | ||
| if (!rec_timeline) return false; |
There was a problem hiding this comment.
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.
…s behind them Upstream's main fails 15 of its own source checks at c356bef (PRs merged without a rebase). These are the eleven outside the upscalers: - vk_memory.cpp: RX_VK_CAPTURE_KEEP_OLD and RX_VK_CAP_BLOCKS are on/off switches and were read as set-means-on, so =0 turned them on (NotRexed#2437). env_on, as the rest. - mouse_native.c: the mouse's kept angles are let go when the camera leaves states 3 and 4 (first-person aim, Z-targeting): they were written back on return, and the view snapped to where it was before (NotRexed#2617; test_mouse_native's yaw). - menu_i18n.h: three English texts changed in vk_menu.cpp (NotRexed#1930, NotRexed#2362, NotRexed#2435), so their 13 translations are keyed and translated again (machine-made, as the file's others). - Fixtures that cut source by its text and missed what the source gained: window_title (NotRexed#2116), dead_import_bytes (NotRexed#1906), env_on in reverb_frame, hos_xinput_pads (NotRexed#2435), wait_recorded's forward declaration (NotRexed#2458); and the Dockerfile check's parser, which took --no-update for a PPA. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
**What this is for:** restore ten game-free checks whose extracted fixtures or expectations no longer match intentional changes on main; [main's CI log](https://github.com/NotRexed/Hookshot/actions/runs/43151254256) shows these failures. **What is measured:** those ten checks on Linux with GCC 16.2.1 and Clang 23.1.3, plus the strict full suite and deliberate regression mutations, as detailed below. **Not measured:** a game run, Windows execution, audio output or GPU behavior; no production file changes in this diff. ## Cause and changes Each row is a separate commit, naming the change that made the fixture stale; the quoted diagnostics come from [main's failed jobs](https://github.com/NotRexed/Hookshot/actions/runs/43149508038). | Introduced by | Checks / CI diagnostic | Repair | |---|---|---| | #1544, after #2042's switch helper | `audio_aux`, `audio_biquad`: `implicit declaration of function ‘env_on’` | Include the real helper before the extracted DSP. | | #2116 | `display_startup_failure`, `wayland_libdecor_hint`: `use of undeclared identifier 'window_title'` | Extract the real title helper along with its callers. | | #1906 | `texel_buffer_ownership`: `use of undeclared identifier 'dead_import_bytes'` | Model the counter and check that it stays charged until completion. | | #2458 | `readback_part`: `extraneous closing brace ('}')` | Select the complete `wait_recorded` definition, not its new forward declaration. | | #2574 | `dlss_checks`: `FAIL under a third wide at line 40`; `framegen_preset` / `rumble_menu`: undeclared `temporal_reset_failure` | Check both sides of the intended two-pixel tolerance; supply a counted reset hook and exercise the real scaling rows. | | #2435 | `mods_size_table`: `undefined reference to hos_xinput_pads` | Supply the fixture's no-controller answer; retain real merge and routing checks. | The diff retains the existing assertions: no check is deleted or skipped, and it does not revert the production behavior named above. ## How it was tested Recorded on 2026-11-11, Linux kernel `7.2.15-1-omarchy`; base `de866017`, head `eeb09c09`, GCC 16.2.1 and Clang 23.1.3 (compiler version output and test records). - Untouched main `a016b7a6`: `455 passed, 15 failed, 20 skipped`; each of these ten failures also reproduced alone with both compilers, matching the linked CI diagnostics. - Branch: all ten repaired checks pass independently with each compiler (`python -I -B tests/run_all.py --strict --jobs 1 <test>` records). - Negative controls: 14 deliberate source regressions were each rejected with both compilers, without a compile failure—28 behavioral rejections (mutation records). These remove Aux sizing, biquad filtering, startup failure handling, the libdecor hint, view ownership release, part-specific waits, preset preservation, temporal reset or mod-table routing; the DLSS controls independently make each axis's tolerance too narrow and too broad. - One full `python -I -B tests/run_all.py --strict` run: **`464 passed, 6 failed, 20 skipped in 141 s`**, 32 workers, load 7.24 → 28.38 (complete suite output). ### Remaining failures and the path to green The complete branch run names these six failures; the first five are outside this fixture-only diff: | Remaining check | Separate fix | |---|---| | `test_env_on.py` | `fix/capture-switch-values`: the three presence-only capture switch reads from #2437. | | `test_linux_docker_packages.py` | `test/docker-ppa-options`: distinguish `--no-update` from its PPA operand. | | `test_mouse_native.py` | `fix/mouse-camera-handover`: discard retained angles while another camera owns the view. | | `test_menu_i18n.py` | `fix/menu-hint-translations`: update three complete hint keys and their translations. | | `test_hdr_encode.py` | Already repaired in #2614; deliberately not duplicated here. | | `test_release_scripts.py` | An intermittent, unchanged release-script concurrency failure: `SHA256SUMS still differs from the files after 5 uploads`. It passed untouched main and current main CI; isolated reruns passed once and failed once. Neither run invokes a C/C++ compiler. | ## Not tested / risks No Windows, game, display, controller, listening or GPU run was performed; the evidence above is game-free only. The fixture extraction still follows source structure, so future helper/signature changes must update its inputs rather than remove its behavioral guards. ## Overlaps The inspected merge hunks overlap #1834's frame-generation fixture helpers and #2622's older readback extraction; neither contains these repairs. #2614 owns the separate HDR initializer check. Other same-file changes merge cleanly in the recorded overlap comparison. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> --------- Co-authored-by: GPT-6 Astra <noreply@openai.com>
Problem
When
vk_timeline_wait(v)waits for a write-back fence valuevstamped on pages during burst recording execution, it submits a wait ondev.timeline(viaVkSemaphoreWaitInfo). If an earlier submission in the queue is currently stopped at an event wait or synchronization point,dev.timelinedoes not advance tov, resulting in a pipeline stall or capture deadlock and timing out after 250 ms.Additionally:
submit_recording()was recording sequence numbers usingstd::map::emplace(v, seq), which silently does nothing ifvwas already present from an earlier pass, leaving stale sequence numbers invalue_rec.wait_recorded()had a redundant check onper_queue_fences_off(), preventing timeline synchronization when per-queue fences were disabled even thoughrec_timelinewas initialized.Solution
vk_timeline_wait(), first querywait_recorded(v, 250000000ull, &ok)to wait onrec_timelinewhere the burst recording actually signalled completion. If the recording sequence is known, wait on that timeline semaphore directly and return early, bypassing stalls ondev.timeline.submit_recording(), use direct subscript assignmentvalue_rec[v] = seq;andvalue_rec[r.value] = seq;to ensure values are mapped to their most up-to-date recording sequence.wait_recorded(), remove theper_queue_fences_off()gate so validrec_timelinewaits are honored.Impact