Repository navigation
perf(agc): give small device-local buffers a pool tier of their own - #8912
Celegans12 wants to merge 1 commit into
Conversation
Device-local buffers under 1 MiB are retained in the small tier, so the large device buffers' churn no longer evicts them. But the small tier's 64 MiB budget is also the small host buffers' budget, and the two now evict each other. Each small device miss is a dedicated vkAllocateMemory and each eviction a vkFreeMemory on the thread that builds the draws. Small device-local buffers now get a tier of their own: a budget of max(64 MiB, device tier budget / 8) and the device tier's class search they had before, apart from the large device buffers and the small host buffers. With four tiers, each tier's slot bound is an eighth of maxMemoryAllocationCount, so half of it stays free as before. The [bufferpool] line reports the new tier. AI-assisted: yes
github-actions
Bot
added
perf
|
This pull request is labeled |
|
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.
|
|
I reviewed the new tier selection, budget/slot bound, and trim/destructor coverage; the focused |
|
@MarcZX10 thanks for going through the tier selection, the slot bound and the trim/destructor coverage. Good to hear Fair point on the A/B: it toggled #8911, #8912 and this PR as a group, on our own Yōtei tree rather than |
|
AMD / Linux (RX 9070 XT, RADV) A/B of #8911 + #8912 + #8913 together on ASTRO BOT: neutral there (no change, no regression). Numbers and trace digest: #8911 (comment) Tested and written by Claude (AI assistant) on a contributor's machine. |
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
Each of these pool changes is small and the reasoning is sound (small device-local buffers evicting small host buffers out of one 64 MiB tier; a hit whose VkBuffer has another exact size paying a destroy and bind). The problem is the set.
- #8911, #8912 and #8913 are measured "together in one in-run open-world A/B (9+9 phases): 6.17 vs 5.16 presents per 10 s", on the author's own Ghost of Yotei tree, not on
main, and "not isolated for this PR". The three cannot be told apart, so none of them is shown to pay for itself, and a regression in one would be hidden by gains in the others. - At least four more open PRs change the same pool tiers and free list: #8748 (keep host-placed device buffers for reuse), #8888 (size classes for large host buffers), #8672 (reclaim idle device buffers under cache pressure) and #8913 (this series). #8913 itself says it overlaps #8748's free-list change. They conflict textually and, more importantly, interact: a size-class change in #8888 changes what #8913's exact-size scan finds, and a separate small tier changes what #8672 reclaims.
- Please agree an order with the other authors and measure each step against the previous one on the same scene, summary lines of main and each PR, no
APS5_PROFILE_*. Until then the maintainers cannot tell which of the five is the one that helps, and a pool tuned by five independent heuristics is hard to reason about when the next title's churn pattern differs.
|
You're right about the set. The 6.17 vs 5.16 A/B toggled #8911, #8912 and #8913 together on our own Yōtei tree, so it can't show that any single one of them pays for itself. And the five pool PRs touch the same tiers and free list, so they interact. Here's an order I'd suggest, for @LeandroLP23, @milescoviello and @eliasbruno124-dev to weigh in on:
#8911 doesn't touch the pool (it skips the queued-draw read check when a storage flush stores nothing), so it can be measured on its own at any point. If any of you would rather go in a different order, I'm happy to rebase ours around it. For our three, I'll measure each one alone against |
|
Thanks @Celegans12, that order works for me, with #8888 second. On the current heads, AI-assisted: yes (Claude Code) |
|
@CnRJay I ran each of the three pool PRs on its own now. Plain main can't take Ghost of Yōtei to the open world (the loading screen stops presenting after about 160 s and the game later aborts on a blend into a 10_11_11 unorm target main doesn't support), so I can't give you main-vs-PR summary lines in the world. What I can give is our tree (our local merge with main of 2026-10-21) with only this PR's switch, On presents this one is in the noise: 5.75 per 10 s with the tier vs 5.16 without, but paired against neighbouring phases it's -0.13 +- 0.62, so I wouldn't claim anything there. The per-draw cost is clearer: room wait on queue 0 goes from 94 to 113 us per draw without the tier (+18 +- 4 us, the one difference here well past noise), buffer allocation per upload from 3.5 to 12.7 us and the import lookup from 15 to 37 us. With the tier on, small device-local buffers hit about 88k times per phase against 357 misses. I also tried a boot-scene pair on plain main, but those captures turned out to be pipeline-cold on this machine, so I'm not using them. On the order with #8748, #8888 and #8672: I'm happy to rerun this A/B on top of whichever lands first. DetailsRun |
|
Linux data point for the test, since @MarcZX10 could not run Head. Negative control. One scoping note on what this shows: Stacked with #8913 ( AI-assisted: yes |
|
@Dextroid17, thank you for the native lavapipe runs on all three (#8911, #8912, #8913). The negative controls were the part I couldn't give myself from WSL, and they fail exactly where each body says. Your point that |
|
@Celegans12 sorry for the late reply. The order works for me, with #8748 first. It is approved and mergeable on current AI-assisted: yes (Claude Code) |
|
Measured alone against main, as asked in review, on an AMD APU (Radeon 780M, Linux, RADV, Mesa 26.0.8). Base upstream main 853baf3, PR head b7fd852 on top, default settings. Diablo II: Resurrected (Rogue Encampment after loading a save), 60 s settle, 120 s counted, two alternating pairs.
The new tier is used: 5951 hits / 656 misses, 652 buffers retained (108 of 165 MiB), and 0 small-pool evictions against 5250 on main. The PR arm was lower in both pairs, but I can't call it a regression yet: on this machine D2R lands at either ~560 or ~1400 ms per frame in the same scene in either arm, so the noise is about the size of the difference. The title is CPU-bound in the driver (busiest worker at 99-100%, GPU ~170-200 ms per present). Logs and aps5 summaries available if useful. This comment was written with help from Claude (AI). |
|
@mrfelek thanks for measuring this alone on different hardware. That's exactly the run we couldn't do, and I'm taking it seriously even though you're careful not to call it a regression yet. The counters show the tier really is used in D2R: 5951 hits against 656 misses, and no small-pool evictions where main had 5250. So whatever the presents are doing, it isn't because the change sat idle. Two questions, if you have time. Do you see the same split between ~560 and ~1400 ms frames on For comparison: on Ghost of Yōtei with an RTX 3080, our own per-PR A/B (this switch alone, flipped every 60 s in one run) was in the noise on presents, -0.13 ± 0.62 per 10 s. Per draw it was clearly better: queue-0 room wait went from 113 to 94 µs. That's NVIDIA with separate VRAM, though. On a 780M the retained 108 MiB of "device-local" buffers is system memory, so a real regression on your title would matter, and I'd rather find it now. If the logs and summaries are easy to attach, I'd like to look at the allocation and import-lookup lines around the slow frames. @LeandroLP23 thanks, #8748 first works for me. I'll rerun our numbers on top of it once it lands. |
|
Follow-up to my earlier numbers: three more alternating pairs on the same setup (Radeon 780M, RADV, main 853baf3 vs PR head b7fd852, D2R Rogue Encampment, #8912 first in each pair).
Across all five pairs main was faster in three and #8912 in two. D2R on this machine settles into one of four frame-time states (~170, ~560, ~1400, ~2600 ms per frame) in either arm, and that swing is far larger than any difference between the arms. So I withdraw the "looks slower" reading: these runs show no speed effect either way. What does hold in every run: the small-device tier is used (about 11-15k hits, ~800 misses, 0 evictions, 97-134 MiB retained), and small-pool evictions stay at 0 with the PR against 2,949-4,616 on main. No vkAllocateMemory failures in any run. This comment was written with help from Claude (AI). |
|
@mrfelek thanks for the three extra pairs, that settles it much better than I could have from here. Four frame-time states swinging far more than the arms differ is a useful thing to know about D2R on an APU, and it explains the first two pairs. The part that holds in every run is the part this PR is about: the small-device tier is used, small-pool evictions stay at 0 against 2,949-4,616 on |
…ar#9289 boykopovar#8911 boykopovar#8912 boykopovar#8913 boykopovar#10382 (PPSA26344 world, 60-present windows)
|
I've finished the second round of world captures I mentioned, this time with 60-present windows (the first round's 16-present windows were too noisy to use). All ten are warm runs of PPSA26344 v1.008 on our own tree (085bd8f, which carries all six of these changes; not With the change switched off, the frame p50 was 2,006 ms against 1,867 and 1,605 ms in the A runs on either side (mean 1,736), and GPU busy was 937 against 955 and 906 ms per present (mean 930). The off run is the slowest of all ten, about 16 % above the mean of its A runs. Those two A runs are 15 % apart on their own, though, so I read it as leaning toward the change helping, not as a measured gain. These are all on one tree, so they don't replace a Files are on our media branch (the comment box can't take 35 MB traces):
|
github-actions
Bot
removed
the
perf numbers missing
✅ NVIDIA 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 (1, not counted against this PR)Validation IDs already present on main (13 IDs in 548 tests)Skipped (48)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. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the current head b7fd852e0cba5c2078b742d220a50262d82a5055 against live main ba5b5330b37724bca4b99f30faab504653635a04.
The pool change and SmallDeviceOwnTier regression are understandable, and the patch merges cleanly onto current main. git diff --check and tools/check_conventions.py pass on that comparison. The author’s reported focused Windows and WSL test runs are useful; I did not independently run the product tests here. On the current PR head, only conventions/labels ran; the Windows, Linux, write-watch, register-context and relinker jobs are skipped.
I’m requesting two follow-ups before approval:
-
Performance evidence: the title-menu main/change captures are well matched, but the report says this scene does not exercise the new small-device tier, and the measured change is within main’s run-to-run spread. The open-world result combines #8911, #8912 and #8913 on a non-main tree, so it cannot attribute the result to this PR. Please provide a same-title/scene/machine comparison on current
mainversus currentmainplus this PR in a workload that actually exercises the tier, including both summary lines, traces andtrace_digest.pyoutput. If this is intended as a correctness-only change, please remove the unsupported performance claim/label and coordinate that scope with maintainers. -
Overlap order: #8748 remains open and the PR body acknowledges a shared
BufferPool::Takeedit, but this PR’s checklist still saysDepends on: noneand I could not find an author-agreed landing order. Please agree the order with #8748’s author and record the dependency on the second PR, as the contribution rules require.
The current-main merge preview, whitespace check and conventions check are clean; they do not resolve the performance attribution or ordering questions above.
|
@MarcZX10 on the landing order: Celegans12 and I agreed it in #8912 (comment). #8748 lands first and #8912 follows, so I have rebased #8748 onto AI-assisted: yes (Claude Code) |
|
Static review plus an independent Linux/AMD run on both trees per the repo AGENTS AGC protocol (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja 1.13.2, Release, What I checked (static)
Focused paired run (VVL env)
Note on run history (transparency, same class as #7985's note) An intermediate pass of mine reported Sequencing note
|
|
On the AI-assisted: yes (Claude Code) |
|
I've recorded the agreed #8748-first order in the checklist and clarified the performance limitation in Tested. #8748 is still open, so this branch will follow it as agreed. The combined Yōtei captures don't establish a speed gain for this PR alone, and the neutral menu captures don't exercise the tier; the requested current-main comparison remains outstanding. I haven't rerun the game or claimed a new performance result in this upkeep pass. |
What
#7056 moved device-local buffers under 1 MiB out of the device tier into the small tier, so the large device buffers' churn stops evicting them. But the small tier's 64 MiB budget is also what the small host buffers live in, so now those two evict each other. Every small device miss is a dedicated
vkAllocateMemoryand every eviction avkFreeMemoryon the draw commit thread. In Yōtei on our tree, after we merged #7056 the small tier had 2.5x the misses of our previous build (492k vs 192k), andBufferPool::Puttook 1.8 s of that thread per 120 s.smallDeviceTier) with a budget of max(64 MiB, device tier budget / 8), on top of the device tier's. They get back the device tier's next-class search they had before fix(agc): keep small device buffers in the small pool tier #7056. The large device buffers and the small host buffers no longer evict them, and they don't evict those.maxMemoryAllocationCount, so four tiers still leave half of it free; the header comment that described three tiers is gone.APS5_BUFFER_POOL_SHAREDstill puts every device buffer in the device tier.trimand the destructor cover the new tier, and the[bufferpool]line reports it.Overlaps: #8748 edits the line after the class search in
Take. The authors agreed that #8748 lands first and this PR rebases over it. #11920 touches the constructor initializer; its author agreed to rebase if this PR lands first. #8672's idle reclaim and #4472's VRAM rule walk only the device tier, so small device buffers stay out of their reach, as they are onmaintoday.Tested
The requested current-
maincomparison in a scene that exercises this tier remains outstanding. The older combined open-world result does not establish a speed gain for this PR; the isolated in-run and title-menu comparisons below show no significant frame-rate gain.-DBUILD_TESTING=ON, RTX 3080 Laptop, based onmain7ab4b2e: ctest 1824/1828. The 4 failures (log_macros,log_macros_timestamps,guest_kqueue, andagc_driver_image_array_window_past_layer_limit, which doesn't compile with MinGW here) fail onmaintoo.SmallDeviceOwnTiercase inagc_buffer_pool: a released 256 KiB device buffer survives 80 MiB of small host buffer churn and is reused without an allocation. Withmain'sBufferPoolit fails: "small host buffer churn evicted a retained small device buffer".main: 1762/1782; the same 20 fail onmain(WSL has no userfaultfd write watching).pb-8912(2026-10-22, our tree onmain557682c, not currentmain):APS5_NO_SMALL_DEVICE_TIERflipped every 60 s in one warm 1,300 s Yōtei run. Paired B−A was −0.13 ± 0.62 presents/10 s (8 pairs): no significant frame-rate gain. Queue-0 room wait was 94 vs 113 µs/draw, and small-device retention was exercised. This does not satisfy the requested current-main-versus-change scene comparison.mainvs this change on the same machine (RTX 3080 Laptop, driver 610.62, Windows 11), Ghost of Yōtei (PPSA26344 v1.008) title menu from the same save with no input, warm caches, interleaved runs in one session, perf-report protocol (onlyAPS5_TRACE_FRAMES=150:60, noAPS5_PROFILE_*):maina5a2c94 againstmaina5a2c94 + this PR. Frame p50 131.9 ms (3 runs) against 132.5 ms (2 runs), GPU busy 77.7 against 81.0 ms per present. That's inside the run-to-run spread (main's three runs: 128.2-135.1 ms), as expected for a scene that doesn't exercise this path; our open-world numbers, from our own tree, are in the comments. Traces andtools/trace_digest.pyoutput per run: pl-main-1, pl-main-2, pl-main-3, pl-8912-1, pl-8912-2.aps5-summary.jsonl, pl-main-3 and pl-8912-2 (the run closest to each side's mean; `title` written as `ō`, since the summary writer double-encodes it and GitHub mangles that escape){"commit":"a5a2c946a3a9","dirty":false,"env":{"APS5_NO_CLEAR_HAND_OVER":"1","APS5_NO_DEFERRED_SETTLES":"1","APS5_NO_PAD_INPUT":"1","APS5_NO_PIPELINE_JOURNAL":"1","APS5_NO_RESIDENT_LINEAR":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_RESOURCE_CACHE_ENTRIES":"1024","APS5_TRACE_FRAMES":"150:60"},"gpu":"NVIDIA GeForce RTX 3080 Laptop GPU","driver":"NVIDIA 610.62","os":"Windows 10.0.26200","cpu":"11th Gen Intel(R) Core(TM) i9-11900H @ 2.50GHz","cpu_threads":16,"memory_mib":32469,"title_id":"PPSA26344","title":"Ghost of Yōtei","content_version":"01.008.000","resolution":"3840x2160","window":{"spec":"frames 150:60","first_present":150,"ms":11388.409},"presents":60,"frame_ms":{"p50":132.237,"p90":378.788,"p99":2488.458,"max":2488.458},"gpu_busy_ms_per_present":76.739,"worker_busy_pct":{"queue worker 0x0":61.336,"queue worker 0x20":12.709,"queue worker 0x52":10.897,"queue worker 0x53":12.001,"queue worker 0x30":43.438,"queue worker 0x38":50.980,"queue worker 0x48":3.931},"top_phases_ms_per_frame":{"Driver.Packet":363.867,"Shader.PrepareArtifact":329.176,"Driver.Packet.dispatch":163.759,"Driver.Packet.memory_wait":130.658,"Driver.Dispatch":116.659,"Driver.Packet.prepare_memory":46.855,"GpuMutex.wait.dispatch":32.057,"Shader.Register":29.105},"trace_dropped":false,"trace":"aps5-trace-a5a2c946a3a9-16452.json.gz"}{"commit":"c9ab24b2a148","dirty":false,"env":{"APS5_NO_CLEAR_HAND_OVER":"1","APS5_NO_DEFERRED_SETTLES":"1","APS5_NO_PAD_INPUT":"1","APS5_NO_PIPELINE_JOURNAL":"1","APS5_NO_RESIDENT_LINEAR":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_RESOURCE_CACHE_ENTRIES":"1024","APS5_TRACE_FRAMES":"150:60"},"gpu":"NVIDIA GeForce RTX 3080 Laptop GPU","driver":"NVIDIA 610.62","os":"Windows 10.0.26200","cpu":"11th Gen Intel(R) Core(TM) i9-11900H @ 2.50GHz","cpu_threads":16,"memory_mib":32469,"title_id":"PPSA26344","title":"Ghost of Yōtei","content_version":"01.008.000","resolution":"3840x2160","window":{"spec":"frames 150:60","first_present":150,"ms":11964.391},"presents":60,"frame_ms":{"p50":133.365,"p90":391.895,"p99":2939.489,"max":2939.489},"gpu_busy_ms_per_present":81.509,"worker_busy_pct":{"queue worker 0x0":60.509,"queue worker 0x52":10.355,"queue worker 0x53":12.095,"queue worker 0x20":11.279,"queue worker 0x30":42.591,"queue worker 0x48":4.669,"queue worker 0x38":50.579},"top_phases_ms_per_frame":{"Shader.PrepareArtifact":386.964,"Driver.Packet":377.535,"Driver.Packet.dispatch":168.934,"Driver.Packet.memory_wait":133.947,"Driver.Dispatch":118.660,"Driver.Packet.prepare_memory":49.840,"GpuMutex.wait.dispatch":32.522,"Shader.Register":28.438},"trace_dropped":false,"trace":"aps5-trace-c9ab24b2a148-25492.json.gz"}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)