Repository navigation
fix(vk): keep the textures the GPU drew that the game samples again after finalizing them (a cached shadow map) - #2368
Joey-Magnus wants to merge 6 commits into
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, the diagnosis is convincing: depth, so not flushable; finalized, so neither keep_finalized (4 MB cap) nor the unflushable rule (!released) covers it, and the remade-over-GPU-storage path then samples it empty. The RX_VK_LOG_REMADE lines and the before/after table make the case well. The rule itself is small, has a switch, and the test covers its cases.
What I'd like changed before this goes in is how the 1 GB is spent, because it matters most on the 6-8 GB cards this is for:
- Oldest first, no age limit. The eviction loop walks
orderfrom least recently used, andkeep_resampled::keephands out the cap first come, first served. So the textures that take the 1 GB are the stalest resampled ones, and once the cap is used up, a younger one (the current area's shadow map) is the one that goes. Nothing ever lets a kept one go except being drawn over, so a shadow map from an area left behind stays for the rest of the session.keep_finalizeddoes this the other way round on purpose (sum first, drop the least recently used beyond the cap), and elsewhere in this loop a course unused for 3600 presents is allowed to go. I'd suggest the same here: count the candidates' total up front and let the oldest go beyond the cap, and/or don't keep one unused for more than N presents (3600 would still cover the 400-frame gap in your log). - The cost on 8 GB. The default budget there is roughly 5-5.5 GB, so a 1 GB keep is about a fifth of the cache that the pass can't touch, and everything else is evicted harder to make up for it. Your runs kept ~320 MB (the two maps); a default nearer that (say 384-512 MB) would fix the reported case with less risk. A run on a real 6-8 GB card, or at least
RX_VK_VRAM_MBaround 4000 in Hyrule Field for a few minutes, would help here, since the 512 MB run with a 12 GB card doesn't show what the driver does near its limit. - #2198 (compaction hold, on by default there). Both PRs add lines right after
const bool overwritten = shifted || drawn_over(tex);, so whichever merges second has a small textual conflict. The semantic one matters more: #2198's reserve line exists to let go of long-unused textures, and a 400-frames-unused 288 MB shadow map is exactly that. With this rule placed first, the hold can't take it and will take live textures instead to meet its line. Fine if that's the intent, but it should be decided when the two are merged together, and the rule's VRAM should probably be counted by the hold.
Smaller things:
resampledis never cleared.releasedgoes back to false when the GPU writes the texture again (lines 2613/3132), butresampledstays, so a reused target that was once sampled after finalizing (e.g. last frame's shadow map read before it's drawn) keeps the flag for good. Clearing it alongsidereleased = falsewould make the flag mean what the comment says.- A small finalized texture that is also resampled is now caught by this rule first, but it is still counted in
finalized_kept, so thekeep_finalizedtotal stays inflated and its LRU drops a few more small ones than it needs to. Minor; skipping resampled ones when summingfinalized_keptwould fix it.
CI: the failing appimage and SPIR-V fixtures jobs fail in the apt/toolchain install step (DNS to archive.ubuntu.com), not in anything this touches; Windows build and the Windows Python checks pass, the Ubuntu ones were still pending.
| static const bool keep_over_only = getenv("RX_VK_KEEP_RENDERED_OVER") != nullptr; | ||
| const uint64_t keep_rendered = keep_rendered::presents(known, used, allowed, keep_over_only); | ||
| const bool overwritten = shifted || drawn_over(tex); // (above) | ||
| if (keep_resampled::keep(overwritten, tex->gpu_written, tex->resampled, tex->bytes, resampled_kept, keep_resampled_max)) { |
There was a problem hiding this comment.
order is oldest first, so this gives the cap to the least recently used resampled textures and evicts the newest ones once it's full; and a kept one never ages out unless drawn over. Could this follow keep_finalized (total first, drop the oldest beyond the cap) and/or skip ones unused for more than ~3600 presents?
There was a problem hiding this comment.
Fixed in 837ee60: the candidates are summed first, the oldest go beyond the cap, and anything unused for more than 1,800 presents is no longer a candidate. Thanks.
| // RX_VK_KEEP_RESAMPLED_MB (1024 by default; 0: evicted as before): keep_resampled.h | ||
| static const uint64_t keep_resampled_max = | ||
| (uint64_t)(getenv("RX_VK_KEEP_RESAMPLED_MB") ? std::max(0, atoi(getenv("RX_VK_KEEP_RESAMPLED_MB"))) : 1024) << 20; | ||
| uint64_t resampled_kept = 0; |
There was a problem hiding this comment.
1024 MB default: on an 8 GB card that's about a fifth of the texture budget pinned. Your runs needed ~320 MB; a default around 384-512 would cover the shadow maps with less pressure on everything else.
There was a problem hiding this comment.
Default is 512 MB now, thanks. The run at RX_VK_VRAM_MB ~4000 (or on a 6-8 GB card) is still open; see the review.
| // (hookshot) one the game finalized and samples after that: a cache it keeps (the shadow map it draws, finalizes and samples | ||
| // for hundreds of frames). The eviction pass keeps it over the budget: keep_resampled.h. Not only "well after it was drawn": | ||
| // the 12288 x 12288 shadow map was sampled within a few frames of being drawn, then went unused 400 frames and was evicted | ||
| if (tex->gpu_written && tex->released) tex->resampled = true; |
There was a problem hiding this comment.
Nit: nothing clears resampled when the GPU draws the texture again (released = false at 2613/3132). Resetting it there would keep the flag to "sampled after its latest finalize".
There was a problem hiding this comment.
Fixed: it is cleared at both places now, and the check keeps the two counts equal.
…fter finalizing them (a cached shadow map) The game draws its 12288 x 12288 shadow map once, finalizes it and samples it for many frames. It is depth, so it cannot be written back; over the budget the eviction pass let it go, it was made again over storage the GPU rendered (not uploaded) and sampled EMPTY until the game drew it again some 40 frames later: shadows missing or wrong, and the dark ground of issue 2209. Such a texture is now marked where it is seen sampled again after finalizing (Texture::resampled) and kept through evictions over the budget, up to RX_VK_KEEP_RESAMPLED_MB (1024 by default; 0: as before). keep_resampled.h, tests/test_keep_resampled.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…p, 1,800-present age limit, 512 MB default - The candidates' bytes are summed before the pass; walking least recently used first, the oldest go until the rest fit, so the cap is spent on the most recently used (as keep_finalized does). - Only one used in the last 1,800 presents is a candidate: the threshold after which NotRexed#2198's reserve lets a long-unused texture go. - RX_VK_KEEP_RESAMPLED_MB defaults to 512. - resampled is cleared wherever released goes back to false (the texture is drawn again). - keep_finalized leaves out what keep_resampled counts, so no texture is in both sums. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…P_RESAMPLED_MB=0 keep_finalized left out the textures keep_resampled counts even with that rule off, so a small resampled one lost the protection it had before. The exclusion now applies only while keep_resampled is on (its size is read before the lambda). Co-authored-by: Cursor <cursoragent@cursor.com>
f34b900 to
2c3692c
Compare
|
Thanks for the review. Rebased on current
Smaller ones:
|
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the rework. Looked at the rebased head (2c3692c) against what I reviewed at f34b900.
Fixed:
- Oldest first / no age limit: the candidates are now summed before the pass and the oldest go until the rest fit, so the cap goes to the most recently used, the same way
keep_finalizeddoes it. Only textures used in the last 1,800 presents count, so a shadow map from an area you've left goes as before. The test covers the cap order and the clamp. - Default: 512 MB instead of 1 GB.
- Nits:
resampledis now cleared at both placesreleased = false, and the Python check makes sure the two counts stay equal (nice).keep_finalizednow leaves resampled textures to this rule, but only while the rule is on (2c3692c). - #2198: the shared 1,800-present line answers the semantic part well enough for now. Counting the kept bytes in the hold's reserve in whichever PR merges second works for me. The textual conflict after the
overwrittenline is still there for whoever goes second.
Still open, and the reason this is a comment and not an approval:
- The capped-memory run. This changes how VRAM behaves on the 6-8 GB cards it's meant for, and the only in-game evidence so far is the 512 MB-budget run on a 12 GB card. That run shows the shadow maps are no longer sampled empty, but it doesn't show what happens near the driver's limit, where 512 MB the pass can't touch pushes everything else out harder. A few minutes in Hyrule Field at
RX_VK_VRAM_MBaround 4000 (or on a real 6-8 GB card), with fps, VRAM and any 'made again' counts against main, would settle it. Also, was the tested-configurations row run before the rework (1 GB cap, no age limit)? If so, please say that in the row, or re-run it with the current version.
New, a small nit inline: past its cap, a small resampled texture no longer falls back to keep_finalized.
CI: the one failure (Python source checks (windows-2022, part 2/2)) is tests/test_native_astc_texture_count.py hitting scanf being deprecated on Windows. That test came from #2288 and main's own regression runs fail the same way, so it's not this PR. Everything else passes. Mergeable.
| tex->first_written != tex->last_written || tex->bytes > (4ull << 20)) | ||
| return false; | ||
| if (keep_resampled_max && tex->resampled && frame_no - tex->last_frame <= keep_resampled::kMaxUnused) | ||
| return false; // (keep_resampled's, below; with it off, kept here as before) |
There was a problem hiding this comment.
Nit: this skips every resampled texture used in the last 1,800 presents, including the ones keep_resampled::keep then lets go because they're over its cap. So a small (<= 4 MB) resampled texture that keep_finalized used to keep goes once the resampled total is over 512 MB. That only matters past the cap and the gap is small. If you want to close it, only skip it here when this rule actually kept it, e.g. by having the pass try keep_finalized after keep says no.
There was a problem hiding this comment.
Closed in ed765d4: past this rule's cap, a small finalized one falls back to keep_finalized again (finalized_after_resampled). It joins keep_finalized's total at that point in the walk, so the older ones judged before it were judged without it and keep slightly more; I took that over a second pre-pass. Only while the rule is on: with RX_VK_KEEP_RESAMPLED_MB=0 the pre-pass already counts it, and adding it again would double it.
…he second review - Conflicts with NotRexed#2198 (compaction on by default): this rule still comes before the hold. - The age limit is vram_compact::kReserveUnused itself and strict (unused < 1,800): a texture this rule keeps is never one the compaction's reserve may take; before, both claimed 1,800. - Past this rule's cap, a small finalized resampled texture falls back to keep_finalized, as before the rule (joins its total at that point). Only while the rule is on: with it off the pre-pass already counts it. - tested-configurations: the row says it measured the first version (1 GB, oldest first, no age limit). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the second pass. Merged current
Still open: the run at |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks. Looked at ed765d4 against 2c3692c. The PR's own diff is still the seven files, and the merge of main brings in #2198 cleanly.
Fixed or answered:
- The
keep_finalizedgap past the cap (my last nit): closed. A candidatekeeplets go now falls back tokeep_finalizedthroughfinalized_after_resampled, only while the rule is on, so with=0nothing is counted twice. Joining the total mid-walk, so the older ones keep a little more, is a fair trade against a second pre-pass. - #2198: using
vram_compact::kReserveUnuseditself as the age line, strictly (< 1800), means a texture this rule keeps is never one the reserve may take, and the two can't drift apart. That's better than counting the bytes in the reserve. For textures unused 600-1,800 presents the rule still goes before the hold's own line, so the hold takes other live textures there. You named that trade-off and the 512 MB cap bounds it. Fine with me. - tested-configurations row: it now says it was the first version (1 GB cap, oldest first, no age limit). Thanks.
Still open, and still why this is a comment and not an approval: the run at RX_VK_VRAM_MB ~4000 in Hyrule Field (or on a real 6-8 GB card) against main, with fps, VRAM and the 'made again' counts. You said it's coming. Once it's posted and shows nothing worse than main, I'm happy to approve.
Two small nits inline: a comment in keep_resampled.h got mangled in the edit, and the new pre-pass could check the cheap flags before the overlap scans.
CI: Python checks (Ubuntu and Windows), the Windows Vulkan build and the SPIR-V fixtures pass. The appimage and Hookshot Builder package and setup failures are both actions/upload-artifact getting HTML back from GitHub after the build succeeded, and Conventional Commit title stopped with no error on a valid title. All three look like GitHub hiccups, so a re-run should clear them. Mergeable, no conflicts.
| // Only while used in the last kMaxUnused presents (unused for fewer): one from an area left behind goes as before (the same 1,800 presents after | ||
| // which the compaction's reserve lets a long-unused texture go, #2198). The 12288 x 12288 map went unused 400 presents before | ||
| // the eviction took it | ||
| // the compaction's reserve's own line, so a texture this rule keeps is never one the reserve may take (unused 1,800 or more) |
There was a problem hiding this comment.
Nit: lines 17-20 read like two versions of the comment got merged. "...went unused 400 presents before the eviction took it" has no full stop and runs into "the compaction's reserve's own line, so...". Something like: "Only while used in the last kMaxUnused presents (unused for fewer), the compaction reserve's own line (#2198): one from an area left behind goes as before, and a texture this rule keeps is never one the reserve may take. The 12288 x 12288 map went unused 400 presents before the eviction took it."
There was a problem hiding this comment.
Fixed in 3fbad81 with your wording: one comment now, ending at "...before the eviction took it."
| uint64_t resampled_total = 0; | ||
| if (keep_resampled_max) | ||
| for (auto& [last, tex] : order) | ||
| if (keep_resampled::candidate(drawn_over_shifted(tex) || drawn_over(tex), tex->gpu_written, tex->resampled, frame_no - last)) |
There was a problem hiding this comment.
Nit: this runs drawn_over_shifted and drawn_over for every GPU-drawn texture on each pass (every 10 presents), and then drops most of them on resampled. Testing tex->resampled (and the age) first would skip the overlap scans for nearly all of them, e.g. if (tex->resampled && keep_resampled::candidate(...)). It gives the same result.
There was a problem hiding this comment.
Done in 3fbad81: the pre-pass tests tex->resampled, tex->gpu_written and the age first, and only then runs drawn_over_shifted / drawn_over through candidate. Same result; tests/test_keep_resampled.py still passes.
…p scans; its age comment made whole Co-authored-by: Cursor <cursoragent@cursor.com>
…Hyrule Field Co-authored-by: Cursor <cursoragent@cursor.com>
|
Here's the capped-memory run you asked for, plus the two nits (3fbad81) and a tested-configurations row (229e0e5). Short version: in Hyrule Field this change does no worse than main, at any budget I tried and with the card full. Frame time, evictions and GPU memory match within noise, and neither side sampled a texture EMPTY. One caveat: in the clean runs the rule itself never had to keep anything (details below). Setup
Results
Caveat: where the rule actedIn the clean runs above, the rule never logged keeping a texture, so they show the 512 MB it reserves doesn't push other textures out harder near the limit, but they don't exercise a kept shadow map under pressure. It kept textures (20 "kept, not evicted" lines, the log's limit) in two earlier runs that went further into the field, with 3.0-3.7 GB of textures and over 3,000 evictions over the budget.
If you want that case covered before approving, a cleaner way to force it would help, say a spot where the cached shadow map is reliably evicted over the budget, like Dodongo's Cavern at 512 MB from the first row. CI: |
When VRAM is tight, the eviction pass throws out the game's cached shadow map, and shadows go missing or come out wrong until the game draws it again. This keeps such textures through evictions forced by the texture budget.
Refs #2209 (dark, moving shadows over Hyrule Field) and #993. Tracking: #995.
What goes wrong
keep_finalizedonly keeps textures of 4 MB or less.From the log (
RX_VK_LOG_REMADE=1, main + this branch's base, Dodongo's Cavern,RX_VK_VRAM_MB=512):The logs on #2209 (GTX 1660 Ti 6 GB), #2297 (RTX 4060 8 GB) and #2238 (RTX 4060 Laptop 8 GB) show the same 12288x12288 map created again two to four times in one session.
The change
Texture::resampledis set where a texture the GPU drew is sampled after the game finalized it, which is how a cache the game keeps behaves.keep_resampled::kMaxUnused, the same line after which fix(vk): stop the VRAM compaction's hold from evicting textures in use #2198's reserve lets a long-unused texture go). One from an area left behind goes as before.RX_VK_KEEP_RESAMPLED_MB(512 by default;0evicts as before). The candidates' total is counted before the pass, and the pass drops the least recently used beyond the cap, askeep_finalizeddoes, so the cap goes to the most recently used. The rule is ingpu/vk/keep_resampled.h.resampledis cleared whereverreleasedgoes back to false (the texture is drawn again).keep_finalizeddoesn't count a texturekeep_resampledalready covers, so none is in both totals. WithRX_VK_KEEP_RESAMPLED_MB=0it keeps them as before.Numbers
Five-minute headless scripted walks in Dodongo's Cavern (
HOS_HEADLESS=1). The texture budget is forced down to 512 MB so the eviction runs constantly, withRX_VK_LOG_REMADE=1. Same save, same build except for this change.kept, not evictedlines from this ruleTested on: Linux (Ubuntu 26.04.1, KDE Plasma 6.6 Wayland), RTX 4070 Ti 12 GB with driver 595.99.02, Ryzen 7 9800X3D, 2560x1440 DLAA. There is a row in
docs/tested-configurations.md.Not tested:
RX_VK_VRAM_MB=512)The cost: up to 512 MB of VRAM kept for textures that would otherwise have been evicted under pressure. In these runs that was about 320 MB, the two shadow maps.
Test
python -I -B tests/test_keep_resampled.py:=0vk_texture.cppstill asks the rule, sets the flag and reads the switchThe full
tests/run_all.pypasses: 361 passed, 0 failed, 14 skipped.🤖 Generated with Claude Code