Repository navigation
fix(agc): wait for the idle flush's unit-shadow publish before returning - #7984
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.
|
|
One integration detail from the overlap check: open #6642 adds the full recorder CTest entry and Windows environment list in |
|
Smoke runs on plain
So the wait is safe on main. These runs do not show the stall rate on plain main without the change, because I have no unpatched plain-main runs to compare against; the 3 in 4 stall figure in the description is from our test set. One related data point: a current-main-based run without this change (run226, base AI-assisted: yes |
|
Thanks for the plain-main smoke runs. Three runs reaching the level without a crash are useful safety evidence, and the caveat that there is no unpatched plain-main baseline keeps the comparison appropriately scoped. The PR description still says the current-main smoke run is queued; please replace that line with this result and its limitation so the summary reflects the latest evidence. |
|
Done, the queued line is replaced with the three plain-main runs and the no-baseline caveat. AI-assisted: yes |
2b78794 to
85e1772
Compare
85e1772 to
04b905c
Compare
04b905c to
ba56c43
Compare
The idle flush (VulkanDevice::StoreIdleResults) publishes the unit shadows over guest memory into the host import and only submits the copies. publishUnits picks the units to store on the CPU, and the slab -> import copy lands when the GPU runs it, so a guest store made between the two is overwritten with the older shadow bytes. The memory-access hook waits through SyncThrough right after its flush; the idle flush has no reader that waits. FlushPending and PublishShadowsOnly now sync the recorder under the GPU mutex after an idle publish that stored units. Other flush reasons are unchanged. Test: agc_idle_publish_wait (a CPU store made after the idle flush returns survives; a label flush's publish stays recorded).
ba56c43 to
db3af40
Compare
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The window it closes is clear (the idle flush records copies of a unit-shadow publish into the open batch and returns before the GPU runs them, so a guest store in between is overwritten), and the evidence is good: 1 of 4 runs loaded without the wait, 4 of 4 with it, at 12.34 presents per 10 s in both arms.
One point on where the wait happens. The PR syncs the recorder "under the GPU mutex they already hold", and notes that it left the hook path alone because there "the wait ran inside the lock". The same objection applies here: while the idle publish waits, every other queue that needs the GPU mutex waits with it. The largest waited publish was 1,034 units (64.6 MiB) in 8.8 ms, which is fine in the measured run, but the wait grows with the amount published (the New Game burst was 50-62 blocks, other titles will publish more), and the time is spent with the packet worker, the draw committer and any compute queue blocked.
- #6926 releases the GPU mutex for the wait when the acquisition is the outermost, as for any CPU access. If the idle flush can do the same (publish under the lock, release it, wait for the copies, and let the callers that need ordering take the lock again), the stall disappears from the other queues without changing what the flush guarantees. Please say why that is not possible here.
- Whichever way it ends up, add the waited publish time to the
[storage]line (units published, time waited, longest) so the cost is visible on titles other than Demon's Souls, and post it for the New Game burst. - The 12.34 against 12.34 pair is a single measuring pair on a base of
mainplus local patches. A repeat on plainmain, as the last bullet of the description says is missing, would also show that the wait does not cost frame time where there is no stall to remove.
|
Checked that the wait is scoped to the "idle" reason and skipped inside a completion, that it follows the existing sync-under- |
What
The idle flush now waits for its unit-shadow publish before it returns.
VulkanDevice::StoreIdleResultsrunsStorageTexture::FlushAllPending("idle")and thenSubmitRecorded, which does not wait. The flush ends inPublishShadow, or inPublishShadowsOnlywhen no image is pending. Both go throughpublishUnits, which decides on the CPU which units to store, and thenrecordCopies, which records the slab to importvkCmdCopyBufferinto the open batch. The bytes reach guest memory only when the GPU runs that copy. A guest store made between the decision and the copy is overwritten with the older shadow bytes, and since no reader goes through the flush hook at that point, nothing waits for the pending write.With this change,
FlushPendingandPublishShadowsOnlysync the recorder under the GPU mutex they already hold, after an idle publish that stored at least one unit. Other flush reasons are unchanged.The memory-access hook already waits right after its flush (
FlushForAccesscallsSyncThroughbefore the accessing thread goes on), so this PR leaves it alone. Our Demon's Souls runs below waited in both places. On the hook path the only difference was that the wait ran inside the lock, so I kept it out of this change.Why
Demon's Souls never starts the level load after New Game in 3 of 4 runs on our test set (#7983). Each stalled run had one large idle or hook publish at New Game over memory the title was writing. With the publish waited, 4 of 4 runs loaded, at the same frame rate.
Related, not overlapping:
StorageTexture::writeBackLayers. It does not touchUnitShadow.cppor the publish, and this PR does not touchwriteBackLayers. They should merge in either order.CMakeLists.txt) and perf(agc): remember structural host import misses #7457 (import-miss tests inCMakeLists.txtandRecorder.cpp) add test registrations next to this PR'sagc_idle_publish_waitentry. No behavior overlap, but whichever lands second needs both sets of entries kept when the lists are merged.Tested
agc_idle_publish_wait(tests/Recorder.cpp): two units are retiled into a unit shadow,FlushAllPending("idle")runs, then the CPU stores into both units and the recorder syncs. The stores have to survive, and a"label"flush's publish has to stay recorded. Without theTexture.cppchange it fails withthe idle flush returned before its publish's copies completed; with it, it passes 5 of 5 repeats. Both results hold on lavapipe and on the RTX 5070 Ti, on this branch at main dbac9fa.agc_unit_shadow,agc_driver_unit_shadow,agc_driver_unit_shadow_budget,agc_shadowed_key_stores,agc_pending_key_stores,agc_pending_write_snapshot,agc_storage_*,agc_space_pending_writes): pass on the RTX 5070 Ti; on lavapipe the four unit-shadow ones skip as before (no watched imports) and the rest pass.tools/check_conventions.py --base origin/mainpasses.ImportWatchedpage-digest fix, KytyPS5's job-worker idle sleep (on in every run), and a diagnostic that logs CPU stores into blocks a GPU-timeline store just covered. Without a wait, 1 of 4 runs loaded the level. In the 3 stalled runs, one publish at New Game stored 67 to 73 64 KiB blocks that the CPU wrote afterwards. With the idle and hook publishes waited, 4 of 4 loaded. The New Game burst (50 to 62 blocks) was still there, and the largest waited publish was 1034 units (64.6 MiB) in 8.8 ms. A measuring pair gave 12.34 presents per 10 s with the wait and 12.34 without.maindbac9fa with only this branch, no local patches: 3 of 3 runs started the level load after New Game with no crash (one with a cold shader cache, one reached the level). I have no unpatched plain-main runs to compare stall rates against, so this shows the change is safe on main, not that it removes a stall there.Fixes #7983
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt; this change adds none.mdfiles or images added to the repository (attach them to this PR)AI-assisted: yes