perf(agc): prepare registered shaders outside the publication lock

[jweidenh]

What

Driver::RegisterShader holds the process-wide preparation writer lock while planning a private shader registration. Independent registering threads therefore wait behind that work, including full preparation when APS5_SYNC_SHADER_PREPARE=1.

Move that acquisition to publication. For an identical registration, capture the matching immutable snapshot under the driver mutex, release the mutex, acquire the preparation writer, then reacquire the driver mutex and publish through PublishRegisteredShader. Fresh registrations plan privately before acquiring the same locks in that order. The transaction still exists from function entry, so a caught registration failure aborts an enclosing preparation transaction.

Current head 871012973005bae1e7e40c63d6c099e010ea4489 is based on main 5aaf26bd325607f65bde28741a4e82ceaaf586c0. It preserves upstream 94cffec3's lazy transaction implementation and existing Hold() API unchanged; Read, Edit and Hold acquire the writer, while an empty Commit does not. No new transaction constructor or locking API is needed. The production diff is confined to registration lookup, planning and publication. Background scheduling, pending/failure waits, header reuse, ABI preparation and rollback retain upstream behavior.

This is a concurrency correction, with no demonstrated affected-title startup benefit. The original reporter has already supplied the requested #2292 measurements; they are summarized below. No startup or FThreadHeartBeat fix is claimed.

Tested

• Source conventions and git diff --check pass for 871012973.
• Windows 11 / RTX 5090 / NVIDIA 617.42: all five selected checks passed without skips. Asynchronous registration completed in 1.360 s; synchronous registration in 0.880 s; prepared publication in 2.174 s; prepared shaders in 1.021 s; preparation shutdown in 0.760 s. The first four exercised native Vulkan; shutdown is a CPU lifecycle check. These are correctness-test runtimes, not benchmark or title-startup measurements. Release executables and patched libraries were rebuilt using portable WinLibs GCC 15.2.0 r7 with SPIRV-Tools enabled.
• The exact Windows validation source was 72fcef76d84a5ae356a746c8d8ed078d0bb4840c: 871012973 plus the Windows build guard from #4021 at 121b16fb4bbd47ff4ce9302c40631bab858e3e71. That historical overlay fixed upstream HostPoll compilation errors; only Socket.cpp differs from this PR. fix(libkernel): restore Windows select build for Demons Souls testing #4021 has since merged as bca7a29d259299aaa8972c94d12694232bb3bb08 on October 11; the recorded validation source remains unchanged, and this PR does not duplicate its patch. Source pins, test inventory, separate preparation modes, exact completion markers, GPU identities, raw JUnit/logs, and 30-second limits were verified. No earlier benchmark or baseline failure was rerun.
• Current-base Linux validation is blocked by new audio dependencies on the existing local runner. Historical Linux results below do not validate this rebased head.

The concurrency tests explicitly hold the publication writer, verify that private registration reaches its expected planning/translation failure without waiting, and check that a duplicate registration does not hold the driver mutex while waiting for publication. They also cover lazy Read/Edit/Hold, nested locking and rollback, eight concurrent registrations/resolutions, and subsequent dispatch. Both asynchronous and explicitly synchronous preparation modes remain separate tests, with 30-second limits.

Affected-title measurements already supplied

The original #2292 reporter's October 11 results identify STAR WARS: Galactic Racer (PPSA28416), Windows 11, RTX 5090, MinGW-w64 GCC 15.2.0, Release. Both arms used newer main 1e8fff68 with identical referenced and unpublished bring-up patches; the reported only difference was this PR's earlier five commits through 58d31147. This is independent integration evidence, not a reproduction of the current rebased head.

Both shader and Vulkan pipeline caches were warm, save data absent, APS5_PROFILE_DRAW=1 and APS5_NO_SNAPSHOT_CHECK=1; the title's command line kept FThreadHeartBeat active. The milestone was the first [present] statistics line, printed per 10-second window containing presents—a coarse upper bound, not exact first-frame latency.

Run	Without this PR	With this PR
1	23.9 s	23.9 s
2	23.2 s	23.7 s
3	23.4 s	22.6 s

All six runs remained alive at the 150-second cap without a heartbeat kill. The results show no clear startup benefit or regression. For cold caches, the reporter observed no preparation-transaction waiters and attributed the remaining heartbeat kill to long NVIDIA compute-pipeline compilation; they attributed removal of the original registration-lock stall to #2583. These are the reporter's observations, not independently reproduced here.

