Skip to content

perf(agc): clear a register fast clear without waiting for a known key store - #3

Closed
DotDebian wants to merge 1 commit into
oneandonlydean:astrobotfrom
DotDebian:feat/register-clear-known-keys
Closed

DotDebian wants to merge 1 commit into
oneandonlydean:astrobotfrom
DotDebian:feat/register-clear-known-keys

Conversation

@DotDebian

Copy link
Copy Markdown

Follow-up to discussion boykopovar#357: materializeRegisterClear only exists on this branch, so the PR targets astrobot.

Problem

When a color target's DCC keys read as the register clear code, materializeRegisterClear clears the resident image to CB_COLOR_CLEAR_WORD. In Astro Bot those keys usually come from a key store the driver recorded itself (NoteKeysFillOnGpu) that the GPU has not run yet. FillClear then refuses with "keys pending", and the fallback waits for that store (SyncThrough) before clearing. That wait happens on the queue worker, every frame.

Change

The wait is not needed when the newest recorded writer of the whole key range is that noted store. ProvedCurrentDccKeys has already answered from the same memo (PendingStoreKeys), so the keys the GPU will hold are known, and they are the code this clear stands for. The image clear and the "uncompressed" key store that follows are recorded into the same queue behind the store, so they land after it. That is the same order the wait gave.

  • KnownPendingDccKeys(meta, bytes) exposes that proof: it returns the keys of a pending noted store covering the range, and nullopt otherwise. It needs GpuMutex and never waits.
  • FillClear takes pendingKeysKnown, which skips only the "keys pending" refusal. Its scan of the keys in memory still runs.
  • Any other pending writer keeps the wait: a title kernel, a later partial or wider write, or a shadowed range. So do keys in memory (still from before the store) that the scan refuses.

APS5_NO_REGISTER_CLEAR_KNOWN_KEYS=1 restores the wait.

Measurement

Host: Windows 11, Core Ultra 9 285K, RTX 5080. Astro Bot (PPSA21564), -lvl hub_crashsite, 150 s runs. The CPU wait of this path dropped from about 145 ms to 4 ms per frame, and the image did not change.

That number comes from the same change on an older base of this branch: b58df66c plus my local stack, including boykopovar#637 for the Windows red-zone crash. I could not re-measure on 88d7bb62. On Windows that head needs a write-watch fix to get past SceSndzAudioOutMain (issue boykopovar#361). With boykopovar#637's commit cherry-picked, the old executable shows black frames, and a fresh relink crashes in RoomLoad_ATQT before the level. With the change on, [recorder] (APS5_PROFILE_DRAW=1) still shows the "recorded store" sync count dropping, but I would not quote those runs.

Tests

  • agc_driver_recorder_tests (not part of the default ctest run):
    • pendingKeyStoreTests: the proof answers only for a pending noted store that covers the range. It returns nullopt when a later writer is over it or when nothing is pending, and asking never waits.
    • metadataPassTests: a register clear recorded behind a pending register-clear key store is refused without the proof and accepted with it, without waiting. After submission, memory holds the clear word and the keys are uncompressed.
    • The suite passes when drawInputReuseTests is skipped. That test already fails on 88d7bb62 without this change ("a shorter vertex read did not reuse the longer snapshot").
  • ctest (Windows, MinGW g++ from WinLibs, Release): 297/299 pass. The two failures are recompiler ALU tests: agc_driver_f32_output_modifier (div:2 overflow) and agc_driver_scalar_sop1 (s_movrelsd_2_b32). This change does not touch the code they test.

Tested and written with AI assistance (Claude Code).

🤖 Generated with Claude Code

…y store

materializeRegisterClear clears the resident image to CB_COLOR_CLEAR_WORD
when the target's DCC keys read as the register clear code. Most of the
time those keys come from a key store the driver recorded itself
(NoteKeysFillOnGpu) and the GPU has not run yet, so FillClear refused with
"keys pending" and the fallback waited for that store (SyncThrough).

That wait buys nothing when the newest recorded writer of the whole key
range is the noted store: ProvedCurrentDccKeys already answered from the
same memo (PendingStoreKeys), so the keys the GPU will hold are known and
are the code this clear stands for. The image clear and the "uncompressed"
key store after it are recorded into the same queue behind that store and
land after it, the order the wait gave. KnownPendingDccKeys exposes that
proof, and FillClear takes a pendingKeysKnown flag that skips only the
"keys pending" refusal; its scan of the keys in memory still runs. Any
other pending writer (a title kernel, a later partial or wider write, a
shadowed range) keeps the wait.

Astro Bot (PPSA21564) at the Crash Site, Windows 11, Core Ultra 9 285K,
RTX 5080: the CPU wait of this path drops from about 145 to 4 ms per
frame, with the same image.

APS5_NO_REGISTER_CLEAR_KNOWN_KEYS=1 restores the wait. Tests: the proof
answers only for a pending noted store covering the range, and a clear
recorded behind a pending register clear key store leaves the clear word
in memory and uncompressed keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oneandonlydean

Copy link
Copy Markdown
Owner

