[aquinas22]

What

Mirrors go to the pools the GPU reads most. When the hottest pool waiting for a mirror does not fit in the budget, it takes the mirrors of pools that read a quarter as much or less, coldest first.

Why

A pool refused for the mirrors' budget got a mirror only once another mirror went unbound for 600 submissions (mirrors_give_back). In play the budget fills with pools that are bound now and then but read little, so they never go idle. A pool bound in nearly every submission then stays in guest memory for the whole session, and the GPU reads it over PCIe.

From hookshot_log.txt on an RTX A1000 Laptop (4 GB, mirror budget 495 MB):

vk: pool 0x7fd7000000 (32 MB) stays in system memory: the mirrors have 480 MB of 495
vk:   bound in guest memory, by pool (the GPU reads these over PCIe; ... over these 600 submits, 33205.3 MB in all):
      0x116b000000 (144 MB, state 1): 4118 binds, 3098 MB; ... 0x107f000000 (160 MB, state 1): 513 binds, 29408 MB; ...

Here the GPU sat at about 70% use, drawing 15 W of its 45 W, and memory sync took 14–19 ms of each frame.

How

• Each Pool counts the bytes of the ranges bound from it. This covers every binding, including bind-memo hits (the memo now keeps the pool), and is one relaxed atomic_ref add. The count is halved every 64 submissions (heat).
• mirrors_swap_for_hot, at a submission's start, only for the hottest wanted pool:
  • victims must be mirrors the GPU is done with (last_value <= mem_completed, the same rule mirrors_give_back uses), made 600+ submissions ago, and each under a quarter of the wanted pool's heat;
  • all victims together must be under a quarter of the wanted pool's heat as well;
  • the wanted pool itself needs 64+ MB of ranges per window;
  • at most one swap per 16 submissions;
  • the wanted pool is made first, so the space freed goes to it.
• Freeing uses the existing mirror_free: the page table goes back to guest memory, and pools_gen changes, which also invalidates the bind memo.
• RX_VK_MIRROR_HEAT=0 restores the old behaviour. It is documented beside RX_VK_MIRROR_MB.
• The choice itself is mirror_swap_pick, a pure function covered by tests/test_mirror_heat.py.

Testing

• Hardware and setup: RTX A1000 Laptop GPU (4 GB, driver 596.58), i7-12800HX, Windows 11. Minimum (4GB) settings: handheld profile, Resolution 50%, frame generation off.

• In play: run as hookshot_vk.dll built from v2.0.11 plus this change. I compiled the same diff on main, but I have not run that build in game.

• First version, now fixed: tried every wanted pool and had no heat floor, so 16 MB pools traded mirrors back and forth (256 swaps), kept the mirrors young, and the 160 MB pool never got in. This version tries the hottest pool only, makes it first, and adds the 64 MB floor.

• This version: the 160 MB pool took the mirrors of four cold pools on its first swap:

  vk: pool 0x107f000000 (160 MB, 158077 MB bound a window) takes the mirrors of 4 colder pools (64 MB, 39 MB bound a window; 1 swaps so; ...)
  

  Over a long session there were 16 swaps, each a hot pool taking cold ones, with no back-and-forth and no device lost.

	Ranges bound in guest memory, per 600 submits	Memory sync per frame
Before	32–33 GB	15–19 ms
After	3.2–3.3 GB	9–10 ms (in a scene with twice the draws)

• Not shown: a clean fps gain. After the change I was in a heavier scene (3,959 draws a frame against 1,600–1,900), where the frame was limited by the replay's draw recording (about 9 µs a draw both before and after), so the frame rates aren't comparable. On this card the change moves traffic off PCIe, but I can't claim a measured fps gain. An A/B with RX_VK_MIRROR_HEAT=0 in the same spot on more cards would settle it.
• python -I -B tests/run_all.py mirror_heat passes.

[hckrman101]