The reporter also saw intermittent heap corruption in parallel preparation-test batches: 3/10 with this PR, 3/10 without it, and 3/10 on clean main 1e8fff68. That comparison does not establish a regression from this PR. The earlier changes-requested review predates the supplied title measurements; maintainer acceptance remains pending.

Historical correctness and synthetic performance evidence — older revisions only

• At 58d31147 on base e8d6de4a, private self-hosted Linux run 38097454684 passed the Release/patched-library builds and 26 executions across 14 preparation/frontend test names, with no failures or CTest skips. Its inventory check found all 598 test executables registered; it did not execute the full suite. A duplicate-path lock-inversion mutation failed both concurrency modes at the intended assertion; restoring the exact source passed both. Environment: Ubuntu 24.04, GCC 13.3, Mesa 25.2.8 lavapipe/LLVM 20.1.2, SPIRV-Tools, required Vulkan and fresh caches.
• The maintainer's independent review reported a Linux AGC build and 371/371 scope tests passing with 58d31147 integrated into then-current main. No exact integration SHA or toolchain was specified. This is reported integration evidence, not validation of 871012973.
• Private self-hosted comparison 38136160956, harness 2567e8f997e8e0728943e801076d3fab07ca13ad, compared e8d6de4a with 58d31147, both including asynchronous preparation. All 16 pilot and 240 full processes passed output/cache checks. Five paired trials per case used 12 tiny/arithmetic compute shaders, one/two/four callers and cold/warm caches; registration return and artifact/ABI readiness were timed separately, with dispatch/readback outside timing. Default-async readiness ratios ranged from 0.898–1.204×, showing mixed results and no consistent overall improvement. The separate synchronous control favored the candidate in all 40 multi-caller pairs, with readiness ratios of 1.763–1.985× for two callers and 2.572–3.412× for four. CPU work did not uniformly decrease. These synthetic results do not measure title startup or this rebased head; physical-host exclusivity was not established.
• Separate compatibility integration 346a6e73 with the earlier perf(agc-driver): prepare shader ABI variants before taking the preparation lock #3159 implementation passed 32 selected executions and lock-order negative controls in run 38138738761. Its transaction design is superseded by upstream's now-merged lazy Hold() implementation; those results are not attributed to this head.
• Earlier independent Windows 11/RTX 5090 validation at 7371e448 passed both concurrency modes and an eager-lock negative control. It included no title or performance measurements. Older synthetic synchronous speedups, including the approximately 3× four-caller results in run 38029920550, predate asynchronous preparation and do not establish current default-path gains.
• An earlier full-suite run at 26a670a7 reported 490 passes, 26 capability skips and one agc_driver_flat_store timeout among 517 tests. The same fence-wait failure was reproduced on unmodified d70b8998; the test and timeout remain unchanged. These historical outcomes are not a current full-suite result.

Author-run Linux evidence used only locally managed self-hosted runners. Linked private workflows/artifacts require validation-repository access. The historical synthetic benchmarks, independently reported title results and current correctness validation are separate evidence sets.

Checklist

• Based on reviewed main 5aaf26bd; preserves merged lazy transaction behavior
• One topic per PR
• No code comments added
• Unsupported paths continue to throw; no silent stubs
• No investigation notes, logs or images added to the upstream repository
• No new third-party code
• Generic behavior
• Depends on: none outstanding; fix(libkernel): restore Windows select build for Demons Souls testing #4021 is merged. The exact historical Windows validation overlay remains identified above and excluded from this PR
• AI-assisted: yes

[Kikoarl]

Tested on Linux (CachyOS x86_64, Intel RPL-S iGPU / Mesa 26.3.0-devel / Vulkan 1.4.363, GCC 16.2.1) at 26a670a7:

• Build: Built cleanly in RAM (/tmp/anyps5_build) with zero warnings/errors.
• Conventions: python3 tools/check_conventions.py --base origin/main --head HEAD -> PASS (0 errors).
• Verification: Verified concurrent shader registration decoupling. Relieves lock contention during heavy parallel shader compilation pipelines without data races.

LGTM!

[popcoder565]

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 26a670a7, merged onto main 8b393b6 in one build with 31 other PRs that had no Windows run.

