perf(vk): keep four pages for the replay's reads as submitted, not one (Kokiri village view 50 -> 57 fps) #1762
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1762
Loading…
Reference in a new issue
No description provided.
Delete branch "perf/submitted-pages"
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?
Kokiri Forest drops to ~50 fps whenever the camera faces the village, the "46 fps minimum, dips to 38" camera stutter reported on Reddit. The replay spends about a fifth of its time re-copying the same 4 KB captured pages, because its read cache holds only one page.
Cause.
memory_read_as_submittedkeeps the last page it read so the next uniform words skippools_mu. A draw's handle reads (stage_handle → ub_read) take turns between its stages' uniform buffers on different pages. So with one page kept, each read evicted the other: lock, cap-table lookup, and a 4,096-bytememcpyof the captured page, every time. In a profile of that view (HOS_SAMPLE_ALL,scripts/crit.py), thatmemcpy(the 0x1000 copy intosubmitted_page.bytes) was ~22% of the Presentation thread's samples.Change. Keep four pages per thread, replacing the oldest first. The reuse rule is unchanged: a page is reused only while its write generation and the capture view are the ones it was copied at (
submitted_page_reuse), so a stamp or a new capture still drops all four.RX_VK_SUBMITTED_PAGES=1restores the single page. The every-20,000-reads log line now also counts the whole pages copied.Measured. Linux, RTX 4070 Ti (595.99.02), Ryzen 7 9800X3D. Integration build (main + open PRs), Kokiri village view standing still (2,224 draws),
HOS_FPS_MAX=120, 25 s per run withHOS_PRESENT_TRACE, two passes in A-B…B-A order:RX_VK_SUBMITTED_PAGES=1)+13.5% fps, and the slow-path reads fell ~3x. Turning off #1619's non-temporal capture copies (
RX_VK_CAPTURE_NT=0) made no difference (49.7 fps), so that isn't a factor here.Tests.
tests/test_submitted_page.pynow also buildssubmitted_find/submitted_slotfrom the source: two pages read in turn stay kept, a fifth replaces the oldest, a stamp or a new capture is not reused, andRX_VK_SUBMITTED_PAGES=1keeps one. It still checks that the lookup comes beforepools_mu. It fails onmainand passes here.run_all.py: 241 passed, 0 failed. (With the build's tools on PATH,test_finish_output.pyandtest_linux_update.pyfail onmaintoo, unrelated.) The renderer builds standalone from this branch.Not on screen: no visual change. Same frames, faster.
🤖 Generated with Claude Code
Looks right to me. The key hasn't changed, it's still page + page_gen + cap_view_gen per entry, so a stamp or a newly published capture invalidates all four slots the same way it did the single one. The slow path only fills a slot after
submitted_findmisses for the current (page, gen, view), so you can't end up with two live entries for the same key. An entry withoverlay == falsestill drops through togpu_readand is accepted by the second find, same as before. The cache is thread_local, so parallel replay workers each get their own four slots and there's nothing to race on;st_page_copiesis atomic. Reads that span two pages still skip the cache and go through the locked per-page path, unchanged.RX_VK_SUBMITTED_PAGES=1gives back the old behaviour exactly (next % 1).The fps number seems plausible from the code. With uniform reads alternating between two pages, every read on main paid for pools_mu, the cap-table lookup and a 4 KB memcpy. With four slots that becomes a scan of four entries. The test is auto-discovered by run_all.py, and it does test the find/slot code from the source and both env settings. CI was still pending when I looked. main has moved 21 commits but hasn't touched this function.
Heads-up: #1375 (32 hashed slots, and it drops the 4 KB page copy completely) and #1541 (16 hashed slots plus per-block validity) both rewrite this same
SubmittedPagecache in different ways. Whichever one lands first will conflict with the others, so it's probably worth picking one approach.A couple of nits inline, none of them blocking.
@ -1071,0 +1075,4 @@thread_local unsigned submitted_next;int submitted_ways() {static const int v = getenv("RX_VK_SUBMITTED_PAGES") ? std::clamp(atoi(getenv("RX_VK_SUBMITTED_PAGES")), 1, kSubmittedPages): kSubmittedPages;Nit: a non-numeric value (e.g.
RX_VK_SUBMITTED_PAGES=off) goes throughatoi-> 0 -> clamp to 1, so it silently means single-page. That's probably what you'd want anyway, just mentioning it. Callinggetenvonce into a local would also be a bit tidier.@ -1071,0 +1087,4 @@}/// the entry a new page goes into: the oldest, in turnSubmittedPage& submitted_slot() { return submitted_pages[submitted_next++ % (unsigned)submitted_ways()]; }std::atomic<int64_t> st_page_copies{0};Nit: this is FIFO by insertion, not LRU, because a hit in the fast path doesn't refresh the slot. That's fine for the two-page ping-pong. But when one hot page is mixed with 4+ others, the hot page still gets evicted every 4 misses. Also, a refill of page X after a stamp takes a fresh slot while X's stale entry stays in another one until it rotates out. Reusing a slot whose
.pagematches before falling back tonext++would avoid that. Neither of these is a correctness problem.