Repository navigation
perf(agc): prepare pipelined draws on a thread pool - #6949
alfalfa-star wants to merge 1 commit into
Conversation
With APS5_DRAW_PREP_THREADS=N (N > 0), queue-0 draws are prepared on a pool of N threads instead of on the packet worker: each draw's shader captures, resource materialization and draw-cache lookups run on a pool thread, the draws are enqueued in submission order, and each prepared draw is validated against the writes of the draws before it, redoing the preparation when an earlier draw wrote memory it read. Commits stay in order on the draw committer. Data-only draw-cache hits keep their cache entry alive until commit and use the patched stage data like the main path. The pool is pinned to the efficient cores (APS5_DRAW_PREP_AFFINITY_MASK overrides) and the committer to the queue workers' cores. The pool is off by default (APS5_DRAW_PREP_THREADS unset or 0). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.
|
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
Off by default, but this is a concurrency design with correctness risks and no test or measurement yet.
const Submission* pooledSubmission = &submission;is used inside the pool job (materializeDrawStage(..., *pooledSubmission, ...)) afterdraw()has returneddrawn(). Nothing in the diff shows what keeps thatSubmissionalive until the job has run. If it is a stack or per-submit object, the job reads a dead object once the worker moves on. Say what guarantees the lifetime, or copy what the job needs intoPooled.- The pool threads are
detach()ed from a leakednew DrawPrepPool()with no stop path. AtAgcDriverShutdowna job can still be running againstthis(the Driver) and the device it captured. Shutdown has toWaitAll()before teardown, and the pool has to be stoppable. - The
poolMiss/poolSeenblock runs on every draw when the pool is on: eight atomics, astd::all_ofoverdataCandidates, the verify and trace probes, and a stderr line every 100k draws. It is diagnostic code in the hottest function. Please remove it or put it behind a profile switch. Enqueue,Drain,PendingLabelandOverlapsnow callWaitAll(), and so does every packet that is not incommutesWithPreparedDraws. Each of those collapses the pool back to the single-threaded path for that stretch. The[prep-pool]line already countswaitAlls; please post it, since it decides whether four threads buy anything in a title that interleaves labels or DMA with draws.- A conflict (
WrittenBetween) redoes the whole preparation. That doubles the CPU cost on exactly the draws that depend on each other. Post theredosratio from the same run. - The pool is pinned to the efficient cores by default on hybrid CPUs, and the description calls this preparation "the CPU bottleneck". Running the bottleneck on E-cores by default needs an A/B against
APS5_DRAW_PREP_AFFINITY_MASKset to the P-cores. Also, "about 20 in flight" cannot come fromAPS5_DRAW_PREP_THREADS=4:maxInFlightiscount * 3= 12. SrtEvaluator.hppdropsexplicitfromIrNode(IrValue*). That makes everyIrValue*convert implicitly toIrNodeacross the recompiler, in a header that has nothing to do with this topic. Revert it, or say why the pool needs it.- No test. Ordering of commits, the redo on a write conflict and
WaitAllat drain are all untested, and each is a place where a race gives wrong pictures instead of a crash. A test that runs two dependent draws (the second reads what the first writes) through the pool, once with the conflict and once without, would cover the redo. - Performance: "about 24 fps for the whole stack" and "a measurement of this change alone will follow" is not yet a report. Please add the summary lines of main and this PR with
APS5_DRAW_PREP_THREADSunset and set, same scene in PPSA01802, noAPS5_PROFILE_*.
|
Data point from a CPU-bound scene on Windows: RTX 3060 12 GB (NVIDIA 617.42), i5-12400F, Windows 10 19045, MinGW GCC Release. Minecraft Dungeons II (PPSA16064 01.100.100), title screen, no input, 200 s rows,
Within the run-to-run spread (about ±1.5 % here), no regression, 0 fatal. AI-assisted: yes (Claude Code) |
|
@alfalfa-star a data point from Hades II (PPSA36082), the level's item pickup UI, where queue worker 0's draw preparation is the limit. Linux, RTX 4090, 7800X3D, NVIDIA 615.78.08. All three builds are
The pool's own line during the window: So in this scene the jobs run one at a time: #6946 changes the same part of LLM assistance used. |
|
GTA V (PPSA04264) data for this PR, as suggested by @teepean on #1203. Setup. i9-13900K, RTX 4090 (NVIDIA 616.92), Windows 11, WinLibs GCC 15.2. Built from main
So nothing measurable on this title. The counters say why. Default gate: most draws stay on the packet worker. With My reading: on GTA, the ordered stage after preparation (the committer and the in-order turn) is what limits the frame, not preparation itself. Prepared draws queue up behind it. The 13900K's E-cores are also noticeably slower for this work than the P-cores, so the default affinity may hurt on hybrid Intel CPUs. Happy to rerun with other settings or a newer head, or to capture a worker profile of the committer if that helps. AI-assisted (Claude Code). |
|
Closing this. On Kena it doesn't pay for its risk, and your review found real problems. Details for anyone who picks up the idea (@tibzejoker, @teepean): Measured on Kena (PPSA01802), forest title scene, no input.
The pool-off rows spread more than the difference. An earlier batch had pool off 210 / 239 against pool on 217 / 223. The Bug found while writing the test you asked for. Pool threads never set Not fixed, and the reason to close rather than finish. The packet worker still decodes vertex tables and validates draw-cache hits against guest memory before a draw reaches the pool. Those reads don't see writes from earlier draws still preparing on the pool. A redo re-runs only the captures, with the worker's snapshots. Making that correct means running the whole draw on the pool from a copy of the queue state and redoing all of it on a conflict, as in @teepean's place-reserving design. Their Stray numbers suggest the committer becomes the limit once the worker isn't. AI-assisted: yes (Claude Code) |
|
Thanks @Raghav314 and @tibzejoker. Your Hades II and GTA V numbers came in while I was measuring Kena, and they say the same thing: the pool holds 0.0–0.3 draws on average because the front drains it, and pooling the data-only hits moves the wait into the in-order commit. That, plus the ordering hole above, is why this is closed in favour of teepean's reserved-places design (perf/reserved-prep, linked on #1203). AI-assisted: yes (Claude Code) |
|
A follow-up for anyone who lands here from the note above: the reserved-places design ( AI-assisted: yes (text with Claude Code). |
6 participants
What
With pipelined draws, every queue-0 draw is still prepared on the packet worker: shader captures, resource materialization and draw-cache lookups run one draw at a time before the draw is queued for the committer. In Kena: Bridge of Spirits (PPSA01802) the forest scene issues around 30,000 draws per second, and this preparation is the CPU bottleneck that keeps the title far below 30 fps.
APS5_DRAW_PREP_THREADS=N(N > 0) prepares queue-0 draws on a pool of N threads:APS5_DRAW_PREP_AFFINITY_MASKoverrides), and the committer to the queue workers' cores.The pool is off by default; nothing changes unless
APS5_DRAW_PREP_THREADSis set.Tested
AMD Radeon RX 9070 XT (RADV GFX1201), Mesa 26.2.4, Arch Linux (kernel 7.2.9), Kena: Bridge of Spirits (PPSA01802).
agc_driver_tessellationsegfaults on main with and without this change).APS5_DRAW_PREP_THREADS=4: the pool prepares about 300,000 draws per 10 s with ~20 in flight. Together with the other work in our local test stack it brings the forest scene to about 24 fps. A measurement of this change alone on top of main will follow in a comment.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)🤖 Generated with Claude Code