Repository navigation
perf(agc): read a sampled texture's DCC keys once per lookup - #8117
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.
|
SWAG (PPSA22102) mechanism check - measured dataClean pair on the heavy spawn scene, same save and pad schedule, 360 s per run, stock ini,
No frame-rate claim. Scene trajectories diverged: run29 recorded 248,781 draws (+20.2%) and 680 presents (+67%) vs run28's 207,013 and 407; Measured takeaway: the memoization is active on SWAG - the direct DCC counters fall about 20%, and the cumulative AI-assisted interpretation (separate from the measured table): the |
|
Tested on RTX 3080 (NVIDIA 617.42) and AMD Radeon RDNA 2 iGPU ( The first case ("a dispatch's first lookup") never failed. AI-assisted: yes (Claude Code). |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the memoization itself reads correctly (the reset after the storage source, after Refresh and after FlushPending covers the paths that can change the keys). One blocker:
core/libs/prx/libSceAgcDriver/tests/execution/DccKeyPresync.cpp:247:RunOneKeyReadPerLookupcomparesKeyWaitCounts().callsbefore and after the dispatch, but that counter is process-wide (DccMetadata.cpp,KeyWaitTotals), so key reads from any other thread (or work still settling from earlier cases) land in the delta. @TheSovietWaffle measured it failing 4/10 runs on an RTX 3080 and 2/10 on a Radeon iGPU, always on the second case ("repeated lookup read the keys ... 4 times"). A flaky ctest entry is not acceptable. Please make the count specific to the lookup under test (for example a per-thread counter, or counting only reads made by the calling thread) or quiesce the other readers before taking the baseline, and show it passes repeatedly (ctest --repeat until-fail:20).
Once that is fixed this looks good to merge.
|
@adriadam10 @TheSovietWaffle Reproduced here: 7 of 30 runs failed, always the repeated lookup with 4 reads (RTX 4070, 617.42). The extra reads were not another thread's: a temporary per-thread counter saw the same 4 reads on the test thread. Only one of them comes from f39d75d counts what the test is about.
With AI-assisted: yes |
|
| Head | Failures | Signature |
|---|---|---|
8cd75d779 (before the own-scan counter) |
3 / 30 (runs 14, 24, 30) | a dispatch's repeated lookup read the keys of its sampled texture 4 times under the GPU mutex with nothing written between the reads, expected at most once — always the 4-read case |
f39d75d62f (current head) |
0 / 30 | — |
Same signature as the RTX 3080 / RTX 4070 reports (always the second case, always exactly 4 reads), and the own-scan counter fixes it here too. So on this driver generation the storage-refresh interleave happens on NVIDIA Ampere as well; 10 % observed rate is in line with the other reports.
AI-assisted: yes (Devin).
cachedTexture read the keys of one range up to three times in a lookup with nothing written in between: the ClearSingle check over a pending image dropped its scan, the staged-clear test scanned even without a pending image and kept nothing, and the final read scanned again. On Windows the key ranges sit in compared host imports, so no proof stands in for these scans; in PPSA31327's gameplay they took 12 % of the draw committer's samples (boykopovar#8068). The lookup now keeps the keys it read until it does something that can change them: the storage source it looks up and may refresh, the pending source's Refresh, or a pending flush, which can publish unit shadows over the surface even when it flushes no image. The staged-clear test reads the keys only with a pending source. Which keys a lookup acts on is unchanged; only repeated reads with nothing between them are gone. DccKeyPresync checks that a dispatch's lookup with no key writer reads the keys at most once under the GPU mutex; main reads them twice.
RunOneKeyReadPerLookup compared the process-wide KeyWaitCounts().calls around a dispatch. The repeated lookup sometimes refreshes the surface's storage image, and its upload and ProvedKeys read the keys three more times on the same thread, so the test failed in about one run of four. cachedTexture now counts the key scans it makes on the calling thread (SampledLookupKeyScans()), and the test compares that count.
f39d75d to
2e7494b
Compare
|
Rebased onto On the rebased head (RTX 4070, 617.42): the seven focused tests pass, @akkadbakkad89 thanks for the RTX 3070 runs. AI-assisted: yes |
|
The flake is addressed: |
What
cachedTextureread the DCC keys of one range up to three times in a lookup with nothing written between the reads:ClearSinglecheck over a pending image scanned and dropped the result when no keys were held yet;observedKeys) scanned on every lookup, though only a lookup with a pending source uses it, and kept nothing;On Windows the key ranges sit in compared host imports, so
CollectWritesis 0 and no proof stands in for these scans (#6209 works on that forProvedClearKeys, which this path does not use). In PPSA31327's gameplay the scans took 12 % of the draw committer's samples, and the committer bounds the frame (#8068).The lookup now keeps the keys it read until it does something that can change them: the storage source it looks up (which may refresh), the pending source's
Refresh, orFlushPending, which can publish unit shadows over the surface even when it flushes no image. The staged-clear test reads the keys only with a pending source. Which keys a lookup acts on is unchanged; only the repeated reads go.ClearSinglekeys, storage-eligibleReads around a
Refreshor a flush are unchanged.Tested
Windows 11 Pro 26200, WinLibs GCC 15.2.0 (ucrt, posix, seh, r7), Release, NVIDIA GeForce RTX 4070 (driver 617.42), Ryzen 7 7800X3D.
Tests on this head, rebased onto
main0c6a5bd. The rebase metmain'srefreshForViewincachedTexture, which replacedsource->Refresh(); it is kept, and the key memo is reset after it, as afterRefreshbefore.With
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363.0; the loader log shows the layer inserted),ctest -R <the seven below> -Vpasses onmain0c6a5bd and with this change, and neither log has aVUID-line or a missing-layer message.agc_driver_dcc_key_presyncpasses--repeat until-fail:20.DccKeyPresyncgainsRunOneKeyReadPerLookup: a dispatch's first and repeated lookup of a texture with no key writer read its keys at most once under the GPU mutex (KeyWaitCounts().calls). With this change 1 and 1; onmainthe test fails, reading them twice.Pass with the change:
agc_driver_dcc_keys_first_write,agc_driver_staged_clear_sampled_read,agc_driver_dcc_key_presync,agc_driver_graphics,agc_pending_key_stores,agc_shadowed_key_stores,agc_copied_direct_ordering. The fullagc_driver_recorder_testsrun (not a ctest entry) stops at(9) a scan under a pending write was keptwith and without this change.tools/check_conventions.py --base origin/mainpasses.PPSA31327 (Yakuza 0 Director's Cut 01.015.000), gameplay after loading the title's autosave, character still, 1920x1080, warm caches.
1c230943a=mainbf2c63f with fix(agc): accept PA_CL_CLIP_CNTL CLIP_DISABLE as depth clamp #6572 and fix(audioout2): keep each port's grain when ports share a buffer #7719, which the title needs to reach gameplay and play sound; the change is this pull request's commit cherry-picked onto that base (5e81d5a85).APS5_PAD_INPUT_FILE(left and cross at the load prompt) withAPS5_NO_PAD_INPUT=1, recordedAPS5_TRACE_SECONDS=150:60with no other trace or profile switch and the log not redirected, and closed the title once the trace was written.Graphics.Drawon the draw committer drops from 273-307 to 244-268 ms per frame; the GPU stays busy 94-110 ms per present in both.Pairs 1-4 used the same code built from the uncommitted change (their summary lines say
dirty); pairs 5 and 6 used the commit. WithAPS5_PROFILE_DRAWin separate runs (16 gameplay intervals of 10 s each), the[dcc]line counts per present 7,319 scans, 1,302 MiB and 37.1 ms of scanning on the base, and 2,635 scans, 442 MiB and 12.9 ms with the change.Summary lines of pairs 5 and 6 (base, change, base, change):
{"commit":"1c230943afb4","dirty":false,"env":{"APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"150:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 617.42","os":"Windows 10.0.26200","resolution":"1920x1080","window":{"spec":"seconds 150:60","first_present":2299,"ms":60092.008},"presents":171,"frame_ms":{"p50":350.748,"p90":358.247,"p99":371.746,"max":455.105},"gpu_busy_ms_per_present":108.426,"worker_busy_pct":{"queue worker 0x0":98.776},"top_phases_ms_per_frame":{"Driver.Packet":346.693,"Driver.Packet.draw":306.647,"Driver.Draw":303.757,"Graphics.Draw":294.760,"Graphics.Draw.draw_and_resource_release":209.813,"Driver.Draw.shader_compile_and_link":161.547,"Shader.DrawStage":127.357,"Driver.Draw.draw_enqueued":112.812},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":3.500,"events_lost":4189934,"totals_full":false},"draw committer":{"events_full_after_s":9.477,"events_lost":1388692,"totals_full":false}},"trace":"aps5-trace-1c230943afb4-4812.json.gz"}{"commit":"5e81d5a85e8b","dirty":false,"env":{"APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"150:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 617.42","os":"Windows 10.0.26200","resolution":"1920x1080","window":{"spec":"seconds 150:60","first_present":2377,"ms":60156.246},"presents":196,"frame_ms":{"p50":305.790,"p90":314.853,"p99":384.782,"max":395.209},"gpu_busy_ms_per_present":98.482,"worker_busy_pct":{"queue worker 0x0":98.502},"top_phases_ms_per_frame":{"Driver.Packet":302.062,"Driver.Packet.draw":265.411,"Driver.Draw":262.615,"Graphics.Draw":252.849,"Graphics.Draw.draw_and_resource_release":174.167,"Driver.Draw.shader_compile_and_link":151.446,"Shader.DrawStage":119.031,"Driver.Draw.draw_enqueued":83.793},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":3.349,"events_lost":4436450,"totals_full":false},"draw committer":{"events_full_after_s":8.891,"events_lost":1509189,"totals_full":false}},"trace":"aps5-trace-5e81d5a85e8b-37776.json.gz"}{"commit":"1c230943afb4","dirty":false,"env":{"APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"150:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 617.42","os":"Windows 10.0.26200","resolution":"1920x1080","window":{"spec":"seconds 150:60","first_present":3175,"ms":60319.718},"presents":174,"frame_ms":{"p50":345.546,"p90":352.959,"p99":362.007,"max":438.244},"gpu_busy_ms_per_present":107.930,"worker_busy_pct":{"queue worker 0x0":98.787},"top_phases_ms_per_frame":{"Driver.Packet":342.046,"Driver.Packet.draw":303.453,"Driver.Draw":300.518,"Graphics.Draw":292.150,"Graphics.Draw.draw_and_resource_release":209.114,"Driver.Draw.shader_compile_and_link":160.670,"Shader.DrawStage":126.834,"Driver.Draw.draw_enqueued":110.735},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":3.566,"events_lost":4207546,"totals_full":false},"draw committer":{"events_full_after_s":9.514,"events_lost":1398521,"totals_full":false}},"trace":"aps5-trace-1c230943afb4-37816.json.gz"}{"commit":"5e81d5a85e8b","dirty":false,"env":{"APS5_NO_PAD_INPUT":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"150:60"},"gpu":"NVIDIA GeForce RTX 4070","driver":"NVIDIA 617.42","os":"Windows 10.0.26200","resolution":"1920x1080","window":{"spec":"seconds 150:60","first_present":2290,"ms":60191.514},"presents":188,"frame_ms":{"p50":319.585,"p90":325.781,"p99":344.166,"max":425.845},"gpu_busy_ms_per_present":102.473,"worker_busy_pct":{"queue worker 0x0":98.635},"top_phases_ms_per_frame":{"Driver.Packet":315.513,"Driver.Packet.draw":277.388,"Driver.Draw":274.481,"Graphics.Draw":264.238,"Graphics.Draw.draw_and_resource_release":181.399,"Driver.Draw.shader_compile_and_link":158.721,"Shader.DrawStage":124.397,"Driver.Draw.draw_enqueued":87.264},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":3.373,"events_lost":4403692,"totals_full":false},"draw committer":{"events_full_after_s":8.862,"events_lost":1507767,"totals_full":false}},"trace":"aps5-trace-5e81d5a85e8b-37824.json.gz"}The twelve traces are attached.
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)