• This PR's tests pass: agc_driver_registration_concurrency.
• Full ctest in that build: 520 of 521. The one failure is guest_offline_imports, broken by feat(libSceRudp): implement sceRudpGetStatus #2359 (reported there).

No title was run.

🤖 Addressed by Claude Code

[oneandonlydean]

Does the title from #2292 get past the FThreadHeartBeat kill with this change? Perf PRs need before/after numbers for the change alone (#218); start-up time on that title with and without it would do.

[adriadam10]

@oneandonlydean you asked whether the title from #2292 gets past the FThreadHeartBeat kill and for before/after numbers for this change alone (#218).

• The PR body now has a paired A/B of base vs base + this PR only (same harness, 5 trials): 4 workers 2.9-3.1x wall time on registration batches, 1 worker no gain (tiny/warm ~13% slower), and more total CPU. It is synthetic; the author has no access to the title and says so, so the heartbeat outcome is unverified.
• Re-checked at 8e5e9a3: agc_driver_registration_concurrency builds and passes locally.

Question: is the synthetic A/B enough for #218, or do you want a start-up measurement on the #2292 title first? If so, @mrk1987 could you time the title with and without this branch?

[oneandonlydean]

The synthetic A/B covers #218 for the lock change itself. The PR is for the #2292 kill, though, so I'd like the start-up time on that title with and without it before merging; the single-worker case is slower, so it should show a gain there.

[klosixgaming-arch]

Independent validation on real hardware (correctness/concurrency, not performance)

Built and ran the PR head 7371e448 on my own box:

• Windows 11 Pro (26200), WinLibs MinGW-w64 GCC 15.2.0 (r7), Release, BUILD_TESTING=ON.
• GPU: NVIDIA GeForce RTX 5090 (I did not use lavapipe; the author's runs were lavapipe + a Windows RTX 3060 Laptop).

Results:

• agc_driver_registration_concurrency (async, APS5_SYNC_SHADER_PREPARE unset) — PASSES (exit 0) on the 5090.
• agc_driver_registration_concurrency_sync (APS5_SYNC_SHADER_PREPARE=1) — PASSES (exit 0) on the 5090.
• Covers deferred operations, nested rollback, a failing private registration not blocking behind an unrelated held writer, and 8 concurrent register/resolve/dispatch workers followed by a real dispatch + wait.

Rigor check (is the test real?): I reverted Driver::RegisterShader to the eager constructor (ShaderPreparationTransaction transaction; instead of (std::defer_lock)), i.e. the base behavior, rebuilt, and the async test failed — "private shader preparation waited for an unrelated publication transaction" (exit 1) — which is exactly the negative-control assertion you describe. Restoring defer_lock → both modes PASS again. So the concurrency test genuinely exercises the deferred-lock change rather than passing unconditionally.

Honest limits of what I verified:

• I verified the correctness/concurrency tests only. I did not measure the performance claim. Your 2.9–3.1× four-worker speedups are from a historical synchronous base, and you explicitly note they "do not establish performance on this base."
• I did not run the maintainer-requested Shader registration runs the recompiler front end under a process-wide lock even on disk-cache hits #2292 title before/after startup measurements (no game/ROM here), nor reproduce the FThreadHeartBeat outcome.
• I only ran on one GPU (the 5090); I did not test lavapipe, Linux, or the 3060 Laptop.

Written with AI assistance.

[adriadam10]

Thanks for the rebase onto #2856 and the extra regression. Checked on Linux at 58d31147 merged on current main: AGC driver scope build, 371/371 ctest, agc_driver_registration_concurrency and _sync pass.

One blocker, already asked by @oneandonlydean above and not yet addressed:

• Start-up time of the #2292 title with and without this change (and whether it gets past the FThreadHeartBeat kill). The synthetic A/B covers the lock change for #218, but the PR exists for that kill, and the body says that outcome is unverified. Also, the benchmark predates the async preparation from #2583, so it doesn't show the gain on the current base (single worker was ~13 % slower in the tiny/warm case).

If the title isn't available to you, ask in #2292 whether mrk1987 can run the two builds and post the numbers. Once that is posted this is ready from our side.

Heads-up: #3159 changes the same transaction defaults; whichever lands second needs a rebase.

[oneandonlydean]

Closing: on the #2292 title start-up is the same with and without this (23.2-23.9 s against 22.6-23.9 s, no heartbeat kill in either), and the registration stall went away with #2583. Reopen with a title where registration measurably waits on the writer lock.