Skip to content

perf(agc): the draw committer polls briefly for the next draw before it sleeps - #5867

Closed
Raghav314 wants to merge 1 commit into
boykopovar:mainfrom
Raghav314:perf/agc-committer-spin
Closed

Raghav314 wants to merge 1 commit into
boykopovar:mainfrom
Raghav314:perf/agc-committer-spin

Conversation

@Raghav314

@Raghav314 Raghav314 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What

With pipelined draws (on by default, depth 8), the committer thread sleeps on the condition variable whenever the queue is empty. In a draw-heavy frame the worker hands over a draw every few microseconds, so the committer goes to sleep after almost every commit and the worker pays a wake for the next one.

  • APS5_PIPELINE_SPIN_US=<us> makes the committer poll the queue count for up to that long before it sleeps (_mm_pause between polls, the clock read every 64 polls, as the queue-0 memory wait does).
  • The default is 0: the committer sleeps at once, as on main. On a 4-core host the poll gave no fps and a worse p99 (below), so it stays opt-in.

PPSA36082

Level UI/menu standing still, APS5_HOST_IMPORT_MIB=6784, one build with the poll switched off and on (50 us) every 15 s in one sitting, 4 windows each, MangoHud fps per window (first 2 s of each window left out):

windows (fps) mean fps p99 frame time
8 cores, off 52.7, 53.0, 53.3, 53.7 53.2 24.1 ms
8 cores, on 55.9, 53.6, 54.2, 55.4 54.8 23.6 ms

On 8 cores the poll won every pair, by about 3%.

On 4 cores (taskset -c 0-3), same procedure: 49.4 fps off, 49.5 on; frame time p99 26.8 ms off, 31.4 ms on. So the default stays 0, and a host with cores to spare can set it.

Tested

Linux (CachyOS), x86-64, GCC, 7800X3D / RTX 4090, NVIDIA 615.78.08, on main f474fb7.

  • The linux job of current build.yml run locally: configure with SPIRV tools on, test registration check, build, libs, ctest 1299, all pass except agc_driver_store_over_untracked, which fails the same way on main.
  • With APS5_PIPELINE_SPIN_US=50, the agc_* suite passes except agc_driver_store_over_untracked, as on main.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none
  • GPUs tested: NVIDIA GeForce RTX 4090, 615.78.08, Linux 7.2.10 (CachyOS)
  • AI-assisted: yes

@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed build shader core/shader agc libSceAgc* labels Oct 14, 2026

@CnRJay CnRJay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Polling briefly before sleeping is a reasonable trade for a hand-off every few microseconds. Two things to check before 50 µs becomes the default:

  • Core count. A 7800X3D has 8 cores and 16 threads to spare. On a 4-core / 8-thread host, or a laptop under a power limit, a committer that spins whenever the queue drains takes a core from the title's own threads and can lower the clock the worker runs at. That can cost more than the wake it saves. A run with the process limited to 4 cores (taskset -c 0-3, or the equivalent affinity on Windows) for both builds would show whether the gain survives.
  • Noise. As the description says, one run each with whole-frame changes inside run-to-run noise leaves draw_enqueued (2.0 → 1.5 ms) as the only signal. Two or three alternated runs per build would make the frame-time line usable as well.

Since this only applies with the opt-in APS5_PIPELINED_DRAWS, defaulting the spin to 0 until those numbers are in would keep the mode's behaviour unchanged for anyone already using it.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 6632 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp L121
PR 6641 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp L16

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.

Pull request Shared files not compared
PR 1896 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp
PR 6188 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp
PR 6208 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp
PR 6538 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp

This was referenced Oct 15, 2026
giljakoki pushed a commit to giljakoki/AnyPS5-macos that referenced this pull request Oct 15, 2026
…perf to the port

Ports onto the APS5_PIPELINED_DRAWS port (still off by default):
- drain before a CB metadata pass and when an entry's runs overlap queued
  writes (Raghav314 c50e231, a9c15be; HTILE note after drain was
  already in place)
- predicated packets drain queued writes to their predicate; a label
  behind a deferred label is not enqueued ahead of it (eliyarson
  56b9448, 1f894c9)
- pattern fills also drain for queued writes to their source dword
  (oneandonlydean c653e9e)
- lock-free span check in Overlaps, spin before the mutex, notify after
  unlock, committer polls 50 us (APS5_PIPELINE_SPIN_US) before sleeping
  (mrzacarias fb88422, 3495924; upstream boykopovar#5867)
- APS5_PIPELINED_DRAWS=1 now means depth 64 (4b53723)
- committer and queue workers run on 64 MiB stacks: a macOS std::thread
  gets 512 KiB (4c65bd4, ff25de8)
- tests: pipelined fill (ordered only), predication read, predicated
  label order, in both pipelined modes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Raghav314
Raghav314 force-pushed the perf/agc-committer-spin branch from a2b5009 to a594e9f Compare October 16, 2026 18:25
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 16, 2026
…re it sleeps

Draws arrive a few microseconds apart, so the committer slept after almost every commit and the worker paid a condition-variable wake for the next one. APS5_PIPELINE_SPIN_US=<us> makes it poll that long while the queue is empty, as the queue-0 wait does before it sleeps. The default is 0 (sleep at once): on 4 cores the poll gave no fps and a worse p99.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Raghav314
Raghav314 force-pushed the perf/agc-committer-spin branch from a594e9f to 3d4f96c Compare October 16, 2026 19:19
@Raghav314

Copy link
Copy Markdown
Contributor Author

Rebased onto main f474fb7 (only the poll commit is left; the rest is in main). The default is now 0, so the committer behaves as on main unless APS5_PIPELINE_SPIN_US is set.

Both checks, level UI/menu, one build switching the poll off and on (50 us) every 15 s, 4 windows each:

  • 8 cores: 53.2 fps off, 54.8 on; the poll won all 4 pairs. p99 frame time 24.1 to 23.6 ms.
  • 4 cores (taskset -c 0-3): 49.4 off, 49.5 on, and p99 26.8 to 31.4 ms. As you expected, it does not hold on 4 cores.

LLM assistance used.

@Raghav314
Raghav314 marked this pull request as ready for review October 16, 2026 19:28
@adriadam10 adriadam10 self-assigned this Oct 16, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the diff, built it merged onto current main and ran the AgcDriver ctest scope: green except agc_driver_tessellation (segfault, same on main on RADV here). With the default 0 the committer behaves exactly as on main; the 4-core measurement you added settles the earlier question.

  • Overlap with #6632: it also makes the draw committer spin before it sleeps (APS5_HANDOFF_SPIN_US, window picked from the usable core count, plus a agc_handoff_spin test) and edits the same part of DrawPipeline::run / DrawPipeline.cpp. Only one of the two can land. Please agree with its author on which approach stays (yours is opt-in and 20 lines; theirs sizes the window by core count and covers the GPU mutex too) and list the other in Depends on or drop the overlapping part.
  • Docs: the other APS5_* switches of the driver are described in docs/user/USAGE.md; if this one stays, add APS5_PIPELINE_SPIN_US (unit, default 0, 4-core caveat) there.

@Raghav314

Copy link
Copy Markdown
Contributor Author

Closing this in favour of #6632, which spins in the same place in DrawPipeline::run and also before the GPU mutex and in DrawPipeline::Drain, with a window picked from the usable core count and its own test. The measurements here back that policy (8 cores: +3%, 4 cores: no gain and a worse p99), so I posted them on #6632 instead.

Thanks @CnRJay and @adriadam10 for the reviews.

LLM assistance used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants