Repository navigation
fix(agc): wait for a shader's registered preparation before taking the transaction lock - #11873
Conversation
…e transaction lock Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
The synchronization change and the new concurrency regression test look internally consistent on source review; the reported focused Vulkan gate also includes main, test-only and full-change arms with 49 layer insertions and zero VUIDs. I have not yet completed the local exact-head build/test, which is still running.\n\nOne repository requirement is not met by the report as written: CONTRIBUTING.md says a PR claiming a speed change must include valid aps5-summary.jsonl lines for main and the change from the same machine and scene, with both traces attached as described by the Performance report form. The description gives a quantified before/after compute-queue duration (1,420,772 µs versus 19,158/21,088 µs) and a dramatic per-submission improvement, but the linked release contains Vulkan test logs only; it has no summary lines or trace captures. Please add the paired performance-report artifacts, or remove the before/after speed claim and keep the report to the correctness/concurrency evidence.
|
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.
|
|
Thanks, the change looks correct: no new lock-order edge, and the original regression distinguishes main from the PR. One request about the performance claim: the speed comparison comes from a private integration build, so we can't check it as it stands. Could you attach a paired same-machine, same-scene comparison (summary JSONL plus both traces) with immutable refs for the two builds? Alternatively, drop the speed comparison from the description and keep the correctness evidence, which stands on its own. |
|
I downloaded and checked the three linked gate logs. The |
|
@oneandonlydean @MarcZX10 thanks for checking. I took the second option: the description no longer quantifies a speed change. The per-submission compute-queue timings came from a private integration tree, so they are removed, and the PR now states that it makes no performance claim. What remains is the correctness change, the concurrency regression test and its negative control (the gate MarcZX10 checked), and the stack sample that explains why the convoy was found. If I later report a measured effect, it will be a paired performance report with summary lines and traces from immutable refs. |
MarcZX10
left a comment
There was a problem hiding this comment.
The requested change is addressed: the description no longer claims a measured speedup, and it now explicitly says there is no performance claim. The synchronization code and regression remain unchanged from the reviewed head. The new test checks that a transaction on a prepared shader completes while another waits on a pending registration; the linked RX 6650 XT gate logs provide main, test-only negative-control, and full-change arms. I checked those logs previously: main and full-change pass 22/22, the test-only arm fails exactly the four modes containing the regression assertion, the validation layer is inserted, and no VUID is reported.
I found no blocking source issue. My local exact-head test could not provide a GPU execution result: native CTest stops at this host's AVX2/FMA guard, and the QEMU/llvmpipe attempt failed on guest-memory readability, so I do not count either as a pass. The author-reported hardware gate is the successful runtime evidence. Hosted Windows, conventions, intake, labels, and report checks pass; the Linux product job is skipped by path policy.
|
Windows + NVIDIA result for head This PR's own tests: the 26 Host: Windows 11 26300, Intel i7-12700K, NVIDIA GeForce RTX 2070 SUPER (vendor 0x10de, device 0x1e84; Vulkan loader 1.4.357 from the NVIDIA driver), Khronos Validation Layers nightly 1.4.366 (b2b44c0d37), WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, Same focused run on both:
The tree also carried the open PRs #11705, #11943, #11721, #11708, #11703, #11702, #11701, #11948, all merged cleanly, so the run is a joint one. Not run on a PS5. AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 391 tests)Skipped (41)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
ShaderPreparationTransactionserializes its writers with one static mutex (State::writers). The firstRead,EditorHoldof a transaction takes it, and the transaction keeps it until it ends.ReadandEditthen callAwaitRegisteredPreparationfor the shader. That call waits, possibly for seconds, for the shader's background registration preparation when it is still pending.A thread whose shader is still queued for preparation therefore holds the global writers mutex while it waits. Every other thread that opens a transaction queues behind it, even for shaders that are already prepared. Such threads include
sceAgcCreateShader→Driver::RegisterShader→Hold, andAgcDriverResolveShaderAbi→Read.With this change,
ReadandEditfirst wait for the shader's registered preparation under that shader's own mutex, when the transaction does not hold the writers mutex yet. Only then do they take the writers mutex. The existing wait inside the locked section stays. It now finds the preparation settled, except when a preparation was queued again in between, in which case it waits as before. A transaction that already holds the writers mutex, at its second and later shaders, is unchanged.Why
Echoes of the End (PPSA25636) from empty shader and pipeline caches. During loading, the registration preparation pool holds a backlog of roughly 250-300 tasks (
APS5_TRACE_PREPARATION_WAIT=1).RegisterShader→Holdand 8 inResolveShaderAbi→Read.Tested
PreparedShadersgainsPendingPreparationOutsideTransactionLock:Focused Vulkan gate. Machine: Linux 7.2.9, RX 6650 XT, RADV (Mesa 26.2.4). Command:
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_LOADER_DEBUG=layer ctest -V -j1 -R '^agc_driver_prepared_shaders'.3fea2a937baShaderRegistry.cpprestored to main (new test only)bcaac162594agc_driver_prepared_shaders,_indirect,_uncachedand_debugfail with "a transaction on a prepared shader waited for another transaction's pending registration preparation"bcaac162594Logs: https://github.com/JJenkx/AnyPS5/releases/tag/transaction-lock-evidence
This PR makes no performance claim. The Echoes observations above (the stack sample) explain why the convoy was found; they are not a measured speed result. Echoes still crashes about 30 s into a first launch from empty caches with this change alone.
Checklist
main; no other open PR implements the same functions. perf(agc): wait at submit only for the preparations of the programs a submission binds #6867 also changesShaderRegistry.cppandPreparedShaders.cpp, in different functions: it narrows the submit-time preparation wait inDriver::Submit. The two changes are independent.NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file (no unimplemented path added).mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code