MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(vk): let render targets go as newer ones together cover them, and reuse their images at once #2766

Open

github-sync wants to merge 1 commit from github/pr/2766 into main

github-sync commented

2026-11-12 12:37:01 +00:00

Owner

@Revens2 opened this pull request on GitHub · 2026-11-12 12:01 UTC · original

Why

The renderer kept render-target images that newer render targets had already drawn over, because each newer target was judged alone: two or more partial overlaps that together cover an eighth of an old target were ignored. In #2180's guard room the target images then grew to 3-4 GB, VRAM reached the driver's allowance and the main and presentation threads stopped. On our A/B save, v2.1.0 let go 4,727 dead targets per run.

Fixes #2180 - v2.1.0, RX 9070 XT, 4K HDR + frame generation: render targets remade ~3.6 times a frame, VRAM reaches the allowed limit, then the main and presentation threads stop (freeze on breaking a jar)

Issue: #2180

What changed

target_overlap.h: DrawnOver adds up the shares of several newer render targets that drew over an old one (the candidates come in start order) instead of judging each one alone against the eighth. A view inside an old target is still not a new target (5bfa6aa, PR #899).
vk_texture.cpp texture_frame: the dead-target rule uses that sum.
vk_texture.cpp destroy_later: a render target's image goes to the pool as it is evicted, not at the next present. Between the two the next frame is recorded, and its target of the same create info made a new image while the old one was destroyed unused later.
Tests: tests/test_target_overlap.cpp (the union and its edge cases), tests/test_target_image_reuse.{cpp,py} (an evicted image is pooled at once and taken by the next frame, with stubs, plus source checks), tests/test_target_image_pool.py updated for the new eviction path.

Testing

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

459 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_target_image_reuse, test_target_image_pool, test_target_overlap pass.

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 3840x2160, render resolution 100%, frame generation on, Kokiri Forest clearing save, no synthetic VRAM allowance (real VRAM ~6352 MB used of 11175 MB).

Union rule, 2 runs per build:

build	run	avg FPS	1% low	p99 ms	max ms	over 33 ms
control v2.1.0	ctrl-4k-a	79.78	69.16	14.46	16.8	0
control v2.1.0	ctrl-4k-b	79.96	69.69	14.35	16.6	0
this PR (union rule)	p2180-4k-a	79.77	68.59	14.58	24.4	0
this PR (union rule)	p2180-4k-b	78.90	68.49	14.60	48.5	2
control mean		79.87	69.40	14.40
PR mean		79.34	68.54	14.59

Pool change added, 2 runs per build:

build	run	avg FPS	1% low	p99 ms	max ms	over 33 ms	let go (N / MB)	images reused / made / destroyed
control v2.1.0	ctrl-4k-c	78.71	68.49	14.60	35.9	1	5735 / 62249	5075 / 1211 / 1019
control v2.1.0	ctrl-4k-d	79.65	69.20	14.45	16.1	0	5167 / 55844	4368 / 1277 / 1094
this PR	p134b-4k-a	79.07	68.73	14.55	23.0	0	7776 / 84298	7207 / 1102 / 908
this PR	p134b-4k-b	79.24	69.44	14.40	15.8	0	7262 / 78704	6397 / 1575 / 1365
control mean		79.18	68.85	14.53			5451 / 59047	4722 / 1244 / 1057
PR mean		79.16	69.09	14.48			7519 / 81501	6802 / 1339 / 1137

Path evidence: dead targets let go 4,727 / 51,467 MB (control, union-rule session) against 7,807 / 85,078 MB with the union rule (+65%). With the pool change, images made new 2,441 -> 1,339 (the control's 1,244), pool reuse 4,722 -> 6,802 (+44%), and the union rule's gains are kept. Speed is neutral within the run-to-run spread (79.87 -> 79.34 -> 79.16 avg FPS). The issue's 16 GB AMD repro was not reproduced on this 12 GB card; the state line and pool counters are the evidence.

Relation to #2484

#2484 (draft) sweeps the dead-target pile more often and drains the pool faster. This PR changes which targets are dead at all (the union accounting) and when an evicted target's image is reused. They touch different hunks of texture_frame and pool_trim, and can land in either order.

AI assistance

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

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

> [@Revens2](https://github.com/Revens2) opened this pull request on GitHub · 2026-11-12 12:01 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2766) ## Why The renderer kept render-target images that newer render targets had already drawn over, because each newer target was judged alone: two or more partial overlaps that together cover an eighth of an old target were ignored. In #2180's guard room the target images then grew to 3-4 GB, VRAM reached the driver's allowance and the main and presentation threads stopped. On our A/B save, v2.1.0 let go 4,727 dead targets per run. Fixes #2180 - v2.1.0, RX 9070 XT, 4K HDR + frame generation: render targets remade ~3.6 times a frame, VRAM reaches the allowed limit, then the main and presentation threads stop (freeze on breaking a jar) Issue: https://git.magicbean.dev/MagicBeanDev/Hookshot/issues/2180 ## What changed - `target_overlap.h`: `DrawnOver` adds up the shares of several newer render targets that drew over an old one (the candidates come in start order) instead of judging each one alone against the eighth. A view inside an old target is still not a new target (5bfa6aa, PR #899). - `vk_texture.cpp` `texture_frame`: the dead-target rule uses that sum. - `vk_texture.cpp` `destroy_later`: a render target's image goes to the pool as it is evicted, not at the next present. Between the two the next frame is recorded, and its target of the same create info made a new image while the old one was destroyed unused later. - Tests: `tests/test_target_overlap.cpp` (the union and its edge cases), `tests/test_target_image_reuse.{cpp,py}` (an evicted image is pooled at once and taken by the next frame, with stubs, plus source checks), `tests/test_target_image_pool.py` updated for the new eviction path. ## Testing Game-free: `python -I -B tests/run_all.py` in the worktree, full suite. - 459 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_target_image_reuse`, `test_target_image_pool`, `test_target_overlap` pass. 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 3840x2160, render resolution 100%, frame generation on, Kokiri Forest clearing save, no synthetic VRAM allowance (real VRAM ~6352 MB used of 11175 MB). Union rule, 2 runs per build: | build | run | avg FPS | 1% low | p99 ms | max ms | over 33 ms | |---|---|---|---|---|---|---| | control v2.1.0 | ctrl-4k-a | 79.78 | 69.16 | 14.46 | 16.8 | 0 | | control v2.1.0 | ctrl-4k-b | 79.96 | 69.69 | 14.35 | 16.6 | 0 | | this PR (union rule) | p2180-4k-a | 79.77 | 68.59 | 14.58 | 24.4 | 0 | | this PR (union rule) | p2180-4k-b | 78.90 | 68.49 | 14.60 | 48.5 | 2 | | control mean | | 79.87 | 69.40 | 14.40 | | | | PR mean | | 79.34 | 68.54 | 14.59 | | | Pool change added, 2 runs per build: | build | run | avg FPS | 1% low | p99 ms | max ms | over 33 ms | let go (N / MB) | images reused / made / destroyed | |---|---|---|---|---|---|---|---|---| | control v2.1.0 | ctrl-4k-c | 78.71 | 68.49 | 14.60 | 35.9 | 1 | 5735 / 62249 | 5075 / 1211 / 1019 | | control v2.1.0 | ctrl-4k-d | 79.65 | 69.20 | 14.45 | 16.1 | 0 | 5167 / 55844 | 4368 / 1277 / 1094 | | this PR | p134b-4k-a | 79.07 | 68.73 | 14.55 | 23.0 | 0 | 7776 / 84298 | 7207 / 1102 / 908 | | this PR | p134b-4k-b | 79.24 | 69.44 | 14.40 | 15.8 | 0 | 7262 / 78704 | 6397 / 1575 / 1365 | | control mean | | 79.18 | 68.85 | 14.53 | | | 5451 / 59047 | 4722 / 1244 / 1057 | | PR mean | | 79.16 | 69.09 | 14.48 | | | 7519 / 81501 | 6802 / 1339 / 1137 | Path evidence: dead targets let go 4,727 / 51,467 MB (control, union-rule session) against 7,807 / 85,078 MB with the union rule (+65%). With the pool change, images made new 2,441 -> 1,339 (the control's 1,244), pool reuse 4,722 -> 6,802 (+44%), and the union rule's gains are kept. Speed is neutral within the run-to-run spread (79.87 -> 79.34 -> 79.16 avg FPS). The issue's 16 GB AMD repro was not reproduced on this 12 GB card; the state line and pool counters are the evidence. ## Relation to #2484 #2484 (draft) sweeps the dead-target pile more often and drains the pool faster. This PR changes which targets are dead at all (the union accounting) and when an evicted target's image is reused. They touch different hunks of `texture_frame` and `pool_trim`, and can land in either order. ## AI assistance Assisted-by: oc-impl-1 (opencode/deepseek-v4.1-flash), oc-upstream-1 (opencode/deepseek-v4.1-flash-free) Tracking: #993 (where #2180 was consolidated).

github-sync added 1 commit

2026-11-12 12:37:01 +00:00

fix(vk): let render targets go as newer ones together cover them, and reuse their images at once 3d4508c929

Assisted-by: oc-upstream-1 (opencode/deepseek-v4.1-flash-free)
Trace-Issue: #2180

github-sync commented

2026-11-12 14:47:46 +00:00

Author

Owner

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

I merged this into current main and ran test_target_overlap, test_target_image_pool and test_target_image_reuse (against Khronos Vulkan-Headers), plus the texture, target, evict and pool checks from run_all. All pass. CI hasn't run on this head yet.

DrawnOver (gpu/vk/target_overlap.h) is correct: by_start is sorted, so the clipped starts never go backwards and merging span by span gives the union with no byte counted twice. The #899 view exclusion is kept.

On pooling at eviction: the only caller of destroy_later, texture_frame's evict loop, skips textures with last_value > completed, and the flush waits first, so a pooled image is never in flight. Skipping compact.in_pass is fine because targets have their own pool.

What's left is reach. The union rule marks more targets dead for everyone (+65% in your numbers), and a false positive recreates a live target empty, which would show on screen. The RTX 5070 run covered Kokiri only, while #2180 is an RX 9070 XT report. A run past a scene change (Castle Town, or #2180's guard room) would settle it. #2484 touches the same parts of texture_frame and pool_trim.

> [@Joey-Magnus](https://github.com/Joey-Magnus) reviewed on GitHub · 2026-11-12 14:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2766#pullrequestreview-5876249963) I merged this into current main and ran test_target_overlap, test_target_image_pool and test_target_image_reuse (against Khronos Vulkan-Headers), plus the texture, target, evict and pool checks from run_all. All pass. CI hasn't run on this head yet. DrawnOver (gpu/vk/target_overlap.h) is correct: by_start is sorted, so the clipped starts never go backwards and merging span by span gives the union with no byte counted twice. The #899 view exclusion is kept. On pooling at eviction: the only caller of destroy_later, texture_frame's evict loop, skips textures with last_value > completed, and the flush waits first, so a pooled image is never in flight. Skipping compact.in_pass is fine because targets have their own pool. What's left is reach. The union rule marks more targets dead for everyone (+65% in your numbers), and a false positive recreates a live target empty, which would show on screen. The RTX 5070 run covered Kokiri only, while #2180 is an RX 9070 XT report. A run past a scene change (Castle Town, or #2180's guard room) would settle it. #2484 touches the same parts of texture_frame and pool_trim.

github-sync referenced this pull request

2026-11-12 14:47:48 +00:00

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

github-sync commented

2026-11-12 16:56:06 +00:00

Author

Owner

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

Thanks for this, and for the A/B tables. I agree with @Joey-Magnus's review: the union is computed right, and pooling at eviction is safe because the evict loop skips anything with last_value > completed. I checked that too: destroy_later has no other caller, and pooled target images are never made in the compacted textures' pool, so skipping the compact.in_pass gate is fine. A few more points.

What the sum actually adds. A newer target fully inside the old one is still excluded, so every one that counts either straddles its start, straddles its end, or covers it whole. All the ones over the start clip to lo, so their union is just the longest of them, and the same goes for the end. So the only new case is a target with one newer target over its head and another over its tail, each under an eighth, together an eighth or more. That's what your edge test checks, and it's a sound reading of the console's memory: if both ends really were drawn over, the game can't sample it there either. Please say in the description (and the header comment) that this is the shape it catches; "several partial overlaps add up" reads broader than it is.

Which targets are exposed. Targets used every frame can't be hit: TAA history, shadow maps, the exposure meter, the presented image. Sampling or drawing one updates last_frame, the rule needs a newer write after that, and the pass only looks at targets unused for 3+ frames. What's exposed is a target that sits unused for a while and is sampled again later, like the pause-screen capture. The finalized textures the game samples again are exposed too, because the sum also feeds keep_finalized (return !drawn_over(tex) && !drawn_over_shifted(tex);). A finalized texture with both edges drawn over now loses its keep and goes without a write-back. That's right only while guest_size matches the size the game gave that storage: any rounding there now counts at both ends. I'm fine with the rule, but this is why the run across scene loads matters.

Evidence. Our rule for memory and performance changes is before/after on the same scene with the hardware named, and for a fix, the bug shown gone in the game. The fps tables show it's speed-neutral on the 5070, thanks. But this is a memory fix and there's no resident memory in it: "let go" and "reused / made / destroyed" are running totals, and the one VRAM figure (~6352 MB) is a single number. Could you add, for control and PR on the same route:

VRAM used, and the textures' MB from the textures retired (... left, N MB) line, at the start and after a few minutes;
a run across scene loads, as Joey asked: Kokiri, Hyrule Field, Castle Town (the market, and the guard room from #2180) and back. Please note whether the pause screen's background, the ground's textures and the post effects look right after each load. A couple of screenshots would help.

#2180 wasn't reproduced (a 12 GB card against the 16 GB RX 9070 XT at 4K HDR with frame generation). That's acceptable if the numbers show the target images no longer climbing in the guard room.

Other PRs.

#2484 (draft) is the other #2180 fix. The code doesn't conflict (different hunks of pool_trim and texture_frame), but both add a test at the same place in tests/test_target_image_pool.py, so whichever lands second needs a small rebase. They also interact: this PR pools images a present sooner and lets more targets go, which raises the pool's bytes and lowers st_pool_made, the two things #2484's drain and pile trigger read. I'd land this one first and have #2484 re-measured on top.
#2759: no overlap. Its vk_texture.cpp change marks a finalized pool's textures released; the render-target trim its description mentions isn't in its diff. Neither makes the other redundant, and they can land in either order.

CI: only the title check has run; the game-free and AppImage workflows are waiting for approval. Joey ran the three focused tests merged with main and they pass. No conflicts with main.

Nits inline. Once the memory numbers and the scene-load run are in, I'm happy to approve.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 16:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2766#pullrequestreview-5878237928) Thanks for this, and for the A/B tables. I agree with @Joey-Magnus's review: the union is computed right, and pooling at eviction is safe because the evict loop skips anything with `last_value > completed`. I checked that too: `destroy_later` has no other caller, and pooled target images are never made in the compacted textures' pool, so skipping the `compact.in_pass` gate is fine. A few more points. **What the sum actually adds.** A newer target fully inside the old one is still excluded, so every one that counts either straddles its start, straddles its end, or covers it whole. All the ones over the start clip to `lo`, so their union is just the longest of them, and the same goes for the end. So the only new case is a target with one newer target over its head and another over its tail, each under an eighth, together an eighth or more. That's what your edge test checks, and it's a sound reading of the console's memory: if both ends really were drawn over, the game can't sample it there either. Please say in the description (and the header comment) that this is the shape it catches; "several partial overlaps add up" reads broader than it is. **Which targets are exposed.** Targets used every frame can't be hit: TAA history, shadow maps, the exposure meter, the presented image. Sampling or drawing one updates `last_frame`, the rule needs a newer write after that, and the pass only looks at targets unused for 3+ frames. What's exposed is a target that sits unused for a while and is sampled again later, like the pause-screen capture. The finalized textures the game samples again are exposed too, because the sum also feeds `keep_finalized` (`return !drawn_over(tex) && !drawn_over_shifted(tex);`). A finalized texture with both edges drawn over now loses its keep and goes without a write-back. That's right only while `guest_size` matches the size the game gave that storage: any rounding there now counts at both ends. I'm fine with the rule, but this is why the run across scene loads matters. **Evidence.** Our rule for memory and performance changes is before/after on the same scene with the hardware named, and for a fix, the bug shown gone in the game. The fps tables show it's speed-neutral on the 5070, thanks. But this is a memory fix and there's no resident memory in it: "let go" and "reused / made / destroyed" are running totals, and the one VRAM figure (~6352 MB) is a single number. Could you add, for control and PR on the same route: - VRAM used, and the textures' MB from the `textures retired (... left, N MB)` line, at the start and after a few minutes; - a run across scene loads, as Joey asked: Kokiri, Hyrule Field, Castle Town (the market, and the guard room from #2180) and back. Please note whether the pause screen's background, the ground's textures and the post effects look right after each load. A couple of screenshots would help. #2180 wasn't reproduced (a 12 GB card against the 16 GB RX 9070 XT at 4K HDR with frame generation). That's acceptable if the numbers show the target images no longer climbing in the guard room. **Other PRs.** - #2484 (draft) is the other #2180 fix. The code doesn't conflict (different hunks of `pool_trim` and `texture_frame`), but both add a test at the same place in `tests/test_target_image_pool.py`, so whichever lands second needs a small rebase. They also interact: this PR pools images a present sooner and lets more targets go, which raises the pool's bytes and lowers `st_pool_made`, the two things #2484's drain and pile trigger read. I'd land this one first and have #2484 re-measured on top. - #2759: no overlap. Its `vk_texture.cpp` change marks a finalized pool's textures released; the render-target trim its description mentions isn't in its diff. Neither makes the other redundant, and they can land in either order. CI: only the title check has run; the game-free and AppImage workflows are waiting for approval. Joey ran the three focused tests merged with main and they pass. No conflicts with main. Nits inline. Once the memory numbers and the scene-load run are in, I'm happy to approve.

github-sync commented

2026-11-12 16:56:06 +00:00

Author

Owner

@hckrman101 commented on gpu/vk/target_overlap.h line 26 on GitHub · 2026-11-12 16:35 UTC · original

With views inside excluded, every candidate over the start clips to a == lo, so this merge just keeps the longest of them, and likewise at the end. The sum only ever differs from the old rule for head + tail. Worth one line in the struct's comment, so nobody later reads it as "any number of pieces add up" and loosens the view exclusion to match.

> [@hckrman101](https://github.com/hckrman101) commented on `gpu/vk/target_overlap.h` line 26 on GitHub · 2026-11-12 16:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2766#discussion_r4553306961) With views inside excluded, every candidate over the start clips to `a == lo`, so this merge just keeps the longest of them, and likewise at the end. The sum only ever differs from the old rule for head + tail. Worth one line in the struct's comment, so nobody later reads it as "any number of pieces add up" and loosens the view exclusion to match.

github-sync commented

2026-11-12 16:56:07 +00:00

Author

Owner

@hckrman101 commented on gpu/vk/vk_texture.cpp line 1859 on GitHub · 2026-11-12 16:35 UTC · original

Nit: sparse_chunks is always empty here (the if checks it), so this line does nothing. The attachments and total_bytes lines below repeat the tail too. if (pooled) pool_put(d); else dead.push_back(std::move(d)); with the shared tail after would keep one copy.

> [@hckrman101](https://github.com/hckrman101) commented on `gpu/vk/vk_texture.cpp` line 1859 on GitHub · 2026-11-12 16:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2766#discussion_r4553306970) Nit: `sparse_chunks` is always empty here (the `if` checks it), so this line does nothing. The attachments and `total_bytes` lines below repeat the tail too. `if (pooled) pool_put(d); else dead.push_back(std::move(d));` with the shared tail after would keep one copy.

github-sync commented

2026-11-12 16:56:07 +00:00

Author

Owner

@hckrman101 commented on tests/test_target_image_reuse.py line 34 on GitHub · 2026-11-12 16:35 UTC · original

Nit: this exact line is also in the by_hold loop above the eviction loop, so the check still passes if the eviction loop's own guard went. Cutting the loop that calls destroy_later and checking inside it would pin the guard that keeps a pooled image out of flight.

> [@hckrman101](https://github.com/hckrman101) commented on `tests/test_target_image_reuse.py` line 34 on GitHub · 2026-11-12 16:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2766#discussion_r4553306975) Nit: this exact line is also in the `by_hold` loop above the eviction loop, so the check still passes if the eviction loop's own guard went. Cutting the loop that calls `destroy_later` and checking inside it would pin the guard that keeps a pooled image out of flight.

github-sync referenced this pull request

2026-11-12 16:56:08 +00:00

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

github-sync referenced this pull request

2026-11-12 17:57:32 +00:00

[Tracking] AMD handhelds and APUs (ROG Ally / Ally X, Legion Go, Radeon 780M/860M/890M): black screen, device lost, reboot #997

Commenting is not possible because the repository is archived.

No reviewers