The astrobot branch is frozen and the work has moved to upstream main, where materializeRegisterClear and the noted key stores exist too (boykopovar#132, boykopovar#458). On Linux with an RTX 4090 the recorder tests pass and the waits from this path go away with the same frames: 101 to none at the Crash Site and 1,393 (3.2 s) to none through the title, menu and first level; the frame rate doesn't move here, because the wait is small on this host. I've ported it to main with you as the commit author (carry/fork3-register-clear-known-keys: the off switch and the comments are removed, and the tests run in ctest as agc_pending_dcc_keys). May I open it upstream with your commit, or would you rather open it yourself?

adriadam10 pushed a commit that referenced this pull request Oct 16, 2026
The pr-template check only accepted "Depends on: #N" or "none" on the same
line, so a checklist answering it as a bulleted list failed CI although
pr_overlap reads exactly that form (test_pr_overlap_dependencies covers
"Depends on:
- #2
- #3"). Allow one list marker before the first answer;
an empty answer is still reported.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zyXkUzTZGh3nBwNMtrLaW
adriadam10 pushed a commit that referenced this pull request Oct 16, 2026
…ll or crash

A GPU hang or a crash in a title leaves no record of what the queues
were last given: APS5_TRACE_GPU prints every submission (too much to
leave on), and the WAIT_REG_MEM timeout message shows only the packet
before the wait on the waiting queue.

Each queue now keeps a ring of its last 16 submissions
(APS5_PM4_RING=<n>, 0 to 4096, 0 disables): the TraceMs time in tenths
of a millisecond, serial, size, guest address and a copy of the first
512 dwords after indirect buffers are inlined, recorded under the
driver lock next to the serial, REWIND tails included. The ring of a
queue and the failure report registration are set up at the top of
Submit, before any output is reserved, so recording under the lock
does not allocate. Disabled, Submit only tests a static flag; enabled,
it costs one bounded copy per submission.

DumpSubmissionRings decodes the kept dwords with Pm4::PacketWords and
the new Pm4::KnownName, which returns the decoder's names as views of
static strings (empty for an unknown opcode, printed as UNKNOWN_0x%02x);
Pm4::Name now builds its std::string from it. Every line is formatted
with integer and string conversions only into a 512-byte stack buffer
and written with one WriteFile / write(2) on the stderr handle, so the
dump does not allocate from the C runtime heap, lines from other
threads do not interleave and the CRT stream lock is not taken (the
rule of boykopovar#3871). The header gives the number of queues and the ring
size, and each queue line the submissions recorded and kept.

libc gets a small registry of failure reports
(LibcRegisterFailureReport_nid_no_patch, Unregister, Run); Run runs
the registered reports the first time it is called and returns at once
after that. The Windows crash report runs it after the thread dump of
an unhandled exception, after SIGABRT (which also covers the
APS5_HANG_DUMP_SECS timed dump, since it aborts) and after an invalid
CRT parameter, so an abort after a reported exception does not print
the rings twice. The driver registers its dump at the first submission
and unregisters at static destruction; the dump there waits a bounded
time for each ring lock. What the crash-path dump cannot do (a ring
held by the crashed thread, rings corrupted by the crash, Linux) is
recorded in technical debt.

APS5_STALL_WATCHDOG_SECS=<n> (0 to 86400, 0 disables) makes a waiter
that has not progressed for n seconds dump the rings once per wait: a
WAIT_REG_MEM (progress as for APS5_GPU_WAIT_TIMEOUT_MS: another packet
runs or completes), a REWIND waiting for the title, and a compute
submission held behind queue 0. The last one prints under the driver
lock, so submissions and the other queues wait for it (USAGE.md says
so). Both variables are read at the top of Submit, so an invalid value
of either throws at the first submission and never later inside a GPU
worker. The order wait behind queue 0 releases its order hold on scope
exit, so a throwing dump cannot leave it counted.

No packet semantics change; packet sizes and names are those of the
existing decoder (PM4 type-3 header layout as in Mesa
src/amd/common/sid.h). The queue bound of Submit and of the rings is
one constant, SubmissionQueueEnd.

Tests: agc_driver_pm4_tests gets ten modes, each its own ctest:
agc_driver_pm4_ring (APS5_PM4_RING=4, six submissions on queue 0 and
two on 0x20: the dump holds #3..boykopovar#6 and #1..#2 in order, nothing
older), agc_driver_pm4_ring_empty (one line), _ring_disabled (one
line), _ring_invalid and agc_driver_pm4_stall_watchdog_invalid
(APS5_STALL_WATCHDOG_SECS=1x: the submission throws and is not
executed), _ring_report (LibcRunFailureReports_nid_no_patch dumps the
rings), _ring_report_heap (Windows: with ucrtbase.dll's HeapAlloc and
HeapReAlloc imports counting calls from the reporting thread, the
failure report of a submission holding INDEX_BUFFER_SIZE names it and
allocates nothing, while Pm4::Name of the same header is seen
allocating), and with APS5_STALL_WATCHDOG_SECS=1 a WAIT_REG_MEM
(agc_driver_pm4_stall_watchdog), a REWIND (_stall_watchdog_rewind) and
a compute submission held behind a queue 0 rendering wait
(_stall_watchdog_order), each held for about 2.5 s, dump once and
complete when released. The PM4 catalog test checks KnownName against
every opcode and custom name. windows_backtrace_failure_report checks
that abort() runs the registered reports once and not an unregistered
one; windows_backtrace_failure_report_once that abort() does not run
them again after an earlier run. With the dump decoding through
Pm4::Name, agc_driver_pm4_ring_report_heap fails with "the failure
report allocated from the C runtime heap"; with Run resetting its
flag, windows_backtrace_failure_report_once prints the report twice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oneandonlydean

Copy link
Copy Markdown
Owner

Thanks @DotDebian. This change is on boykopovar/AnyPS5 main now as a61c70f (via boykopovar#6247), so I'm closing it here. The astrobot branch is frozen; anything new is best opened against main directly.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants