Repository navigation
fix(agc): drain library prebuilds before device replacement - #6814
oneandonlydean merged 2 commits into
Conversation
|
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.
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.
|
|
Ghost of Tsushima (PPSA02225) hits the same crash on Linux/NVIDIA, and this PR's head fixes it there. Host: Ubuntu 24.04, RTX 4070 Ti (NVIDIA 595.99.02), i7-13700K, release-like libs, pad input, warm shader cache.
Forspoken (PPSA03984) hits it too, with a warm cache on the same host:
Three ps5-opengl homebrew apps, PS5 OpenGL Showcase (PPSA99005), PS5MC (PPSA99420) and OpenNOW (PPSA99082), show the same pattern:
Not checked: AMD, Intel, Windows. AI-assisted: yes |
|
Thanks for the detailed title runs and core backtraces. The matching prebuild-thread fault on Ghost of Tsushima and Forspoken, and the head getting past that crash in the latter, are useful independent evidence for the lifecycle race. I will keep the stated validation scope as mock Vulkan plus your reported Linux/NVIDIA title runs; I have not run a title or GPU validation locally. |
yabanana
left a comment
There was a problem hiding this comment.
What this change does: it keeps the library maps alive while Vulkan creates a part or links a pipeline, and drains registration prebuilds before replacing the device. I checked head 81e3db853c00453a04668e2c62e8b969f24d324b and reproduced the original lifetime failure independently.
1. Must fix: the new test is missing its Windows DLL search path.
agc_pipeline_library_clear_waits_for_build runs agc_driver_graphics_tests, but the adjacent if(WIN32) PATH setup names only agc_pipeline_journal. The test executable is in build/tests; the PRX DLLs are in the library output directory. Add the new test to that existing ENVIRONMENT_MODIFICATION list so it can load those DLLs in the standard Windows CTest setup. Otherwise the regression can fail in the loader before exercising this fix. This is a configuration finding; I have not run Windows.
Test patch and one-line CMake correction, based on this exact head. The regression now also fails a library creation and the final link while clearing waits, checks exception propagation and object release, and uses atomic accounting for concurrent mock destruction. It reuses the existing Pipeline constructor.
Verified:
- macOS 27.2 ARM64, Apple Clang 21, ASan+UBSan, production
PipelineLibrary.cppwith mocked Vulkan: main82ddae75breproduces a heap-use-after-free; this PR passes 40 cases (part/link × success/error × 10). The probe disables background LTO and does not run journal replay. - Ubuntu 24.04 amd64, GCC 13.3 Release, QEMU 10.2.3
-cpu max: the patched focused CTest passes 10/10 fresh processes. CPU guards and assertions are unchanged.agc_pipeline_journal,agc_driver_address_space_teardown, andagc_embedded_vertex_fetchalso pass. - The general graphics suite still hits the pre-existing zero-word vertex fixture failure tracked in #6792. I am not counting that run as a pass or attributing it to this PR.
- Conventions and diff whitespace checks pass.
Merge preview with #6641 has one conflict in LibraryStore: keep this PR's activeBuilds and #6641's worker/stopping; do not restore the replaced started field. This is the textual resolution, not yet a combined runtime result.
Independent sanitizer probes, source pins and reproduction commands.
Verdict: fix the Windows test registration before merging; the lifetime fix passes the exercised paths.
Not checked: Stray, physical GPUs, Windows execution, or the full platform suites. No title-unblock or performance claim.
AI-assisted: yes.
|
Fixed in |
|
Thanks for the additional title runs. The matching |
|
The combination with #6641 is now verified at dd8824b63629d68b7f205e66bcc578abb4a42e60. Keep This includes the existing #6789 fixture fix. The general graphics CTest exceeds 30 s under emulation; a separate bounded diagnostic completes, with write-watch cases unavailable. No full-suite, Windows or title result is claimed. AI-assisted: yes. |
|
Duplicate pair heads-up for the maintainers: this PR and #6827 carry byte-identical diffs (217 lines across the same six files — |
|
Thanks for flagging the duplicate pair. I checked #6827: its body marks it as a follow-up to #6814 and adds the Windows DLL search-path correction for the new CTest. The current #6814 head has the same correction, so that change is already present here too. The two PR diffs now overlap byte-for-byte; I’ll leave the maintainer choice about which PR remains open to you. No separate patch is needed on #6814 for the DLL path. |
|
A duplicate check confirms #6827 is also open with the Windows DLL search-path correction and carries a byte-identical production diff. Since #6827 is specifically the test configuration follow-up, could a maintainer advise whether #6814 or #6827 should be the surviving PR? I will keep both untouched pending that direction. |
|
Correction to my earlier note: I described #6827 as a duplicate, but its author explicitly presents it as a follow-up depending on #6814. The branch includes #6814’s implementation and adds the separate Windows test PATH correction, so these are stacked changes rather than duplicate submissions. I withdraw the request for a maintainer to choose between them; #6827 correctly records |
|
Two homebrew apps added to the catalog today crash on the same race on Linux/NVIDIA. Host: RTX 4070 Ti (NVIDIA 595.99.02),
AI-assisted: yes |
|
Thanks for the new PS5LM and Afterglow results. The PS5LM comparison adds another Linux/NVIDIA reproduction of the registration-prebuild crash and shows 4/4 runs passing the affected point on this PR head. For Afterglow, I understand the later retirement failure is attributable to its bundled 1.0.0 watchdog: with the local 30-second adjustment, this PR reaches the 60-second limit 2/2, so I will not attribute that exit to this change. I have not reproduced either title locally; I will keep the evidence scoped to your reported setup and runs. |
|
Thanks for consolidating the Linux/NVIDIA title runs and the Windows results. I see the focused pipeline-clear test and |
|
Thanks for confirming. I verified that #6827 is closed without merging and that the Windows DLL path correction is already in this head. #6641 is still open, so there is no integration change to make now; if it lands first, I will preserve the |
|
Another title with #6805's crash: Ghost of Tsushima (PPSA02225), Linux 6.8, RTX 4070 Ti (NVIDIA 595.99.02), Release, pad input, warm shader cache.
Crashing thread from the core of a Ghost ran to its limit on Update (homebrew): ProsperoEden 1.004.000 (PPSA99008, the catalog build from today), pad input, warm cache, 30–60 s runs. It has the same frames, from the core of a crashing run:
Earlier ProsperoEden versions (1.001.000, 1.003.000) reached their limits in all my earlier sweeps (40–180 s). AI-assisted: yes (Claude Code) |
|
Windows check of head 83eb3ed, since Windows execution was still listed as unvalidated: Windows 11 10.0.26200.9711, WinLibs GCC 15.2.0 posix UCRT r7, Release, BUILD_TESTING=ON, PR merged onto main f3263df (clean merge, no new build errors), standard ctest setup.
Not run: a title, the full graphics suite, or a negative control against main's PipelineLibrary.cpp. AI-assisted: yes |
|
Thanks for checking the exact head on Windows and repeating |
|
Ghost of Tsushima on |
|
Another title with the #6805 crash, Grand Theft Auto III – The Definitive Edition (PPSA03527 v01.007.000), and one cost of the fix: this PR (head 83eb3ed) stops the crash, but the window attach now waits about 67 s on a warm cache. Tree:
Every run that went on to the limit presented 900 frames in each 30 s summary interval (30 fps, p50 frame about 33.37 ms) after its first interval. Until the window device is created, the title logs
AI-assisted: yes (Claude Code, Claude Opus 5.5), operating under my supervision; I am accountable for this report and will respond to review. |
|
Thanks for the GTA III measurements. The 67 s attach delay is a material cost of the current fix: Since you offered to rerun either variant, would you be willing to compare a device-scoped cancellation variant on the same warm-cache setup? The useful distinction would be canceling queued prebuilds for the retiring device while still waiting for any prebuild already active on it, then recording attach time and whether the title progresses. I have not implemented or validated that alternative, so I am treating the 67 s result as an observed tradeoff rather than assuming cancellation is safe. |
|
I ran the device-scoped cancellation on GTA III DE. It brings the window attach back to 8–10 s, and the title runs on as it does with 83eb3ed. Variant (local, measurement only, on top of 83eb3ed): each prebuild pool task records the Setup: same tree and settings as my earlier comment (
In all five runs, the first interval had 779–794 presents and every later one 900, with a p50 frame of 33.37 ms. Before the window device was created, the title logged What the variant does not cover: a registration preparation that is still running can submit a prebuild for the old device after the cancel has returned. The ticket in
AI-assisted: yes (Claude Code, Claude Opus 5.5), operating under my supervision; I am accountable for this report and will respond to review. |
|
Thanks for the additional Ghost of Tsushima A/B and the standard Windows CTest run. The current head includes the DLL-path correction, and the reported 9/9 main crash versus 5/5 runs reaching the limit adds useful evidence for the lifetime fix. |
|
On the attach delay: in the two titles from my earlier comment it doesn't show. I ran them with this head (
So the first 10 s window already runs at the full rate, and the wait in AI-assisted: yes (Claude Code) |
|
Thanks for measuring both titles on the current head. Starting the first 10-second window at 591/600 and 602/600 presents shows this drain did not delay attach in those two warm-cache runs. That is useful alongside the Stray reproductions; it does not establish cold-cache or broader title behavior. |
|
Thanks for identifying the late-enqueue window. I traced it through I also checked the lifetime impact. The prebuild closure captures a A complete fix needs to stop new |
|
I have neither a test seam nor a measured case. In the three variant runs the title did not crash or stall: all reached the 240 s limit at 900 presents per 30 s interval after the first. But the variant does not log a prebuild submitted after the cancel returns, and the title presents only black frames in this setup (#6994), so these runs cannot show whether a late prebuild happened or had an effect. If a version that also gates AI-assisted: yes (Claude Code, Claude Opus 5.5), operating under my supervision; I am accountable for this report and will respond to review. |
|
Title check for #6805 on High on Life (PPSA15388), Linux, RTX 4090, GCC 16.2.1, Release. Tree:
With the PR, two runs reached the main menu. The third passed the window attach and then hit a different crash about 80 s in: a jump to Tested and written by Claude (AI assistant) on a contributor's machine. |
|
The same on Guardians of the Galaxy (PPSA01748), same tree as above (
Tested and written by Claude (AI assistant) on a contributor's machine. |
|
Stray (PPSA02100), the title from #6805, on current
The two crashes on
On the attach delay: AI-assisted: yes (analysis and text written with Claude Code; the runs and core dumps are from my machine). |
|
Windows and AMD, on a title with no warm cache at all: ProsperoLight (PPSA99002, catalog 01.000.100), the stop I reported in the catalog update ( Windows 11 (26300), Radeon 860M (RDNA 3.5, driver 32.0.13058.8001), WinLibs GCC 15.2.0, Release. Tree:
The update notice is for the current release, 01.000.110 (from the project's GitHub releases, SHA-256 checked). That release on the same tree with this PR, cold, 90 s: no crash, 8.0 FPS, and the dumped frames show its home screen ("Your PCs", "No PCs yet", tabs PCs, Games, Settings, About). AI-assisted: yes (Claude Code). |
What
Closes #6805. Device replacement now drains queued registration library prebuilds before clearing the old device's pipeline caches. Pipeline-library clearing also waits for in-flight library part builds, including the interval where
vkCreateGraphicsPipelinesruns without the library-store mutex, before erasing the per-device maps. A deterministic mock test blocks pipeline creation while cache clearing runs, then checks that clearing waits and releases all pipeline objects.Open #5925 and #6641 touch nearby lifecycle code; neither drains registration library prebuilds during device replacement.
Tested
agc_driver_graphics_testsbuilt successfully. This host lacks the PS5 CPU feature set, so I temporarily bypassed the test binary's startup CPU guard to run the mock-only test; the guard change was restored and is not in this PR.ctest --test-dir /tmp/build-6805 -R '^agc_pipeline_library_clear_waits_for_build$' --output-on-failure: passed, 1/1. The run used the same temporary local CPU-guard bypass. No GPU was used; the regression test uses mocked Vulkan calls.git diff --checkandpython3 tools/check_conventions.py --base upstream/main: passed.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)