Repository navigation
perf(agc): read a large watched texture straight into its upload staging buffer - #10845
milescoviello wants to merge 1 commit into
Conversation
…ing buffer Since 6849eea, a sampled-texture snapshot over 256 KiB that the write tracker covers keeps no host copy. Its bytes were still read into a vector, which Texture's constructor copied into the upload's staging buffer before the vector was parked in the spare pool. Such a surface with uncompressed keys is now read straight into the pooled host staging buffer, and the texture is created from it. That saves one copy of the whole surface per upload. DCC clear and single-key surfaces keep the vector: ExpandSingleBlocks reads back from the span it fills, and the staging memory may be write-combined. recycleTests now requires a big watched snapshot to leave the spare pool empty; on main it fails with the 1 MiB host copy parked. spareAgeTests feeds the pool with a snapshot that a view of its imported storage image replaces. The large tracked snapshot test also reads its 512x512 texture back on the GPU and requires the texels to be the surface's words, with zeros for uncommitted pages; the partially resident pass reuses staging buffers the committed pass filled. The texel fetch helpers leave the Windows-only block and dispatch one group per texel in two dimensions, so 512x512 stays within the 65535 groups a dimension may hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed head 5513eb7d6557bb7e3c49ad118c7d291cedf33a1d and its three-dot patch against current main.
The large uncompressed snapshot path reads into the same host-visible staging buffer later copied to the tiled device buffer. ReadTextureSurface uses ReadCommitted, preserving zero-filled uncommitted ranges; the cache only selects this path when the snapshot is tracker-covered and no untracked write is pending. The recorder retains the staging buffer through command completion; the synchronous path submits and waits before returning. DCC clear and ClearSingle remain on the vector path, which avoids the write-combined staging readback concern described in the PR. The new texel test checks both committed and partially resident large surfaces and exercises reused staging storage.
Local checks on this exact patch: git diff --check upstream/main...upstream/pr-10845 and python3 tools/check_conventions.py --base upstream/main --head upstream/pr-10845 pass. I could not run the product tests here; hosted product build/test jobs are skipped by repository policy. The Linux/Windows/Vulkan-validation runs and profiles in the description are author-reported, not independently reproduced in this review.
I found no code blocker in the current diff. I am leaving this as a comment rather than approving because CONTRIBUTING.md requires authors to agree on merge order for overlapping PRs and record the ordering in Depends on. The current checklist says Depends on: none, while the overlap report names #10773, #7085 and #10064. The proposed order for #10773 and #7085 has not yet received confirmation from those authors. #10064 is a draft and its author has been asked about the opposite ordering; the !entry.slabs exclusion and constructor forwarding described in that thread will also need to be retained when the branches are reconciled. Please update the checklist once the affected authors have agreed on the order.
AI-assisted review: yes.
|
@MarcZX10 thanks for the review. I've asked all three authors (#10773, #7085, #10064). The checklist now records the proposal, and I'll turn it into AI-assisted: yes (Claude Code) |
|
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.
|
|
Windows run on NVIDIA and an AMD UMA iGPU. Windows 11 10.0.26300, WinLibs GCC 15.2.0, Release;
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@wachi35676 thank you, especially for the control with AI-assisted: yes (Claude Code) |
|
@milescoviello Thank you for this, and for how thoroughly you tested it. #9884 makes the same change with the same rule in the same function and was opened first, so I'm closing this one as a duplicate of #9884. Your evidence isn't lost: I've summarized it on #9884 with a link back here and credited you (the red/green on the sampled-cache budget tests, the GPU read-back of the large tracked snapshot, the validation run, the CPU profiles, @wachi35676's Windows run with its control, and the merge-order agreements with #10773, #10064 and #7085). If you think something in your version is worth having on top of #9884, such as the read-back test, please say so there. |
|
@oneandonlydean agreed, and thank you for carrying the evidence over to #9884. My duplicate search missed it, which I should have caught before opening. I'll offer the read-back test on #9884. AI-assisted: yes (Claude Code) |
What
Since #6982, a sampled-texture snapshot larger than 256 KiB that the write tracker covers keeps no host copy of its guest bytes. The tracker covers it when it has a write generation, or an untracked key with no write pending. The bytes were still read into a vector first, then
Texture's constructor copied the vector into the upload's staging buffer, and then the vector was dropped (since #8026, parked in the spare pool). This change reads the guest bytes straight into the staging buffer. That saves one full copy of the surface per upload, and the vector allocation (zero-filled) whenever the spare pool has no buffer of that size.Forspoken's opening has a texture-heavy stretch where the draw committer is about 90 % busy and three quarters of the process's CPU samples are under
cachedTexture. There, the share inmemmovefalls from 20.0 % to 15.4 % (profiles under Tested).How
Texturegets a constructor that takes the stagingBuffer. The span constructor makes the staging buffer from its span and delegates, so its callers are unchanged.cachedTexturedecides before the read whether the snapshot will be kept. The rule is perf(agc): drop the guest byte snapshot of a large cached texture the write tracker covers #6982's: uncompressed keys, more than 256 KiB, and a generation or an untracked key with no write pending. When it won't be kept, the surface is read into a pooled host staging buffer and the texture is created from that buffer. No snapshot vector is made, so none is parked.ExpandSingleBlocksreads back from the span it fills, and the staging memory is host-coherent memory that may be write-combined. Those surfaces still park their dropped buffer as before. The spare pool still serves them, and entries that a storage-image view replaces.copyCommittedwrites every byte of its destination (zeroes, then the committed ranges), so a recycled pool buffer can't leak stale bytes into the texture, the invariant perf(agc): reuse sampled snapshot byte buffers across cache entry replacement #8026 relies on for its spares.Tested
Linux: Ubuntu 26.04.1, GCC 15.2, Release, RTX 4070 Ti, NVIDIA 615.71.09, on
main52a8bed.Red/green:
recycleTestsnow requires a 1 MiB snapshot of write-watched memory to leave the spare pool empty, since no host copy is made. With only the test changes onmain(4551d4f9b4),agc_driver_sampled_cache_budgetandagc_driver_sampled_cache_budget_without_storage_image_multisamplefail 3 of 3 witha big snapshot of watched memory made a 1048576-byte host copy and parked it, instead of reading guest memory into its staging buffer. With this change both pass 3 of 3.New cases:
agc_driver_large_tracked_snapshotnow also reads its 512x512 texture back on the GPU, through a texel fetch. Detiling only permutes 32-bit words, so the sorted texels must equal the sorted words of the surface, with zeros for uncommitted pages. It runs once with the surface committed and once with a single 64 KiB block committed. The second run reuses staging buffers the first one filled, so a staging buffer that kept stale bytes would fail it.spareAgeTestskeeps its age bounds: a spare idle for 17 presents is released, one idle for 16 is kept, and a same-size snapshot takes it. Its spare now comes from a 1 MiB snapshot that a view of its host-imported storage image replaces, the pool's other source.Focused tests: the 29 tests matching
sampled|snapshot|texture|dcc|detile|^agc_driver_graphics$|staged_clearpass 3 of 3.Windows 11 VM (GT 1030, driver 582.78, WinLibs GCC 15.2.0 r7), the same regex:
agc_driver_sampled_cache_budget_memory, fails the same way on both: it expects 1024 MiB of storage-image budget, but the GT 1030's heap (about 1966 MiB) caps it at 983 MiB, half the heap.Khronos validation 1.4.363 over the same 29, each arm run as
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 ctest -R "$R" -V -j4: 29 of 29 pass onmain52a8bed and on this head. Neither log has a layer-missing line or aVUID-message.Forspoken (PPSA03984) CPU profiles: release libraries, warm caches, 16G, pad input, no trace or profile switch. The PR side was built from 83334037e1, whose library code is this head's; later commits changed only tests.
perfwas attached about 10 s in, then recorded DWARF stacks at 299 Hz from 190 s to 210 s. Shares are of all the process's samples:main__memmove_avx_unaligned_erms(self)Texture::Texture(inclusive)ReadTextureSurface(inclusive)cachedTexture(inclusive)ReadTextureSurfacegrows because it now writes the staging memory itself. The copy that goes away is the oneTexture::Texturemade from the vector. That's one profile per side. Each side had a second profile run that stopped before the window, on the depth-plane refusal noted below.Depth-plane stop (not this change): two Forspoken runs stopped with
sampling the depth plane of depth surface ... (1920x1080, vk format 130) as a 1920x1080 texture of guest format 22 (vk 100), tile mode 7, dimension 1, levels 0-4, slice 0 is not implemented: one onmain(179 s), one on this PR (101 s). It comes fromDepthSurfaceTexture, before the snapshot path this PR changes. I'll follow it up separately.Performance captures
Forspoken (PPSA03984),
APS5_TRACE_SECONDS=130:60, warm caches, same machine (Linux, RTX 4070 Ti, NVIDIA 615.71.09). A warm-up run ofmaincame first, then three runs per side, interleaved.mainrun 2 hit the 16G memory cap at 188 s and left no capture. The window holds only 6 to 9 presents of 6 to 12 s each, and both sides get faster from run to run as caches warm:main: 6 and 7 presents, p50 11.3 and 8.3 s.So I don't claim a frame-time change; the profiles above are the measurement. The last pair is below. All runs, their traces, digests and summary lines are in the release on my fork.
main52a8bed (run main-3):{"commit":"52a8bed576e5","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"130:60"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.0.0-38-generic","cpu":"13th Gen Intel(R) Core(TM) i7-13700K","cpu_threads":24,"memory_mib":30993,"title_id":"PPSA03984","title":"Forspoken","content_version":"01.020.000","resolution":"1920x1080","window":{"spec":"seconds 130:60","first_present":560,"ms":62882.190},"presents":7,"frame_ms":{"p50":8267.748,"p90":10903.792,"p99":10903.792,"max":10903.792},"gpu_busy_ms_per_present":3324.257,"worker_busy_pct":{"queue worker 0x0":99.955,"queue worker 0x20":99.999,"queue worker 0x28":100.000},"top_phases_ms_per_frame":{"Driver.Packet":26911.412,"Driver.Packet.memory_wait":17661.471,"Graphics.Draw":8221.968,"Graphics.Draw.draw_and_resource_release":8162.619,"Resources.Complete":8102.600,"Driver.Packet.draw":7787.542,"Driver.Draw":7784.785,"Driver.Draw.draw_enqueued":7595.656},"trace_dropped":false,"trace":"aps5-trace-52a8bed576e5-3.json.gz"}tools/trace_digest.py
seconds 130:60 from present 560: 7 presents, frame p50 8267.7 / p90 10903.8 / p99 10903.8 / max 10903.8 ms, GPU busy 3324.3 ms per present
NVIDIA GeForce RTX 4070 Ti, NVIDIA 615.71.09, Linux 7.0.0-38-generic, 1920x1080, commit 52a8bed, dropped: no
{"commit":"52a8bed576e5","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"130:60"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.0.0-38-generic","cpu":"13th Gen Intel(R) Core(TM) i7-13700K","cpu_threads":24,"memory_mib":30993,"title_id":"PPSA03984","title":"Forspoken","content_version":"01.020.000","resolution":"1920x1080","window":{"spec":"seconds 130:60","first_present":560,"ms":62882.19},"presents":7,"frame_ms":{"p50":8267.748,"p90":10903.792,"p99":10903.792,"max":10903.792},"gpu_busy_ms_per_present":3324.257,"worker_busy_pct":{"queue worker 0x0":99.955,"queue worker 0x20":99.999,"queue worker 0x28":100.0},"top_phases_ms_per_frame":{"Driver.Packet":26911.412,"Driver.Packet.memory_wait":17661.471,"Graphics.Draw":8221.968,"Graphics.Draw.draw_and_resource_release":8162.619,"Resources.Complete":8102.6,"Driver.Packet.draw":7787.542,"Driver.Draw":7784.785,"Driver.Draw.draw_enqueued":7595.656},"trace_dropped":false,"trace":"aps5-trace-52a8bed576e5-3.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): Driver.Packet 25858.5, Driver.Packet.memory_wait 16668.6, Graphics.Draw 8176.3, Graphics.Draw.draw_and_resource_release 8113.0, Resources.Complete 8101.1, Driver.Packet.draw 7746.4, Driver.Draw 7743.3, Driver.Draw.draw_enqueued 7565.4, batch 3165.2, Driver.Packet.pipeline_drain 750.2
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
This PR, 83334037e1 (the head's library code, run pr-3):
{"commit":"83334037e1d2","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"130:60"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.0.0-38-generic","cpu":"13th Gen Intel(R) Core(TM) i7-13700K","cpu_threads":24,"memory_mib":30993,"title_id":"PPSA03984","title":"Forspoken","content_version":"01.020.000","resolution":"1920x1080","window":{"spec":"seconds 130:60","first_present":582,"ms":60562.854},"presents":9,"frame_ms":{"p50":6635.682,"p90":7383.433,"p99":7383.433,"max":7383.433},"gpu_busy_ms_per_present":2350.433,"worker_busy_pct":{"queue worker 0x0":99.943,"queue worker 0x28":90.692,"queue worker 0x20":99.717},"top_phases_ms_per_frame":{"Driver.Packet":19485.847,"Driver.Packet.memory_wait":12527.982,"Graphics.Draw":6235.515,"Graphics.Draw.draw_and_resource_release":6175.233,"Resources.Complete":6113.778,"Driver.Packet.draw":5701.221,"Driver.Draw":5698.727,"Driver.Draw.draw_enqueued":5523.932},"trace_dropped":false,"trace":"aps5-trace-83334037e1d2-3.json.gz"}tools/trace_digest.py
seconds 130:60 from present 582: 9 presents, frame p50 6635.7 / p90 7383.4 / p99 7383.4 / max 7383.4 ms, GPU busy 2350.4 ms per present
NVIDIA GeForce RTX 4070 Ti, NVIDIA 615.71.09, Linux 7.0.0-38-generic, 1920x1080, commit 83334037e1d2, dropped: no
{"commit":"83334037e1d2","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_SECONDS":"130:60"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.0.0-38-generic","cpu":"13th Gen Intel(R) Core(TM) i7-13700K","cpu_threads":24,"memory_mib":30993,"title_id":"PPSA03984","title":"Forspoken","content_version":"01.020.000","resolution":"1920x1080","window":{"spec":"seconds 130:60","first_present":582,"ms":60562.854},"presents":9,"frame_ms":{"p50":6635.682,"p90":7383.433,"p99":7383.433,"max":7383.433},"gpu_busy_ms_per_present":2350.433,"worker_busy_pct":{"queue worker 0x0":99.943,"queue worker 0x28":90.692,"queue worker 0x20":99.717},"top_phases_ms_per_frame":{"Driver.Packet":19485.847,"Driver.Packet.memory_wait":12527.982,"Graphics.Draw":6235.515,"Graphics.Draw.draw_and_resource_release":6175.233,"Resources.Complete":6113.778,"Driver.Packet.draw":5701.221,"Driver.Draw":5698.727,"Driver.Draw.draw_enqueued":5523.932},"trace_dropped":false,"trace":"aps5-trace-83334037e1d2-3.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): Driver.Packet 19426.6, Driver.Packet.memory_wait 12528.0, Graphics.Draw 6186.7, Graphics.Draw.draw_and_resource_release 6122.8, Resources.Complete 6112.2, Driver.Packet.draw 5659.5, Driver.Draw 5656.5, Driver.Draw.draw_enqueued 5492.7, batch 2367.0, Driver.Packet.pipeline_drain 716.5
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
Overlap (
tools/pr_overlap.py, andgit merge-treeof each head with this one):Texture.cppwhere this PR removes the staging buffer's creation and copy. The merge takes fix(agc): reuse one tiled and one linear texture upload scratch per recorded batch #10773's side without its twostaginglines, since the staging buffer is now the constructor's argument. The two changes are complementary: fix(agc): reuse one tiled and one linear texture upload scratch per recorded batch #10773 removes the per-upload device buffers, and this PR removes a host copy.Texture.cpp. The merge keeps this PR's check (on the staging buffer) and feat(agc): back extra guest mip levels with storage images #7085's report.snapshotSize, reads it throughreadSnapshot, and passespackedSlabsto the span constructor. The merge keeps slab snapshots on the vector path (stagedalso requires noslabs) and gives the staging-buffer constructor the samepackedSlabsargument, which the span constructor forwards.mainaeff26b): RTX 5070 Ti (621.01) and an AMD RDNA2 iGPU (proprietary driver 2026.04.17). The 15 snapshot and sampled-cache tests pass on both. Withmain'sTexture.hpp/.cppandShaderResources.cpp, the twoagc_driver_sampled_cache_budgetvariants fail on both with the new assertion. 257agc_tests fail the same way with and without the change (agc_host_import_cut_lifetime,agc_driver_depth_plane_partial_store).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)🤖 Generated with Claude Code