Repository navigation
fix(agc): keep resident heap mirrors without host copies - #15296
Prophet069 wants to merge 6 commits into
Conversation
A heap mirror slab refused by a full device heap stopped the title. The slab now falls back to host memory, as buffers and images already do, and a refusal lowers the heap mirror budget, evicts down to it and retries; a failed bind gives back the blocks it took. A slab is refused before the device heap is exhausted: when all live slabs would pass APS5_HEAP_MIRROR_MIB or when the device heap would keep less than 256 MiB of its reported budget, so the shrink and sweep path runs before Windows pages device memory. A budget lowered by refusals is a soft target a mirror may pass up to the configured budget when nothing can be evicted, and the headroom refusal is not repeated within two seconds on one thread. Blocks are taken from the fullest slab, so partly used slabs drain and are freed. Nonzero scans remember each 64 KiB block with its write generation and read only blocks written since. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — comments
The PR merges and builds, the heap-mirror tests pass, and the validation comparison shows no new VUIDs. But the new tests also pass with main's production code, and the new slab-refusal, budget-shrink and nonzero-scan-cache paths have no test.
What I ran
Head a64a9a4a280cb5be0f80bb01ce98812e56bcda6d, main 5cb015caaaaf83f8ee431414ee6a457b5b307d49, merged tree a5a36646c486d472781b0f1732a84113e9acecda (--no-ff), Debian 13 VM, GCC 14.2.0, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 5cb015caaaaf83f8ee431414ee6a457b5b307d49...a64a9a4a280cb5be0f80bb01ce98812e56bcda6d
python3 tools/check_conventions.py --base 5cb015caaaaf83f8ee431414ee6a457b5b307d49
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "heap_mirror|lod_clamp"
# negative control: the five production files from main
git show 5cb015caaa…:core/libs/prx/libSceAgcDriver/Graphics/{include/Context.hpp,include/GuestBufferMemory.hpp,include/ResidentMirror.hpp,src/GuestBufferMemory.cpp,src/ResidentMirror.cpp} > …
ninja -C build -j32 -l32 agc_driver_resident_heap_mirror_tests agc_driver_image_sample_lod_clamp_tests
ctest --test-dir build --output-on-failure -R "^agc_driver_resident_heap_mirror$|lod_clamp_shader_lane"
# restore the merged files from a5a36646c4…, rebuild everything
ninja -C build -j32 -l32
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Configure / build / libs (script) | ok |
agc_driver_resident_heap_mirror (with the new PartialBlockTests and ConcurrentWriteTests) |
passed |
agc_heap_mirror_*, agc_sparse_heap_mirror_* (10 tests) |
passed |
agc_driver_image_sample_lod_clamp_shader_lane (listed by the script as a new failure) |
fails, and fails the same way with main's production files, so it comes from main (baseline was the older 096c45570f), not from this PR |
unwrap_self_..._destination_directory_group (listed as a new failure) |
a Python tool test failing on os.chown in a temporary directory (EINVAL); this PR changes no tool or relinker file, so it is the environment, not the PR |
| Validation layer (script, 759 tests) | layer loaded in 758, missing in none, 0 tests with new VUID- lines |
| Worktree after restore | git status --porcelain empty |
lod_clamp_shader_lane output (same on merged tree and with main's production files)
AGC graphics: image_sample_lz through the S# that holds the LOD at 0 with T# MIN_LOD 768 over BASE_LEVEL 0 with a second S# of words 1 0 and 2 5242880, and a second T# of MIN_LOD 0 and LAST_LEVEL 0: thread 0 component 1 is 0.000000, expected 0.062500
Negative control
I put back main's versions of Context.hpp, GuestBufferMemory.hpp, ResidentMirror.hpp, GuestBufferMemory.cpp and ResidentMirror.cpp, kept the PR's tests/execution/ResidentHeapMirror.cpp, and rebuilt. agc_driver_resident_heap_mirror still passed (2.23 s).
That can be expected: the new partial-block, in-flight CPU write and double-dispatch cases describe behaviour that main's host-copy design already had. So they guard against regressions in the rewrite, but they don't show the change itself. Nothing tests the host-memory saving, the slab refusal, the budget shrink-and-retry loop or the nonzero-scan cache. I then put the merged files back, rebuilt the whole tree, and checked that the worktree is clean.
Findings
Blocking
None.
Non-blocking
- No test reaches the slab-refusal and budget-shrink path.
ResidentMemory::refuseSlab(core/libs/prx/libSceAgcDriver/Graphics/src/ResidentMirror.cpp:176),shrinkHeapMirrorBudget(core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp:2814) and the 8-attempt retry loops (GuestBufferMemory.cpp:2849,GuestBufferMemory.cpp:3387) are the fix the PR depends on for PPSA02015. They run only when the device heap fills or when slabs passAPS5_HEAP_MIRROR_MIB. The existing tests pass onmainwithout any of this code. Suggested fix: add a case that sets a small budget withSetHeapMirrorBudget(whichHeapMirrorSlabBudgetalready reads). It should check that a slab past it is refused, the budget is lowered, an older mirror is evicted, and the new mirror is still built. - The lowered budget never goes back up.
heapMirrorMemoryLimit(GuestBufferMemory.cpp:2714) is only ever lowered, atGuestBufferMemory.cpp:2824, and nothing raises it again. So one refusal caused by a temporary peak in device-heap use leavesheapMirrorBudget()lowered for the rest of the process. The soft-target escape inreserveHeapBytes/growResidentonly applies on a retry, after a sweep has already evicted mirrors. So after one temporary peak, mirrors keep being evicted early for the whole session. Suggested fix: raise the limit again once the reported heap has headroom, or say in the description that the drop is permanent by design. - The refusal is recognised by matching the error text.
shrinkHeapMirrorBudgettestserror.what()for"of a heap mirror slab". A change to either message (refuseSlaborallocate's operation string) would silently turn off the retry and bring back the stop this PR fixes. Suggested fix: use a dedicated exception type derived fromstd::runtime_error, or a flag returned byTake. Context::HeapReport::Fitsis never used (core/libs/prx/libSceAgcDriver/Graphics/include/Context.hpp:340).refuseSlabdoes its own headroom arithmetic. Either useFitsor remove it.- The nonzero-scan cache is shared global state.
GuestNonzeroBlocksnow keeps a process-wideunordered_mapof up to 4M entries (ResidentMirror.cpp:379-380) behind one mutex, which it takes once or twice per 64 KiB block. The whole map is dropped when it fills (ResidentMirror.cpp:408). Its correctness depends onUnchangedSincecoming back false for pages that were unmapped and mapped again (ForgetPages). I didn't find a test of that for this cache (ReusedAddressTestspasses, but it also passed before the cache existed). This is the third separate change in the first commit; see the next item. - One topic per PR. Besides dropping the host copies, the PR also includes a cherry-picked commit by another author (
8a30d9a70f8b, "store a resident heap mirror's write-back from its readback buffer"), the slab-refusal and fallback work, fullest-slab allocation, and the scan cache (1743177ae4af). The checklist namesDepends on: #15295, #12827. If those PRs carry some of these commits, the commits would be merged twice or conflict. Please state which commits belong to which PR, or move the separate fixes into their own PRs.
Not verified
- The claimed host-memory saving and the PPSA02015 world-load result: I launched no title. lavapipe has a 64 GiB host-backed heap, so the device-heap headroom refusal and the host-memory fallback never run here.
- The Windows/NVIDIA validation run in the description: I only ran on lavapipe.
- The contents of #15295 and #12827: no context was available, so I couldn't check the overlap described in finding 6.
- Whether a
RecordStagingCopiesflush that uploads newly marked ranges aftertakeHeapReferencesrecorded the device reference could put those bytes into the write-back. That would happen if the range differs between the reference and the readback without any shader store. I didn't build a test for this ordering. The newConcurrentWriteTestsdoesn't cover it.
To get this to a good state
- Add a test that reaches the refusal, shrink, sweep and retry path (finding 1). Ideally also add one for the scan cache across an unmap and remap at the same address (finding 5).
- Decide whether the lowered budget should recover (finding 2), and replace the error-text match (finding 3).
- Remove or use
HeapReport::Fits(finding 4). - Clarify how this PR's commits relate to #15295 and #12827, or split the separate fixes out (finding 6).
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Hi Prophet069, This head contains the exact commit from #15295 plus four follow-up commits, while its checklist says Please also settle the overlap/order with open #12827 (the same readback-vector-copy removal) and #12840 (BDA heap-store work). At their current heads, combining this PR with #12827 conflicts in |
…ost fallback A slab refused for headroom or for APS5_HEAP_MIRROR_MIB while the heap mirrors held 256 MiB or less stopped the title: the budget shrink has nothing to evict there. Refuse a slab only while more than that is held, so the allocation and its host-memory fallback run otherwise. Refusals and failed slab allocations throw HeapMirrorSlabRefused instead of being matched by their text, a budget lowered by a refusal recovers after 30 s, and the headroom check uses HeapReport::Fits. A test reports the device heap full and checks that small mirrors are still made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…k buffer Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit c12a20f)
A resident heap mirror kept a 16 MiB host copy per slab, so rebuilding a multi-GB mirror needed as much host memory again. Refreshes now only mark the changed ranges, the flush reads them straight from guest memory into bounded staging and zero-fills new blocks on the device, and the write-back reference of each use is a device copy recorded after the uploads and before the shader. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… references inside completions Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…waves RecordStagingCopies flushed changed resident heap mirror blocks without allowing a wait, so a multi-GiB mirror upload held every 64 MiB staging segment until the batch finished, gigabytes of host memory at once. It now waits after each 256 MiB wave like UploadFinish does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a64a9a4 to
7bbf091
Compare
|
Thanks. This PR is stacked: it builds on #15295 (its first two commits) and on #12827 (the cherry-picked write-back commit); its own commits are "drop the host copies of resident heap mirrors", "count heap mirror transfer buffers as kept bytes and record references inside completions" and "let staging-copy recording wait between heap mirror upload waves". The checklist says On the review points: the refusal type, the budget recovery and |
|
Hi Prophet069, Thanks for declaring
|
|
@Prophet069, on the order with #12840 (BDA stores into writable heap mirrors), which oneandonlydean asked about above: #12840 can go after this PR.
#12840's checklist will say |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 525 tests)Skipped (49)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validated but not compared (56, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 519 tests)Skipped (54)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The new commit 89e3292072 ("let a small heap mirror reach the slab allocation and its host fallback") does four things. It replaces the error-text match with a HeapMirrorSlabRefused type. It lets a lowered budget recover after 30 s. It uses HeapReport::Fits. It refuses a slab only while the heap mirrors hold more than 256 MiB. Its new FullDeviceHeapTests case fails when that last guard is removed. The other commits were rebased onto it with the same content.
What changed since my last review
- New commit
89e329207201, between the slab-refusal commit and the cherry-picked write-back commit. The later commits were rebased onto it. Per the range-diff, they only change around the new test struct and thetry/catchinResidentMemory::Take. The head commit7bbf091cf8is unchanged. - The author explained in discussion how the commits split across PRs. The first two commits are #15295,
7c184fb3aeis the cherry-pick from #12827, and the last three are this PR's own. The checklist saysDepends on: #15295, #12827. The author also agreed with EagleBearerYS that #12840 lands after this PR.
What I ran
Head 7bbf091cf83a767b16e38f966d2d72d9321b0d81, main d1ae628bc2f62459cce1e9872ba5632c938c71f2, merged tree bf1ec647053b1866f995446eb40a85dcac8f0323 (--no-ff). Debian 13 VM, GCC 14.2.0, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check d1ae628bc2f62459cce1e9872ba5632c938c71f2...7bbf091cf83a767b16e38f966d2d72d9321b0d81
python3 tools/check_conventions.py --base d1ae628bc2f62459cce1e9872ba5632c938c71f2
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "heap_mirror"
# negative control: delete `if (live <= Reserve) return;` from ResidentMemory::refuseSlab
sed -i "/if (live <= Reserve) return;/d" core/libs/prx/libSceAgcDriver/Graphics/src/ResidentMirror.cpp
ninja -C build -j32 -l32 agc_driver_resident_heap_mirror_tests
ctest --test-dir build --output-on-failure -R "^agc_driver_resident_heap_mirror$"
# restore
git show bf1ec647053b1866f995446eb40a85dcac8f0323:core/libs/prx/libSceAgcDriver/Graphics/src/ResidentMirror.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/ResidentMirror.cpp
ninja -C build -j32 -l32
ctest --test-dir build --output-on-failure -R "^agc_driver_resident_heap_mirror$"
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Configure / build / libs (script) | ok |
heap_mirror tests (10, including agc_driver_resident_heap_mirror with FullDeviceHeapTests) |
all passed (2.5 s) |
| Validation layer (script, 767 tests) | layer loaded in 766, missing in none, 0 tests with new VUID- lines |
unwrap_self_..._destination_directory_group (script: new failure) |
os.chown on a temporary directory fails with EINVAL. This PR changes no tool file, so the cause is the environment, as in my last review |
| Negative control (guard removed) | agc_driver_resident_heap_mirror fails |
| After restore | test passes, git status --porcelain empty |
Negative control output
[heap-mirror] refused: slabs 0 (0 MiB), free slots 0 MiB, live mirrors 0 bound 0 MiB, budget 24576 MiB
AGC graphics: heap mirror of 0x7f7b55800000+0x2000000: vkAllocateMemory of a heap mirror slab refused: its device heap would keep less than 256 MiB of its reported budget; heap mirrors hold 15 MiB of 24576 MiB (APS5_HEAP_MIRROR_MIB)
I didn't do a full restore of main's production files. The new test calls SetDeviceHeapFullForTests, which doesn't exist on main, so it wouldn't compile. Removing only the guard is the narrower control, and it shows the new test exercises the headroom refusal.
Previous findings
- No test reaches the refusal/shrink path. Partly resolved, still open for the shrink.
FullDeviceHeapTests(core/libs/prx/libSceAgcDriver/tests/execution/ResidentHeapMirror.cpp:370) now reachesrefuseSlab(see the negative control). But it holds less than 256 MiB, soshrinkHeapMirrorBudgetreturns atcore/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp:2841. The lowering, the eviction sweep and the 8-attempt retry are still not run by any test. Non-blocking. - The lowered budget never recovers. Resolved.
heapMirrorBudgetputs the limit back after 30 s (GuestBufferMemory.cpp:2734), and the time a refusal lowers it is stored atGuestBufferMemory.cpp:2842. No test covers it. - Refusal matched by error text. Resolved. It is now
dynamic_cast<const HeapMirrorSlabRefused*>(GuestBufferMemory.cpp:2834). Both the refusal and a failed slab allocation throw that type (core/libs/prx/libSceAgcDriver/Graphics/src/ResidentMirror.cpp:155-159,:199). That is the same set of errors the old text match caught. HeapReport::Fitsunused. Resolved. It is used atResidentMirror.cpp:193.- Nonzero-scan cache is shared global state, with no test across unmap and remap. Still open, unchanged (
ResidentMirror.cpp:381). Non-blocking. - One topic per PR. Resolved by declaration. The commits are attributed to #15295 and #12827 in discussion, and the checklist says
Depends on: #15295, #12827. The landing order is author-reported. I couldn't check the other PRs' heads.
New findings
Blocking
None.
Non-blocking
- A flush after the reference copy is still possible. oneandonlydean raised this, and it was under "Not verified" in my last review. The author said they would look at it next, and the head commit hasn't changed. I can confirm the order this would happen in.
recordTransferrecords uploads intorecorder->Commands()in sequence (ResidentMirror.cpp:61-68), and so doesRecordReference(ResidentMirror.cpp:616-627). So any range still marked whenRecordStagingCopiescallsResidentBuffer::FlushChanged(context, true)(GuestBufferMemory.cpp:6928) is uploaded after the reference thattakeHeapReferencescopied (GuestBufferMemory.cpp:6715-6718). The write-back would then treat it as a shader write. I didn't build a case that puts a mark between the two calls. Suggested fix: show that no mark can land there, or make the write-back ignore ranges flushed after the reference. Add a test either way. - Recovery is by time only. After 30 s the limit goes back to unlimited, whether or not the device heap has room again (
GuestBufferMemory.cpp:2734). On a heap that stays full, this can mean another refuse, shrink and evict cycle every 30 s. That is acceptable as a design choice, but a short note in the commit or description would help.
Not verified
- The host-memory saving and the PPSA02015 world-load result: I launched no title.
- The NVIDIA/Windows run in the description (author-reported). lavapipe reports a large host-backed heap, so the real headroom refusal and the host fallback only ran through the test override.
- The shrink, evict and retry loop, and the 30 s recovery: no test reaches either, and I didn't write one.
- The contents and landing order of #15295, #12827 and #12840.
|
A real-world data point for this PR, from Ghost of Tsushima DC (PPSA02225) on a 16 GB RAM / 8 GB VRAM Windows laptop (RTX 4060 Laptop), main b11c7bf + #12840 + #12792.
I'll build this PR on top of that tree and report the private-memory, freeze and logo results on the same machine. |
|
@sktharunbalaji13-cmd a note before you build it: this PR and #12840 don't merge together yet. Both change |
|
@EagleBearerYS thanks, that matches what I saw: this PR doesn't apply on top of #12840 (the write-back and resident-copy hunks are rejected). So I'm testing the two memory PRs separately on the same 16 GB / 8 GB machine:
Each is compared with that tree's run, on the same machine and in the same conditions: free RAM and private bytes over time, freeze length after the intro soundtrack starts, how far the intro gets, and whether the 135 s VRAM abort still happens. I'll post the numbers here and on #15295. |
|
Results from Ghost of Tsushima DC (PPSA02225) on the 16 GB RAM / 8 GB VRAM laptop (RTX 4060 Laptop, driver 617.42). This PR was applied onto
Logs are available on request. |
|
One more data point on the same 16 GB RAM / 8 GB VRAM laptop: main b11c7bf + this PR, PPSA02225, nothing else open, Defender off, 11.3 GB free at launch. The VRAM abort from the base tree is gone, but after the budget was lowered this run stopped at 147 s with a different abort: The 4.7 GB heap 0x1200000000 is over the 3.9 GB host-import budget, and once the lowered budget refuses its mirror, the only path left is the copy, which Is that path meant to evict and retry, or to let the mirror pass the lowered budget? Your description says a lowered budget is a soft target that a mirror may pass when nothing can be evicted. The earlier run of this PR on the same machine (no abort in 150 s) is in the comment above. |
What
A resident heap mirror kept a 16 MiB host copy of every slab, so building a multi-GiB mirror needed as much host memory again, and a rebuild while the old mirror was alive needed it twice. The host copies are gone.
A refresh now only marks the changed ranges (
ResidentBuffer::MarkChanged, an interval map in which a later mark wins). The flush reads the marked ranges straight from guest memory into bounded 64 MiB staging segments, waits after every 256 MiB when the caller allows it, and fills newly bound or zeroed blocks on the device withvkCmdFillBuffer.The write-back reference of each use is a device copy of the written ranges recorded after the uploads and before the shader (
ResidentBuffer::RecordReference); the write-back compares the readback after the shader with it, so only bytes the shader changed reach the guest and CPU writes made meanwhile are kept. Non-resident heap mirrors keep their pooled host references.Reference, upload and readback buffers count as kept bytes of the recorder, so its existing bound submits before they pile up, and references made inside a batch completion are recorded in order with the uploads.
Tests: partly written 64 KiB blocks, CPU writes made while a dispatch is in flight, and two dispatches writing the same range.
Tested
NVIDIA GeForce RTX 3080 Laptop GPU, driver 616.64, Windows 10 Pro 22H2 (19045.6466).
Focused tests on
main(9090972) and on this branch withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1,ctest -V -R "heap_mirror|^agc_driver_graphics$|^agc_driver_key_miss$|^agc_driver_bda_device$|^agc_driver_recorder$|native_multisample_depth|depth_storage|^agc_host_import_retire$|^agc_host_import_sparse$":main: 17 of 20 selected tests passed (agc_host_import_sparse skips on this machine; the three agc_driver_depth_storage_* tests were not built in this run); validation layer loaded in both logs;VUID-errors: none.VUID-errors: none.agc_driver_resident_heap_mirrorincludes the new partial-block and in-flight CPU write cases.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code