Thanks, this is a well-argued change and the numbers from the A1000 are convincing. mirror_swap_pick is clean and the test covers the cases that matter, the heat counting on memo hits is safe (a stale memo can't hit after pools_gen moves, and retired nodes stay alive), and I don't see a conflict with #1127 or #1376: the hunks don't overlap, victims are only taken from imported (never the retired pools #1127 inherits from), and the swap runs before mirror_make, so a remade hot pool still inherits through #1127's page filter.

One thing I'd like sorted before merge, about which mirrors can be victims (inline). Freeing a mirror is only lossless if guest memory already has everything the GPU wrote into it. The write-back copies just the bound written ranges. It doesn't copy the bare-table guesses past the first 64 bytes, the pointer table's ranges, or stores through the page table: the comments around rb_hazard_locked / RX_VK_PROTECT_BARE and the EvilOOT note in retire_pool describe exactly that GPU-only data (Kokiri's foliage records). Today mirrors_give_back frees a live mirror only after 600 submissions without a sync, so a pool that's bound every frame keeps its mirror. The swap drops that guard. A pool bound every frame through a small binding (64 bytes of a bare entry), with the culling writing past it, has almost no heat, so it's the first victim. Its records go back to stale guest memory, and the remade-larger inheritance can't help because nothing is retired. That's the missing-foliage / trees-as-columns class of bug again, and it would show on small-VRAM cards, which are the ones this targets.

Options: skip victims the GPU may have written outside their write-backs (a per-pool flag set from rb_hazard_locked / written binds, cleared on mirror free/make), or at least require mirror_epoch - p.synced_epoch > some idle window on top of made_epoch. Either keeps the A1000 win, since the cold pools there look read-mostly.

Smaller things, all nits:

• After the victims are freed, mirror_make(w) can still fail (RX_VK_MIRROR_TIGHT, or create_device_buffer failing), and then the victims were dropped for nothing. You could check the result and log it. It's rare, so fine to leave.
• heat = heat/2 + window settles at about 2x a window's bytes, so the "MB bound a window" in the log and the 64 MB floor are really about 32 MB a window. It's worth wording the log or the floor to match.
• Heat counts bound range bytes, not reads, so a wide binding that's read sparsely counts as hot, and page-table reads count as nothing. That's fine as a heuristic, but the description's "the pools the GPU reads most" is a bit generous.

CI didn't report any checks on this branch. I haven't built it here.

[hckrman101]

Here a victim only needs last_value <= mem_completed and an old mirror. It can still be in use every frame. If the GPU wrote into it outside the write-back's ranges (bare-entry stores past 64 bytes, page-table stores, see rb_hazard_locked), mirror_free sends the page table back to guest memory that doesn't have those writes, and since such a pool binds little it's the coldest candidate. Could you exclude pools with GPU write hazards since their last sync (or require mirror_epoch - p.synced_epoch > kIdleEpochs-ish, as mirrors_give_back does)?

[hckrman101]

Nit: mirror_make(*w) below can still fail (RX_VK_MIRROR_TIGHT, or a failed create_device_buffer), and then these were freed for nothing. Maybe make w right here and log if it fails?

[hckrman101]

Nit: with heat/2 + window the steady state is about 2x one window's bytes, so the 64 MB floor is really about 32 MB a window, and the log's "MB bound a window" overstates by about 2x.

[hckrman101]

Thanks for merging main. The only new commit since my last review is that merge (the run_all.py conflict looks resolved correctly), so the earlier points are where they were:

• Still open: which mirrors can be victims (the inline thread on mirrors_swap_for_hot). A victim still only needs last_value <= mem_completed and made_epoch 600+ submissions back. A pool that's bound every frame through a small range, while the GPU writes outside its write-backs (bare-entry stores, page-table stores, see rb_hazard_locked), is still the coldest candidate, and freeing it drops those writes. A per-pool "GPU wrote outside write-backs" flag, or a mirror_epoch - p.synced_epoch idle window like mirrors_give_back uses, would settle it.
• The three nits (unchecked mirror_make after freeing, the ~2x heat steady state against the 64 MB floor and the log wording, "reads" vs bound bytes) are also unchanged. They're still fine to leave or fix as you like.

New, and it's this PR's own: CI's "Python source checks" fails on both Linux and Windows in tests/test_parallel_rb_refresh.py (ValueError: substring not found). That test, from main, cuts gpu_buffer_bind from the mirror_sync(*p); line up to the marker return remember({p->mirror. This PR changes that line to return remember(p, {p->mirror, so the marker no longer exists. It isn't one of main's known failures. Updating the end marker in the test to "return remember(p, {p->mirror" (or just "return remember(") should fix it, since the cut stops before that call anyway. Everything else is green, including the Windows Vulkan build.

[hckrman101]

