Repository navigation
perf(agc): prefer a pooled buffer of the requested exact size - #8913
Conversation
A pooled buffer is retained by size class, and a hit whose VkBuffer was created for another exact size destroys it and binds a new one to the same memory (BindExactBuffer). Copying draw inputs on NVIDIA's Windows driver takes one such buffer per index or vertex range per draw, so most hits in that path pay a vkDestroyBuffer, vkCreateBuffer and bind. Take now scans the newest 64 slots of the class for one whose buffer has the requested size and takes the newest slot as before when none has. The memory class, usage and properties are the same, so only the choice among equivalent slots changes. AI-assisted: yes
github-actions
Bot
added
perf
|
This pull request is labeled |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The Take change preserves the existing newest-slot fallback and removes only the selected slot; the regression exercises both exact sizes and checks the buffers are reused. I ran python3 tools/check_conventions.py --base upstream/main --head HEAD and git diff --check upstream/main...HEAD; both pass. I could not run agc_buffer_pool from this Linux checkout because there is no matching configured test binary here. One reporting gap: the performance result combines this PR with #8911 and #8912, while CONTRIBUTING.md and the perf-label bot ask for matched main/change trace summaries and both trace artifacts. Please attach the required artifacts or keep the quantified claim clearly scoped to the combined set.
|
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.
|
|
@MarcZX10 thanks for running the conventions check and You're right that the speed result mixed this PR with #8911 and #8912. It came from our own Yōtei tree, not |
|
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. |
|
Here's this PR measured alone, as asked on #8912. 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 this is our tree (our local merge with main of 2026-10-21) with only The result is mixed. Presents lean its way: 7.23 per 10 s with the exact-size scan vs 6.78 without, paired -0.34 +- 0.28 with it off, about 1.2 SE. But committer time per commit was a little lower with the scan off (258 vs 234 us, paired -19 +- 15 us), so the scan itself isn't free. It does what it says on the buffers: per phase about 490k pooled hits of the exact size with it on against 365k off, and rebinds to another size drop from about 733k to 476k. 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 quoting them. Given milescoviello's point about #8888's size classes, I'd rerun this after #8888 lands rather than tune the scan now. DetailsRun |
|
Linux data point for the test, since the review says Head. Negative control. Scoping: AI-assisted: yes |
…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 1,469 ms against 1,605 and 1,634 ms in the A runs on either side (mean 1,620), and GPU busy was 808 against 906 and 877 ms per present (mean 892). This one goes the wrong way: with the change switched off, frames were about 9 % shorter and GPU time per present about 9 % lower, and the two A runs around it agree within 2 %. An earlier, shorter round leaned the same way. So on our tree right now this change looks like it costs time rather than saving it. I'm running an in-run A/B of the switch (both settings alternating inside one run, so session drift drops out) to check, and I'll post that result here either way. 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):
|
|
Follow-up to my last comment: the in-run A/B doesn't back up the -9 %, so please disregard that reading. I alternated 60 s phases with and without Per-phase means (A = as is, B = APS5_NO_EXACT_SIZE_REUSE=1) |
github-actions
Bot
removed
the
perf numbers missing
|
Lana numbers for this PR, on macOS, where the draw committer hits the rebuild path on most draws. Setup: M4 Pro, macOS 27, Rosetta 2, MoltenVK 1.4.2.
All three #8913 runs are above seven of the eight base runs. Where it hits: in a 15 s Time Profiler capture of the base, the committer thread spends 626 samples in For the same call site I also tried copying the moved words into the recorder's draw upload ring ( Not measured: this PR's own exact-size hit counter, and |
|
@imellag thanks, this is the first measurement that shows where the exact-size hit actually lands. On Yōtei I could only say the change was neutral to slightly positive, and I never pinned down which Your #11142 looks like the better fix for that particular caller, since it skips making the buffer at all instead of reusing one. I agree the two don't get in each other's way: once #11142 lands, this PR still covers the other buffers that come through |
What
The pool retains buffers by size class. When a hit's
VkBufferwas made for another exact size,BufferandDeviceBufferdestroy it and bind a new one to the same memory (BindExactBuffer). Since #6763 copies draw inputs on NVIDIA's Windows driver, each draw takes such a buffer per index or vertex range, andTakealways hands out the newest slot of the class, whatever its size. In a Yōtei profile on our tree the draw commit thread spent 1.3 s per 120 s inBindExactBufferand another 0.36 s in the destroys.Takenow scans the newest 64 slots of the class for one whose buffer has the requested size, and takes the newest slot as before when none does.Overlaps: #8748 changes the free-list key a few lines above in
Take(textual only).Tested
-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.ExactSizeReusecase inagc_buffer_pool: with a 3000-byte and then a 4000-byte buffer released into one class, a 3000-byte request gets the first one back without a rebuild and a 4000-byte request the second. Withmain'sBufferPoolit fails: "a pooled buffer of the requested exact size was passed over for a newer one that had to be rebuilt".main: 1762/1782; the same 20 fail onmain(WSL has no userfaultfd write watching).main, with perf(agc): skip the queued-draw read check when a storage flush stores nothing #8911, perf(agc): give small device-local buffers a pool tier of their own #8912 and perf(agc): prefer a pooled buffer of the requested exact size #8913 switched on and off together in one in-run open-world A/B (9+9 phases): 6.17 vs 5.16 presents per 10 s (paired -0.95 +- 0.20 SE) for the combined set. This PR's own counter in that run: 436k vs 253k exact-size hits, 425k vs 493k rebinds to another size. Themain-vs-change summary lines, traces andtrace_digest.pyoutput for the perf label are still owed.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.7 ms (2 runs), GPU busy 77.7 against 81.5 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-8913-1, pl-8913-2.aps5-summary.jsonl, pl-main-3 and pl-8913-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":"ef29bab589cf","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":11646.443},"presents":60,"frame_ms":{"p50":132.185,"p90":393.353,"p99":2677.602,"max":2677.602},"gpu_busy_ms_per_present":79.738,"worker_busy_pct":{"queue worker 0x0":60.693,"queue worker 0x20":12.172,"queue worker 0x52":9.480,"queue worker 0x53":11.304,"queue worker 0x30":42.585,"queue worker 0x48":4.456,"queue worker 0x38":50.368},"top_phases_ms_per_frame":{"Driver.Packet":365.333,"Shader.PrepareArtifact":355.582,"Driver.Packet.dispatch":160.389,"Driver.Packet.memory_wait":127.861,"Driver.Dispatch":112.743,"Driver.Packet.prepare_memory":48.419,"GpuMutex.wait.flush":30.404,"GpuMutex.wait.dispatch":30.058},"trace_dropped":false,"trace":"aps5-trace-ef29bab589cf-21524.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)