Repository navigation
perf(agc): bind moved read-only words from the draw upload chunk - #11451
johandavidcm wants to merge 1 commit into
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 perf evidence is not yet reviewable under the repository rule. Both pasted aps5-summary.jsonl records identify commit ef5836d37150; only the environment differs (APS5_NO_MOVED_WORD_UPLOAD=1 versus unset), so the first record does not identify a capture on main and the second does not identify this PR head. Please provide a same-title, same-scene, same-machine capture on current main and on the changed head, paste the two actual summary records, attach both trace files, and include the output of tools/trace_digest.py. The branch is also based on 3ead2d7251 while main has advanced to 51f5c57dd9; please sync it and rerun the required checks.
|
@johandavidcm, a note on how this meets #9712, which keeps draw snapshots in resizable BAR memory. There the draw upload chunk ( The two overlap on one line: both add a test to the Windows AI-assisted: yes |
|
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.
|
aaef484 to
1941801
Compare
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 1941801408e7eeddc39326643ae2bf1f4695c2e9. The code patch is unchanged from the previously reviewed commit, but the required performance evidence is still not present: both pasted summary lines identify the same commit ef5836d37150 and differ only by APS5_NO_MOVED_WORD_UPLOAD. That is a useful configuration comparison, but it is not a capture on main and one on this PR head as required by CONTRIBUTING.md.
The head is based on f616a1397f9b618036ac7c89f461f0bd46fbd6d6; the live main head is 2dbe9b99f577f5fad8ee675932a0603439e5eb2a. Please provide a same-title, same-scene, same-machine pair captured on those exact revisions, attach both traces, and include the actual tools/trace_digest.py output. Until those artifacts identify a main-vs-change comparison, the perf numbers missing label is accurate.
Local checks I ran against current main and this exact head: python3 tools/check_conventions.py --base upstream/main --head upstream/pr-11451 passed; git diff --check upstream/main...upstream/pr-11451 passed; the merge-tree preview is clean. Hosted eligibility, conventions, labels and report checks pass; relinker and product jobs were skipped. I did not run the Windows/GPU tests locally.
1941801 to
29c9624
Compare
github-actions
Bot
removed
the
perf numbers missing
|
@MarcZX10 rebased onto Captures on two builds, as asked: base
Base, commit b5e8d63b2seconds 140:60 from present 2994: 280 presents, frame p50 210.0 / p90 227.3 / p99 312.2 / max 327.1 ms, GPU busy 76.4 ms per present {"commit":"b5e8d63b20cc","dirty":false,"env":{"APS5_DRAW_INPUTS_IN_PLACE":"1","APS5_DRAW_PIPELINE":"1","APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"140:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 7800X3D 8-Core Processor","cpu_threads":16,"memory_mib":31892,"title_id":"PPSA31327","title":"Yakuza 0 Director's Cut","content_version":"01.015.000","resolution":"1920x1080","window":{"spec":"seconds 140:60","first_present":2994,"ms":60172.649},"presents":280,"frame_ms":{"p50":209.978,"p90":227.347,"p99":312.175,"max":327.108},"gpu_busy_ms_per_present":76.376,"worker_busy_pct":{"queue worker 0x0":98.155},"top_phases_ms_per_frame":{"Driver.Packet":210.788,"Graphics.Draw":183.394,"Driver.Packet.draw":179.783,"Driver.Draw":178.258,"Driver.DrawFinish":160.736,"Driver.Draw.prepare":150.34,"Graphics.Draw.draw_and_resource_release":149.14,"Driver.DrawFinish.shader_compile_and_link":110.619},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":4.229,"events_lost":3483105,"totals_full":false},"draw finisher":{"events_full_after_s":8.221,"events_lost":1702625,"totals_full":false},"draw committer":{"events_full_after_s":12.164,"events_lost":1055416,"totals_full":false}},"trace":"aps5-trace-b5e8d63b20cc-89144.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 76.4, Graphics.Draw 21.5, Graphics.Draw.draw_and_resource_release 14.9, Driver.DrawFinish 14.2, Driver.Packet 9.7, Driver.Packet.draw 8.2, Driver.Draw 8.1, Driver.DrawFinish.shader_compile_and_link 7.9, Driver.Draw.prepare 6.5, VideoOut.Flip 6.2 Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
This change, commit 1d307e1e0seconds 140:60 from present 3034: 315 presents, frame p50 190.0 / p90 198.8 / p99 206.4 / max 216.7 ms, GPU busy 69.3 ms per present {"commit":"1d307e1e0282","dirty":false,"env":{"APS5_DRAW_INPUTS_IN_PLACE":"1","APS5_DRAW_PIPELINE":"1","APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"140:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 7800X3D 8-Core Processor","cpu_threads":16,"memory_mib":31892,"title_id":"PPSA31327","title":"Yakuza 0 Director's Cut","content_version":"01.015.000","resolution":"1920x1080","window":{"spec":"seconds 140:60","first_present":3034,"ms":60033.79},"presents":315,"frame_ms":{"p50":190.032,"p90":198.844,"p99":206.373,"max":216.711},"gpu_busy_ms_per_present":69.31,"worker_busy_pct":{"queue worker 0x0":98.118},"top_phases_ms_per_frame":{"Driver.Packet":186.916,"Driver.Packet.draw":157.677,"Graphics.Draw":157.572,"Driver.Draw":156.396,"Driver.DrawFinish":140.348,"Driver.Draw.prepare":130.889,"Graphics.Draw.draw_and_resource_release":127.694,"Driver.DrawFinish.shader_compile_and_link":99.737},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":4.258,"events_lost":3459324,"totals_full":false},"draw finisher":{"events_full_after_s":8.484,"events_lost":1623402,"totals_full":false},"draw committer":{"events_full_after_s":14.175,"events_lost":861186,"totals_full":false}},"trace":"aps5-trace-1d307e1e0282-105208.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 69.5, Graphics.Draw 19.2, Graphics.Draw.draw_and_resource_release 13.5, Driver.DrawFinish 12.3, Driver.Packet 8.3, Driver.DrawFinish.shader_compile_and_link 7.3, Driver.Packet.draw 7.0, Driver.Draw 6.9, VideoOut.Flip 6.1, Driver.Present 5.9 Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
AI-assisted: yes |
|
@panicDa3m0n thank you for checking it against #9712; writing the moved words straight into the resizable BAR chunk is exactly where this should end up, and this path only writes the slice. Either order works for me as well: whichever lands second keeps both test names on that |
|
A Windows run of
AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed exact head 29c9624db5ee20768914e4011865299466cf504d and the new performance evidence. I withdraw my earlier request for missing capture data. I downloaded both attached pair-2 traces and independently ran tools/trace_digest.py with their posted summary lines. The base trace reports 280 presents and p50 209.978 ms; the change trace reports 315 presents and p50 190.032 ms. Both digests match the posted capture windows and expose trace_dropped: true; the full-window summary metrics remain available, while event-level details are incomplete as the report discloses. The four alternating pairs and same-machine/title/scene controls are documented. I found no implementation defect in the moved-word upload change.
The remaining repository-rule update is branch freshness: the tested base/head are on 7903201bb426, while current main is b0f1183ca12b. The merge-tree preview is clean, and main has made no AGC runtime or test changes since the tested base. Please sync to current main and let the required checks rerun; the captured comparison remains relevant because those intervening commits do not touch the changed AGC paths.
A draw that hits a template whose read-only guest buffers moved binds the moved words through an override buffer that PrepareDrawBindings created on every such draw, keeping at most eight in movedWordBuffers. In Yakuza 0 (PPSA31327) 80,000 of 390,000 draws per 10 s take that path, so the committer created a Vulkan buffer per draw for a fifth of its draws: about 4 % of the committer in a sampled profile. An override of at most APS5_DRAW_UPLOAD_MAX bytes now takes a slice of the recorder's draw upload chunk, as the small snapshot uploads do: the words and the data patches are copied into the slice and the descriptor points at the chunk with the slice's offset. Larger overrides keep the buffer per draw. APS5_NO_MOVED_WORD_UPLOAD keeps every override in a buffer of its own.
29c9624 to
d4b86f8
Compare
|
@MarcZX10 synced: rebased onto @ycbuzi thank you for the three-GPU run and the control. AI-assisted: yes |
|
#11142 (merged today) already copies moved draw words into the draw upload ring, without the 4 KiB cap, the eight-slot cache or the opt-out switch. This branch now conflicts with main in ShaderResources.cpp and both tests. Could you close it, or reduce it to what main still lacks (for example the agc_moved_word_upload test) and explain why the cap and the switch are still wanted? |
What
A draw that hits a resource-cache template whose read-only guest buffers moved (
MovedReadOnlyBuffers) binds the moved words through an override buffer:PrepareDrawBindingscreated a VulkanBufferfor them on every such draw, kept at most eight inmovedWordBuffers, and otherwise created another. In Yakuza 0 Director's Cut (PPSA31327) 80,000 of 390,000 draws per 10 s take that path, so the committer created a buffer per draw for a fifth of its draws:Buffer::Bufferand the driver's allocation under it were about 4 % of the committer in a sampled profile.APS5_DRAW_UPLOAD_MAXbytes (4 KiB by default) now takes a slice of the recorder's draw upload chunk (Recorder::DrawUpload), the same 1 MiB chunk the small snapshot uploads use: the words and the data patches are copied into the slice, and the descriptor points at the chunk with the slice's offset. Larger overrides keep the buffer per draw and the eight-slot cache as before.APS5_NO_MOVED_WORD_UPLOAD=1keeps every override in a buffer of its own.docs/dev/ENVIRONMENT.mdgets the row.Tested
Windows 11 Pro 26200, WinLibs GCC 15.2.0, Release,
BUILD_TESTING=ON, head d4b86f8 onmainf95acfc.New test
agc_moved_word_upload(tests/Recorder.cpp,--moved-word-upload-only): two draws of one template with different moved words must share one buffer, the chunk, at different offsets, each slice must hold its draw's words, and the descriptor written for the second draw must point at the chunk with that slice's offset.two draws with moved words got a buffer each instead of slices of the recorder's draw upload chunk.--repeat until-fail:20.agc_draw_descriptors(drawDescriptorWriteTests) and the snapshot descriptor check inagc_driver_graphicsexpect the moved binding's descriptor at the slice's offset now instead of offset zero, and read the words there.Pass with the change:
agc_draw_descriptors,agc_draw_snapshot_cap,agc_draw_snapshot_epoch,agc_draw_inputs_in_place,agc_draw_inputs_copied,agc_queued_write_groups,agc_recorder_read_list,agc_descriptor_pool_recycling,agc_driver_draw_cache_reuseand its_draw_pipeline,_entry_verify,_inline,_offand_verifyvariants,agc_driver_graphics,agc_driver_address_passes,agc_driver_address_passes_targets_split.With
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(VK_LOADER_DEBUG=layershowing the layer inserted),ctest -R <those tests> -Vpasses onmainf95acfc (17 of 17) and on this head (18 of 18, with the new test); both logs show the layer inserted, neither containswas not found but was requested by env var VK_INSTANCE_LAYERS, and neither contains aVUID-line (0 onmain, 0 on the head).PPSA31327 01.015.000, gameplay in Kamurocho, scripted boot through the first dialogue, 1920x1080,
APS5_DRAW_PIPELINE=1 APS5_DRAW_INPUTS_IN_PLACE=1,APS5_TRACE_SECONDS=140:60, runs alternated, two builds from the same tree and machine:mainf616a13 + fix(audioout2): keep each port's grain when ports share a buffer #8098 (commit b5e8d63b2; fix(audioout2): keep each port's grain when ports share a buffer #8098 is the title's audio fix, without which it does not run, and it toucheslibSceAudioOutonly).Between f616a13 and f95acfc,
mainchangeslibSceAgcDriveronly inCMakeLists.txt, the newtests/ShaderMemory.cpp, two lines ofExecution/src/Driver/Packets/PacketExecution.cpp,tests/MaskedSelect.cppandtests/Pm4.cpp, none on the moved-word path, so the captures stand for this head.performance_comparelistswindow.first_presentas the only configuration difference between the records.Graphics.Drawper frame moves by -7.6 %, -14.1 %, -3.9 % and -8.0 %; the GPU busy per present follows the frame.Summary lines (pair 2: base, then this change)
{"commit":"b5e8d63b20cc","dirty":false,"env":{"APS5_DRAW_INPUTS_IN_PLACE":"1","APS5_DRAW_PIPELINE":"1","APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"140:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 7800X3D 8-Core Processor","cpu_threads":16,"memory_mib":31892,"title_id":"PPSA31327","title":"Yakuza 0 Director's Cut","content_version":"01.015.000","resolution":"1920x1080","window":{"spec":"seconds 140:60","first_present":2994,"ms":60172.649},"presents":280,"frame_ms":{"p50":209.978,"p90":227.347,"p99":312.175,"max":327.108},"gpu_busy_ms_per_present":76.376,"worker_busy_pct":{"queue worker 0x0":98.155},"top_phases_ms_per_frame":{"Driver.Packet":210.788,"Graphics.Draw":183.394,"Driver.Packet.draw":179.783,"Driver.Draw":178.258,"Driver.DrawFinish":160.736,"Driver.Draw.prepare":150.340,"Graphics.Draw.draw_and_resource_release":149.140,"Driver.DrawFinish.shader_compile_and_link":110.619},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":4.229,"events_lost":3483105,"totals_full":false},"draw finisher":{"events_full_after_s":8.221,"events_lost":1702625,"totals_full":false},"draw committer":{"events_full_after_s":12.164,"events_lost":1055416,"totals_full":false}},"trace":"aps5-trace-b5e8d63b20cc-89144.json.gz"}{"commit":"1d307e1e0282","dirty":false,"env":{"APS5_DRAW_INPUTS_IN_PLACE":"1","APS5_DRAW_PIPELINE":"1","APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"140:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 7800X3D 8-Core Processor","cpu_threads":16,"memory_mib":31892,"title_id":"PPSA31327","title":"Yakuza 0 Director's Cut","content_version":"01.015.000","resolution":"1920x1080","window":{"spec":"seconds 140:60","first_present":3034,"ms":60033.790},"presents":315,"frame_ms":{"p50":190.032,"p90":198.844,"p99":206.373,"max":216.711},"gpu_busy_ms_per_present":69.310,"worker_busy_pct":{"queue worker 0x0":98.118},"top_phases_ms_per_frame":{"Driver.Packet":186.916,"Driver.Packet.draw":157.677,"Graphics.Draw":157.572,"Driver.Draw":156.396,"Driver.DrawFinish":140.348,"Driver.Draw.prepare":130.889,"Graphics.Draw.draw_and_resource_release":127.694,"Driver.DrawFinish.shader_compile_and_link":99.737},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":4.258,"events_lost":3459324,"totals_full":false},"draw finisher":{"events_full_after_s":8.484,"events_lost":1623402,"totals_full":false},"draw committer":{"events_full_after_s":14.175,"events_lost":861186,"totals_full":false}},"trace":"aps5-trace-1d307e1e0282-105208.json.gz"}The traces of pair 2 and the output of
tools/trace_digest.pyon each are in the comment below the review.Earlier, on
main9f386f3 + #8098 with one build and the switch (APS5_NO_MOVED_WORD_UPLOAD=1as the base arm), four alternated pairs gave 198.6 / 203.9, 205.6 / 199.8, 211.3 / 194.4 and 206.7 / 189.8 ms (mean -4.2 %), and a sampled committer profile of each arm (APS5_SAMPLE_COMMITTER, gameplay only, one run per arm) showed the mechanism:APS5_NO_MOVED_WORD_UPLOAD=1PrepareDrawBindings(inclusive)Buffer::BufferAllocate+Recorder::KeepBytes+ shared-pointer releaserecordPreparedDraw(inclusive)prepareDraw(inclusive, untouched by the change)Frame dumps at the same points of the scene (intro, title, two street frames) match between this change and the base build.
tools/check_conventions.py --base origin/mainpasses.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)