Repository navigation
perf(agc): copy moved draw words into the draw upload ring - #11142
Conversation
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 ring-slice change and descriptor offset handling look consistent in the PR-specific diff. I am requesting changes only for the required performance-report provenance.
Both pasted aps5-summary.jsonl records identify commit 61fc572ceef8 and set dirty:true; only their trace filenames/window values differ. CONTRIBUTING.md requires two valid summary lines from different commits, one on main and one with the change. As written, these records do not identify a clean base commit and a clean commit containing this PR, so the automated perf numbers missing label is correct even though the traces compare two configurations.
Please provide clean, different base/change commit summaries and the associated traces/digests, or ask maintainers to decide whether this dependency-stack/toggle comparison is an accepted exception and update the classification accordingly. I did not rerun the reported hardware Vulkan suite because this host has no matching macOS/MoltenVK GPU setup; the PR reports the required same-machine base/change validation runs.
|
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.
|
github-actions
Bot
removed
the
perf numbers missing
|
@MarcZX10 Thanks, that's fair. The description now has two captures from clean, different commits. The files are in the comment above and the summary lines are pasted in the description:
The base cannot be a commit on |
|
Correction to my earlier review rationale: the author has now supplied clean, distinct base/change commits, the two trace attachments, summary lines and digests, so my earlier statement that both records pointed to the same dirty commit is no longer current. The remaining question is narrower: the base capture is a combined dependency stack rather than a commit on current |
klosixgaming-arch
left a comment
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Reviewed head 159007bd43 by reading the diff and the existing review thread. I did not build or run this, so everything below is from the code, not from a test of mine.
What it does. Moved-word snapshots no longer go through the per-ShaderResources 8-slot movedWordBuffers cache. They are copied into the recorder's draw upload ring with recorder.DrawUpload(size), and the descriptor uses the slice's own buffer and offset. The tests are adjusted for a non-zero offset (item.offset, .subspan(offset)), which is the right place to touch them.
What I checked. DrawUpload aligns to max(minStorageBufferOffsetAlignment, 16), so a storage-buffer descriptor at that offset is legal, and the std::shared_ptr<Buffer> in the slice keeps the ring buffer alive for the draw. Removing the mutable cache from ShaderResources also removes shared mutable state from a const method, which is a good thing independent of speed.
Concerns:
- No size bound on this path. The other
DrawUploadcall site is guarded byDrawUploadLimit()(4096 bytes by default). The moved-word path has no such limit, andDrawUploadallocates a freshmax(1 MiB, bytes)buffer whenever the ring is full or the request does not fit. The old code bounded this to 8 retained buffers. Ifoverride->sizecan be large, one draw per frame can now allocate a dedicated buffer where it used to reuse one. Please state the maximumoverride->sizeor add the same limit and fallback. - Dedup is gone. The old cache returned the same buffer when
allocation,sizeand words were equal, so a repeated draw with identical moved data created no new buffer. Now every such draw copies the words again. The perf numbers in the thread are what justifies that trade, and I can't verify them. They come from the author's captures and MarcZX10 has asked for clean base and change commits, which the author supplied. I take them as given. patch.adjustmentis still written as a single byte (static_cast<std::byte>). That is carried over unchanged, so I'm not blaming this PR.
Leaving a comment: point 1 is a question I'd like answered; the rest is fine.
AI-assisted: yes. Written with AI assistance and reviewed by me.
|
@klosixgaming Thanks for the review.
|
|
A Windows/NVIDIA run, since this was tested on macOS only. One test fails with this PR: Windows 11 Pro 26200, Intel Core i5-14600K, NVIDIA GeForce RTX 5070 Ti (driver 610.88), WinLibs GCC 15.2.0 r7, Release. Head
The check is const bool replaced = key.first == (allocation == 0 ? 2u : 7u);
const auto expected = replaced ? draw->snapshots.front().buffer->Handle() : before.buffer;
const auto expectedOffset = replaced ? draw->snapshots.front().offset : before.offset;
Require(after.buffer == expected && after.offset == expectedOffset && after.range == before.range, "snapshot update replaced the wrong buffer or changed its range");
Full |
…escriptor Co-authored-by: SeaSunPT <24361469+SeaSunPT@users.noreply.github.com>
|
@SeaSunPT Thanks, you're right. I reproduced it on macOS with MoltenVK: |
|
Correction to the description and to my comment above: the base is not only the six PRs I listed. To build and boot on macOS it also carries other open macOS PRs and, for the AGC driver files and a few tests that no open PR covers yet, local patches from #8658's branch. The description now says so; the base commit |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the diff against |
What
ShaderResources::PrepareDrawBindingscopies a draw's moved words into a slice of the recorder's draw upload ring (Recorder::DrawUpload) instead of aBufferof their own.movedWordBufferscache made a newBuffer(VK_BUFFER_USAGE_STORAGE_BUFFER_BIT). The buffer pool usually hands back a slot whoseVkBufferhas another size, so the constructor destroys it and binds a new one (BindExactBuffer), and the buffer is destroyed again when the draw retires.movedWordBufferscache goes away: a ring slice is only written once, but caching it perShaderResourceswould keep its 1 MiB chunk alive for as long as the entry lives.agc_driver_graphicschecked that a moved-word descriptor starts at offset 0; it now checks the snapshot's offset and reads the words from there.Why: with #11130's mimalloc on, the draw committer paces Planet of Lana on macOS. In a 15 s Time Profiler capture it spends 626 samples in
Buffer::Bufferand 471 invkDestroyBuffercalled from it, out of 10,921, all from this call site.Tested
macOS 27, M4 Pro, Rosetta 2, MoltenVK 1.4.2.
maindoes not boot a title on macOS yet, so both sides aremain61fc572 plus the macOS changes needed to build and boot one: open macOS PRs (among them #10366, #10552, #7631, #7804, #10889, #10898, #11047, #11048 and #11049) and, for the AGC driver files and a few tests that no open PR covers yet, local patches taken from #8658's branch. #11130 is on both sides, withAPS5_MIMALLOC=1; the change side adds this PR. The base commit below is that whole tree, so its exact diff againstmainis public.Planet of Lana (PPSA19450), village gameplay from New Game with the same scripted pad input, 120 s per run, fps from 85 s, runs alternated in one session. These fps runs were on the same PRs uncommitted, plus a local experiment active on both sides (a
DrawTailpool, neutral when measured), which the clean commits below leave out:Every run with this PR is above every base run.
Driver.Packetgoes from 21 to 22 ms per frame to 19 ms.tools/trace_digest.pyofAPS5_TRACE_SECONDS=95:10 APS5_TRACE_EVENTS=1048576captures, same machine and scene, from two clean commits:7f2e300c68dc:main61fc572 with the macOS changes described above and feat(libs): let relinked macOS games opt into mimalloc #11130, committed as one tree;84cf9d0f359e: that commit plus this PR's diff.aps5-summary.jsonllinesbase:
{"commit":"7f2e300c68dc","dirty":false,"env":{"APS5_ASYNC_PIPELINES":"1","APS5_MIMALLOC":"1","APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"95:10","APS5_VIDEOOUT_TELEMETRY_DIR":"<path>"},"gpu":"Apple M4 Pro","driver":"MoltenVK 1.4.2","os":"Darwin 27.0.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":12,"memory_mib":24576,"title_id":"PPSA19450","title":"Planet of Lana","content_version":"01.010.000","resolution":"3840x2160","window":{"spec":"seconds 95:10","first_present":4314,"ms":10006.822},"presents":335,"frame_ms":{"p50":29.699,"p90":31.546,"p99":35.005,"max":49.648},"gpu_busy_ms_per_present":20.229,"worker_busy_pct":{"queue worker 0x0":74.217},"top_phases_ms_per_frame":{"Driver.Packet":21.741,"Driver.Packet.draw":16.331,"Driver.Draw":16.173,"Graphics.Draw":16.158,"Graphics.Draw.draw_and_resource_release":12.321,"Driver.Draw.shader_compile_and_link":9.298,"VideoOut.Flip":7.205,"Driver.Present":6.389},"trace_dropped":false,"trace":"aps5-trace-7f2e300c68dc-24308.json.gz"}this PR:
{"commit":"84cf9d0f359e","dirty":false,"env":{"APS5_ASYNC_PIPELINES":"1","APS5_MIMALLOC":"1","APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"95:10","APS5_VIDEOOUT_TELEMETRY_DIR":"<path>"},"gpu":"Apple M4 Pro","driver":"MoltenVK 1.4.2","os":"Darwin 27.0.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":12,"memory_mib":24576,"title_id":"PPSA19450","title":"Planet of Lana","content_version":"01.010.000","resolution":"3840x2160","window":{"spec":"seconds 95:10","first_present":4348,"ms":10010.079},"presents":343,"frame_ms":{"p50":28.891,"p90":31.006,"p99":37.188,"max":46.898},"gpu_busy_ms_per_present":19.400,"worker_busy_pct":{"queue worker 0x0":66.822},"top_phases_ms_per_frame":{"Driver.Packet":19.199,"Driver.Packet.draw":14.585,"Driver.Draw":14.437,"Graphics.Draw":13.030,"Graphics.Draw.draw_and_resource_release":9.421,"VideoOut.Flip":9.220,"Driver.Draw.shader_compile_and_link":8.939,"Driver.Present":8.414},"trace_dropped":false,"trace":"aps5-trace-84cf9d0f359e-27576.json.gz"}Driver.Packetgoes from 21.7 to 19.2 ms per frame andGraphics.Drawfrom 16.2 to 13.0; the draw committer's busy time goes from 9.4 to 5.3 ms, andVideoOut.Flipgrows from 7.2 to 9.2 ms, since the frame now waits on the GPU. A second capture of this PR's commit gave 342 presents and p50 29.0 ms; the base capture before it was not written because Rosetta was still translating the new libraries when the window opened.AGC tests, same machine and GPU, base and this PR, with
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363),ctest -R 'agc_draw_snapshot|agc_driver_graphics$|agc_recorder|agc_pending_write_snapshot|agc_shader_table_snapshot|agc_driver_partially_resident_snapshot|agc_snapshot|agc_driver_prepared_graphics' -V:agc_driver_prepared_graphics_prebuild_*).was not found but was requested by env var VK_INSTANCE_LAYERS.VUID-VkDeviceCreateInfo-pProperties-04451(MoltenVK's portability subset at device creation) and no otherVUID-.agc_driver_graphicsfails at the offset check described above.Full
ctest -R '^agc_' -j6withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1and the validation layer, on the base and on this PR with44c6919b64: the same 44 of 992 fail on both sides and the same 80 skip; no test fails only with this PR. An earlier run withoutANYPS5_REQUIRE_VULKAN=1had skippedagc_draw_descriptors, which SeaSunPT found failing on Windows;44c6919b64fixes its expectation.Not built on Linux or Windows.
pr_overlap.pyreports #8963 and #9508, which edit lines next to the removedmovedWordBuffersmembers inShaderResources.hpp. #8913 changes how the buffer pool picks a slot and helps the old path (33.91 fps on the same bench); after this PR that call site no longer makes buffers, and #8913 still applies to the other callers.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)