This rename breaks tests/test_parallel_rb_refresh.py: it cuts gpu_buffer_bind up to the marker return remember({p->mirror, which is now gone (CI: ValueError: substring not found on Linux and Windows). Updating the marker in the test to return remember(p, {p->mirror should do it.

[hckrman101]

Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!

[aquinas22]

Thanks for the careful review. Merged main again (the conflicts were tests/run_all.py, now per-file declarations, so test_mirror_heat.py declares its own RUN_ALL_ARGS, and the unbound-binding path in gpu_buffer_bind, where main's shadow_forget and this PR's remember(p, ...) both stay). Then:

• Victims (the open point): went with the per-pool flag. Pool::gpu_only is set when the GPU may write a mirror somewhere no write-back copies: every pool memory_note_gpu_write touches (a bare entry's guessed range past its 64 bytes, the pointer table's, a wide binding), and a written binding whose write-back main's new RX_VK_WB_MAX_KB skips. It's cleared when the mirror is made or freed, and mirrors_swap_for_hot never takes a gpu_only mirror. Stores through the page table aren't seen anywhere, so with RX_VK_FOLLOW_STORES set the swap is off entirely. mirrors_give_back's own 600-idle rule is unchanged.
• CI: test_parallel_rb_refresh.py's end marker is now "return remember(p, {p->mirror".
• Nits: the swap now makes the hot pool's mirror itself (taking it out of mirror_wanted) and logs when that fails after the victims were freed. The log and comments now say heat is about 2x a window's bytes ("~N MB bound a window" prints heat/2; the 64 MB floor is described as ~32 MB a window). I left the floor's value alone, since that's what was tuned on the A1000. I'll also reword the description from "reads" to "binds".

test_mirror_heat.py also checks that the swap skips gpu_only mirrors and follows the FOLLOW_STORES rule. Built the Windows Vulkan back end locally with the builder's llvm-mingw.

[hckrman101]

Thanks, the points are all handled, and I read the new code.

Fixed:

• Victims: Pool::gpu_only is set from memory_note_gpu_write (bare-entry guesses, the pointer table, wide bindings) and from written bindings whose write-back RX_VK_WB_MAX_KB skips. It's cleared in mirror_make and mirror_free, and mirrors_swap_for_hot skips such pools. With RX_VK_FOLLOW_STORES the swap is off, which covers the page-table stores nothing sees. That closes the stale-guest-memory case I was worried about. The flag is conservative (a pool stays gpu_only until its mirror is remade), which is the safe side.
• CI marker: test_parallel_rb_refresh.py is updated and passes.
• Nits: the hot pool's mirror is now made right after the victims are freed, with a log line if that fails, and the log and comments say heat is about 2x a window's bytes.

CI: the only red checks are test_bake_mimo, test_bake_skeleton (Linux) and test_bake_ds, test_log_batch (Windows), and the last completed main runs fail the same ones with the same counts, so they are not from this PR. test_mirror_heat and the Windows Vulkan build pass. I haven't built or run this here.

Good to merge from my side.

[hckrman101]

Heads-up: this conflicts with main now. Could you rebase onto main and resolve the conflicts? Thanks!

[Joey-Magnus]

The approval still stands, and the gpu_only change addresses the victim concern. What blocks it now is a single conflict hunk in gpu/vk/vk_memory.cpp, in the not-mirrored path of gpu_buffer_bind. Main replaced the inline direct_write_epoch and wb_pages_note lines with note_direct_write(*p, off, size). The fix is to keep main's call and keep your return remember(p, {p->buffer, off, size, 0});.

I checked the other remember( sites: main and the branch call it in the same five places, so heat counting doesn't miss any binding path added since. Since #2343, GCN cards have no VRAM mirrors by default, so the swap does nothing there. That's fine.

After the rebase, please re-run CI so the Python source checks can be compared against current main's results.

[Joey-Magnus]

The approval and the gpu_only victim rule still stand, but main has moved under them. #2154 (merged) added its own Pool::gpu_only and gpu_written in vk_memory.cpp, and they mean something different: on main both flags are sticky, and they keep a mirror from ever being given back in mirrors_give_back. A trial merge of 59edbbe into current main conflicts in four hunks: memory_note_gpu_write, and three remember() returns in gpu_buffer_bind that now call note_in_place_read. The parts that merge silently are the real problem:

• The struct ends up with two bool gpu_only members (lines 80 and 86 of the merge), so it won't build.
• Your clears in mirror_make and mirror_free would reset main's flag, which undoes #2154's keep rule (its fix for cyan after a pause on the RX 5500M).
• mirrors_swap_for_hot checks gpu_only but not gpu_written, so a swap could still free a mirror that #2154 keeps.

I'd drop your flag, use main's gpu_only and gpu_written as they are (never cleared), and have the swap skip both. The only red in CI on this head is test_env_on on HOS_FPS_LOG, which was main's own failure at the time.