Repository navigation
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks. I rebased only this PR's commit onto current main (#2583 landed with a different commit, so it applies cleanly) and ran the libSceAgcDriver tests. Findings:
agc_driversegfaults intermittently in the default (background) mode: 10/30 runs on this PR, 0/30 withAPS5_SYNC_SHADER_PREPARE=1. Plainmain(#2583) does the same, 8/30 default and 0/30 sync, so this PR didn't introduce it. But it adds more concurrent preparation (pendingAbis,abiFailures, count of in-flight jobs) on top of a crash we can't yet explain, so we can't merge it until the default mode is stable. Could you find the cause (a backtrace ofagc_driver_testsin default mode would be enough to start)? A loop likefor i in $(seq 30); do ctest -R '^agc_driver$'; donereproduces it for us.- The new cases in
PreparedShaders.cpponly compile against the new members, so they don't show the behaviour differs frommain. Please add one that fails onmainfor observable behaviour (e.g.ResolveShaderAbireturning before the failure is raised). - The speed-up numbers are from your Windows/RTX 5090 run only; we can't reproduce them here. A synthetic before/after on Linux (same harness, several runs) would back it up (#218).
Please also add a short paragraph to docs/dev/ARCHITECTURE.md (shader section) covering the background ABI resolution and the in-flight job count.
|
A data point on the failure handling here: some titles call On LEGO Horizon Adventures (PPSA14632), the call comes from With this PR, if I read it right, the first call queues the job and returns, but a later |
|
Additional platform validation on standalone PR head Linux: passed — Arch Linux in WSL2 (kernel 6.6.87.2), GCC 16.2.1 20260810; CMake 4.3.1 / Ninja. Release / Tests: Reproduce after building the test targets: Vulkan: Mesa lavapipe 26.2.4 software Vulkan; no native Linux GPU/retail-title validation. Integration limitation: a local merge against main |
|
Data from 007 First Light (PPSA11386 v01.018.000) on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ryzen 9 7950X3D, RTX 3080 with NVIDIA 615.71.09), Release with This head does not merge with current 007 currently stops while presenting that flip, on a bindless image table limit. The measure is therefore the process lifetime from exec to that stop. It covers boot, the shader registration burst and the first draws (40 to 46 traced per run). Runs alternated between the two builds.
* This run overlapped a git checkout and a CMake configure on the same machine; without it the median is 26.1.
AI-assisted: yes (Claude Code) |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the work here. Since there was no push after our last review, we looked at the new data in the thread. Still changes:
- Slower on Linux. @SleglJan measured this commit cherry-picked on current
main(007 First Light, PPSA11386, RTX 3080, alternating runs): median 27.5 s → 29.8 s cold cache and 24.6 s → 26.5 s warm; every run with the commit is slower than every run without it. The Windows/RTX 5090 numbers in the description come from a local build that is not this branch. Please add a before/after that shows a gain on Linux, or explain where the extra time goes. - Stored failure reaches the game. As @alfalfa-star describes, a later
ResolveShaderAbicall for the same key re-raises the background failure into game code (sceAgcCreateInterpolantMapping), which ends the title wheremainwould not have reached the call yet. A failed ABI job should behave like a deferred registration (logged, then prepared or refused at the draw that uses it). agc_driversegfault in default mode is still unexplained (10/30 here, 8/30 onmain, 0/30 withAPS5_SYNC_SHADER_PREPARE=1). A backtrace ofagc_driver_testsin default mode would be a good start; this PR adds more concurrent preparation on top of it.- The new cases in
PreparedShaders.cpponly compile against the new members; add one that fails onmainfor observable behaviour. docs/dev/ARCHITECTURE.md(shader section) needs a paragraph on the background ABI resolution and the in-flight job count.- The PR conflicts with
main(ShaderRegistry.cpp,PreparedShaders.cpp): #2583 landed with a different commit, so please rebase onto it and drop its commit from this branch.
|
Hi! Since #2583 merged, this PR's stacked base is gone from its diff and it now conflicts with The rebase needed care because
It builds; The rebased branch is available to pull: Feel free to ignore this if you'd rather rebase yourself. |
ResolveShaderAbi for compute and pixel shaders compiled each new ABI variant on the calling guest thread. Titles resolve hundreds of them in a row from a loader thread (sceAgcCreateInterpolantMapping), so that thread spent its time compiling. The calling thread now plans the variant (PlanRegistered still reports register and validation errors there), records its key as pending and queues the compile on the registration preparation pool. A resolution whose key is registered, pending (including the registration job's own key) or failed returns at once. Draws, dispatches and transaction reads wait for the snapshot's ABI jobs. A waiting thread runs a job the pool has not started yet itself, so it does not wait behind other shaders in the queue, and transaction reads drain the jobs before taking the preparation writers lock. A failed ABI job is logged and its key is recorded as failed. It is not rethrown into the guest: the draw or dispatch that needs that state prepares it at use, and fails there if the shader cannot be prepared. APS5_SYNC_SHADER_PREPARE=1 keeps the synchronous path.
4d8dd9d to
2f9697b
Compare
|
Thanks for the reviews and the data. I pushed a reworked version, 2f9697b, as one commit on
Still missing: a performance trace of a title scene in the template's form. I have not captured one for this head yet. Benchmark harness (built as a local test executable linked like
|
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rework. Checked on the current head (merged onto main): build, agc_driver_prepared_shaders* including _background_abi and _background_abi_sync (12/12 pass), and agc_driver plus the prepared-shader tests repeated 40 times with no failure. The design reads sound: ABI jobs never take the preparation lock, and Read / Edit wait before taking it.
What is still open:
- Linux regression not re-measured. The blocker on the previous version was @SleglJan's alternating-run series on 007 First Light (PPSA11386): median 27.5 s → 29.8 s cold, 24.6 s → 26.5 s warm. The description says this version should fix it, but nobody has repeated that series on this head, and the synthetic harness doesn't cover draws waiting on queued jobs. Please post a before/after on a real title on Linux (or get @SleglJan's run on
2f9697bb5); a gain or no change is enough to continue. - Conflict with #5007. It touches
tests/execution/PreparedShaders.cppnear line 669; one of the two will need a rebase after the other lands.
Once the real-title number is in, this is ready for a final pass.
|
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.
|
|
@gjonn @adriadam10 Repeated on 2f9697b with 007 First Light (PPSA11386 v01.018.000) on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ryzen 9 7950X3D, RTX 3080 with NVIDIA 615.71.09), Release with Result, warm shader cache, 6 runs each: with 2f9697b the first presented frame comes at about the same time (6.4 against 6.5 s, ranges overlap), frame 169 comes 0.5 s later, and from frame 300 to 1000 007 renders about 3% slower (2.21 against 2.27 FPS). On On this branch 007 gets past the stop of the earlier series (its first flip), so the measure changed, and the earlier 1.5 to 2.3 s gap does not compare directly. The measure is the time from exec to the first presented frame and to frames 169, 724 and 1000, back-dated from the window-title FPS counter. No input is given, so none of these frames is gameplay. By frame 169 most shader preparation is done: the first 10-second shader cache report, before frame 125 in every run, has 2,091 of the run's 2,193 to 2,199 lookups, and about 100 more follow by frame 200. From about frame 230 007 renders at mostly 2.0 to 2.5 FPS, so frames 724 and 1000 measure draw throughput after that. Runs alternated between the builds (BAAB). Only warm runs: a cold boot on this branch takes about 13 minutes, most of it NVIDIA compiling a few compute pipelines of up to 4.3 million SPIR-V words on the queue worker. Every run starts from a fresh copy of the same shader cache (the recompiler cache plus the Vulkan pipeline cache), so shader preparations, this PR's ABI jobs included, are cache hits (no miss in 11 of the 12 runs, 6 in the other). The recompile work this PR moves to the background pool is therefore not exercised here. The sandbox gives the driver an empty home, so NVIDIA's own disk cache is empty in every run. Switches:
Medians in seconds (FPS in the last column), ranges in parentheses. Counts below are of the 36 pairs of a run without the commit and a run with it.
Each run (seconds, in run order)
AI-assisted: yes (Claude Code) |
|
Thanks @SleglJan for repeating the series, and @adriadam10 for the reviews. The bar was a gain or no change on a real title, and this head misses it. On PPSA11386 it is about 3% slower in steady rendering, and frame 169 arrives 0.5 s later. The run without the commit wins 31 to 34 of the 36 pairs. The profile shows no extra CPU work, so the cost is waiting. My reading is this: since #3159, Closing. The test for a failure raised in the background ( AI-assisted: yes |
What
Rebased onto current
mainas one commit. #2583 has landed, so its commit is gone from this branch.Driver::ResolveShaderAbifor compute and pixel shaders (types 0 and 1) compiles each new ABI variant on the guest thread that calls it. Titles resolve hundreds of these in a row from a loader thread throughsceAgcCreateInterpolantMapping, so that thread spends its time compiling. Since #3159, the compile no longer runs under the preparation writer lock. It still runs on the calling thread, one variant after another.With this change,
ResolveShaderAbidoes the following instead:PlanRegisteredon the calling thread, so register and validation errors are still reported by the call. It then records the key inpendingAbis, queues the compile on the registration preparation pool as anAbiPreparation, and returns.Waiting for ABI jobs:
SourceHandleFor,InvocationFor) and transactionRead/Editwait for the snapshot's unfinished ABI jobs (abiPreparations).Read/Editfinish these jobs before they take the preparation writer lock, so no thread holds that lock while it waits on the pool.A failed ABI job does not reach the guest:
failedAbis, so later resolutions of that key return at once.APS5_SYNC_SHADER_PREPARE=1keeps the synchronous path.ResolveGraphicsStagesAbiand the rectangle-list path are unchanged.docs/dev/ARCHITECTURE.mddescribes the background ABI resolution and the job list.Tested
Unit tests. New cases in
agc_driver_prepared_shaders:--background-abi, run asagc_driver_prepared_shaders_background_abi(default) and_background_abi_sync. It registers a pixel shader whose code fails only once it is built for its ABI.ResolveShaderAbireturns, and repeated calls for 500 ms never throw.main, the default case fails:ResolveShaderAbithrowsbranch at program counter 0x00000000 targets invalid program counter 0x00000008into the caller. This was checked by building the same test againstmainc13dc7e on Linux.QueuedAbiPreparation, in the main run:SourceHandleFor/InvocationFor, and its artifact is reused.failedAbis. It leaves the snapshot readable, and the dispatch prepares at use instead of rethrowing.Results:
^agc_CTest suite, 452/452 passed (1 skipped).agc_driver_prepared_shadersin all modes passed, including both new ones.Synthetic before/after on Linux. Same machine and build flags;
mainc13dc7e against this PR, alternating runs, 5 each. Ubuntu in WSL2, GCC 15.2, Release,ANYPS5_ENABLE_SPIRV_TOOLS=OFF, Mesa lavapipe, 24 threads. The harness source is in the review reply below.The harness does the following:
0x1b6= 1), so registration does not compile them.mainloader (s)maintotal (s)Every PR run finished before every
mainrun.The earlier Linux slowdown (PPSA11386). It was measured on the previous version of this commit. That version had two places where waits got longer:
Read/Edittook the global preparation writer lock first, then waited for the snapshot's in-flight jobs. An ABI job queued behind a registration burst kept every other registering thread blocked until the pool reached it.This version waits before it takes the lock and runs a job that is still queued on the waiting thread. I have not re-run that title. @SleglJan, if you can repeat your series on this head, that would confirm it.
Not tested here: a performance trace of a title scene in the form the template asks for.
agc_drivercrash in default mode. It does not reproduce on currentmainor on this head:ctest -R '^agc_driver$': 0/30 on this PR and 0/30 onmainc13dc7e on Linux (WSL, lavapipe). 0/30 on this PR on Windows (RTX 5090).mainalone, also 0/480 in parallel runs and 0/96 under ASan.The likely cause was the one #2976 (2dd453d) fixed:
Driver::stopreset the device without waiting for the registration pool.SpirvTargetextension names at exit.#2976 merged after the first review's runs, and this branch is based on it.
CloseRegistrationPreparationdrains ABI jobs as well, because they run on the same pool.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)sceAgcCreateInterpolantMapping, but I have not run that title.