Repository navigation
perf(agc): reuse draw snapshots across draws that nothing wrote between - #5906
Conversation
|
Tested on Sifu (PPSA03001, Unreal Engine 4, #5908): this PR gets it past its loading screen. Windows 11 (build 26300), NVIDIA GeForce RTX 5090 (610.88), MinGW GCC 15.2.0. main 8dfaafe with and without this PR. Without it,
The title screen runs at about 0.5 FPS; the remaining per-draw cost is elsewhere, and I'm following it up separately. |
CnRJay
left a comment
There was a problem hiding this comment.
The Sifu numbers (PrepareDrawBindings from about 480 µs to about 12 µs per draw) and the verify run (1.8 M reused snapshots, 0 differing) make a strong case. I checked the interaction with GPU writes on main: an allocation whose range has pending GPU writes is skipped before the snapshot code (if (!direct || recorder.PendingWriteOverlaps(item.address, item.size)) continue;). The new UnwrittenDrawSnapshot lookup sits after that, so a buffer that a dispatch in the same submission writes still binds in place and is never served a stale copy.
One gap, in the moved-words ring:
const auto held = std::find_if(movedWordBuffers.begin(), movedWordBuffers.end(), [&](const MovedWordBuffer& entry) { return entry.allocation == index && entry.size == override->size && entry.words == override->words; });The buffer built on a miss is override->words plus every dataPatches entry for that allocation (buffer->Bytes()[patch.byte] = patch.adjustment). The key compares only the allocation index, size and words, not the patches. Two draws with the same captured words and different adjustments would share the first draw's patched bytes. If the adjustments follow from the words alone, saying so (or asserting it under APS5_VERIFY_DRAW_SNAPSHOTS) settles it. Otherwise the patches, or a hash of them, belong in MovedWordBuffer's key. This path isn't covered by the verify switch, which only checks the UnwrittenDrawSnapshot answers.
Two small points:
- The map is keyed by address and checks the size on lookup, so a range bound at one address with two sizes in alternating draws evicts itself on every draw. That costs speed, not correctness, so it's fine as is. Keying by
(address, bytes)likedrawSnapshotswould avoid it. - The Tested section has the RetroArch run as presents and audio rate. Adding the
aps5-summary.jsonllines ofmainand of this change (the run already usedAPS5_TRACE_SECONDS) would put it in the Performance report form.
adriadam10
left a comment
There was a problem hiding this comment.
Checked on Linux (build + the 546 libSceAgcDriver tests, agc_draw_snapshot_epoch passes and fails without the change). The stamp logic and the moved-words ring look right; one thing to fix:
Recorder::unwrittenSnapshots(Recorder.cpp:2525) keeps ashared_ptr<Buffer>per address up to 4,096 entries with no byte limit, and an entry whose stamp no longer matches is never dropped until the same address is stored again. Each buffer can be up to the 64 MiB snapshot cap, so this holds device memory outside the 1 GiBDrawSnapshotBudgetthat boundsdrawSnapshots. Please bound it by bytes (reuse thedrawSnapshotsbudget/recency, or drop entries whose stamp is stale on lookup) and cover the limit in the test.
On @CnRJay's dataPatches question: they are a member of ShaderResources set at construction, so the same words always get the same patches within one object; the ring needs no extra key.
The conflict with main is only the #1441 line in docs/dev/debt/libSceAgcDriver.md; we can rebase it once the above is done.
…hat nothing wrote between Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… again after a store Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… snapshot budget Co-Authored-By: Claude <noreply@anthropic.com>
0976e1d to
b0001cb
Compare
|
Rebased onto current
Linux: the full |
|
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.
|
adriadam10
left a comment
There was a problem hiding this comment.
The unbounded unwrittenSnapshots is fixed in b0001cb: retired on a stamp change and capped in bytes and entries, with a test. Windows build green (run 38635781892).
|
Besiege (PPSA20313), |
|
The full no-argument
Same failure 3 of 3 on NVIDIA and RADV at b9d9ec6. The test memsets the element to 0x22 between the two In a title I do not see it: Astro's Playroom on 6870cc2 with |
|
On the question above (test block outside the stamp, or the test needs updating): on lavapipe (main eea428c, default unoptimized CMake build), with the (9) hunk of 6bd076c applied, adding AI-assisted: yes |
…hat nothing wrote between (PR boykopovar#5906)
What
On Windows a draw copies every guest buffer it reads in place into a new snapshot, whether or not the bytes changed since the last draw. Host-pointer imports of the guest mapping are compared instead of write-watched there (#1441), so
CollectWritesreturns 0 for them,KeepDrawSnapshotnever keeps the snapshot, and the next draw copies the range again. The debt entry for #1441 already notes that this disables snapshot reuse on excluded blocks.In RetroArch's Flycast core (PPSA99169) on Windows, that is a 2 MiB vertex buffer copied for nearly every draw: about 12,500 copies and 16 GB a second in the Crazy Taxi attract demo. A local measurement found 99.95% of those copies byte-identical to the previous copy of the same range.
PrepareDrawBindingsstamps a draw with the write tracker's generation, the collect epoch and the guest allocation registry's generation. The last snapshot of each address is kept with its stamp, and a draw with the same stamp and size reuses it.docs/dev/ARCHITECTURE.mdalready gives unwatched memory and the collect memo gives watched memory. A store into watched memory moves the tracker generation and ends reuse at once.ReusableDrawSnapshot, then a new copy).APS5_VERIFY_DRAW_SNAPSHOTS=1checks every reused snapshot against the slow path, or against guest memory where the slow path keeps nothing, and reports differences.No runtime switch turns it off; with the verify switch a difference is reported, not corrected.
Tested
Windows 11, NVIDIA GeForce RTX 5070 Ti, MinGW-w64 GCC 15.2.0, on main
63fb0cd92.ctest: 1168 of 1185 pass, 15 skipped.guest_call_trace_abortandwindows_backtrace_reportfail, and fail the same way on plain main here.agc_driver_prepared_graphics_prebuild_0/_1andundeliverable_host_exceptionfail under my test runner because it skips their environment changes and pass pattern; run with those, they pass.agc_draw_snapshot_epoch(agc_driver_recorder_tests --draw-snapshot-epoch-only), next todrawSnapshotPatchTestsand on the same host-imported guest block, but run where imports are compared as well: a second draw in one collect epoch gets the same snapshot; after a store and a new epoch the next draw gets a new snapshot with the new bytes, and the draw after it reuses that one. Here imports are compared. With the epoch left out of the stamp, the test fails with "a draw snapshot does not hold the guest bytes of its draw".ANYPS5_PRESENT_MODE=immediate, measured on maina14ae8e23plus local changes outside this PR that the frontend needs to boot, unprofiled,APS5_TRACE_SECONDS=60:30: 202 presents in 30 s and audio at 5.8% of real time without this change, against 928 and 934 presents and 26.1% and 27.0% with it.APS5_VERIFY_DRAW_SNAPSHOTS=1: 1,800,000 reused snapshots checked against guest memory, 0 differing.Related: #5609 contains this change as one of its commits; with this PR it depends on it instead. #5542 caches read-only snapshot copies of address-based builds by comparing bytes on each use; this PR covers draw inputs and compares nothing. #5900 (draft) moves watched vertex and index ranges from in-place binding to reused snapshot copies in
InPlaceDrawInput; this PR is about the storage-buffer snapshots ofPrepareDrawBindingson compared imports, a different path, and touches neitherDraw.cppnor the in-place rules.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code