perf(vk): fix the late pages on the GPU, word by word, instead of waiting for their write-back #248
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!248
Loading…
Reference in a new issue
No description provided.
Delete branch "GummiNaomi/Longshot:perf/late-pages-fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
gpu/vk/vk_memory.cpp, the block// ---- the late pages' fix (longshot, 2026-11-13) ----; newgpu/vk/shaders/late_fix.comp):take_capturecopies the late pages again with no wait.late_fix_buildthen records, for the write-backs still on the GPU, which words they wrote.late_fix_emitdispatcheslate_fix.compafter a full barrier (it can run several times in one recording's upload command buffer, withshadow_sync_copywriting 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.late_fix_build):wb_diff.compnow stamps the submission's value on exactly the words it changes, in 64 KB stamp blocks tied to the pool's write-back shadow.snap_pages_reclaimgives 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.late_fix_reap).note_direct_write, now recorded inwb_direct);late_fix_cpu_read).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.RX_VK_LATE_FIX=0, underRX_VK_PARALLEL>1, or when the pipeline can't be made. Capture buffers get storage/address usage only with the fix on.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.pytests/test_late_fix_shaders.py: the shaders' structs match the C++ ones, and the SPIR-V headers match their sources.Flags and settings
RX_VK_LATE_FIX=0: the late pages' wait, as beforeRX_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 beforeRX_VK_PARALLEL>1, the fix is offRX_VK_SHADOW_FOLLOWS_SYNC=0: the write-back shadow is filled from guest memory at the write-back, as beforeThe two new ones are documented in
docs/development.mdbesideRX_VK_WB_DIFF;RX_VK_PARALLELis 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 eachMethod: 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, onHow to reproduce: a save on the grass by Lon Lon Ranch,
!cheat 112(noon), stand and turn;RX_VK_FRAME_TRACE=1On 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.
Testing
Tested:
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).RX_VK_FLASH=1) at the ranch and in Kokiri Forest;mirror_makereading before the fix;Not tested:
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.Needs other testers:
RX_VK_LATE_FIX=0against default. Expect the late-pages wait gone (the frame trace'slate_wait_ms~0) and no flashes (T-5RMT, the request thetesting wantedlabel opened).RX_VK_LATE_FIX=0against default; expect the late-pages wait gone and no flashes (T-5RMT, the request thetesting wantedlabel opened).testing wantedlabel 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, andRX_VK_SHADOW_FOLLOWS_SYNC=0for the shadow part. Reverting the commit is also enough.Reviewer notes
late_fix_build(when it fixes and when it waits) andlate_fix_emit(where it records).fix(vk)PR if you prefer.Issues
See NotRexed/Hookshot issue 2655 (the late-pages wait). No Longshot issue yet.
Checklist
For everyone
type(scope): description).main(f87557ae).python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above). Not yet for this head (04e86241): the checks reading the changed files passed at1c567b5a(39/39); this head's CI runs and a full run are to be read.third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.docs/tested/for each new measured run (format indocs/tested-configurations.md; no appended row or assembled view). With the Longshot A/B, still to do.mainand passing here. Added; passed at1c567b5a, not run yet on this head (the new lines intests/test_late_fix_build.py).tests/that reads changed source updated in this PR, not weakened.1c567b5a; this head not built yet on either.🤖 Generated with Claude Code
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
sincekeep a new block's stamps out until its recording is done. Re-tied or freed stamp blocks and changed shadows drop the fix inlate_fix_emitand fall back to the wait. The table and stamp lifetimes are tied tomem_rec_value/mem_completed. Take and record are serialised underreplay_mu(submit_memory_work), so the window betweenlate_fix_takeandlate_fix_keptis safe.One thing to change before merge:
late_fix_emithas no barrier before its dispatch (inline).late_fix_emitcan run more than once in one recording's upload command buffer, at everymirror_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 trailingfull_barrier(up)of the previous emit. Addfull_barrier(up)beforevkCmdBindPipeline; the dedupe makes it free when nothing came in between. It would be worth a line intest_late_fix_build.pyor the shader test so it stays.Smaller (inline): a comment that belongs to
late_fix_shadows_freedsits onshadow_synced_freed. Snapshot slots andlate_shadow_poolsare 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.pybox can be settled from those runs once they finish.pr statusalso notes thatRX_VK_PARALLEL(an existing switch the fix reads) is missing from Flags and settings. One line there saying the fix is off underRX_VK_PARALLEL>1would 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;Snapshot slots are never given back.
snap_pagesentries stay after their pool or mirror goes, andlate_shadow_poolsonly 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 aSnapPagewhoserangesare 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);No barrier between what earlier commands in this upload command buffer wrote and this dispatch's reads.
late_fix_emitruns at everymirror_sync/mirror_make/memory_end_submissionof a recording. Between two of those calls,shadow_sync_copywritesp.shadowin the sameup, and this dispatch reads the shadow assrcfor the diff fixes. Only the previous emit's trailingfull_barrier(up)comes before that copy, not after it. With a guest-memory sync (p.bufferas the GPU gets there), the two can give different words, so the result depends on ordering, and sync validation will flag it. Suggestfull_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 itsNit: this comment ("a stamp block whose shadow was freed counts for none...") describes
late_fix_shadows_freedbelow, notshadow_synced_freed, which it now sits above. Move it down to its function.1c567b5a7304e8624165Review fixes for the review, rebased onto main @
f87557ae, pushed as04e86241(the author's two commits are now8301269aandf8ee479b, unchanged in what they add and remove; the fixes are a third commit,fix(vk): ...).late_fix_emitrecordsfull_barrier(up, "late pages' fix (before)")before it binds the pipeline, so whatevershadow_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.pynow readslate_fix_emitas text and fails if nofull_barrier(upcomes after the lastvkCmdbefore the bind.shadow_synced_freedtolate_fix_shadows_freed.snap_pages_reclaim(called fromsnap_page_make, underpools_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 inlate_snap_wholeor reads it in a fix recorded bylate_fix_emit) must be done, and the page must have nolate_pagesentry with a fix not recorded yet or not done. A reused slot's new page starts with no ranges andsince= the next recording, so its fixes never read the old page's words.late_shadow_poolsdrops pools that are gone, every 64 reaps. New test case intest_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).RX_VK_PARALLELadded 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/).take_capture, where main'sRangeWalk(in_tex.in(...)) met this PR's late-page skip. Both are kept.test_vk_range_walk.py's count ofin_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:
run_all.pyrun (including the new lines intest_late_fix_build.py);docs/tested/files;The PR stays WIP.
WIP: perf(vk): fix the late pages on the GPU, word by word, instead of waiting for their write-backto perf(vk): fix the late pages on the GPU, word by word, instead of waiting for their write-back