Repository navigation
perf(agc): keep released image memory in the buffer pool for reuse by type and size - #7870
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
fogydes
left a comment
fogydes
left a comment
There was a problem hiding this comment.
Verified on Windows 11 with MinGW-w64 GCC 15.2.0 (Release):
- Architecture & Implementation: Caching released image
VkDeviceMemorywithinBufferPoolby type index and size underAPS5_IMAGE_POOL_MIBdirectly addresses allocations churn during dynamic resolution changes without introducing device memory unbounded growth. - Conventions & Docs: Environment variable
APS5_IMAGE_POOL_MIBdocumented indocs/dev/ENVIRONMENT.md. Zero code comments added;tools/check_conventions.pypasses with 0 findings. - Unit Testing: Built
agc_buffer_pool_tests.exeand verifiedctest --test-dir build-libs -R "^agc_buffer_pool$" -Vpasses in 0.21s:All recovery and slab validation fixtures passed.1/1 Test #288: agc_buffer_pool .................. Passed 0.21 sec
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The profile is convincing (47% of resolveImageTables samples in StorageTexture::release, 40% in Create, 70% ending in kernel calls) and the budget rules (nothing retained over budget, nothing for host-placed or unknown-size allocations) are thought through. The gain is real but smaller than the headline, and one hazard needs a test.
- The A/B is 12 presents per capture on a scene running at 5 to 8 s per frame. The two
maincaptures differ by 6% at p50 and 12% over the 12 presents; the gain is 17 to 27%. That is outside the spread but not by much, and 12 frames of one loading card is the best case for this pool (an identical image size repeating at every resolution step). The same run's own[bufferpool]line shows 68,989 hits against 221,690 misses and 0 retained in 13 of 19 lines, so 24% of re-creations reuse memory. Please add a pair in a scene that is not the churn card (gameplay, same title), to show the cost where sizes do not repeat. The description names that case as untested. - The hazard the description states: a block is bound to the next image of that size at once, while on
maina driver that defers an in-use free hid a missing keep (APS5_NO_KEEP_NEW_STORAGEis one). So every missing keep in the tree, today invisible, becomes a use-after-reuse that corrupts a texture. The Khronos validation comparison is "still owed". Please run it with synchronization validation on the focused tests and on a Wolverine capture before this lands, since the validation layer is the only thing that finds this class. - Each release scans the device tier's keys for the oldest image, on the queue worker, at a measured 219,556 evictions in 200 s. That is a linear scan per release on the path the PR is speeding up. Use an LRU list or an ordered index so a release and an eviction are O(1).
- The pool holds up to a quarter of the device tier's budget as idle device memory. For a title with a heap already near its budget (#4472, #5987) the pool should give it back first. State what happens when
AllocateMemoryfails: does the recovery trim free the pooled image memory before it reports the failure (the description says it does) and is that tested with a full pool? - Only Windows and an RTX 4080 SUPER. An AMD or Linux run of
agc_buffer_pool*and the same trace pair would show the behavior is not specific to the NVIDIA driver's slowvkFreeMemory.
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Read the diff; the accounting (imageSlots/imageBytes in take, put, both evictions and trim), the over-budget rule and the OOM trim are consistent, and the new tests pin allocation/free counts. What is still missing before this can land, in line with @CnRJay's review:
- Validation comparison (CONTRIBUTING): the VUID set with and without the change is still owed. It matters more here than usual: a retained block is bound to the next image of that size at once, so any missing keep that
mainhides behind a deferred driver free becomes a texture overwrite. Please run the focused tests and a Wolverine capture with the Khronos layer (synchronization validation on) and post the VUID diff. BufferPool.cpp:414-418(evictOldestImage) scans every key of the device tier on each eviction and over-budget release, on the queue worker, at ~220k evictions per 200 s. Keep the images in an ordered index or LRU list so release and eviction are O(1).- A pair in a scene that is not the churn card (gameplay of the same title), where sizes do not repeat, to show the cost of delayed frees and idle pool memory. The 12-present card is the best case and
mainvaries 6-12 % between its own captures. - Peak device memory (budget
usage) and RSS before/after on that workload, since up to a quarter of the device tier idles in the pool. - If you can, one run of
agc_buffer_pool*and the trace pair on AMD or Linux, to show the gain is not specific to the NVIDIAvkFreeMemorycost.
… type and size An image's own VkDeviceMemory was allocated and freed one at a time, and in a scene that re-creates its storage images at every dynamic-resolution step the dispatching queue worker spent half of its image-table resolution freeing evicted images and most of the rest allocating new ones. The pool now keeps the device memory of a released storage or sampled image under its own budget (APS5_IMAGE_POOL_MIB, a quarter of the device tier's budget by default) and hands it to the next image of the same memory type and size.
A release scanned every key of the device tier for the oldest retained image, on the queue worker, once per retained image the budget does not hold. Retained images are now also kept in one list in release order, and each retained slot holds its own node, so a release picks and evicts the oldest image in constant time and a take removes its node in constant time. The device-tier eviction that a release can also trigger when the buffer budget or the slot count is reached still scans the tier's keys; that path predates this change and is untouched.
99ef6fa to
bafc97b
Compare
|
The out-of-memory trim with a full pool is covered by The branch was rebased onto Not done here: the Khronos validation comparison, the capture pair in a scene whose image sizes do not repeat, peak device memory and RSS on that workload, and a run on AMD or Linux. |
|
Validation and memory evidence for the remaining review items. Setup. Windows 11 Insider build 26300 (reported as 10.0.26300), Ryzen 7 5800X3D, 32 GB, RTX 4080 SUPER 16 GB, driver 617.42, WinLibs GCC 15.2.0, Khronos validation layer 1.4.363 with synchronization validation requested (the layer's Focused tests. Wolverine, splash card, no input, 900 s cap per arm: identical VUID sets, Memory, four 300 s runs alternating main/PR/main/PR, mean of the per-run peaks over two runs per arm: working set 14,717 vs 14,696 MiB, private bytes 27,951 vs 27,932 MiB, whole-device VRAM from Not done: AMD or Linux; a scene after the splash card (the title presented 2 to 32 frames in 300 to 900 s with no input, so none was reached); a VUID set scoped to an |
|
Re-checked against the earlier points: |
346 upstream merges since b509767. 37 conflicted files / 100 hunks, resolved per research/structural/merge21 notes (execution, graphics, recompiler+tests). Versions: FormatVersion 50, RuntimeAbi 18, request serialization 42 (upstream 39 mergedHull and 41 f32 denorm as written; our single-workgroup flag is 42). Upstream's image memory pool (boykopovar#7870) is opt-in with APS5_IMAGE_POOL=1. AI-assisted: yes
- ImageArrayWindowPastLayerLimit (upstream test): MinGW GCC refuses alignas(65536) on a static object; the texels now come from an aligned heap allocation. - BufferPool ImageMemoryOverBudget (boykopovar#7870): uses our mock's heap budget and usage fields. - WriteTracking CheckImportWindowOverUnwatchedPart: with boykopovar#8172's per-span unwatch the watched blocks stay watched; the unwatch hook must cover the part that leaves the watch. AI-assisted: yes
What
The buffer pool keeps released device buffers for reuse, but an image's own
VkDeviceMemoryis allocated withvkAllocateMemoryand freed withvkFreeMemoryone image at a time. In a scene that re-creates its storage images at every dynamic-resolution step (Marvel's Wolverine's "Sony Interactive Entertainment presents" card, the churn of #6099), the dispatching queue worker spends most of its image-table resolution allocating and releasing images: of the 4129 samples insideShaderResources::resolveImageTablesin a 300 sAPS5_SAMPLE_WORKERcapture ofmain(withAPS5_PROFILE_DRAW=1, whose prints took 98 of them), 1960 (47 %) are inStorageTexture::release(vkFreeMemory,vkDestroyImageand the views' destruction, which the stacks do not tell apart) and 1634 (40 %) inStorageTexture::Create(936 of them inBufferPool::AllocateMemory); 2888 (70 %) end inwin32u.dll, the kernel calls behind the NVIDIA driver's allocation and release (collapsed stacks in the evidence zip,sampled-main/).BufferPool::PutImageMemorykeeps the device memory of a released image in the device tier, keyed by memory type and exact size, under its own budget:APS5_IMAGE_POOL_MIB, a quarter of the device tier's budget by default (0, orAPS5_STAGING_POOL_MIB=0, frees image memory at once, asmaindoes). It retains nothing while any device-local heap'sVK_EXT_memory_budgetusage is over its budget (read at most every 100 ms; never without the extension) and then also frees the image memory it holds, while retained blocks are still handed out meanwhile; host-placed allocations, allocations of unknown size and allocations larger than the image budget are freed at once. Retained image memory counts in the device tier's budget and slot bound and is evicted least recently used together with the tier's buffers, so a burst of image releases can also push retained staging buffers out.BufferPool::TakeImageMemoryhands a retained allocation of the same type and size to the next image before allocating;AllocateImageMemorygoes through it and reports the type it allocated.AllocateMemorytrims the pool again (every tier and the spare slab blocks) after a cache release, since the images that release destroys now return their memory to the pool, and it also runs forVK_ERROR_TOO_MANY_OBJECTS, since retained allocations hold allocation handles.StorageTextureand the sampled textures'OwnedImagerecord their allocation's size and type and release it throughReleaseImageMemory. Depth surfaces, multisampled targets, render targets and the attachment proxies still allocate throughAllocateImageMemory, so they take a retained block of a matching type and size when there is one, but keep freeing their memory directly.[bufferpool]profile line gains the image counts (hits, misses, evictions, retained).Which images are created, when they are evicted and what they hold do not change: a retained allocation is bound to a new image that starts in
VK_IMAGE_LAYOUT_UNDEFINEDand is uploaded as before. An image is released only once no recorded batch keeps it, which is what already letmainfree its memory there; a keep that was missing would have been masked onmainby the driver deferring an in-use free, while with the pool the block is bound to the next image of that size at once (APS5_NO_KEEP_NEW_STORAGEis such a case, noted in its row ofdocs/dev/ENVIRONMENT.md).Tested
Windows 11 Pro 26300, Ryzen 7 5800X3D, 32 GB RAM, RTX 4080 SUPER 16 GB (NVIDIA 617.42), WinLibs GCC 15.2.0 r7, Release,
BUILD_TESTING=ON.agc_buffer_poolwith the new cases (ImageMemoryReuse: the retained allocation comes back for the same type and size, another size allocates, a host-visible or unsized allocation is freed, least-recently-used eviction under the image budget, the out-of-memory trim frees the retained memory;ImageMemoryReclaim: an allocation failure whose cache release returns an image's memory to the pool still recovers;ImageMemoryOverBudget: nothing is retained while the heap is over its reported budget),agc_buffer_pool_small_device_shared,agc_buffer_pool_small_device_no_staging: pass. WithAPS5_IMAGE_POOL_MIB=0theImageMemoryReusecase fails on its retention expectations and then stops at its out-of-memory step, which has nothing to trim (unit-tests/pool-test-off-final.login the evidence zip); that is the behaviour ofmain.APS5_TRACE_FRAMES=10:12, every capture onmainf78675a + feat(libs): restore libSceNpCppWebApi for dumps that ship no copy #6901 (the title needs feat(libs): restore libSceNpCppWebApi for dumps that ship no copy #6901 to load: without it its libSceNpCppWebApi imports are absent, as reported on feat(libs): restore libSceNpCppWebApi for dumps that ship no copy #6901) withAPS5_KEPT_DESCRIPTOR_SETS=0(Marvel's Wolverine runs out of device memory since #6454: kept descriptor sets keep evicted textures alive #7801, since fix(agc): let a kept descriptor set's table record hold its textures weakly #7802 is open) andANYPS5_SHADER_CACHE_DIRpointing at the warm cache (the summary lines'envlistsAPS5_*only), same machine and session,maincaptured twice; the change was captured as commit 58d293532 on that base, the same patch as this PR's commit (git patch-idequal):{"commit":"c024ba034b49","dirty":false,"env":{"APS5_KEPT_DESCRIPTOR_SETS":"0","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"10:12"},"gpu":"NVIDIA GeForce RTX 4080 SUPER","driver":"NVIDIA 617.42","os":"Windows 10.0.26300","resolution":"3840x2160","window":{"spec":"frames 10:12","first_present":10,"ms":76850.693},"presents":12,"frame_ms":{"p50":6009.057,"p90":8511.643,"p99":8604.445,"max":8604.445},"gpu_busy_ms_per_present":1106.740,"worker_busy_pct":{"queue worker 0x0":99.923,"queue worker 0x28":0.038,"queue worker 0x20":98.760},"top_phases_ms_per_frame":{"Driver.Packet":12681.928,"Driver.Packet.dispatch":6325.430,"Driver.Dispatch":6324.631,"Driver.Packet.memory_wait":6274.826,"Vulkan.Dispatch":5490.262,"Vulkan.Dispatch.shader_resources":5488.643,"Resources.Complete":5488.297,"Driver.EndSubmission":44.524},"trace_dropped":false,"trace":"aps5-trace-c024ba034b49-41080.json.gz"} {"commit":"c024ba034b49","dirty":false,"env":{"APS5_KEPT_DESCRIPTOR_SETS":"0","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"10:12"},"gpu":"NVIDIA GeForce RTX 4080 SUPER","driver":"NVIDIA 617.42","os":"Windows 10.0.26300","resolution":"3840x2160","window":{"spec":"frames 10:12","first_present":10,"ms":67608.415},"presents":12,"frame_ms":{"p50":5667.689,"p90":5858.166,"p99":6023.004,"max":6023.004},"gpu_busy_ms_per_present":1058.698,"worker_busy_pct":{"queue worker 0x0":99.955,"queue worker 0x20":98.671,"queue worker 0x28":0.137},"top_phases_ms_per_frame":{"Driver.Packet":11138.148,"Driver.Packet.dispatch":5554.701,"Driver.Dispatch":5554.102,"Driver.Packet.memory_wait":5504.770,"Vulkan.Dispatch":4817.111,"Vulkan.Dispatch.shader_resources":4815.729,"Resources.Complete":4815.452,"Driver.EndSubmission":60.152},"trace_dropped":false,"trace":"aps5-trace-c024ba034b49-40472.json.gz"} {"commit":"58d293532c03","dirty":false,"env":{"APS5_KEPT_DESCRIPTOR_SETS":"0","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"10:12"},"gpu":"NVIDIA GeForce RTX 4080 SUPER","driver":"NVIDIA 617.42","os":"Windows 10.0.26300","resolution":"3840x2160","window":{"spec":"frames 10:12","first_present":10,"ms":56301.046},"presents":12,"frame_ms":{"p50":4683.760,"p90":4742.898,"p99":4891.576,"max":4891.576},"gpu_busy_ms_per_present":989.094,"worker_busy_pct":{"queue worker 0x0":99.584,"queue worker 0x20":98.537,"queue worker 0x28":0.055},"top_phases_ms_per_frame":{"Driver.Packet":9252.764,"Driver.Packet.dispatch":4600.420,"Driver.Dispatch":4599.650,"Driver.Packet.memory_wait":4575.680,"Vulkan.Dispatch":3994.348,"Vulkan.Dispatch.shader_resources":3992.903,"Resources.Complete":3992.595,"Driver.EndSubmission":45.090},"trace_dropped":false,"trace":"aps5-trace-58d293532c03-9900.json.gz"}main(1st)main(2nd)Resources.Completeper frameDriver.Packet.memory_waitper frameAgainst the second
maincapture: frame p50 −17 %, the 12 presents −17 %,Resources.Completeper frame −17 %; against the first: −22 %, −27 %, −27 %. The twomaincaptures differ between themselves by 6 % at p50 and 12 % over the 12 presents, so the gain is 17–27 % on these figures here, not a sharper number. An earlier capture of this change (commit 62f3819a2548, before the review fixes: the over-budget rule, the recovery trim,VK_ERROR_TOO_MANY_OBJECTSand the device-budget bound on what is retained) gave p50 4991 ms, p90 7467 ms and 67992 ms for the 12 presents; it is attached as well.tools/trace_digest.pyoutput of every capture andtools/performance_compare.pyofmain(1st) against this PR are in the evidence zip. Attachments: image-memory-pool-traces.zip holds the fouraps5-trace-*.json.gzcaptures unchanged (GitHub refuses a.json.gzattachment on its own), image-memory-pool-evidence.zip the summary lines, digests, sampled stacks, test logs and pool counters.In a separate 200 s run of this PR with
APS5_PROFILE_DRAW=1(profile-pool2/in the evidence zip), the last[bufferpool]line (the 19th, printed at a buffer take at least 190 s in) readsimages: 68989 hits, 221690 misses, 219556 evictions, 0 retained (0 MiB of 477): the 477 MiB image budget holds about a tenth of a second of this scene's churn (the run's last cumulative[texture]line counts 293300 storage images of 1153687 MiB in 200 s, about 5.6 GiB/s), so a quarter of the re-creations take their memory from the pool and the rest allocate; 13 of the 19 lines show 0 retained, the others 68 to 303 images. A largerAPS5_IMAGE_POOL_MIBwould raise the hit rate at the price of idle device memory; I kept the quarter.agc_buffer_pool,agc_buffer_pool_small_device_shared,agc_buffer_pool_small_device_no_staging,agc_storage_upload_lifetime,agc_storage_write_back,agc_driver_graphics,agc_driver_image_table_reuse,agc_driver_storage_cache_budget) onmainf78675a + feat(libs): restore libSceNpCppWebApi for dumps that ship no copy #6901 and on this PR, withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationset in the shell that ranctest -V(the logs list only the variables CMake sets): all pass on both (focused-tests/in the evidence zip). The Khronos validation layer is not installed on this PC, so neither log holdsVUID-lines and the comparison of validation errors that CONTRIBUTING asks for is still owed; I will add it once the layer is installed.Not tested: Linux; other GPUs; an integrated GPU (where the device-local types are usually host-visible too, so
DeviceTieredis false and nothing is retained; one that exposes a device-local-only type retains as a discrete card does); a title whose image sizes do not repeat (every release then retains and evicts another: the frees are delayed and up to the image budget of device memory idles in the pool).Overlaps: #6481 changes the line of the sampled texture's constructor that this PR changes (
Texture.cppL350 onmain, hereowned->Allocate(...)). #4472 changes the device tier's retention, #7119AllocateMemory's failure diagnostics and the slot bound, and #7800AllocateMemory's signature and the cache release it calls; they conflict withmaintoday, sotools/pr_overlap.pycould not compare them, and whichever lands second merges by hand. The report lists about thirty more open PRs that shareTexture.cpp,Texture.hppordocs/dev/ENVIRONMENT.md(and, for #6292 and #6615,BufferPool.hpp, whose mutex type they change) and conflict withmain; those are textual. Retained image entries sit in the device tier, so #4472's pressure policy would cover them only once it is made to seePutImageMemory.Checklist
Based on current
main; no other open PR implements the same functionsOne topic per PR; follow-ups go in a new PR
No comments in code except technical debt
Unimplemented paths throw (
NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt fileNo notes, investigation
.mdfiles or images added to the repository (attach them to this PR)New third-party code is a submodule built from source
Generic behaviour, not specific to one title
Rebase: the branch was rebased onto
main1e774a3 and now has a second commit,bafc97bb1e, which answers the reviews' O(1) request. Mergingmainduplicated thephysical,memoryPropertiesandmemoryProperties2members (mainadded them for the failure diagnostics, this PR for the heap budget), so the rebased commit keeps one declaration of each, and the profile commit58d293532is no longer patch-identical to the PR's first commit. Re-tested on the new base: full build with WinLibs GCC 15.2.0 (Windows 11), exit 0;agc_buffer_pool,agc_buffer_pool_small_device_sharedandagc_buffer_pool_small_device_no_stagingpassed on two consecutive runs (2.88 s / 0.41 s for the first). With onlybafc97bb1ereverted the same three tests still pass, as a behaviour-preserving refactor should; withAPS5_IMAGE_POOL_MIB=0theagc_buffer_pooltest still fails at the out-of-memory step, as described above.Depends on: none
Fixes: none
Unblocks: none
GPUs tested: NVIDIA GeForce RTX 4080 SUPER, driver 617.42, Windows 11 Pro 26300
AI-assisted: yes