fix(vk): graceful retired-pool eviction where retired imports were kept (Windows AMD and Intel) #1906
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1906
Loading…
Reference in a new issue
No description provided.
Delete branch "memory-leak-only"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
On Windows, AMD and Intel kept every retired texture pool's import for the whole session. This gives them back after an idle wait instead, so they stop accumulating. Nothing changes off Windows or on Windows NVIDIA.
Why
main's default per OS and vendor for a retired pool's import:
Kept, those retired pools only grow: multi-GB in a long session, until the system heap's budget is passed and new imports fail.
RX_VK_KEEP_RETIRED=0gives them back with no wait at all, which is the behaviour the device losses were reported against.Changes
RX_VK_RETIRE_SECSseconds (default 30), oldest first. This happens whatever the memory pressure.RX_VK_IMPORT_HIGHpercent of its budget (default 85,0turns it off), the wait isRX_VK_RETIRE_MIN_SECSseconds (default 2), until enough is queued to come down toRX_VK_IMPORT_LOWpercent (75). Unlikeimports_give_back's valve this one is on by default on Windows and is not limited to budgets under 12 GB, as it only frees pools nothing uses any more; the comments in both places say so.RX_VK_KEEP_RETIRED:1keeps them for the session;0,off,falseornogives them back as soon as the GPU is past them; unset or empty is the default in the table. Applies to every OS and vendor, as before.dead_importsis still in the driver's figure until the GPU is past it, so both valves count it as given back (only where retired imports wait; elsewhereimports_give_backreads what it read on main).env_int, the on/off switch withenv_on.gpu/vk/retired_give_back.hwith a check that needs no game or GPU:tests/test_retired_give_back.py.Testing
tests/test_retired_give_back.pypasses (the defaults by OS and vendor, the wait and its floor, the watermarks, a 20-area tour kept / at once / waited, the two valves not asking twice);gpu/vk/vk_memory.cppcompiles for Windows with llvm-mingw.AI assistance disclosure: the design and fix were developed with AI assistance.
Thanks for picking this up, the session-long accumulation on Windows is real and a grace period is a reasonable idea. A few things before it can go in:
The branch carries two other PRs. The diff has 3 commits: #1439 (FSR 1 EASU+RCAS, the taau shaders and the vk_draw.cpp changes) and #1905 (FR_READBACK, vk_replay.cpp), with this PR's own commit 8fb0290 on top. The description says one file, 44/10, but the PR is +1200/-69 over 7 files, and it is 921 commits behind main and conflicts (#1439 and #1905 are also dirty). Please rebase onto current main with only the vk_memory.cpp commit so it can be reviewed and merged on its own. CI hasn't run on it either.
Linux changes behaviour too, and the description doesn't mention it. On main, Linux already gives retired imports back as soon as the GPU is past them (
keepis false there). That was the fix for the amdgpu userptr submit cost (issues 312/413: every retired import checked at each CS, 3-4 fps in Kokiri). With this PR Linux waits 30 s, so the retired pools from a burst of merges stay in every submission's BO list for that long again. I'd keep Linux at once by default and put the grace period on the Windows path only.The NVIDIA part is already fixed on main. The description and the new comment say every vendor leaked, but main's Windows default already gives retired imports back on NVIDIA (
vendorID != 0x10DE, issue 1057, the RTX 3070 case). What this PR really changes is the Windows default for AMD and Intel, where give-back was turned off on purpose after device losses (Arc B390 entering Kokiri every time, issue 17; Ally X 309; RX 6600 571). The idea that those losses came from giving back too soon is plausible, but nothing has tested it yet: the one test is an RX 6700 XT (which OS? on Linux main would already show retired imports given back, so I guess Windows), and the pressure path, which drops the wait to 2 s and is close to the old behaviour, was never exercised. This needs runs on Windows Intel Arc and on the AMD parts that lost the device before the default flips there, and I'd leave that call to NotRexed.The pressure valve and imports_give_back fire on the same submission and both act on the same overshoot (inline). Freeing retired imports doesn't lower the heap usage until
dead_importsis reaped, soimports_give_backreads the sameuseda moment later and evicts live idle pools for the same deficit. On Linux with a budget under 12 GB both valves are on by default.Nits: the comment in
imports_give_back("Off by default on Windows, as giving retired imports back is") is out of date after this.RX_VK_IMPORT_HIGHnow defaults to 85 for the retired valve on Windows and 0 for the live one, and the retired valve skips thebudget < 12 GBgate. That may be what you want, but say so in the comment.On Linux this used to be the whole default:
keepfalse, so given back as soon as the GPU is past (issues 312/413, the amdgpu userptr CS cost). Withidle_msapplied below, Linux now waits 30 s too. Suggestat_oncedefaulting to true off Windows, unless you've measured that the 30 s don't bring back the submit stall on RADV."every vendor leaked by it": on main NVIDIA was already excluded from
keep(issue 1057), so only AMD and Intel on Windows leaked. Worth saying, since those are the vendors whose device losses the old default was protecting against.This counter and
imports_give_back's both run once per submission and reach% 16on the same submission. What is pushed todead_importshere isn't freed until the GPU is past it, sosystem_heap_usagereturns the sameusedtoimports_give_backright after, and it evicts live idle pools for the samewant. Maybe pass the bytes queued here toimports_give_backand subtract them, or only probe in one place.Rebased as a single commit off current main (42ae0cd5), unstacked per review: only gpu/vk/vk_memory.cpp and the leak's test fixture. Recap of the eviction policy: retired pools return to the driver only when the GPU has been quiet, the pool has been idle at least 2 s (RX_VK_RETIRE_MIN_SECS, default 2000 ms), and committed memory pressure has crossed 85% (relaxing back to 75% once under, so pools don't ping-pong).
That rebase landed as of this morning: the branch is now a single commit (42ae0cd5) off current main — only gpu/vk/vk_memory.cpp and the leak's test fixture, +44/-10. The two stacked PRs you saw are gone (they live in #1439, itself rebased to main, and #1905 which closed in favour of #1276). The hysteresis numbers in the recap comment are corrected too: idle floor 2 s (RX_VK_RETIRE_MIN_SECS, default 2000 ms), pressure 85% with the 75% relax.
Thanks for the rebase. The branch is now the one vk_memory.cpp commit on top of main and mergeable, so the stacking point is fixed.
The code itself is unchanged, though: the vk_memory.cpp diff at 42ae0cd is line for line the same as 8fb0290, so the rest of the last review still applies:
at_oncethe default off Windows.imports_give_backreads the sameusedbeforedead_importsis reaped and evicts live idle pools too). Still open, inline thread.imports_give_back, theRX_VK_IMPORT_HIGHdefault split and the 12 GB gate) are untouched.New:
keepdefault and uses 2 s. You also mention a test fixture for the leak, but the PR has only vk_memory.cpp. Please make the description and the commit message match what the code does, and add the test if you meant to.fix(vk): give retired pools back after an idle grace period.retired_epochinstruct Pool, andmirrors_give_back), so whichever lands second needs a small rebase. #2198, #2368 and #2401 are in the texture cache and replay, not this path.I checked the head (42ae0cd) against main's
retired_imports_give_backin gpu/vk/vk_memory.cpp. The code is the same as in hckrman101's last two reviews, so those points are all still open:keepis false off Windows, so retired imports go back as soon as the GPU is past them (the amdgpu submit cost, issues 312/413). Hereat_onceis true only withRX_VK_KEEP_RETIRED=0, so by default Linux now waits 30 s.Smaller things: the three new number switches use
atoi(getenv(...)). Main hasenv_intin gpu/vk/compat/hookshot_env.h for this.at_onceonly recognises a literal0, soRX_VK_KEEP_RETIRED=offkeeps the 30 s wait. The title still needs a conventional type, e.g.fix(vk): ....You're right, and it was worse than one overshoot counted twice. Walking through it at 42ae0cd:
memory_begin_submissionrunsreap_mirrors(), thenretired_imports_give_back(), thenimports_give_back(), all underpools_mu, once per submission.static uint64_t calls, and each only counts when its own valve is enabled. Both conditions are fixed for the run (statics read once), so when both valves are on they count from the same first submission and stay in phase: they always hit% 16 == 0on the same submission, not just sometimes.dead_importswithvalue = max(mem_rec_value, last_value). Nothing is freed there.vkFreeMemoryonly happens inreap_mirrors()oncemem_completedreaches that value, andreap_mirrors()has already run for this submission, so the earliest free is the next submission, and only if the GPU has finished this one.system_heap_usageis the driver's figure (VK_EXT_memory_budget), so it cannot drop before that free.imports_give_backread the sameuseda few lines later, computed the samewant, and evicted live idle pools for bytes that were already on their way out.And as you said, at 42ae0cd this was on by default on Linux under a 12 GB budget, since the retired valve had no OS split.
What the new head (
4808b1c) does, taking your first suggestion:dead_import_bytescounts what sits indead_imports: added at the push, subtracted inreap_mirrors()at the actual free.want(settled(used, dead_import_bytes), ...), so what it queued earlier counts as given back until it really is.imports_give_backdoesused = settled(used, dead_import_bytes)before its watermark test, so it only evicts live pools for what the queued retired imports will not cover.imports_give_backskips the subtraction there, so Linux reads exactly what it read on main. The interplay is left only on Windows AMD/Intel withRX_VK_IMPORT_HIGHset by hand (the live valve defaults to off there), and that is the case the subtraction covers.I kept two probes rather than one because the two valves have different gates on purpose (the live one is off on Windows and limited to budgets under 12 GB, the retired one is neither; the comments in both now say so).
tests/retired_give_back_test.cpphas the case: 1 GB over, three pools queued, and the next probe with the sameusedasks for nothing.Pushed
4808b1c: still one commit, now on current main (ef81aee). This time the code changed, going through both reviews:Linux is back to main's behaviour. Off Windows a retired import goes back as soon as the GPU is past it, by default, exactly as on main: same gates, no wait, and the pressure probe is not even run there. The wait applies only where main's default was to keep them, Windows AMD and Intel. Windows NVIDIA stays at once too (issue 1057).
RX_VK_KEEP_RETIREDis read withenv_on:1keeps for the session;0,off,falseornogives back at once; unset or empty is the OS/vendor default. The numbers (RX_VK_RETIRE_SECS,RX_VK_RETIRE_MIN_SECS,RX_VK_IMPORT_HIGH,RX_VK_IMPORT_LOW) are read withenv_int.The two valves and the same overshoot (inline thread, answered there): what is queued in
dead_importsis now counted as given back by both valves until the GPU is past it, soimports_give_backno longer evicts live pools for the same bytes.Comments, commit message, title and description now say what the code does. Only AMD and Intel on Windows leaked (NVIDIA was already excluded on main); a retired pool goes back after 30 s whatever the pressure, and 85% only shortens that to 2 s (not 5); the Windows keep default is what goes away for those two vendors. The stale "Off by default on Windows, as giving retired imports back is" comment in
imports_give_backis rewritten, and both places now say that the retired valve defaults to 85 on Windows and skips the 12 GB gate, and why. Title:fix(vk): .... My two earlier recap comments here were wrong on these points; the description is the accurate one now.The test fixture. My earlier comment mentioned one that was never in the PR, and there was none to bring over. It is written now: the rule moved to
gpu/vk/retired_give_back.h, checked bytests/test_retired_give_back.py+tests/retired_give_back_test.cpp(no game or GPU).Diff against main: 4 files, +288/-13 (
vk_memory.cpp+62/-13, the rest is the header and the test).Still not done, and unchanged by this push: no run on Windows Intel Arc or on the AMD parts that lost the device (Ally X, RX 6600), and the pressure path has not fired on real hardware. The RX 6700 XT run in the description was of the earlier revision. Whether the default should flip for those two vendors without those runs is yours and @NotRexed's call;
RX_VK_KEEP_RETIRED=1gives main's behaviour back.Overlap with #2154 noted: whichever lands second, I'll rebase.
github-sync referenced this pull request2026-11-12 06:22:54 +00:00