Repository navigation
Conversation
…surface that is no longer fully write-watched markLayersPending saved the guest bytes of a newly pending unit only when it had no write generation. A unit that was last stored or uploaded while its memory was watched keeps a generation, so when a block of the surface left the write watch while the unit was idle (a flush for the access that caused the unwatch had just stored it, so the unwatch hook found it not pending) and the unit became pending again afterwards, it had neither the watch nor saved bytes to tell the CPU's writes from its results. Its write-back then refused: storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented. One such unit makes the whole span untracked, so every unit of the write-back needs its bytes. A unit without saved bytes and without a save on its way now takes its guest bytes when it becomes pending over a surface that is not fully watched, whatever its generation. The blocks the CPU or the driver wrote since the unit's generation are still told from the stamps while the unit's own blocks are watched and keep their bytes at the store, exactly as when the hook saves a pending unit (keepWrittenSince, shared with saveBeforeUnwatch); their writes are collected unit by unit, since a collect over a range holding an unwatched block does nothing. A unit whose own block is unwatched gets its bytes alone.
… is write-watched A write-back routes each kept range that is wholly watched into the unit shadow's slab, and marks the pieces shadowed at the generation its collect over the whole stored span returns. When a block of the span has left the watch that collect returns 0, MarkShadowed ignores a generation of 0, and the pieces stay in the slab without a record: never published, their results lost. Over a span with an unwatched block every piece now goes to the import, as the saved bytes of the store already are decided by the span.
…urface left the write watch A storage image over write-watched memory is uploaded, so its units have a write generation and no saved guest bytes. A block of the surface then leaves the write watch while nothing is pending, which the unwatch hook does not see, and the image is marked written again without a lookup (as the colour mirror of a depth plane is after each dispatch). The write-back cannot tell the CPU's writes from the results without saved bytes and refuses. agc_storage_pending_after_unwatch runs that flow with no CPU write, with the CPU writing a block of the surface after the results became pending again and with it writing before; the store has to succeed, keep the CPU's block and store the rest, the block that left the watch included. Where the image's host import takes the surface out of the write watch (a Windows runner), the test reports that it was not run.
|
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.
|
|
Hardware run on Windows, where the description's runner reported the new test as not run: here it runs, since on this AMD driver host imports of write-watched memory keep the watch ( Windows 11 (26300), Radeon 860M (RDNA 3.5, driver 32.0.13058.8001), WinLibs GCC 15.2.0, Release.
AI-assisted: yes (Claude Code). |
|
Windows run on real GPUs: i7-13700K, RTX 4080 (617.42) and Intel UHD 770 (101.6129), Windows 11, WinLibs GCC 15.2.0, Release,
AI-assisted: yes (Claude Code) |
…ay differ from its upload A unit uploaded while watched has no saved bytes. Once its block left the write watch the CPU's writes there are unstamped, and capturing the current bytes as the baseline when the results turn pending without a lookup stored the stale upload content over them for every texel the GPU did not write. Capture only when the unit is still watched or equals the bytes it was uploaded from; otherwise keep the refusal for that unit.
|
In-game regression check on Linux, RX 7600 (RADV NAVI33, Mesa 25.2.8), Ubuntu 24.04, GCC 13.3 Release: Epic Mickey: Rebrushed (PPSA15657, UE 4.27), head fe1fa2f, six short runs to the first level, no environment switches. All six reached the level and played to the end of the run: no driver throw, device loss or title fatal error. This title no longer reaches the throw this PR fixes. It hit it on Windows (RTX 4080) until #5671, and it never did on Linux. So this run only shows that nothing regressed in a UE4 title that keeps storage images pending over its own memory. AI-assisted: yes (Claude Code) |
|
@AlxcBTW thanks for 695f8b9. Refusing pending results over an unwatched unit closes a real silent-corruption path (stale upload bytes over unwatched CPU writes), and it fails closed, which is the right direction. One gap: So before this merges, could you do a PPSA16064 run on 695f8b9 and post whether the rows that used to end at |
|
@oneandonlydean good catch, thanks. The shadow-atlas mirror that motivated this is 32 MiB, so it is exactly the case above the snapshot limit. Two things are underway:
I'll post the rows and the commit here when both are done. |
… its upload bytes Surfaces above 1 MiB keep no byte snapshot of their upload, so an unwatched unit with no saved bytes fell back to the unsaved-outside-watch refusal even when the CPU never wrote there. Keep a 128-bit hash of each unit's bytes taken when the unit was uploaded and compare it at markLayersPending: equal means the current bytes are the upload and are saved as the baseline, a different hash or none keeps the refusal. Units whose content came from elsewhere (borrowed from an alias, cleared, stored by a write-back) drop their hash. The pending-after-unwatch test also runs a 1024x1024 surface: an unwatched unit the CPU did not write is stored, one it wrote is refused with its bytes intact.
|
@oneandonlydean the PPSA16064 rows you asked for. Six in-world gameplay walks (~180 s each, RTX 5070 Laptop, Windows 11), on a lab stack (our best10 = current
So on this title every unit that needed a baseline was still watched when it became pending. 695f8b9 alone does not bring the terminate back here, and the large-surface branch you pointed at is never reached. The hash commit is still the right behaviour for a title where it is reached; on this one it changes nothing. In every row 6-9k write-backs went over a span with an unwatched block and needed no saved bytes. Without this PR that is the situation where the refusal used to end ~5 % of our rows (7 rows over 15.-17.10.). Counter branch: fork |
|
Astro Bot (PPSA21564, content 01.018.000) on Windows / RTX 5060 Ti: reaches the intro cutscene with #7048, 3.5–20 fps With Environment
Build
Input preparation
What happensv0.1.2 release build (relinker + prx-windows v0.1.2): the engine starts, picks the 5060 Ti, creates the swapchain, then aborts about 8 s after launch: It reproduces on every run.
Performance (shader cache warm after the first pass)
First run: about 1,750 shader-cache misses and a shader cache of about 1.3 GB. Later windows still show new misses when new content appears. Excerpts from a window in the world map / intro ( Host observations while it ran at 3.5 fps: GPU utilization 89–99% at P0 and 2782 MHz, but only about 57 W (the card is rated around 180 W); about 1.9 CPU threads busy of 28; disk about 0% busy (NVMe); 28 GiB private memory, 22 GiB resident. So the GPU looks busy but is mostly stalled on serialized work. Nearly all of the sync wait is on pending GPU writes. Not tested
Side noteThe web converter fails with AI-assisted: yes (Claude Code did the builds, runs and log analysis; I ran the game and checked the screen). |
|
Thanks for this. We ran it against two Astro Bot game runs on an RTX 4090 (head 2dd5117, merged onto main 52798f7, each 450 s against two main runs on the same build). Rendering and checkpoints are fine and Dreaming Sarah passes. But Astro Bot is clearly slower, and both runs show the same pattern:
CPU load and clocks match main and GPU load is lower, so the GPU is waiting on the CPU. That fits the one cost the PR adds: every settled re-upload of a surface larger than 1 MiB now does an extra full read and hash pass over its guest bytes (the generation baseline is only kept for surfaces of 1 MiB or less). Full-screen targets are well over 1 MiB, so that pass runs on most frames. Could you avoid the extra pass for large surfaces? For example, compare only the blocks that hold an unsaved unit, or keep a per-block baseline above 1 MiB, rather than reading and hashing the whole surface. We'll run Astro again on the new head. |
|
NHL 26 (PPSA26785, 01.060.000) on NVIDIA Ada, where this refusal was the title's next stop after #7346 (my #7346 comment: frameIndex 423, Setup: RTX 4090 (NVIDIA 616.92, Vulkan 1.4.341), Windows 11 26200, Ryzen 7 5800X, MinGW-w64 GCC 15.2.0 r7, Release
So on this GPU the PR does for NHL 26 what it did for PPSA16064: the refusal that ended every warm run on Two notes for the cost discussion above:
Logs in the details blocks (stderr trimmed to the driver lines; per-shader registration notes, missing-file opens and the 10 s counters removed), full logs on request. run 3, warm cache, title screen until the cap: stderr (14 of 673 lines kept)run 2, partly cold cache, no stop: stderr (14 of 674 lines kept)run 3, the title's one breadcrumb report (stdout)AI-assisted: yes (Claude Code). |
github-actions
Bot
added
the
unblocks title
|
@oneandonlydean the open points, all in
Evidence: no local build. Our fork's CI built and tested the same diff on our main (anyplaynet#48): the Windows job passed, https://github.com/anyplaynet/AnyPS5/actions/runs/40720659323 (full build, every registered ctest). |
|
@AlxcBTW The current composition includes #10026’s lazy unwatch hashes and its byte-based tests; the earlier eager-upload hash path is gone. The existing performance hold still needs matched main/current-head measurements before landing: PPSA21564 intro/title/menu, with the previously routed PPSA10872 logo and PPSA01825 menu evidence, plus agc_storage_pending_after_unwatch at both sizes and full CI. Please bind the runs to 730c721 and record the same machine, scene, duration, settings and warm-cache conditions for both arms. The old eager-head slowdown reports do not establish this head’s performance. #7048 and #10026 currently produce the same merged tree, so please coordinate one landing composition. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 730c721773cd3d12422b715c725a2678122e85f7. The current-main merge preview is clean, but the landing/performance blockers remain on this composition:
- This head and open #10026 produce the same current-main merged tree. Please record which PR is the landing vehicle and how the duplicate branch will be handled, with the matching checklist/order updated; the current checklist still says
Depends on: noneand does not identify #10026. - The material slowdown previously measured for #7048 alone is not cleared by the new code composition. The latest review asks for matched current-head/main measurements on PPSA21564 intro/title/menu, the routed PPSA10872 logo and PPSA01825 menu, plus
agc_storage_pending_after_unwatchat both sizes, with full CI. The old eager-head slowdown and fork CI report do not establish performance for head730c721773.
I do not have a new source-level correctness finding. The upstream Windows workflow passes, while product Linux and GPU jobs are skipped; I did not run a local GPU build or capture. Please post exact-head results and clarify the single landing composition before requesting another review.
✅ NVIDIA GPU test: passedPR head
Failing on main too (5, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 436 tests)Skipped (39)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. |
|
Follow-up to my earlier comment: with this PR's current head
On 2026-10-26 a local merge of the older head The merge onto In each freeze the audio keeps playing but presents stop, nothing is logged, and Own base ( On It looks like a lock-order inversion. The main thread waits for the GPU mutex inside the write-watch flush hook, while the queue worker waits for a lock inside The full dumps are available if they'd help. AI-assisted: yes (Claude Code) |
|
Thanks for the table and the thread stacks, @XatiX. They're enough to work from. They fit a lock-order cycle between the write-watch tracker lock and the GPU mutex:
The unwatch hook ( We're mapping the lock order from the code and writing a test that reproduces the cycle without the game. If it confirms, we'll offer the fix as commits that @AlxcBTW and @winstrex can take, and ask you then for a Mewgenics (PPSA37318) rerun. |
|
@XatiX @EagleBearerYS I reproduced the deadlock with a test that fails 3 of 3 without the game. It comes from our #10026 commits: ec6ceec makes the unwatch hook take the GPU mutex whenever a live storage image larger than 1 MiB overlaps the range, and f27fcde extends that to the unproven case. The lock that closes the cycle is The fix and the test are on their way as commits for @AlxcBTW to take, so there's no need to duplicate the work. |
|
@winstrex @EagleBearerYS thanks. A RelWithDebInfo build of main
Before your commits arrived I'd tried a narrower stopgap. It's at XatiX/AnyPS5
With the stopgap, Mewgenics plays through the title menu, the save load, the battle tutorial ( I'll rerun Mewgenics on your fix once it's on this PR. AI-assisted: yes (Claude Code) |
|
@XatiX @EagleBearerYS @AlxcBTW the fix is 53c3cd4ee on #10026's branch. Cause, as XatiX's read of the GPU mutex owner shows: the map-time import ( Fix: one predicate, Test: Cost: a temporary counter (not in the commit) counted the map-time imports the new guard defers. The Pathless (PPSA01825) up to its menu made 2,329 map imports and Hades (PPSA03355) in gameplay 3; the guard deferred none of them. Both ran without a freeze on #7048's head with the fix, as The Pathless also did without it. @AlxcBTW could you take 53c3cd4 into #7048? On this head it conflicts in one line of |
|
@XatiX thanks for the test and the stopgap. I ran c41c1e3 against our fix: it fails with "the unwatch hook waited for the GPU mutex", as expected. It measures your stopgap's contract (the hook never waits for the GPU mutex), not ours (the hook may wait for it when its caller holds no lock that the mutex's owner can need). In the test, the thread that holds the GPU mutex waits for the unwatching thread directly, which the driver doesn't do. The real cycle needs I also checked indirect waits (memory waits, joins, idle waits). The only path that reaches the hook without the GPU mutex is the map-time import's own thread. The guest thread that joins it holds neither the GPU mutex nor the allocation registry or direct-memory lock at that point. Memory waits poll without the GPU mutex, and To catch a regression of this kind, 8b14bde3c adds a check: the host imports mutex notes when it's held, and the unwatch hook aborts if it would take the GPU mutex while its thread holds that mutex without it. It's always on in builds without |
|
An AMD RDNA 4 run on Windows of Windows 11 Pro 10.0.22631, AMD Radeon RX 9070 XT (
AI-assisted: yes (Claude Code) |
|
Two Windows runs of this PR in titles that stop on its message. RTX 4070 SUPER, NVIDIA 616.92, Release. The base is
const auto destination = guestBytes <= SmallStoreBytes || !spanWatched ? std::optional<ShadowDestination>{} : ShadowDestinationFor(context, import, guestBegin, pieceEnd);The
Limits:
AI-assisted: yes (Claude Code) |
|
A third title for the runs above, and this one still stops with the PR: OCTOPATH TRAVELER II (PPSA09809), same machine and same Without the PR it stopped at 602 s on the original message, with a person playing into the first story scene. With the PR it stops at 465 s, scripted input, on the case your message now names: So the path that compares the saved bytes refuses here. One run with the PR, and the input differs between the two runs, so I can't say the stop comes at the same point in the game. I still have this run's full log if a specific part of it helps. Hello Neighbor 2 and Scorn did not hit this in their runs above. AI-assisted: yes (Claude Code) |
What
StorageTexture::markLayersPendingsaved the guest bytes of a unit that becomes pending only when the unit had no write generation. A unit last stored or uploaded while its memory was watched keeps a generation. When a block of the surface leaves the write watch while that unit is idle (the flush for the very access that caused the unwatch has just stored it, so the unwatch hook finds nothing pending) and the unit becomes pending again afterwards, it has neither the watch nor saved bytes to tell the CPU's writes from its own results. Its write-back then refuses: "storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented". One such unit makes the whole stored span untracked (ChangedBlocksis all-or-nothing), so every unit of that write-back needs its bytes. The colour mirror of a depth plane is marked pending after every clear dispatch without a lookup (ShaderResources::MarkGpuWrites), which is how an idle unit gets here.fix(agc): save the guest bytes of results that become pending ...: a unit without saved bytes and without a save on its way takes its guest bytes when it becomes pending over a surface that is not fully watched, whatever its generation. The blocks the CPU or the driver wrote since the unit's generation are still told from the stamps while the unit's own blocks are watched, and keep their bytes at the store, exactly as when the unwatch hook saves a pending unit (keepWrittenSince, shared withsaveBeforeUnwatch). Their writes are collected unit by unit, because a collect over a range that holds an unwatched block does nothing. A unit whose own block is unwatched gets its bytes alone.fix(agc): retile into the unit shadow only when the whole stored span is write-watched: with 1. the write-back goes through, and a second loss shows.writeBackWindowsroutes each wholly watched kept range into the unit shadow's slab and marks the pieces shadowed at the generation of a collect over the whole stored span. Over a span with an unwatched block that collect returns 0,MarkShadowedignores generation 0, and the pieces stay in the slab without a record and are never published. Over such a span every piece now goes to the import, since the span already decides the saved bytes of the store.test(agc): store results that become pending after ...:agc_storage_pending_after_unwatch.Review follow-up,
fix(agc): refuse pending results over an unwatched unit whose bytes may differ from its upload(695f8b9): the capture in 1. took the current guest bytes as the baseline whatever had happened since the unit's upload. If a block had left the watch and the CPU wrote it after the upload, that write cannot be seen: the image holds the upload's content for texels the GPU does not write, and storing the unit would silently overwrite the CPU's bytes there. The capture now trusts the current guest bytes only while the unit is still write-watched or still equals the bytes it was uploaded from (StorageTexture::guestMatchesUploadagainst the upload snapshot). Otherwise the unit keeps the previous refusal. That covers surfaces above the 1 MiB snapshot limit, and an earlier write-back that made the snapshot stale. Units with a stamped CPU write still go through the kept-block path. The same staleness exists on the older capture for units without a generation, which this PR does not change; I can follow up on it separately.Review follow-up,
fix(agc): compare an unwatched unit of a large surface with a hash of its upload bytes(5cf43e5): the upload snapshot behind 4. exists only for surfaces up to 1 MiB, so on a larger surface (the 32 MiB shadow-atlas mirror that motivated this PR) an unwatched unit went back to the old refusal. Each unit of a larger surface now keeps a 128-bit hash of its upload bytes (ShaderRecompiler::HashByteswith two seeds over 64 KiB chunks, 16 bytes per unit). The hash is taken where the snapshot is, and dropped wherever the GPU content stops being the upload's (alias borrow, fast clear, a write-back of the unit). An unwatched unit whose current bytes hash to the same value takes them as the baseline. A different or missing hash keeps the refusal. Exactness assumes no collision of the 128-bit non-cryptographic hash between two 64 KiB contents of guest memory. The next commit (2dd5117) hashes through a local buffer instead of athread_localvector, so the library registers no thread_local destructor.Review follow-up, the upload-path hash: 5cf43e5 and 2dd5117 hashed every settled upload above 1 MiB, which cost 45 to 80 % of the presents on Astro Bot, PPSA02100 and The Pathless. The hash is now taken in the unwatch hook, while the range is still watched, only for an idle unit with a generation, no pending results, no saved bytes and no CPU write since that generation;
upload()andrefreshhash nothing. This is fix(agc): hash an idle unit when its range leaves the write watch instead of at every upload #10026 by winstrex (ec6ceece9,f27fcde50,d73e64951,43a9245e8,cfbe00cb4,660cef6fe), merged into this branch unchanged so the two land together. It also covers bothImportWatchedexits that skip the hook, names the cause when a unit has no baseline, and drops the unconditionalRequire(refused, ...)for the largeUnwatchedPartialWritecase intests/Recorder.cpp.Conflict with
maininTexture.cpp(markLayersPendingandsaveBeforeUnwatch): merge730c72177ofmainf8f5b24.keepWrittenSincetakes the generation of each 64 KiB block fromunitBlockGeneration(main's per-block completion stamps), so a blockmainstamped at a completion is compared with that stamp and not with the unit's generation, exactly assaveBeforeUnwatchdoes onmainnow.After the merge of
main(base 0c6a5bd, merge commitad54ca800) the build broke on a secondGenerationBaselineBytesconstant inTexture.cppthat the merge left next to the onemainnow has;da9f0ead4(fix(agc): drop the duplicate generation baseline constant left by the merge) removes the two duplicate lines and nothing else.Both fixes only change what happens for a surface that is not fully write-watched; a fully watched surface takes the old path in both places.
Why it is correct: the refusal exists because the write-back needs a baseline to tell the CPU's writes from the GPU's results. The unit now has that baseline from the moment it can no longer get one from the watch, taken at the same point and with the same merge of stamped blocks that the unwatch hook uses for a unit that is pending when the block leaves.
Tested
agc_storage_*,agc_unit_shadow*,agc_driver_*,agc_recorder*,agc_import_watch*,agc_pending*and related tests pass, the new one included. Windows Server runner, Mesa 26.2.4 lavapipe for Windows, fork CI: the 7 storage write-back tests pass, the new one included (it ran there; the surface stayed watched).agc_storage_pending_after_unwatch(tests/Recorder.cpp, also called from the fullagc_driver_recorder_testssequence). An image over watched memory is uploaded (units with a generation, no saved bytes), the last of its 4 blocks leaves the write watch while nothing is pending, the image is cleared on the GPU and marked written again without a lookup, then the store runs three ways: with no CPU write, with a CPU write to block 2 after the results were pending again, and with that write before. The store must succeed, keep the CPU's page and hold the results in every other block, the unwatched one included. Without 1. it fails with the refusal above; with 1. alone it fails on the second defect (units 0 and 1 not stored). Where the image's host import takes the surface out of the write watch (a Windows runner) the test reports that it was not run.std::terminateon the driver failure (in the three I checked, after the world had loaded), among them one on plainmain(636b1b9) and one onmainof 17.10. with a lab switch on. In one row it came from 29 of the 512 units of the 4096x4096 R16 colour mirror of a D16 shadow atlas, each with one write generation and no saved bytes. On our lab stacks that carry the first two commits, 27 rows reached gameplay without it; game rows with the follow-up commit are still to come. Without the fix the rate is a few percent of rows, so these rows are consistent with the fix but do not prove it by themselves; the test reproduces the flow deterministically.main, fork CI on Linux (Ubuntu 24.04, Mesa lavapipe), at the headda9f0ead4: the run at the merge commitad54ca800(run 39677534763, regexagc_|Texture|Storage|Recorder) did not build (the duplicateGenerationBaselineBytes, fixed byda9f0ead4). The run atda9f0ead4(run 39678944715, same regex, plus the CI scaffolding commit): 876 of 877 passed, among themagc_storage_pending_after_unwatch;agc_driver_sgpr_pattern_fill_keysfailed with "the dispatch completed 17 times without a pending write over the keys". That test (tests/execution/SgprPatternFillKeys.cpp) is not touched by this PR; it submits a compute dispatch over a DCC key buffer and polls for the recorder's pending write note, and fails when the dispatch has retired 16 times without the poll seeing it, which is a timing condition of the test. A rerun ofda9f0ead4withsgpr_pattern_fill_keys|storage_pending(run 39683919241): 2 of 2 passed. The test did not fail in any other run I have: it passed in 2 runs ofmain(a6dcea9; runs 39688450938 and 39693413422) and in 9 runs of perf(agc): answer Refresh's registry and host-import lookups from an index and a cache #7514's and perf(agc): clear the mirror of a depth plane instead of dispatching the UAV clear over it #7449's heads (39677504672, 39683281454, 39677622121, 39688330136, 39688369669, 39688423114, 39693268189, 39693318038, 39693362034). So one failure in 13 runs that held the test, none of the 2 onmain; it is not shown to be flaky onmainand not shown to depend on this PR.python3 tools/check_conventions.py --base upstream/mainpasses on730c72177.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)Neighbours: #6490 (parl0v) also changes
writeBackWindowsand #6960 adds a line next to the changed one inmarkLayersPending; neither implements this, and #6308 runs the unwatch hook for a different exit (an import that leaves the watch). Whichever lands second rebases.