Repository navigation
perf(agc): list a dispatch or draw as a copied writer only while it has a CPU write-back - #2792
Conversation
…as a CPU write-back A recorded dispatch or draw whose use needs completion work goes into the copied-writer lists (VulkanDevice's copiedWriters, DrawCopiedWriters). Fills, buffer copies and indirect argument reads treat every write of a listed object as a store the CPU makes when its batch is reaped, and wait for that batch. Completion work is also needed for the BDA fault check alone, and the written sub-ranges of GPU copies are copied back by the GPU (RecordCopyBacks) before the listing. Such an object has no CPU write-back (HasCopiedWrites is false), and its stores are ordered by the barriers like direct writes, yet it was listed and waited for. Both lists now take an object only when HasCopiedWrites holds after MarkGpuWrites. The completion action still runs for the fault check. agc_driver_copied_writer_copy records a dispatch that reads through global (BDA) loads and stores through a bound buffer into registered memory, then copies that output on the GPU (VulkanDevice::CopyBuffer) before the batch ran. It checks that the copy is a transfer between the host imports without a sync, and the copied bytes. Without the change the copy waits for the dispatch's batch. PPSA14632 (LEGO Horizon Adventures): every one of the DMA_DATA copy waits counted over 75 s came from such writers. Queue 0's GPU-mutex hold for copies drops from 3.5 s to 30 ms per 10 s, and presents rise from about 102 to 109 per 10 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head
No title was run. 🤖 Addressed by Claude Code |
Adapt upstream PRs boykopovar#1315, boykopovar#2791 and boykopovar#2792 to the current descriptor ABI and synchronization paths. Bound immutable binding plans to 1024 entries while retaining live resource addresses and specialization identity. Keep real CPU writebacks and fault completion active. Capture generation baselines only over settled guest bytes. Add focused cache, texture baseline and copy synchronization regressions.
Dextroid17
left a comment
Dextroid17
left a comment
There was a problem hiding this comment.
Read against the consumers rather than the summary: the change is right, and there are three things I would want settled — none of them blocking.
The completion path is untouched, which is the claim that mattered. The new guard sits inside if (resources.NeedsCompletion()) — the block that registers the write-back — so the OnComplete action (erase from the list, then WriteBackBuffers) still runs for the same objects; only the listing changed. The BDA fault check therefore sees what it saw before, as the description says.
The two halves are consistent, and the ordering fix on the draw side is real. On the dispatch side MarkGpuWrites(recorder) already ran before the listing, so asking HasCopiedWrites() there is well-defined. On the draw side listed was decided before MarkGpuWrites, which is why that side needed the extra condition rather than a reordering to match.
The one consumer that is not a wait. Some of the places that read these lists (VulkanDevice.cpp:1618 and :1822) only decide whether to sync — waiting is a cost, and not paying it is the point of the change. The indirect-argument decision (std::any_of(... WritesOverlap(arguments, 12) ...)) is different in kind: it chooses whether the CPU reads the arguments or the GPU reads them in place. Dropping an entry there changes that choice, and what makes it safe — the GPU copies those regions back, so the barriers order them like direct writes — is the part I cannot see tested. The new case asserts the CopyBuffer path (a transfer between host imports with no sync) and that the copied bytes match, which is the sync consequence, not the argument-read one. It would help to say explicitly which of the two the change intends to affect, and whether a case for the other is easy to add.
A cross-PR note, since both are open at the same time: #2990 moves this same decision from the other side — it stops the look-ahead before an indirect dispatch whose arguments an earlier dispatch may write, precisely because once the CPU has read them it is too late. The two probably compose, but they are reasoning about the same CPU-read choice, and merging them with that in mind seems worth a sentence.
The cost this removes is already instrumented. The [indirect] report carries copied-writer lists scanned per decision: dispatch avg … (max …), draw avg … — writersScanned and maxWriters for the dispatch pair, drawWritersScanned for the draw average. A before/after on that figure would put a number on the win, which otherwise is argued rather than shown — and it is the honest way to see whether the conservatism is narrower than the change suggests.
Written with AI assistance.
|
Checked on the PR merged onto current On the indirect-arguments point raised in review: the GPU copy-back in |
4 participants
What
A recorded dispatch or draw whose use needs completion work is listed as a copied writer (
VulkanDevicecopiedWriters,DrawCopiedWriters). Fills,CopyBufferand indirect argument reads treat every write of a listed object as a store the CPU makes when its batch is reaped, and wait for that batch (SyncThrough).Completion work is also needed for the BDA fault check alone (
NeedsCompletionisbda != nullptr || HasCopiedWrites()). The written sub-ranges of GPU copies are copied back by the GPU inRecordCopyBacks, whichMarkGpuWritesruns before the listing. Such an object has no CPU write-back (HasCopiedWrites()is false), and its stores are ordered by the barriers like direct writes, yet it was listed and its writes waited for. For draws,listedwas decided beforeMarkGpuWrites, so a draw whose copied regions the GPU copies back was listed too.Both lists now take an object only when
HasCopiedWrites()holds afterMarkGpuWrites. The completion action still runs, so the fault check is unchanged.New execution test
agc_driver_copied_writer_copy: it records a dispatch that reads through global (BDA) loads and stores through a bound buffer into registered memory, then callsVulkanDevice::CopyBufferon that output before the batch runs. It checks that the copy is a transfer between the host imports (path 1) without a sync, and that the copied bytes match what the dispatch stored. Without this change it fails ("... waited for its batch, although no CPU write-back follows it"). It skips (77) without a Vulkan device or host imports.Tested
Linux (Arch), RX 9070 XT, RADV, Release:
maindb343a9 plus perf(agc): sync only the source for DMA_DATA copies into host-only memory #1157, feat(agc): depth planes sampled as 2D arrays, cubes and integer views #425, perf(agc): read a fast-cleared surface's guest bytes only when the import is compared #2699, feat(agc): vertex clip and cull distances #423/feat(agc): render layered draws into 3D color targets #1077, perf(agc): take a storage image's generation baseline only when its guest bytes are settled #2791 and local stand-ins needed to reach gameplay). With a temporary counter, all of about 1,400 DMA_DATA copy waits over 75 s came from listed writers without a CPU write-back. Per 10 s, steady state:Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code