Repository navigation
perf(agc): prune and gate the label take records - #13252
Merged
oneandonlydean merged 1 commit intoOct 30, 2026
Merged
Conversation
A queue's passes on label dwords and the takes its own stores make of them move to LabelTakes. A wait only considers stores recorded after its submission entered the driver, so a pass or take at a serial no newer than that entry can no longer hide a store: each queue's passes are kept in serial order and dropped when its worker starts a submission received after them (for a rewind tail, the oldest submission of the queue still pending bounds it). noteLabelIssue reads a per-(queue, dword) count of live passes without the lock and takes labelStoresMutex only when a pass may exist. Passes and takes of a queue, and its stores that take them, all run on that queue's worker.
12 tasks done
|
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.
|
Contributor
|
Windows + NVIDIA result for head Host: Windows 11 26300, Intel i7-12700K, NVIDIA GeForce RTX 2070 SUPER, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, Same machine, both trees, with
No new validation errors. AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
Contributor
Author
|
@maskym thanks for the Windows/NVIDIA run with the validation layer on both trees. |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 390 tests)Skipped (40)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. |
12 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #12923, from CnRJay's cost review there. The per-queue label pass and take records #12923 added (
labelPasses,labelTakeninDriver) are never erased, andnoteLabelIssuetakeslabelStoresMutexfor every store packet the label history keeps. In The Elder Scrolls V: Skyrim (PPSA03747) both maps grow for the whole session once gameplay starts, and every kept store packet takes the lock:main:labelPasses/labelTakenlabelStoresMutex:main/ this PROn
mainboth maps grow by about 400 entries a second once gameplay starts (~140 s) and never shrink. With this change the table never held more than 416 entries and 423 records over the run, and 8 % of the kept store packets took the lock (11 % during gameplay). A first version with 1024 counters locked for 22 % (31 % during gameplay), hence 4096. The label history itself (labelStores, one 4-store array per stored dword, from #178) grows the same way, 134,520 dwords at 400 s on both builds; that's outside this change.How
LabelTakes(Driver/Synchronization/LabelTakes.hpp), keyed by queue and dword as before, which also keeps each queue's passes in serial order.storedSincealready skips every store whose stamp is no newer than the waiting submission'sreceivedserial, and a take only hides stores no newer than the take. So once every submission the queue can still run entered the driver after a pass or a take, that entry can't change any result. The queue's worker retires them right before it executes a submission (retireLabelTakes(id, submission.received)): a queue's submissions run in order and theirreceivedserials grow in that order. A rewind tail gets a newer serial than the submissions of its queue still pending, so it retires up to the oldest of those (or its own serial when none is pending). Suspend boundaries have serial 0 and retire nothing.noteLabelIssuereads a count of live passes per (queue, dword) hash slot (4096 relaxed atomic counters, 16 KiB) without the lock and takeslabelStoresMutexonly when one may exist. The counters only change under the lock. A queue's passes, the stores of that queue that take them, and its retires all run on that queue's worker thread, so the read always sees the queue's own passes; another queue's pass in the same slot only costs a lock that finds nothing.storedSince,noteLabelPassand which stores a take hides are unchanged.Tested
Linux (Arch, RADV, RX 9070 XT), Release.
main409fe64 (aee6ce2) and rerun there with the layer: the focused set passes 15 of 15 and the wider set of 113 gives the same per-test results as below, noVUID-lines.agc_driver(tests/Submit.cpp):LabelTakeson its own (a take survives a retire at an older serial and another queue's retire, goes at its own serial; an older pass record doesn't retire a newer pass on the same dword; one queue's retire leaves the other queue's pass), and a driver case: two queue 0x20 submissions entered before the producer's store, the first takes and resets the label, the second must stay blocked with the take still recorded, and a third submission entered after both waits drops the take once it runs. As mutation checks on this head: retiring up to a serial far in the future fails an existing take-and-reset case (a label store its own queue had taken and reset satisfied the queue's next wait, or the wait expired), dropping the worker's retire fails the new driver case (a take was kept after a submission of the queue entered after it started), and a gate that never opens fails the same existing case as the first.agc_driver_same_queue_label_wait, with Khronos validation 1.4.363 (VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1, layer insertion checked withVK_LOADER_DEBUG=layer): 15 of 15 onmain102c4f9 and on this branch (agc_driver_copied_writer_label_orderskips on both), noVUID-lines.^agc_.*(label|wait|queue|submit|pipeline|eop|gated|idle), 113 tests), with the layer: identical per test onmain102c4f9 and this branch, noVUID-lines;agc_idle_publish_waitfails on both withthe idle publish's bytes are wrongon this GPU, andagc_small_tiled_shadow_labeland bothagc_driver_copied_writer_label_ordertests skip on both.tools/trace_digest.pyover the same 60 s window (APS5_TRACE_SECONDS=60:60: main menu, New Game, first loading), two 200 s runs each:Driver.Packetms per framemain102c4f9 + #11506main102c4f9 + #11506The window is vsync-bound and the two
mainruns differ by more than either differs from this change, so this shows no regression, not a speedup. No run had a label wait timeout.trace_digest.pyheadersmain, run 1:
seconds 60:60 from present 3315: 3329 presents, frame p50 16.7 / p90 16.7 / p99 16.8 / max 1357.5 ms, GPU busy 8.5 ms per present
AMD Radeon RX 9070 XT (RADV GFX1201), radv Mesa 26.2.6-arch1.1, Linux 7.2.11-arch1-1, 3840x2160, commit 5ade8c491953, dropped: no
{"commit":"5ade8c491953","dirty":null,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"60:60","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"AMD Radeon RX 9070 XT (RADV GFX1201)","driver":"radv Mesa 26.2.6-arch1.1","os":"Linux 7.2.11-arch1-1","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":8,"memory_mib":31192,"title_id":"PPSA03747","title":"Skyrim","content_version":"01.001.010","resolution":"3840x2160","window":{"spec":"seconds 60:60","first_present":3315,"ms":60432.977},"presents":3329,"frame_ms":{"p50":16.683,"p90":16.687,"p99":16.785,"max":1357.503},"gpu_busy_ms_per_present":8.52,"worker_busy_pct":{"queue worker 0x0":6.188},"top_phases_ms_per_frame":{"VideoOut.Flip":15.743,"VideoOut.Flip.vblank_wait":14.974,"Driver.Packet":1.121,"Graphics.Draw":0.538,"Graphics.Draw.draw_and_resource_release":0.422,"Driver.Packet.draw":0.421,"Driver.Draw":0.417,"VideoOut.Flip.present":0.404},"trace_dropped":false,"trace":"aps5-trace-5ade8c491953-2119064.json.gz"}main, run 2:
seconds 60:60 from present 3317: 3309 presents, frame p50 16.7 / p90 16.7 / p99 17.7 / max 1431.1 ms, GPU busy 8.2 ms per present
AMD Radeon RX 9070 XT (RADV GFX1201), radv Mesa 26.2.6-arch1.1, Linux 7.2.11-arch1-1, 3840x2160, commit 5ade8c491953, dropped: no
{"commit":"5ade8c491953","dirty":null,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"60:60","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"AMD Radeon RX 9070 XT (RADV GFX1201)","driver":"radv Mesa 26.2.6-arch1.1","os":"Linux 7.2.11-arch1-1","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":8,"memory_mib":31192,"title_id":"PPSA03747","title":"Skyrim","content_version":"01.001.010","resolution":"3840x2160","window":{"spec":"seconds 60:60","first_present":3317,"ms":60014.239},"presents":3309,"frame_ms":{"p50":16.683,"p90":16.688,"p99":17.663,"max":1431.087},"gpu_busy_ms_per_present":8.215,"worker_busy_pct":{"queue worker 0x0":8.594},"top_phases_ms_per_frame":{"VideoOut.Flip":15.271,"VideoOut.Flip.vblank_wait":14.495,"Driver.Packet":1.555,"Graphics.Draw":0.602,"Driver.Packet.draw":0.554,"Driver.Draw":0.549,"Graphics.Draw.draw_and_resource_release":0.474,"VideoOut.Flip.present":0.375},"trace_dropped":false,"trace":"aps5-trace-5ade8c491953-2216330.json.gz"}this PR, run 1:
seconds 60:60 from present 3312: 3305 presents, frame p50 16.7 / p90 16.7 / p99 16.7 / max 1466.6 ms, GPU busy 8.0 ms per present
AMD Radeon RX 9070 XT (RADV GFX1201), radv Mesa 26.2.6-arch1.1, Linux 7.2.11-arch1-1, 3840x2160, commit ccd54f4ea010, dropped: no
{"commit":"ccd54f4ea010","dirty":null,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"60:60","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"AMD Radeon RX 9070 XT (RADV GFX1201)","driver":"radv Mesa 26.2.6-arch1.1","os":"Linux 7.2.11-arch1-1","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":8,"memory_mib":31192,"title_id":"PPSA03747","title":"Skyrim","content_version":"01.001.010","resolution":"3840x2160","window":{"spec":"seconds 60:60","first_present":3312,"ms":60377.42},"presents":3305,"frame_ms":{"p50":16.683,"p90":16.687,"p99":16.7,"max":1466.634},"gpu_busy_ms_per_present":8.037,"worker_busy_pct":{"queue worker 0x0":5.964},"top_phases_ms_per_frame":{"VideoOut.Flip":15.775,"VideoOut.Flip.vblank_wait":15.082,"Driver.Packet":1.087,"Graphics.Draw":0.543,"Driver.Packet.draw":0.43,"Graphics.Draw.draw_and_resource_release":0.428,"Driver.Draw":0.425,"VideoOut.Flip.present":0.349},"trace_dropped":false,"trace":"aps5-trace-ccd54f4ea010-2435599.json.gz"}this PR, run 2:
seconds 60:60 from present 3326: 3338 presents, frame p50 16.7 / p90 16.7 / p99 16.7 / max 2382.9 ms, GPU busy 8.2 ms per present
AMD Radeon RX 9070 XT (RADV GFX1201), radv Mesa 26.2.6-arch1.1, Linux 7.2.11-arch1-1, 3840x2160, commit ccd54f4ea010, dropped: no
{"commit":"ccd54f4ea010","dirty":null,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"60:60","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"AMD Radeon RX 9070 XT (RADV GFX1201)","driver":"radv Mesa 26.2.6-arch1.1","os":"Linux 7.2.11-arch1-1","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":8,"memory_mib":31192,"title_id":"PPSA03747","title":"Skyrim","content_version":"01.001.010","resolution":"3840x2160","window":{"spec":"seconds 60:60","first_present":3326,"ms":61750.513},"presents":3338,"frame_ms":{"p50":16.683,"p90":16.687,"p99":16.698,"max":2382.857},"gpu_busy_ms_per_present":8.206,"worker_busy_pct":{"queue worker 0x0":5.922},"top_phases_ms_per_frame":{"VideoOut.Flip":15.682,"VideoOut.Flip.vblank_wait":15.31,"Driver.Packet":1.093,"Graphics.Draw":0.519,"Graphics.Draw.draw_and_resource_release":0.42,"Driver.Packet.draw":0.416,"Driver.Draw":0.412,"VideoOut.Flip.present":0.26},"trace_dropped":false,"trace":"aps5-trace-ccd54f4ea010-2490675.json.gz"}mainruns above are repeats: the first tries aborted at 25 s and 11 s onNGS2: this buffered voice parameter is not implemented(fix(ngs2): buffer voice setups, waveform blocks and patches sent during processing #12995, unrelated). The second run with this change aborted on the same error at 191 s, after its window had closed.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)