Repository navigation
Conversation
…CPU reads or writes them OoT writes ~68 MB a frame through the GPU into bindings over 1 MB (culling lists and per-frame buffers), and every byte was copied back to guest memory at the end of its submission. The CPU touches only small parts of them: four 4.3 MB buffers it reads in full, and ~12 pages a frame it writes inside the culling pool. A binding over RX_VK_WB_MAX_KB (now 1024 by default) is written back whole for its first RX_VK_WB_PROBATION writes (8) while the CPU's reads (invalidate) and the writes a mirror sync copies are noted by range; after that only those ranges are written back. A range the CPU first touches later is written back from then on, and logged. Hyrule Field, RTX 3060 Ti: 67.4 -> 17.9 MB written back a frame, 56.8 -> 58.0 fps (mean of three alternated runs each), no CPU touch of a range left out. RX_VK_WB_MAX_KB=0 writes everything back as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, this is a careful follow-up to #384's experiment: the plain cap's two hazards are clearly explained, and learning what the CPU touches, rather than guessing from size, is the right shape. wb_learn.h is small, has no Vulkan in it, and is unit-tested, which makes it easy to review. CI is green.
What I checked and found sound:
- Ordering in
invalidate. The read is noted before the write-back wait. So work that is recorded during the catch-up /sync_settlewait already keeps the range, andtest_wb_learn.pypins that order. Ranges the CPU reads keep their write-back, somemory_writeback_valuestill finds them and the readback wait (exposure meter, PR 430's refresh) is unchanged for them. - CPU writes in
mirror_sync. They are noted before the page copy, and at page granularity, so a page the CPU writes during probation keeps the GPU's rest of the page from then on. That is the flicker case from the whole-page sync. - Learned state. It is keyed by guest range, not by pool, so retiring or re-importing a pool doesn't lose it. A binding at the same start with a different size starts a new probation.
gpu_writtenalready keeps these pools' mirrors from being given back, so the left-out data isn't dropped bymirrors_give_back. - Locking.
wb_learn_muis always taken last: afterpools_mufrom the bind path andmirror_sync, and on its own frominvalidate. I didn't find a lock-order problem with the parallel replay or the readback refresh. - Off switch.
RX_VK_WB_MAX_KB=0restores the old behaviour exactly.
Points I'd like addressed before this becomes the default (details inline):
- Probation counts bind calls, not submissions. Written binds aren't memoized, so a buffer that several dispatches write in one frame uses up its 8 writes in a frame or two. The CPU's first read of that buffer can come after the replay has already recorded the next frame. In that case the read finds stale data, which is exactly the case probation is meant to cover. Counting at most one write per submission would make
RX_VK_WB_PROBATION=8mean what the README says. - First touch is stale by design. A range the CPU first reads or writes after probation gets the old copy once. The logs in the description show this is rare in these scenes, but it is silent wrong data when it happens, and it's on by default. Could the late touch be repaired instead of only logged? For example,
invalidatecould queue that range's write-back from the mirror, which still holds the GPU's data, and wait for it as it does for kept ranges. If that's not practical, please at least say in the README that an occasional one-off stale read is the trade-off. - The table only grows.
seen_andread_are never pruned, and every invalidated range goes intoread_, including small readbacks that are in no followed binding. Once either table reaches 8192 entries,full_latches and the session silently falls back to writing everything back. Please report thebindings followed/ranges the CPU readcounts from the log after a long session (30+ min, several scene changes, a save reload). Filtering reads throughfollowed(), as writes already are, would also help. - Per-page cost in
mirror_sync. Every dirty page now takeswb_learn_muand walksseen_back up to 1 GB, and that work runs while the replay threads plan their binds. The walk is cheap while there are few followed bindings, but it grows with them (see 3). Checking once per pool whether it overlaps any followed binding would keep this off the common path.
Measurements (our rule from #2712). Thanks for the before/after table: it covers the same scenes, alternates the arms, names the GPU and driver, and includes the late-touch counts. That's what we ask for. For a new default I'd still like:
- A second machine, ideally Linux and/or AMD, and the slower-PCIe case the description expects to gain most (laptop or x8). As measured, the gain is about 2% on the 3060 Ti in Hyrule Field and Kokiri Forest, and the Market is noise-bound, as you note. The default needs to stand on more than one NVIDIA/Windows box.
- The screenshot pairs attached. The description says they match. Please also say explicitly that no white flashes, exposure steps or fog from the previous frame showed up during the runs, since those are the past symptoms of stale write-backs.
Nit: the comment in gpu_buffer_bind quotes "57 -> 61 fps and no frame over 50 ms". 61 fps is the plain cap's figure in the description, and this PR measured about 58.8 -> 60.0 median, so I'd quote this PR's own numbers.
No overlap with #2338 or #1377 that would block this, but both edit the same writebacks.push_back block in gpu_buffer_bind. #1377 marks a binding over the cap gpu_only. Whichever lands second needs a small rebase there, and with this PR "not written back" becomes partial rather than whole.
| it->second = Seen{hi, 0, false}; // (another binding at this start: its own probation) | ||
| } | ||
| if (it->second.writes < probation) { | ||
| it->second.writes++; |
There was a problem hiding this comment.
writes counts every written bind, and written binds skip the bind memo. A buffer that N dispatches write per frame therefore finishes probation after about 8/N frames, possibly before the CPU's first invalidate of its results is seen (the replay can be a frame ahead of that read). Counting at most once per submission would fix it: pass the recording's submission value into plan() and only increment when it changes.
| if (it->second > hi) hi = it->second; | ||
| it = read_.erase(it); | ||
| } | ||
| if (read_.size() >= max_read) full_ = true; |
There was a problem hiding this comment.
Once full_ is set it never clears, and neither seen_ nor read_ is ever trimmed. Every invalidated range ends up here, including ones outside any large binding, so a long session may drift into writing everything back with only one log line saying so. Two suggestions: filter reads through followed() in memory_note_cpu_access, as writes already are, and please report the counts from a long run.
| } | ||
| // (hookshot) the CPU wrote this page: in a large binding whose write-back may be left out, the copy below would bring | ||
| // guest memory's stale rest of the page over the GPU's: that binding is written back from now on (wb_learn.h) | ||
| if (dirty) memory_note_cpu_access(page << kPageShift, 1ull << kPageShift, true); |
There was a problem hiding this comment.
This takes wb_learn_mu and runs followed()'s backward walk over seen_ (up to 1 GB below) for every dirty page of every synced pool. Meanwhile the replay threads' binds wait on the same mutex in wb_leave_out. It might be worth checking once per pool, before the page loop, whether [p.lo, p.hi) overlaps anything followed, and skipping the per-page call otherwise.
| { | ||
| std::lock_guard lk(wb_learn_mu); | ||
| if (write && !wb_table.followed(guest, guest + size)) return; | ||
| stale = wb_table.read(guest, guest + size); |
There was a problem hiding this comment.
When stale is true, the CPU is about to read (or the sync is about to copy) a range whose last write-back was left out. The mirror still has the GPU's data, since gpu_written keeps it. Could the read path queue that range's write-back and wait for it here, instead of only logging? That would turn the first-touch case from wrong data once into a slow read once.
| // (hookshot) a binding over RX_VK_WB_MAX_KB (1024 by default; 0: none) that the CPU has not read through its | ||
| // probation queues no write-back: OoT writes tens of MB of culling lists and per-frame buffers through the GPU that | ||
| // the CPU never reads, and copying them back was most of a submit's memory sync (issue 384: 68 MB a frame; Hyrule | ||
| // Field on an RTX 3060 Ti, 57 -> 61 fps and no frame over 50 ms). Learned, not a plain size cap: the CPU does read |
There was a problem hiding this comment.
Nit: 57 -> 61 fps is the plain cap's result in the description. This PR measured about 58.8 -> 60.0 median and 57.2 -> 58.6 mean in Hyrule Field, so I'd quote that, or just the 68 -> 18 MB a frame.
A binding that starts or ends inside a page shares that page with what is next to it. The 15 MB culling lists end 0x80 bytes into a page the CPU writes, and a mirror sync copies that page whole, so the binding's last bytes were found stale once (Goron City, Zora's Domain) before the page was learned. Those pages are now written back from the binding's first write: at most 8 KB a binding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stead of latching full Review of NotRexed#2731 (hckrman101): - A binding bound by several dispatches in one submission counted each bind toward its probation, so it could finish in a frame. Probation now counts the submissions that write it (mirror_epoch), and a binding is left out only from the submission after its last one. - Past its bounds the table latched "full" and wrote everything back for the rest of the session. It now forgets everything and every binding starts its probation again; the log counts resets. - CPU reads are noted only in a large binding the table follows, as writes already were, so small readbacks elsewhere do not fill it. - A mirror sync asks once per pool whether it overlaps a followed binding before noting its pages. - The readme states the trade-off: a range first touched after the probation is stale once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the careful review. Pushed
Nit fixed (the comment now quotes 68 -> 18 MB a frame). Measurements on the final code (with #2711 merged locally in both arms, since it changes this card's VRAM picture a lot; Hyrule Field, same test as the description, arms alternated):
So with #2711 in, Hyrule Field on this card is within noise (GPU ~0.2 ms less); the description's ~2% was measured with Second machine. I only have this PC (RTX 3060 Ti, Windows). The other box I can reach is a Steam Deck, where RADV runs without buffer mirrors, so this path never runs there. A Linux/AMD desktop or a laptop / x8 card would need someone else; it is fine by me if this waits for one, or goes in default-off until then. Screenshots and flashes. One round of the same 12-scene tour with this PR and one with |
|
One case in When the table of CPU-touched ranges is one short of A small check against the real header (from the repository root: The lines are the capacity test inside How likely: not seen in play here. A five-scene tour on this PC ended with 31 bindings and 43 ranges, far from 8,192, so it would take a long session with very scattered CPU reads. But when it happens nothing says so, and it only matters once the default leaves large bindings out, so it seemed worth fixing before that. Not tested for this note: Windows (the check compiles for the Windows target; it was not run there). |
…d for good in Hyrule Field RX_VK_WB_SKIP_LEARN keeps +1.4% in Kokiri Forest with stale first touches: not taken. The slow no-cap run in Hyrule Field was a 224 MB pool whose mirror allocation failed once and was refused for good (5.1 ms readback waits); upstream NotRexed#2618 is taken for it, and NotRexed#2731 is to be measured against this branch's learning cap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…of two speeds Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… both When the table of CPU-touched ranges was one short of full, edges() added the first page, the second add reset the table and only the last edge page was kept, so the first shared page could be left out (found by mooki000). edges() now makes room for both before adding either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@mooki000 thanks, confirmed and fixed in |
|
The same check on |
Joey-Magnus
left a comment
There was a problem hiding this comment.
I merged 309e6c3 into current main and ran tests/test_wb_learn.py, test_direct_buffer_writes.py and test_parallel_rb_refresh.py: all pass.
Against hckrman101's review: the probation now counts submissions (plan() takes mirror_epoch), the table resets instead of latching, reads are filtered through followed(), mirror_sync checks once per pool, and the nit is fixed. mooki000's edge-page case is fixed in edges() and covered by a test. The point still open is the second one: a range first touched after probation still gets stale data once. That is now documented, not repaired.
So what remains is a product call. The default goes from off to 1024 KB for everyone on the strength of one RTX 3060 Ti on Windows. With #2711 in, Hyrule Field is within noise there. The Steam Deck never runs this path, and AMD/Intel desktops and slower PCIe links haven't been run. mooki000's Linux game results are still to come. I'd ship it default-off until a second machine shows a gain, as you offered.
One small fix whichever way that goes: RX_VK_WB_PROBATION is read with getenv/atoi (gpu/vk/vk_memory.cpp:2710). Please use env_int.
|
Test request T-SRQJ: pass Hardware: AMD Radeon RX 580 8 GB (RADV Polaris10), Mesa 26.2.6, CachyOS Linux, 8 threads, 15 GB RAM. Built bce3d17 (the PR head) against main 51da046 (main is 3 commits past the PR's merge base, none of them in What I checked: Kokiri Forest (the test save), then Debug teleports to Hyrule Field and the Market; in each a 34 s window starting 8 s after the scene first drew, the camera turning (right stick held 30 s). On this card the PR's code does not run by default. GCN keeps the VRAM mirror off ( Default (no mirror), median fps per run → median of runs; 1% low:
The PR's diagnostics (mirror on): Correctness (mirror on, the arm where the PR acts):
Summary: it's safe on this card and changes nothing in its default config, because GCN has no VRAM mirror and so nothing to write back. With the mirror forced on it writes back ~50 MB a frame less and shows nothing stale, but there's no fps gain on an RX 580. |
Why. Every frame OoT writes ~68 MB through the GPU into bindings over 1 MB (culling lists and per-frame buffers), and all of it is copied back to guest memory at the end of its submission (#384: ~4.4 ms a submit on an RTX 3080). The CPU touches only small parts of those bindings.
RX_VK_WB_MAX_KB=1024(opt-in, #384's experiment) skips them all and is fast, but it is not safe:invalidate, 236 of every 3,000 CPU reads in Hyrule Field landed in a skipped range: four 4.3 MB buffers (+0x44b040) read in full after every frame the GPU writes them, and an 8.4 MB range now and then. Those reads got what guest memory held before the GPU wrote them. The same in Kokiri Forest (168 of 3,000), the Market (76) and Kakariko (128).0x1178000000), always the same ones. A mirror sync copies such a page whole from guest memory, so with the plain cap the rest of the page went back to stale bytes over the GPU's: ~28,000 pages in a 40 s run.This keeps most of the saving and drops both hazards. It also makes it the default.
What
gpu/vk/wb_learn.h(plain rules, no Vulkan), held byvk_memory.cppunder its own mutex:RX_VK_WB_MAX_KB(now 1024 by default) is written back whole in the firstRX_VK_WB_PROBATIONsubmissions that write it (8; several binds in one submission count once).invalidate, before its write-back wait) and the CPU writes a mirror sync copies (each dirty page in a binding the table follows) are noted by range.vk: the CPU read/wrote ..., in a binding whose write-back was left out).resetsin the log line). CPU reads and writes are noted only inside a binding the table follows, and a mirror sync asks once per pool.RX_VK_WB_MAX_KB=0writes every binding back, as before.RX_VK_LOG_WB_SKIPS=1lists the ranges left out every 600 submits. A log line every 600 submits gives the count and MB left out.Measured
Windows 11, RTX 3060 Ti 8 GB (driver 32.0.16.2114), Ryzen 5 5600X, 2560x1440 DLSS,
main5b96a55; 40 s with the camera turning after a settle,RX_VK_FRAMEGEN=0,HOS_FPS_MAX=240,RX_VK_IMAGE_PREALLOC=0in both arms (#2690), arms alternated.RX_VK_WB_MAX_KB=0(as before)The Market's GPU time changes from launch to launch on 8 GB cards (blushOFF's note in #995), so two runs each there are suggestive, not proof. Late CPU touches of a range left out, which the log names: one page in one Market run; and, in the three runs started from a later save, one sweep of ~1,690 consecutive pages (
0x117ffbd000on, one 6.3 MB buffer) during the teleport's scene load, right after the new scene's pools are imported. That is the CPU setting up memory the title screen's buffers had used, whole pages, so nothing stale is mixed in; those pages are written back from then on. Fps of those runs: 58.2 / 57.6 / 55.7 mean.The frame here is mostly the GPU and the once-a-frame exposure wait, so 50 MB less PCIe traffic a frame is worth ~2% on this card; it should be worth more where the copies are slower (laptops, x8 cards). The plain cap was faster (~61 fps, 9 MB a frame) because it also left out the pages the CPU writes and reads, which is the stale data above.
Screenshots at the same spot with and without it match.
Updates after review, the long-session counts and the flash check: see the comments below.
Tests
tests/test_wb_learn.pycompiles and runstests/test_wb_learn.cpp(probation, a binding read every write, pages written during probation, touches clipped to the binding, a late touch reported once, merging, the bounds) and checks thatvk_memory.cppandvk_replay.cppstill call it where they should.tests/direct_buffer_writes_test.cppandtests/parallel_rb_refresh_test.cppget stubs for the new calls (every write-back queued, as before).python -I -B tests/run_all.pyon Windows: 455 passed, 3 failed, 34 skipped. The three fail on unchangedmaintoo here (nx2/test_nup_paths.pyandtests/test_input_background.py: this machine's file links and input;tests/test_finish_output.pyonly when run beside the others, and passes alone).🤖 Generated with Claude Code