Repository navigation
fix(vk): let render targets go as newer ones together cover them, and reuse their images at once - #2766
Conversation
… reuse their images at once Assisted-by: oc-upstream-1 (opencode/deepseek-v4.1-flash-free) Trace-Issue: NotRexed#2180
Joey-Magnus
left a comment
Joey-Magnus
left a comment
There was a problem hiding this comment.
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.
hckrman101
left a comment
hckrman101
left a comment
There was a problem hiding this comment.
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_trimandtexture_frame), but both add a test at the same place intests/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 lowersst_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.cppchange 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.
| if (olo >= lo && ohi <= hi) return; // (a view of a part of it, not a new target: 5bfa6aa, PR #899) | ||
| const uint64_t a = std::max(lo, olo), b = std::min(hi, ohi); | ||
| if (b <= a) return; | ||
| if (open && a <= end) { |
There was a problem hiding this comment.
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.
| } else { | ||
| for (auto& kv : tex.views) | ||
| if (kv.second) d.views.push_back(kv.second); | ||
| d.memory = tex.sparse_chunks; |
There was a problem hiding this comment.
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.
|
|
||
| def check_source(tex): | ||
| frame = block(tex, "void texture_frame(uint64_t completed)") | ||
| if "if (tex->last_value > completed) continue;" not in frame: |
There was a problem hiding this comment.
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.
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:DrawnOveradds 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 fix(vk): let go of render targets drawn over from another start #899).vk_texture.cpptexture_frame: the dead-target rule uses that sum.vk_texture.cppdestroy_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/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.pyupdated for the new eviction path.Testing
Game-free:
python -I -B tests/run_all.pyin the worktree, full suite.tests/test_input_background.py, a declared known environment failure on this PC (no MSVC SDK).tests/test_precompile_compute_guard.pyandtests/test_recorder_sync.pypassed in this run.test_target_image_reuse,test_target_image_pool,test_target_overlappass.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:
Pool change added, 2 runs per build:
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_frameandpool_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).