MagicBeanDev/Hookshot
Archived
Watch
2
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

fix(vk): remake write-back shadows given back under VRAM pressure #2767

Closed
github-sync wants to merge 1 commit from github/pr/2767 into main
pull from: github/pr/2767
merge into: MagicBeanDev:main
Conversation 3 Commits 1 Files changed 2 +288 -5
github-sync commented 2026-11-12 12:37:02 +00:00
Owner

@Revens2 opened this pull request on GitHub · 2026-11-12 12:06 UTC · original
Merged on GitHub in f1d0a67932

Why

When VRAM is tight the renderer gives back its write-back shadows, and it made one again only when a pool's own write-back reached 256 KB. After a scene change a pool's write-backs can stay under that size for good, so the shadow never came back: in the log on #2238, 48 of 352 MB stayed whole copies for the rest of the session while the others were made again. That leaves the pool without a shadow and its next write-back reads the mirror instead.

Fixes #2238 - FPS drops heavily and stays low after entering a new scene

Issue: #2238

What changed

  • gpu/vk/vk_memory.cpp: a pool whose write-back was worth a shadow is marked (Pool::shadow_wanted), and the mark stays after VRAM takes the shadow.
  • shadows_check_room now runs every 120 write-back submissions when a wanted pool has a mirror and no shadow, even with shadow_bytes at 0. Once the pressure rule says there is room, it makes up to 8 shadows per check (each is an allocation of the pool's whole size), skipping a pool whose mirror is gone or whose shadow cannot fit now without spending a try. It logs what this check made, not the session total.
  • tests/test_wb_shadow_remake.py runs the real functions from the source over stubs: the remake after a give-back, only wanted pools, the issue's exact give-back sequence, 8 per check, retired pools never, mirror-less and too-big pools not spending tries, the make lines, and VRAM unknown.

Testing

Game-free: python -I -B tests/run_all.py in the worktree, full suite.

  • 462 passed, 1 failed, 33 skipped (Windows, llvm-mingw).
  • Failed: tests/test_input_background.py, a declared known environment failure on this PC (no MSVC SDK).
  • The two declared environment failures tests/test_precompile_compute_guard.py and tests/test_recorder_sync.py passed in this run.
  • Focused: test_wb_shadow_remake passes.

In game: NVIDIA GeForce RTX 5070 12 GB, driver 32.0.16.2114 (Vulkan 1.4), Windows 11 build 26200, i5-14600KF. Hookshot v2.1.0, Vulkan back end, windowed 2560x1440, default quality preset, frame generation on, frame pacing on, 240 cap, Kokiri Forest clearing save. Pressure harness: RX_VK_TEST_ALLOWED_MB=4700 + RX_VK_VRAM_MB=4096. The allowance is synthetic: the driver really allows 11175 MB, so this exercises the rule but cannot reproduce real paging.

build run avg FPS 1% low p99 ms VRAM max
control v2.1.0 ctrl-p4700-a 94.75 77.16 12.96 5368 MB
control v2.1.0 ctrl-p4700-b 93.54 76.45 13.08 4908 MB
this PR p2238-p4700-a 96.10 77.76 12.86 4890 MB
this PR p2238-p4700-b 93.66 77.46 12.91 5368 MB
control mean 94.15 76.81 13.02
PR mean 94.88 77.61 12.89

Path evidence: the shadows given back fired in every run (1 + 1 per build) at 5322 MB used of the synthetic allowance, and over the driver's video-memory allowance fired once per run. shadows made again (the branch this PR adds) was 0 in all four runs: after the give-back the count stays above 97% of the synthetic allowance on this route, and the pools involved (112 MB + 16 MB) had no room under shadow_fits. The issue's repro needs a scene transition (Dodongo's Cavern); this route has none. fps is neutral within run-to-run spread (93.5-96.1 for both builds, same-binary repeats up to 4.5% apart). Not a real VRAM test: the driver never paged.

Relation to #2368

#2368 keeps drawn shadow maps through evictions (the cached 12288x12288 map sampled again). This PR remakes write-back shadows after a give-back under pressure. They affect different textures and files and can land in either order.

AI assistance

Assisted-by: oc-impl-4 (opencode/deepseek-v4.1-flash), oc-upstream-1 (opencode/deepseek-v4.1-flash-free)

Tracking: #993 (where #2238 was consolidated).

> [@Revens2](https://github.com/Revens2) opened this pull request on GitHub · 2026-11-12 12:06 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2767) > **Merged on GitHub** in https://git.magicbean.dev/MagicBeanDev/Hookshot/commit/f1d0a679320472dab62dc11aaca2b75fb8baa538 ## Why When VRAM is tight the renderer gives back its write-back shadows, and it made one again only when a pool's own write-back reached 256 KB. After a scene change a pool's write-backs can stay under that size for good, so the shadow never came back: in the log on #2238, 48 of 352 MB stayed whole copies for the rest of the session while the others were made again. That leaves the pool without a shadow and its next write-back reads the mirror instead. Fixes #2238 - FPS drops heavily and stays low after entering a new scene Issue: https://git.magicbean.dev/MagicBeanDev/Hookshot/issues/2238 ## What changed - `gpu/vk/vk_memory.cpp`: a pool whose write-back was worth a shadow is marked (`Pool::shadow_wanted`), and the mark stays after VRAM takes the shadow. - `shadows_check_room` now runs every 120 write-back submissions when a wanted pool has a mirror and no shadow, even with `shadow_bytes` at 0. Once the pressure rule says there is room, it makes up to 8 shadows per check (each is an allocation of the pool's whole size), skipping a pool whose mirror is gone or whose shadow cannot fit now without spending a try. It logs what this check made, not the session total. - `tests/test_wb_shadow_remake.py` runs the real functions from the source over stubs: the remake after a give-back, only wanted pools, the issue's exact give-back sequence, 8 per check, retired pools never, mirror-less and too-big pools not spending tries, the make lines, and VRAM unknown. ## Testing Game-free: `python -I -B tests/run_all.py` in the worktree, full suite. - 462 passed, 1 failed, 33 skipped (Windows, llvm-mingw). - Failed: `tests/test_input_background.py`, a declared known environment failure on this PC (no MSVC SDK). - The two declared environment failures `tests/test_precompile_compute_guard.py` and `tests/test_recorder_sync.py` passed in this run. - Focused: `test_wb_shadow_remake` passes. In game: NVIDIA GeForce RTX 5070 12 GB, driver 32.0.16.2114 (Vulkan 1.4), Windows 11 build 26200, i5-14600KF. Hookshot v2.1.0, Vulkan back end, windowed 2560x1440, default quality preset, frame generation on, frame pacing on, 240 cap, Kokiri Forest clearing save. Pressure harness: `RX_VK_TEST_ALLOWED_MB=4700` + `RX_VK_VRAM_MB=4096`. The allowance is synthetic: the driver really allows 11175 MB, so this exercises the rule but cannot reproduce real paging. | build | run | avg FPS | 1% low | p99 ms | VRAM max | |---|---|---|---|---|---| | control v2.1.0 | ctrl-p4700-a | 94.75 | 77.16 | 12.96 | 5368 MB | | control v2.1.0 | ctrl-p4700-b | 93.54 | 76.45 | 13.08 | 4908 MB | | this PR | p2238-p4700-a | 96.10 | 77.76 | 12.86 | 4890 MB | | this PR | p2238-p4700-b | 93.66 | 77.46 | 12.91 | 5368 MB | | control mean | | 94.15 | 76.81 | 13.02 | | | PR mean | | 94.88 | 77.61 | 12.89 | | Path evidence: `the shadows given back` fired in every run (1 + 1 per build) at 5322 MB used of the synthetic allowance, and `over the driver's video-memory allowance` fired once per run. `shadows made again` (the branch this PR adds) was 0 in all four runs: after the give-back the count stays above 97% of the synthetic allowance on this route, and the pools involved (112 MB + 16 MB) had no room under `shadow_fits`. The issue's repro needs a scene transition (Dodongo's Cavern); this route has none. fps is neutral within run-to-run spread (93.5-96.1 for both builds, same-binary repeats up to 4.5% apart). Not a real VRAM test: the driver never paged. ## Relation to #2368 #2368 keeps drawn shadow maps through evictions (the cached 12288x12288 map sampled again). This PR remakes write-back shadows after a give-back under pressure. They affect different textures and files and can land in either order. ## AI assistance Assisted-by: oc-impl-4 (opencode/deepseek-v4.1-flash), oc-upstream-1 (opencode/deepseek-v4.1-flash-free) Tracking: #993 (where #2238 was consolidated). <!-- github-sync:item:2767 -->
Assisted-by: oc-upstream-1 (opencode/deepseek-v4.1-flash-free)
Trace-Issue: #2238
github-sync commented 2026-11-12 14:47:48 +00:00
Author
Owner

@Joey-Magnus reviewed on GitHub · 2026-11-12 14:35 UTC · original

I merged this into current main and ran tests/test_wb_shadow_remake.py and the memory, shadow, write-back, mirror and vram checks from run_all (18 pass). It also merges cleanly with #2759 and #2766, which touch the same files. CI hasn't run on this head yet.

The change is sound. shadow_wanted is set only where a shadow was first made, and the remake in shadows_check_room goes through shadow_room, which reads VRAM again before each allocation. Shadows are only dropped above 95/99% (shadows_go) and only remade below 90/97% (shadow_fits), and that gap keeps it from making and dropping them every 120 submissions. Capping at 8 attempts per check and skipping pools without a mirror both make sense.

As the description says, the new branch fired 0 times in all four runs, so this is untested in game. The fps table only shows the change does no harm. A run that reaches the remake would show it works: a scene change under the synthetic allowance, with the allowance raised after the give-back, until write-back diff: N shadows made again appears in the log. Dodongo's Cavern from #2238 would be ideal.

> [@Joey-Magnus](https://github.com/Joey-Magnus) reviewed on GitHub · 2026-11-12 14:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2767#pullrequestreview-5876250416) I merged this into current main and ran tests/test_wb_shadow_remake.py and the memory, shadow, write-back, mirror and vram checks from run_all (18 pass). It also merges cleanly with #2759 and #2766, which touch the same files. CI hasn't run on this head yet. The change is sound. shadow_wanted is set only where a shadow was first made, and the remake in shadows_check_room goes through shadow_room, which reads VRAM again before each allocation. Shadows are only dropped above 95/99% (shadows_go) and only remade below 90/97% (shadow_fits), and that gap keeps it from making and dropping them every 120 submissions. Capping at 8 attempts per check and skipping pools without a mirror both make sense. As the description says, the new branch fired 0 times in all four runs, so this is untested in game. The fps table only shows the change does no harm. A run that reaches the remake would show it works: a scene change under the synthetic allowance, with the allowance raised after the give-back, until `write-back diff: N shadows made again` appears in the log. Dodongo's Cavern from #2238 would be ideal. <!-- github-sync:r:5876250416 -->
github-sync commented 2026-11-12 16:56:08 +00:00
Author
Owner

@hckrman101 reviewed on GitHub · 2026-11-12 16:33 UTC · original

Thanks for this, and for being upfront that the new branch never fired in your runs.

The code is safe. I checked what happens when a shadow is made again partway through a frame:

  • A remade shadow starts with shadow_known all zero. shadows_check_room runs at the top of the write-back block, after shadows_forget_written. So the first write-back that uses the shadow copies each page it touches from guest memory into the shadow, in the command stream, before the diff pass reads it. That's the same state as a brand-new shadow from the 256 KB path.
  • Writes the GPU hasn't written back yet are in the mirror, and a remake doesn't touch the mirror. The diff compares the mirror against a shadow just filled from guest memory, so every word the GPU changed still gets written.
  • The old shadow goes through dead_shadows, so it isn't destroyed until its submissions are done. Pools whose mirror was freed or that were retired are skipped. tries++ == 8 allows exactly 8 attempts.

Joey-Magnus already covered the hysteresis between shadows_go and shadow_fits, and I agree with him.

What's missing is evidence that this fixes #2238. Your table shows the change does no harm, but the remake count was 0 in all four runs, so the fix hasn't been seen working. I'm also not convinced this is what caused the drop. The pools that stay without a shadow are, by construction, the ones whose write-backs stay under 256 KB per submission. Their whole copies are small. The 48 MB in the issue's log is the size of the shadows they'd get, not how much they copy each frame. For a "drops heavily and stays low" report on an 8 GB laptop card, I'd expect the cause to be the VRAM pressure itself rather than those copies.

It also changes a trade-off. These pools now get a shadow the size of the whole pool, under pressure, for write-backs that the original 256 KB rule decided weren't worth one. shadow_fits limits the risk, but the gain needs showing.

Could you get one run where the branch actually fires? Same synthetic allowance, raise it after the give-back (or take a route with a scene change, ideally Death Mountain into Dodongo's Cavern from the issue), until shadows made again shows up in the log. Then post fps and p99 for that scene before and after the remake. Logs with RX_VK_LOG_WRITEBACK=1 would also show how many bytes those pools really copy each frame. If the gain is real it'll show there. If it isn't, this probably belongs in #993 as a finding rather than as the fix for #2238.

The test reads the real functions from the source, declares RUN_ALL_ARGS, and covers the cases that matter (mirror-less pools and pools that are too big don't use up a try, retired pools aren't remade).

Merge order: this merges cleanly with main, including #2731 (now merged), and Joey checked #2759 and #2766. #2794 (sparse mirrors) adds its own fields to Pool right after shadow_known, the same spot as shadow_wanted, so whichever lands second has a one-line conflict to resolve. Nothing interacts beyond that: a shadow is still sized to the whole pool under sparse mirrors.

CI on this head is waiting for workflow approval. So far only the title check has run.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 16:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2767#pullrequestreview-5878211349) Thanks for this, and for being upfront that the new branch never fired in your runs. **The code is safe.** I checked what happens when a shadow is made again partway through a frame: - A remade shadow starts with `shadow_known` all zero. `shadows_check_room` runs at the top of the write-back block, after `shadows_forget_written`. So the first write-back that uses the shadow copies each page it touches from guest memory into the shadow, in the command stream, before the diff pass reads it. That's the same state as a brand-new shadow from the 256 KB path. - Writes the GPU hasn't written back yet are in the mirror, and a remake doesn't touch the mirror. The diff compares the mirror against a shadow just filled from guest memory, so every word the GPU changed still gets written. - The old shadow goes through `dead_shadows`, so it isn't destroyed until its submissions are done. Pools whose mirror was freed or that were retired are skipped. `tries++ == 8` allows exactly 8 attempts. Joey-Magnus already covered the hysteresis between `shadows_go` and `shadow_fits`, and I agree with him. **What's missing is evidence that this fixes #2238.** Your table shows the change does no harm, but the remake count was 0 in all four runs, so the fix hasn't been seen working. I'm also not convinced this is what caused the drop. The pools that stay without a shadow are, by construction, the ones whose write-backs stay under 256 KB per submission. Their whole copies are small. The 48 MB in the issue's log is the size of the shadows they'd get, not how much they copy each frame. For a "drops heavily and stays low" report on an 8 GB laptop card, I'd expect the cause to be the VRAM pressure itself rather than those copies. It also changes a trade-off. These pools now get a shadow the size of the whole pool, under pressure, for write-backs that the original 256 KB rule decided weren't worth one. `shadow_fits` limits the risk, but the gain needs showing. Could you get one run where the branch actually fires? Same synthetic allowance, raise it after the give-back (or take a route with a scene change, ideally Death Mountain into Dodongo's Cavern from the issue), until `shadows made again` shows up in the log. Then post fps and p99 for that scene before and after the remake. Logs with `RX_VK_LOG_WRITEBACK=1` would also show how many bytes those pools really copy each frame. If the gain is real it'll show there. If it isn't, this probably belongs in #993 as a finding rather than as the fix for #2238. The test reads the real functions from the source, declares `RUN_ALL_ARGS`, and covers the cases that matter (mirror-less pools and pools that are too big don't use up a try, retired pools aren't remade). Merge order: this merges cleanly with main, including #2731 (now merged), and Joey checked #2759 and #2766. #2794 (sparse mirrors) adds its own fields to `Pool` right after `shadow_known`, the same spot as `shadow_wanted`, so whichever lands second has a one-line conflict to resolve. Nothing interacts beyond that: a shadow is still sized to the whole pool under sparse mirrors. CI on this head is waiting for workflow approval. So far only the title check has run. <!-- github-sync:r:5878211349 -->
github-sync closed this pull request 2026-11-12 17:59:05 +00:00
github-sync commented 2026-11-12 17:59:07 +00:00
Author
Owner

@Arkex-ai commented on GitHub · 2026-11-12 17:54 UTC · original

The new fixture fails Windows source CI when clang uses MSVC CRT headers: getenv is deprecated and -Werror rejects it in both hookshot_env.h and the fixture. Completed job and log.

The test, vk_memory.cpp and hookshot_env.h are byte-identical to main 86a9cda2 in the affected ASTC branch; the RomFS branch has the same failure with identical base inputs too. Could you carry the CRT compatibility define used by the existing tests/test_game_font.py into this fixture, preserving -Werror? This is a test compilation failure, not evidence against the shadow-remake behavior.

> [@Arkex-ai](https://github.com/Arkex-ai) commented on GitHub · 2026-11-12 17:54 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2767#issuecomment-6831568825) The new fixture fails Windows source CI when clang uses MSVC CRT headers: `getenv` is deprecated and `-Werror` rejects it in both `hookshot_env.h` and the fixture. [Completed job and log](https://github.com/NotRexed/Hookshot/actions/runs/43268770726/job/131359347036). The test, `vk_memory.cpp` and `hookshot_env.h` are byte-identical to main `86a9cda2` in the affected ASTC branch; the RomFS branch has the same failure with identical base inputs too. Could you carry the CRT compatibility define used by the existing `tests/test_game_font.py` into this fixture, preserving `-Werror`? This is a test compilation failure, not evidence against the shadow-remake behavior. <!-- github-sync:c:6831568825 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
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/Hookshot!2767
No description provided.