perf(vk): fix the late pages on the GPU, word by word, instead of waiting for their write-back #248

Merged
HumanBean merged 3 commits from GummiNaomi/Longshot:perf/late-pages-fix into main 2026-11-14 01:55:31 +00:00
Contributor

Why

In busy places (Lon Lon Ranch by day, crowds, many characters) the game drops from 60 to 50-55 fps, and on some PCs to the 30s, because the renderer waits on the CPU every frame for the GPU to finish writing an earlier frame's results back to game memory: "late pages". At Lon Lon Ranch that wait is ~4.7 ms of a 17.8 ms frame; a player with an RTX 5090 measured ~5.8 ms a frame with 15-30 ms spikes, and an RTX 3080 player saw 31-34 fps (NotRexed/Hookshot issue 2655, the thread of NotRexed/Hookshot PR 2789). This removes the wait on PCs whose pools have VRAM mirrors.

What changed

  • The wait becomes a GPU fix-up (gpu/vk/vk_memory.cpp, the block // ---- the late pages' fix (longshot, 2026-11-13) ----; new gpu/vk/shaders/late_fix.comp):
    • take_capture copies the late pages again with no wait. late_fix_build then records, for the write-backs still on the GPU, which words they wrote.
    • late_fix_emit dispatches late_fix.comp after a full barrier (it can run several times in one recording's upload command buffer, with shadow_sync_copy writing the shadows it reads in between; the dedupe leaves the barrier out when nothing came in between), which writes those words into the capture's buffer before anything copies out of it: mirror_sync, mirror_make, the write-back. The result is word for word what the wait's copy gave.
  • Which words (late_fix_build):
    • For the write-back diff pass, per-word stamps: wb_diff.comp now stamps the submission's value on exactly the words it changes, in 64 KB stamp blocks tied to the pool's write-back shadow.
    • For whole copies, a 4 KB snapshot per late page that every whole copy also writes. 256 slots; once all are taken, snap_pages_reclaim gives back the slots of pages not late for a whole copy in 1024 recordings, with no recording not done yet writing or reading the slot and no fix of theirs pending.
    • Pools of 16 MB or less with whole-copied late pages get a shadow instead (+8 MB of VRAM at the ranch). A pool that is gone leaves that set (late_fix_reap).
  • Falls back to the wait, as before, whenever it can't be exact:
    • no stamps or snapshot yet (the first time a page is late);
    • a page written in place by the GPU (note_direct_write, now recorded in wb_direct);
    • both kinds pending;
    • a page whose earlier fix isn't recorded yet;
    • a shadow that changed;
    • no fix table free;
    • a CPU read of the page before its fix (late_fix_cpu_read).
  • The write-back shadow follows the mirror's sync (shadow_sync_copy, shadow_synced_known, RX_VK_SHADOW_FOLLOWS_SYNC). This fixes an existing write-back bug the fix exposed: the diff put stale words back over newer CPU writes every frame. In OoT, the per-character animation records' padding (0x6f6f6f6f) went back over floats the CPU had written, which showed as black flashes in a play test of an earlier version.
  • Off: with RX_VK_LATE_FIX=0, under RX_VK_PARALLEL>1, or when the pipeline can't be made. Capture buffers get storage/address usage only with the fix on.
  • Tests:
    • tests/test_late_fix_build.py: when a capture gets a fix and when it waits; the snapshot slots given back; and, from the text, the barrier before the fix's dispatch.
    • tests/test_wb_shadow_follows_sync.py
    • tests/test_late_fix_shaders.py: the shaders' structs match the C++ ones, and the SPIR-V headers match their sources.

Flags and settings

Flag What it does Default now Recommendation Helps on Doesn't help on Untested on
RX_VK_LATE_FIX =0: the late pages' wait, as before on on: exact by construction (0 differing words in 306 M compared), falls back to the wait where it can't be exact PCs with VRAM mirrors in busy scenes (the wait is per frame there) iGPUs / Steam Deck (no mirror, so no shadow: it waits as before); PCs that never wait (Joey's Linux PC: 0 waits) AMD, Intel, Linux
RX_VK_PARALLEL (existing, read here) >1: the parallel replay; the late pages' fix is then off (its workers record copies out of the captures in any order) and captures wait as before unchanged (off) leave as is — under RX_VK_PARALLEL>1, the fix is off the fix under the parallel replay (off by design)
RX_VK_SHADOW_FOLLOWS_SYNC =0: the write-back shadow is filled from guest memory at the write-back, as before on on: without it the diff writes stale words over the CPU's newer ones any PC with write-back shadows — AMD, Intel, Linux

The two new ones are documented in docs/development.md beside RX_VK_WB_DIFF; RX_VK_PARALLEL is already documented there.

Measurements

WIP: the Longshot A/B (main against this PR at the current head, the ranch, interleaved) is still to do. The numbers below come from the same change on Hookshot, with the switch off and on in one build, before the review fixes (the barrier before the fix's dispatch, the snapshot slots given back) and before the rebase onto main @ f87557ae; nobody has measured this head.

Baseline: Hookshot @ 56fbc038, RX_VK_LATE_FIX=0
Why not main: measured before the move to Longshot (the switch off is the wait, as main). To be replaced by main against this PR at its head.
Hardware: RTX 4070 SUPER 12 GB, NVIDIA 617.42, i7-14700K, 32 GB, Windows 11 Pro 26200
Scene: Lon Lon Ranch at noon, 2560x1440 DLAA, cap 60, unattended scripted run (ranch-day3), 90 s each
Method: the frame trace (RX_VK_FRAME_TRACE=1), 2-second windows at presents 1920-4800; the slow stretch is presents 3840-4320; three runs: on, off, on
How to reproduce: a save on the grass by Lon Lon Ranch, !cheat 112 (noon), stand and turn; RX_VK_FRAME_TRACE=1

Hookshot @ 56fbc038, RX_VK_LATE_FIX=0 Hookshot @ 56fbc038, fix on (run 1 / run 2)
fps, slow stretch 53.0 57.4 / 58.3
memory sync a frame, slow stretch 5.04 ms 0.37 / 0.37 ms
late-pages wait a frame (whole run) 1.47 ms 0.08 / 0.10 ms
2-second windows under 55 fps 4 0 / 0
write-back shadows 412 MB 420 MB

On build 134 (before review, same method): fix on 57.9-60.2 fps, median 16.4-16.5 ms, 19-23 frames over 20 ms of 481; off 53.1-55.8 fps, 18.4 ms, 48.

Correctness (a TEMP verify patch, not part of this PR): every late page's bytes after the fix were compared with what the wait would have copied.

vk: TEMP verify: 298800 pages compared: words same 305971200, cpu 0, other 0 (0 on pages written back again since the capture, 0 equal to guest memory now); pages the wait changed 1387 (1400 words, the fix got 1400); pages written back again since the capture 0

Testing

Tested:

  • The game-free checks that read the changed files, on Windows with clang, at 1c567b5a (before the review fixes and the rebase): 39 of 39 pass, including the 3 new tests. Each new test fails when the rule it guards is reverted (20 mutations, one at a time).
  • In the game (Hookshot builds of this change):
    • the TEMP verify above;
    • ranch timing, on and off;
    • the flash detector (RX_VK_FLASH=1) at the ranch and in Kokiri Forest;
    • one player session on an RTX 4070 SUPER: an earlier version showed black flashes, traced to the stale-shadow bug fixed here.
  • Three independent code reviews (GPU ordering, CPU side and locks, tests and rules) found 9 problems, all fixed:
    • a stamp block re-tied to another pool;
    • in-place writes;
    • the stamp block's first recording;
    • mirror_make reading before the fix;
    • two captures of one page;
    • unbounded snapshot ranges;
    • shadow-follows-sync and freed shadows;
    • switch parsing;
    • usage flags with the fix off.

Not tested:

  • This head (04e86241: the review fixes, rebased onto main @ f87557ae): no build, no checks and no game run yet. The fixes were written by reading the code; the CI runs for this head are to be read, and the game-free checks run.
  • Longshot builds of this change in a player's session.
  • AMD, Intel and Linux.
  • Long sessions: with the slots given back, a page needs a slot only while late; more than 256 pages late for whole copies within 1024 recordings would still wait.

Needs other testers:

  • AMD (RADV and Windows): the ranch or another busy spot, RX_VK_LATE_FIX=0 against default. Expect the late-pages wait gone (the frame trace's late_wait_ms ~0) and no flashes (T-5RMT, the request the testing wanted label opened).
  • Linux/NVIDIA: the ranch or another busy spot, RX_VK_LATE_FIX=0 against default; expect the late-pages wait gone and no flashes (T-5RMT, the request the testing wanted label opened).
  • Steam Deck: no change expected (no mirror: falls back to the wait). Confirm nothing breaks (T-5RMT, the request the testing wanted label opened).

Screenshots

Nothing on screen (frame pacing only); flash-detector runs found nothing.

Risk and rollback

A wrong word in a capture would show as a one-frame artifact (a flash). A slot given back too early would be one too: it is given back only once every recording that wrote or read it is done, and a new page's fixes read only the ranges its own copies wrote. Design limit: 256 snapshot slots at once; a page late for a whole copy beyond them waits, as before. Every case that can't be exact falls back to the old wait, and the verify found 0 differing words. To turn it off: RX_VK_LATE_FIX=0, and RX_VK_SHADOW_FOLLOWS_SYNC=0 for the shadow part. Reverting the commit is also enough.

Reviewer notes

  • Start at late_fix_build (when it fixes and when it waits) and late_fix_emit (where it records).
  • The shadow-follows-sync part fixes an older write-back bug on its own. It can be split into its own fix(vk) PR if you prefer.
  • This answers the review of NotRexed/Hookshot PR 2789 (the GPU path never ran inside a burst): the fix is recorded lazily in the same part's upload command buffer, so it runs in bursts too. It also merges word by word, not page by page.

Issues

See NotRexed/Hookshot issue 2655 (the late-pages wait). No Longshot issue yet.

Checklist

For everyone

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main (f87557ae).
  • python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above). Not yet for this head (04e86241): the checks reading the changed files passed at 1c567b5a (39/39); this head's CI runs and a full run are to be read.
  • Tested in the game, or what wasn't is named under Testing.
  • Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests): T-5RMT.
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. Pending: the Longshot A/B.
  • No game files, dumps, build output or generated files.
  • third_party/ untouched (a new folder with a PROVENANCE.md instead).
  • The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

  • AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
  • Flags noted, and the switch reference updated (docs/development.md, the README).
  • A file in docs/tested/ for each new measured run (format in docs/tested-configurations.md; no appended row or assembled view). With the Longshot A/B, still to do.
  • The SPIR-V headers regenerated with glslang (a shader changed).
  • A game-free test added for the fix, failing on main and passing here. Added; passed at 1c567b5a, not run yet on this head (the new lines in tests/test_late_fix_build.py).
  • A check under tests/ that reads changed source updated in this PR, not weakened.
  • Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. Built on Windows (clang) at 1c567b5a; this head not built yet on either.
  • Docs updated.

🤖 Generated with Claude Code

## Why In busy places (Lon Lon Ranch by day, crowds, many characters) the game drops from 60 to 50-55 fps, and on some PCs to the 30s, because the renderer waits on the CPU every frame for the GPU to finish writing an earlier frame's results back to game memory: "late pages". At Lon Lon Ranch that wait is ~4.7 ms of a 17.8 ms frame; a player with an RTX 5090 measured ~5.8 ms a frame with 15-30 ms spikes, and an RTX 3080 player saw 31-34 fps ([NotRexed/Hookshot issue 2655](https://github.com/NotRexed/Hookshot/issues/2655), the thread of [NotRexed/Hookshot PR 2789](https://github.com/NotRexed/Hookshot/pull/2789)). This removes the wait on PCs whose pools have VRAM mirrors. ## What changed - **The wait becomes a GPU fix-up** (`gpu/vk/vk_memory.cpp`, the block `// ---- the late pages' fix (longshot, 2026-11-13) ----`; new `gpu/vk/shaders/late_fix.comp`): - `take_capture` copies the late pages again with no wait. `late_fix_build` then records, for the write-backs still on the GPU, which words they wrote. - `late_fix_emit` dispatches `late_fix.comp` after a full barrier (it can run several times in one recording's upload command buffer, with `shadow_sync_copy` writing the shadows it reads in between; the dedupe leaves the barrier out when nothing came in between), which writes those words into the capture's buffer before anything copies out of it: `mirror_sync`, `mirror_make`, the write-back. The result is word for word what the wait's copy gave. - **Which words** (`late_fix_build`): - For the write-back diff pass, per-word stamps: `wb_diff.comp` now stamps the submission's value on exactly the words it changes, in 64 KB stamp blocks tied to the pool's write-back shadow. - For whole copies, a 4 KB snapshot per late page that every whole copy also writes. 256 slots; once all are taken, `snap_pages_reclaim` gives back the slots of pages not late for a whole copy in 1024 recordings, with no recording not done yet writing or reading the slot and no fix of theirs pending. - Pools of 16 MB or less with whole-copied late pages get a shadow instead (+8 MB of VRAM at the ranch). A pool that is gone leaves that set (`late_fix_reap`). - **Falls back to the wait, as before,** whenever it can't be exact: - no stamps or snapshot yet (the first time a page is late); - a page written in place by the GPU (`note_direct_write`, now recorded in `wb_direct`); - both kinds pending; - a page whose earlier fix isn't recorded yet; - a shadow that changed; - no fix table free; - a CPU read of the page before its fix (`late_fix_cpu_read`). - **The write-back shadow follows the mirror's sync** (`shadow_sync_copy`, `shadow_synced_known`, `RX_VK_SHADOW_FOLLOWS_SYNC`). This fixes an existing write-back bug the fix exposed: the diff put stale words back over newer CPU writes every frame. In OoT, the per-character animation records' padding (`0x6f6f6f6f`) went back over floats the CPU had written, which showed as black flashes in a play test of an earlier version. - **Off:** with `RX_VK_LATE_FIX=0`, under `RX_VK_PARALLEL>1`, or when the pipeline can't be made. Capture buffers get storage/address usage only with the fix on. - **Tests:** - `tests/test_late_fix_build.py`: when a capture gets a fix and when it waits; the snapshot slots given back; and, from the text, the barrier before the fix's dispatch. - `tests/test_wb_shadow_follows_sync.py` - `tests/test_late_fix_shaders.py`: the shaders' structs match the C++ ones, and the SPIR-V headers match their sources. ## Flags and settings | Flag | What it does | Default now | Recommendation | Helps on | Doesn't help on | Untested on | |---|---|---|---|---|---|---| | `RX_VK_LATE_FIX` | `=0`: the late pages' wait, as before | on | on: exact by construction (0 differing words in 306 M compared), falls back to the wait where it can't be exact | PCs with VRAM mirrors in busy scenes (the wait is per frame there) | iGPUs / Steam Deck (no mirror, so no shadow: it waits as before); PCs that never wait (Joey's Linux PC: 0 waits) | AMD, Intel, Linux | | `RX_VK_PARALLEL` (existing, read here) | `>1`: the parallel replay; the late pages' fix is then off (its workers record copies out of the captures in any order) and captures wait as before | unchanged (off) | leave as is | — | under `RX_VK_PARALLEL>1`, the fix is off | the fix under the parallel replay (off by design) | | `RX_VK_SHADOW_FOLLOWS_SYNC` | `=0`: the write-back shadow is filled from guest memory at the write-back, as before | on | on: without it the diff writes stale words over the CPU's newer ones | any PC with write-back shadows | — | AMD, Intel, Linux | The two new ones are documented in `docs/development.md` beside `RX_VK_WB_DIFF`; `RX_VK_PARALLEL` is already documented there. ## Measurements **WIP: the Longshot A/B (main against this PR at the current head, the ranch, interleaved) is still to do.** The numbers below come from the same change on Hookshot, with the switch off and on in one build, before the review fixes (the barrier before the fix's dispatch, the snapshot slots given back) and before the rebase onto main @ f87557ae; nobody has measured this head. Baseline: Hookshot @ 56fbc038, RX_VK_LATE_FIX=0 Why not main: measured before the move to Longshot (the switch off is the wait, as main). To be replaced by main against this PR at its head. Hardware: RTX 4070 SUPER 12 GB, NVIDIA 617.42, i7-14700K, 32 GB, Windows 11 Pro 26200 Scene: Lon Lon Ranch at noon, 2560x1440 DLAA, cap 60, unattended scripted run (`ranch-day3`), 90 s each Method: the frame trace (`RX_VK_FRAME_TRACE=1`), 2-second windows at presents 1920-4800; the slow stretch is presents 3840-4320; three runs: on, off, on How to reproduce: a save on the grass by Lon Lon Ranch, `!cheat 112` (noon), stand and turn; `RX_VK_FRAME_TRACE=1` | | Hookshot @ 56fbc038, RX_VK_LATE_FIX=0 | Hookshot @ 56fbc038, fix on (run 1 / run 2) | |---|---|---| | fps, slow stretch | 53.0 | 57.4 / 58.3 | | memory sync a frame, slow stretch | 5.04 ms | 0.37 / 0.37 ms | | late-pages wait a frame (whole run) | 1.47 ms | 0.08 / 0.10 ms | | 2-second windows under 55 fps | 4 | 0 / 0 | | write-back shadows | 412 MB | 420 MB | On build 134 (before review, same method): fix on 57.9-60.2 fps, median 16.4-16.5 ms, 19-23 frames over 20 ms of 481; off 53.1-55.8 fps, 18.4 ms, 48. **Correctness** (a TEMP verify patch, not part of this PR): every late page's bytes after the fix were compared with what the wait would have copied. ``` vk: TEMP verify: 298800 pages compared: words same 305971200, cpu 0, other 0 (0 on pages written back again since the capture, 0 equal to guest memory now); pages the wait changed 1387 (1400 words, the fix got 1400); pages written back again since the capture 0 ``` ## Testing **Tested:** - The game-free checks that read the changed files, on Windows with clang, at 1c567b5a (before the review fixes and the rebase): 39 of 39 pass, including the 3 new tests. Each new test fails when the rule it guards is reverted (20 mutations, one at a time). - In the game (Hookshot builds of this change): - the TEMP verify above; - ranch timing, on and off; - the flash detector (`RX_VK_FLASH=1`) at the ranch and in Kokiri Forest; - one player session on an RTX 4070 SUPER: an earlier version showed black flashes, traced to the stale-shadow bug fixed here. - Three independent code reviews (GPU ordering, CPU side and locks, tests and rules) found 9 problems, all fixed: - a stamp block re-tied to another pool; - in-place writes; - the stamp block's first recording; - `mirror_make` reading before the fix; - two captures of one page; - unbounded snapshot ranges; - shadow-follows-sync and freed shadows; - switch parsing; - usage flags with the fix off. **Not tested:** - This head (04e86241: the review fixes, rebased onto main @ f87557ae): no build, no checks and no game run yet. The fixes were written by reading the code; the CI runs for this head are to be read, and the game-free checks run. - Longshot builds of this change in a player's session. - AMD, Intel and Linux. - Long sessions: with the slots given back, a page needs a slot only while late; more than 256 pages late for whole copies within 1024 recordings would still wait. **Needs other testers:** - AMD (RADV and Windows): the ranch or another busy spot, `RX_VK_LATE_FIX=0` against default. Expect the late-pages wait gone (the frame trace's `late_wait_ms` ~0) and no flashes (T-5RMT, the request the `testing wanted` label opened). - Linux/NVIDIA: the ranch or another busy spot, `RX_VK_LATE_FIX=0` against default; expect the late-pages wait gone and no flashes (T-5RMT, the request the `testing wanted` label opened). - Steam Deck: no change expected (no mirror: falls back to the wait). Confirm nothing breaks (T-5RMT, the request the `testing wanted` label opened). ## Screenshots Nothing on screen (frame pacing only); flash-detector runs found nothing. ## Risk and rollback A wrong word in a capture would show as a one-frame artifact (a flash). A slot given back too early would be one too: it is given back only once every recording that wrote or read it is done, and a new page's fixes read only the ranges its own copies wrote. Design limit: 256 snapshot slots at once; a page late for a whole copy beyond them waits, as before. Every case that can't be exact falls back to the old wait, and the verify found 0 differing words. To turn it off: `RX_VK_LATE_FIX=0`, and `RX_VK_SHADOW_FOLLOWS_SYNC=0` for the shadow part. Reverting the commit is also enough. ## Reviewer notes - Start at `late_fix_build` (when it fixes and when it waits) and `late_fix_emit` (where it records). - The shadow-follows-sync part fixes an older write-back bug on its own. It can be split into its own `fix(vk)` PR if you prefer. - This answers the review of [NotRexed/Hookshot PR 2789](https://github.com/NotRexed/Hookshot/pull/2789) (the GPU path never ran inside a burst): the fix is recorded lazily in the same part's upload command buffer, so it runs in bursts too. It also merges word by word, not page by page. ## Issues See [NotRexed/Hookshot issue 2655](https://github.com/NotRexed/Hookshot/issues/2655) (the late-pages wait). No Longshot issue yet. ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main` (f87557ae). - [ ] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). Not yet for this head (04e86241): the checks reading the changed files passed at 1c567b5a (39/39); this head's CI runs and a full run are to be read. - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests): T-5RMT. - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. **Pending: the Longshot A/B.** - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] Flags noted, and the switch reference updated (docs/development.md, the README). - [ ] A file in `docs/tested/` for each new measured run (format in `docs/tested-configurations.md`; no appended row or assembled view). With the Longshot A/B, still to do. - [x] The SPIR-V headers regenerated with glslang (a shader changed). - [ ] A game-free test added for the fix, failing on `main` and passing here. Added; passed at 1c567b5a, not run yet on this head (the new lines in `tests/test_late_fix_build.py`). - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [ ] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. Built on Windows (clang) at 1c567b5a; this head not built yet on either. - [x] Docs updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ported from Hookshot perf/late-fix-main (b8e4ffda) onto Longshot main as one change:
the late pages' fix (shaders/late_fix.comp, per-word stamps from wb_diff.comp, 4 KB page
snapshots for whole copies, small pools given a shadow), the write-back shadow following
the mirror's sync (RX_VK_SHADOW_FOLLOWS_SYNC), the fixes from review, and the game-free
tests (test_late_fix_build, test_wb_shadow_follows_sync, test_late_fix_shaders).
RX_VK_LATE_FIX=0: the wait, as before.

Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 <[email protected]>
docs: RX_VK_LATE_FIX and RX_VK_SHADOW_FOLLOWS_SYNC in the switch reference
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 9s
Website / Website on Cloudflare (pull_request_target) Has been skipped
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
1c567b5a73
Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 <[email protected]>
HumanBean added this to the v3.0.0 milestone 2026-11-13 17:46:05 +00:00
HumanBean left a comment

Review of PR 248 (late pages' fix) at 1c567b5a. Code review only, no build or test run here.

The design holds up as far as I can follow it. The stamps only mark words a pending write-back changed. The fill value and since keep a new block's stamps out until its recording is done. Re-tied or freed stamp blocks and changed shadows drop the fix in late_fix_emit and fall back to the wait. The table and stamp lifetimes are tied to mem_rec_value/mem_completed. Take and record are serialised under replay_mu (submit_memory_work), so the window between late_fix_take and late_fix_kept is safe.

One thing to change before merge:

  • late_fix_emit has no barrier before its dispatch (inline). late_fix_emit can run more than once in one recording's upload command buffer, at every mirror_sync/mirror_make/memory_end_submission. Between two of those calls, shadow_sync_copy (new in this PR) writes the shadow in the same command buffer. The next dispatch then reads that shadow (src) with nothing ordering the two. The only barrier is the trailing full_barrier(up) of the previous emit. Add full_barrier(up) before vkCmdBindPipeline; the dedupe makes it free when nothing came in between. It would be worth a line in test_late_fix_build.py or the shader test so it stays.

Smaller (inline): a comment that belongs to late_fix_shadows_freed sits on shadow_synced_freed. Snapshot slots and late_shadow_pools are never given back, so a long session runs out of the 256 slots for good. The PR names that under Not tested, but it is a design limit rather than a test gap.

Checklist / state: it is still WIP, rightly: the Longshot A/B, the tested-configurations rows and the three test requests are open. CI for this head (runs 1783 and 1784: Python source checks, SPIR-V fixtures, third-party) was still pending when I looked, so the run_all.py box can be settled from those runs once they finish. pr status also notes that RX_VK_PARALLEL (an existing switch the fix reads) is missing from Flags and settings. One line there saying the fix is off under RX_VK_PARALLEL>1 would clear that.

Review of PR 248 (late pages' fix) at 1c567b5a. Code review only, no build or test run here. The design holds up as far as I can follow it. The stamps only mark words a pending write-back changed. The fill value and `since` keep a new block's stamps out until its recording is done. Re-tied or freed stamp blocks and changed shadows drop the fix in `late_fix_emit` and fall back to the wait. The table and stamp lifetimes are tied to `mem_rec_value`/`mem_completed`. Take and record are serialised under `replay_mu` (`submit_memory_work`), so the window between `late_fix_take` and `late_fix_kept` is safe. **One thing to change before merge:** - `late_fix_emit` has no barrier before its dispatch (inline). `late_fix_emit` can run more than once in one recording's upload command buffer, at every `mirror_sync`/`mirror_make`/`memory_end_submission`. Between two of those calls, `shadow_sync_copy` (new in this PR) writes the shadow in the same command buffer. The next dispatch then reads that shadow (`src`) with nothing ordering the two. The only barrier is the trailing `full_barrier(up)` of the *previous* emit. Add `full_barrier(up)` before `vkCmdBindPipeline`; the dedupe makes it free when nothing came in between. It would be worth a line in `test_late_fix_build.py` or the shader test so it stays. **Smaller (inline):** a comment that belongs to `late_fix_shadows_freed` sits on `shadow_synced_freed`. Snapshot slots and `late_shadow_pools` are never given back, so a long session runs out of the 256 slots for good. The PR names that under *Not tested*, but it is a design limit rather than a test gap. **Checklist / state:** it is still WIP, rightly: the Longshot A/B, the tested-configurations rows and the three test requests are open. CI for this head (runs 1783 and 1784: Python source checks, SPIR-V fixtures, third-party) was still pending when I looked, so the `run_all.py` box can be settled from those runs once they finish. `pr status` also notes that `RX_VK_PARALLEL` (an existing switch the fix reads) is missing from *Flags and settings*. One line there saying the fix is off under `RX_VK_PARALLEL>1` would clear that.
@ -1905,0 +1977,4 @@
VkDeviceAddress snap_address = 0;
uint32_t snap_used = 0;
bool snap_page_make(uint32_t pg) {
if (snap_pages.count(pg) || snap_used >= kSnapSlots) return false;
Owner

Snapshot slots are never given back. snap_pages entries stay after their pool or mirror goes, and late_shadow_pools only grows. So after 256 distinct whole-copied late pages, every new one falls back to the wait for the rest of the session (fail(3) each time). That is fine for a first version, but worth a reclaim: for example, drop a SnapPage whose ranges are all done and that hasn't been late for N captures. Or say in the description that this is a limit of the design, not only an untested case.

Snapshot slots are never given back. `snap_pages` entries stay after their pool or mirror goes, and `late_shadow_pools` only grows. So after 256 distinct whole-copied late pages, every new one falls back to the wait for the rest of the session (`fail(3)` each time). That is fine for a first version, but worth a reclaim: for example, drop a `SnapPage` whose `ranges` are all done and that hasn't been late for N captures. Or say in the description that this is a limit of the design, not only an untested case.
@ -1905,0 +2403,4 @@
flush_mapped(t->alloc, bytes);
GprofSpan gp(up, "late pages' fix (shadows, snapshots -> capture buffers)");
gp.items = (int)late_fixes.size();
vkCmdBindPipeline(up, VK_PIPELINE_BIND_POINT_COMPUTE, late_fix_pipe.pipe);
Owner

No barrier between what earlier commands in this upload command buffer wrote and this dispatch's reads. late_fix_emit runs at every mirror_sync/mirror_make/memory_end_submission of a recording. Between two of those calls, shadow_sync_copy writes p.shadow in the same up, and this dispatch reads the shadow as src for the diff fixes. Only the previous emit's trailing full_barrier(up) comes before that copy, not after it. With a guest-memory sync (p.buffer as the GPU gets there), the two can give different words, so the result depends on ordering, and sync validation will flag it. Suggest full_barrier(up, "late pages' fix (before)"); before the bind; the dedupe makes it free when nothing came in between.

No barrier between what earlier commands in this upload command buffer wrote and this dispatch's reads. `late_fix_emit` runs at every `mirror_sync`/`mirror_make`/`memory_end_submission` of a recording. Between two of those calls, `shadow_sync_copy` writes `p.shadow` in the same `up`, and this dispatch reads the shadow as `src` for the diff fixes. Only the previous emit's trailing `full_barrier(up)` comes before that copy, not after it. With a guest-memory sync (`p.buffer` as the GPU gets there), the two can give different words, so the result depends on ordering, and sync validation will flag it. Suggest `full_barrier(up, "late pages' fix (before)");` before the bind; the dedupe makes it free when nothing came in between.
@ -4072,9 +4633,34 @@ template <class F> void wbd_split(uint64_t at, uint64_t size, F&& emit) {
}
if (end > body_end) emit(body_end, end - body_end, false);
}
// (pools_mu held) the late pages' fix: a stamp block whose shadow was freed counts for none: filled again and tied to its
Owner

Nit: this comment ("a stamp block whose shadow was freed counts for none...") describes late_fix_shadows_freed below, not shadow_synced_freed, which it now sits above. Move it down to its function.

Nit: this comment ("a stamp block whose shadow was freed counts for none...") describes `late_fix_shadows_freed` below, not `shadow_synced_freed`, which it now sits above. Move it down to its function.
HumanBean force-pushed perf/late-pages-fix from 1c567b5a73
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 9s
Website / Website on Cloudflare (pull_request_target) Has been skipped
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
to 04e8624165
Some checks failed
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been skipped
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been skipped
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been skipped
Third-party parts / The same as upstream (pull_request) Has been skipped
Website / Website on Cloudflare (pull_request) Has been skipped
CI / Conventional Commit title (pull_request) Successful in 6s
CI / Workflow lint (actionlint) (pull_request) Successful in 11s
Third-party parts / As their notes say (pull_request) Successful in 15s
PR description / Description follows the template (pull_request_target) Failing after 4s
Website / Website on Cloudflare (pull_request_target) Has been skipped
2026-11-14 01:50:39 +00:00
Compare
Owner

Review fixes for the review, rebased onto main @ f87557ae, pushed as 04e86241 (the author's two commits are now 8301269a and f8ee479b, unchanged in what they add and remove; the fixes are a third commit, fix(vk): ...).

  • Barrier before the fix's dispatch (must): late_fix_emit records full_barrier(up, "late pages' fix (before)") before it binds the pipeline, so whatever shadow_sync_copy (or the stamps' fills) wrote earlier in the same upload command buffer is ordered before the fix reads it. The dedupe leaves it out when nothing came in between. tests/test_late_fix_build.py now reads late_fix_emit as text and fails if no full_barrier(up comes after the last vkCmd before the bind.
  • Comment (nit): the stamp-block comment moved from shadow_synced_freed to late_fix_shadows_freed.
  • Snapshot slots (should): when all 256 are taken, snap_pages_reclaim (called from snap_page_make, under pools_mu) gives back the slots of pages that haven't been late for a whole copy in 1024 recordings, and only when nothing still in flight uses the slot: SnapPage.used (the last recording that writes the slot in late_snap_whole or reads it in a fix recorded by late_fix_emit) must be done, and the page must have no late_pages entry with a fix not recorded yet or not done. A reused slot's new page starts with no ranges and since = the next recording, so its fixes never read the old page's words. late_shadow_pools drops pools that are gone, every 64 reaps. New test case in test_late_fix_build.py. Still a design limit: more than 256 pages late for whole copies within 1024 recordings wait as before (named under Risk).
  • Description: RX_VK_PARALLEL added to Flags and settings. The Baseline line now names its column. Hookshot links are full URLs. The test lines name T-5RMT. The tested-runs box uses main's new wording (docs/tested/).
  • Rebase: one conflict in take_capture, where main's RangeWalk (in_tex.in(...)) met this PR's late-page skip. Both are kept. test_vk_range_walk.py's count of in_tex.in( is still 3. No other conflicts, and the diff against main touches only this PR's 9 files.

Nothing was built or run here: no compiler, no checks, no game. The C++ and the test were written by reading. Still needed:

  • this head's CI and a run_all.py run (including the new lines in test_late_fix_build.py);
  • a Windows and a Linux build;
  • the Longshot A/B and its docs/tested/ files;
  • T-5RMT's results.

The PR stays WIP.

Review fixes for [the review](https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/248#issuecomment-15543), rebased onto main @ f87557ae, pushed as 04e86241 (the author's two commits are now 8301269a and f8ee479b, unchanged in what they add and remove; the fixes are a third commit, `fix(vk): ...`). - **Barrier before the fix's dispatch** (must): `late_fix_emit` records `full_barrier(up, "late pages' fix (before)")` before it binds the pipeline, so whatever `shadow_sync_copy` (or the stamps' fills) wrote earlier in the same upload command buffer is ordered before the fix reads it. The dedupe leaves it out when nothing came in between. `tests/test_late_fix_build.py` now reads `late_fix_emit` as text and fails if no `full_barrier(up` comes after the last `vkCmd` before the bind. - **Comment** (nit): the stamp-block comment moved from `shadow_synced_freed` to `late_fix_shadows_freed`. - **Snapshot slots** (should): when all 256 are taken, `snap_pages_reclaim` (called from `snap_page_make`, under `pools_mu`) gives back the slots of pages that haven't been late for a whole copy in 1024 recordings, and only when nothing still in flight uses the slot: `SnapPage.used` (the last recording that writes the slot in `late_snap_whole` or reads it in a fix recorded by `late_fix_emit`) must be done, and the page must have no `late_pages` entry with a fix not recorded yet or not done. A reused slot's new page starts with no ranges and `since` = the next recording, so its fixes never read the old page's words. `late_shadow_pools` drops pools that are gone, every 64 reaps. New test case in `test_late_fix_build.py`. Still a design limit: more than 256 pages late for whole copies within 1024 recordings wait as before (named under Risk). - **Description:** `RX_VK_PARALLEL` added to *Flags and settings*. The Baseline line now names its column. Hookshot links are full URLs. The test lines name T-5RMT. The tested-runs box uses main's new wording (`docs/tested/`). - **Rebase:** one conflict in `take_capture`, where main's `RangeWalk` (`in_tex.in(...)`) met this PR's late-page skip. Both are kept. `test_vk_range_walk.py`'s count of `in_tex.in(` is still 3. No other conflicts, and the diff against main touches only this PR's 9 files. **Nothing was built or run here:** no compiler, no checks, no game. The C++ and the test were written by reading. Still needed: - this head's CI and a `run_all.py` run (including the new lines in `test_late_fix_build.py`); - a Windows and a Linux build; - the Longshot A/B and its `docs/tested/` files; - T-5RMT's results. The PR stays WIP.
HumanBean changed title from WIP: perf(vk): fix the late pages on the GPU, word by word, instead of waiting for their write-back to perf(vk): fix the late pages on the GPU, word by word, instead of waiting for their write-back 2026-11-14 01:55:28 +00:00
HumanBean deleted branch perf/late-pages-fix 2026-11-14 01:55:31 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Longshot!248
No description provided.