Repository navigation
perf(agc): drop the guest byte snapshot of a large cached texture the write tracker covers - #6982
Conversation
|
AMD / Windows run of this PR at 12e687d: Radeon 780M (RDNA 3, 0x15bf), AMD driver 25.10.20.01 (32.0.21020.1007), Windows 11 Home 10.0.26100, WinLibs GCC 15.2.0 r7, Release. Every ctest entry built from the test targets this PR changes passes (4):
AI-assisted: yes (runs made on the device above). |
… write tracker covers Every snapshot texture in the sampled texture cache kept a host copy of its guest bytes, used only when the write tracker reports the surface written, to tell a rewrite of the same bytes from a change. The cache budget counts the GPU allocation alone, so the host copies grew with it: PPSA05684 held 2.7 GB of them in its first level. A snapshot larger than 256 KiB is released after the upload when the write tracker (a write generation or an untracked key) can prove the surface unchanged. A tracked write to such a surface re-uploads it instead of comparing bytes; smaller snapshots, and surfaces the tracker cannot vouch for, keep their copy as before. agc_driver_large_tracked_snapshot samples a 1 MiB surface, committed and partially resident: an unchanged surface is reused without reading guest memory, a CPU write is uploaded, and a driver store of the same bytes is uploaded again because no snapshot is left to compare. AI-assisted: yes
12e687d to
6849eea
Compare
|
Title numbers for this change, Linux, RTX 5080 (NVIDIA 615.78.08). Both sides are
App VRAM is 189 MiB higher on Playroom and equal on Astro Bot. A Playroom burst of every 6th present shows the same picture on both sides. The Astro Bot frame difference is within run variance; the Playroom one comes from a single round. Demon's Souls gave no comparison:
One note on the test: it shows the release only through its cost, the re-upload after a same-bytes store. Nothing checks that the snapshot memory is actually freed for a tracked surface over 256 KiB, and the threshold itself has no measurement behind it in the PR. |
|
Thanks @fma-rc for the title runs. On the test: the same-bytes driver store is the observable for the release. On the threshold: you're right, 256 KiB has no measurement behind it. It keeps the byte compare for small surfaces, where the copy costs little and a same-bytes rewrite is cheap to catch. The PPSA05684 figure (2.7 GB) came from surfaces far above it. I have no data that favours 256 KiB over another cut. AI-assisted: yes |
github-actions
Bot
added
perf
|
Traces of the two captures in the description, and
Both open in ui.perfetto.dev. Without the change ( Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): Driver.Packet 127.8, Driver.WorkerWait 106.1, Driver.WorkerWait.dequeue_order 105.9, batch 81.1, Driver.Packet.memory_wait 65.7, Driver.Packet.dispatch 42.8, Driver.Dispatch 27.7, Driver.Packet.prepare_memory 10.6, Driver.Dispatch.materialize 8.9, Driver.Dispatch.capture_resources 8.9 Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
With the change ( Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): Driver.Packet 128.5, Driver.WorkerWait 107.2, Driver.WorkerWait.dequeue_order 106.9, batch 81.3, Driver.Packet.memory_wait 65.9, Driver.Packet.dispatch 43.8, Driver.Dispatch 28.4, Driver.Packet.prepare_memory 10.3, Driver.Dispatch.materialize 9.2, Driver.Dispatch.capture_resources 9.1 Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
AI-assisted: yes |
|
@XairRrR thanks, the fallback paths all re-upload and fail closed, so the change looks correct to me. One question before merge: in your own capture |
|
Tested on Demon's Souls (PPSA01342 01.004.000), Linux 7.2.11, RX 6900 XT (Mesa 26.2.5 RADV), i7-13700K, 32 GiB. Base
With this PR the level loads (HUD, Outpost Passage) and the heap there is 7566 MiB; the largest holders are now shader results ( AI-assisted: yes |
|
@oneandonlydean I measured it, and the Same machine and scene as the captures in the description (PPSA05684 main menu, my integration build on
So I have not added the keep-after-rewrite rule. Here it would never trigger, since nothing is rewritten. I did try it: keeping the snapshot whenever a lookup replaces a stale entry is a two-line change, and it gives the same 31-33 ms. I can add it if you'd still like it as a guard for titles that rewrite large textures with identical bytes. @Mouhand-Kaddo thank you for the Demon's Souls numbers (live heap 10085 → 5585 MiB, and the level loads on 32 GiB). AI-assisted: yes |
|
@XairRrR thanks, that settles the 33 → 47 ms: your A-again run matches B, so it was the first-run No need to add the keep-after-rewrite rule now. One note on the counter: |
5 participants
What
Each snapshot texture in the sampled texture cache (
cachedTextureinShaderResources.cpp) keeps a host copy of its guest bytes. The copy is used only when the write tracker reports the surface written, to tell a rewrite of the same bytes from a change. The cache budget counts only the GPU allocation, so these host copies grow with the cache without being counted. In Uncharted: Legacy of Thieves Collection (PPSA05684), they held 2.7 GB of heap in the first level (the boat).surfaceBytes) for the hit check and the generation check, because the byte vector can now be empty.equalsCommittedreturns false with no snapshot). The cost is that a same-bytes rewrite of a large texture is uploaded again.Overlap: #6937 changes the same
unchangedline (it checks pending writes before both generation proofs). The two are independent; whichever merges second needs a one-line rebase.Tested
Linux (7.2.9-zen), RTX 4070 Ti SUPER.
New
agc_driver_large_tracked_snapshot(--large-trackedmode ofagc_driver_partially_resident_snapshot_tests). It samples a 512x512 R32 surface (1 MiB) twice: once fully committed and once with only its first 64 KiB block committed. It checks that:agc_driver_partially_resident_snapshot).Negative controls, each reverted afterwards:
equalsCommittedtreating an empty snapshot as equal (a tracked write is missed), the test fails with "sampling the large committed surface after a CPU write kept the texture uploaded before it".ctest -j4: 1315/1316 pass. The one failure isagc_driver_store_over_untracked, which also fails 3/3 on plainmainon this machine (fix(agc): save pending storage results in every 64 KiB block that leaves the write watch #5671, fixed by test(agc): keep waiting for the neighbour copy case's result #6385).python3 tools/check_conventions.py --base origin/main: passed.PPSA05684: the 2.7 GB of snapshot copies was measured in the boat level with a heap profile before the change. The change has been in my local integration build, which plays the boat level. This branch on its own was not run in the title.
Performance capture, PPSA05684 main menu after the intro, 60 s window (
APS5_TRACE_SECONDS=150:60), warm caches (each build ran the menu once and the title was restarted), no otherAPS5_TRACE_*orAPS5_PROFILE_*switch. The title does not run on plainmainyet, so both builds are my local integration branch (main864729d + open PRs + local patches, the sameAPS5_*switches):0a80c84ef6a4carries this change (its driver code is identical to 6849eea; only the test differs), and896ddc7a8982is the same branch with this change reverted. The launch script writes the log to a file. Frame times are the same within noise (p50 108.8 -> 110.0 ms, p99 136.5 -> 131.1 ms, GPU busy 81.1 -> 81.3 ms per present). The saving is the host memory of large snapshots, which grows in levels, not in the menu. In both captures queue worker 0x0 filled its event buffer after about 8 s (events_lost, totals complete), so its slices after that are missing from the traces, and the summary numbers are complete.Without the change (
896ddc7a8982):{"commit":"896ddc7a8982","dirty":false,"env":{"APS5_COMPLETION_STORE_SYNC":"1","APS5_DISPATCH_CACHE_ENTRIES":"4096","APS5_DRAIN_COMPLETION_LABELS":"1","APS5_DUMP_FRAMES":"0","APS5_DUMP_FRAMES_EVERY":"40","APS5_HOST_IMPORT_MIB":"16384","APS5_IMAGE_TABLE_KEY_SCAN":"262144","APS5_LAZY_SHADER_PREPARE":"1","APS5_LOCAL_NULL_INVALID_BUFFERS":"1","APS5_LOCAL_REBASE_HUGE_ARRAYS":"1","APS5_NO_LABEL_BATCHING":"1","APS5_NO_LIBRARY_PREBUILD":"1","APS5_NO_PIPELINE_LTO":"1","APS5_NO_SNAPSHOT_CHECK":"1","APS5_PAD_INPUT_FILE":"","APS5_TRACE_DIR":"","APS5_TRACE_SECONDS":"150:60"},"gpu":"NVIDIA GeForce RTX 4070 Ti SUPER","driver":"NVIDIA 615.78.08","os":"Linux 7.2.9-zen1-1-zen","resolution":"2560x1440","window":{"spec":"seconds 150:60","first_present":1625,"ms":60006.870},"presents":540,"frame_ms":{"p50":108.827,"p90":126.267,"p99":136.510,"max":219.750},"gpu_busy_ms_per_present":81.079,"worker_busy_pct":{"queue worker 0x0":99.329,"queue worker 0x43":99.861,"queue worker 0x21":4.303},"top_phases_ms_per_frame":{"Driver.Packet":226.047,"Driver.Packet.memory_wait":79.417,"Driver.Packet.dispatch":63.328,"Driver.Dispatch":48.361,"Driver.Packet.draw":33.292,"Driver.Draw":33.106,"Driver.Packet.device_lock":32.826,"Driver.Draw.shader_compile_and_link":22.071},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.796,"events_lost":1737049,"totals_full":false}},"trace":"aps5-trace-896ddc7a8982-412912.json.gz"}
Traces and
tools/trace_digest.pyoutput: in the comment below.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)