Repository navigation
perf(agc): cross uniform runs of cached guest pages 64 pages per load - #6921
milescoviello wants to merge 3 commits into
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.
|
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The profile (runEnd at 13.8% of all samples in MXGP 26, 8.5 million range checks per 130 s averaging 150 MiB) justifies work here. I read the mark protocol (write pages, fence, settle; mark, fence, re-read and take the mark back) and could not find an interleaving that leaves a stale mark. Points:
- This speeds up the scan, but the call count is the problem:
addDescriptorRegioncallsDescribeCommittedover hundreds of MiB for every dispatch that binds the same heap. The description counts 8.5 million of them. Caching the result per descriptor range against the registry generation (the generation the lease snapshot in #6917 already uses) removes the scan for repeated bindings instead of making each one cheaper. Say why that wasn't done, or measure both. - No before/after. A profile share before the change is not a result: please post the summary lines of main and this PR (plus #6917, as the run uses it) from the same scene in PPSA30839, no
APS5_PROFILE_*, and the sameperfshare after. RunPageBlockTestsis#if defined(__linux__), but the allocation (VirtualAllocofcount + blockCount) and the main-image table on Windows change as well. Run theagc_*suite on Windows and say so, or add the Windows case.- The block tables are written with
memory_order_relaxedbyte stores plusseq_cstfences. Please add a short stress test that flips a page in a marked block from one thread while another callsrunEndover it, and requires that after the writer finishesrunEndstops at that page.
|
Data point from a CPU-bound scene on Windows: RTX 3060 12 GB (NVIDIA 617.42), i5-12400F, Windows 10 19045, MinGW GCC Release. Minecraft Dungeons II (PPSA16064 01.100.100), title screen, no input, 200 s rows,
Within the run-to-run spread (about ±1.5 % here), no regression, 0 fatal. AI-assisted: yes (Claude Code) |
|
Thanks for the review. Memo per descriptor range vs. this change: I measured both. I made two measurement-only builds, neither of them a PR, on top of the stack plus #6917. In both,
Same scene and protocol as in the description: PPSA30839, 100–110 s, interleaved, two rounds, no
Before/after.
Summary lines and traces for both sessions: https://gist.github.com/milescoviello/336bdc50227a65c6ea50f86e84ffbe1d, under Windows. I did not build or run anything on Windows: there's no Windows machine here, and the supported MinGW (GCC 15.2) isn't installed. The only Windows data is parl0v's runs above: Minecraft Dungeons II, Stress test. 8a2c242 adds it to
The flipped page moves through 22 blocks and every offset within a block ( Negative controls, 3 runs each:
AI-assisted: yes (Claude Code) |
|
An update on the outlier in my last comment (the run with about 500
AI-assisted: yes (Claude Code) |
|
Follow-up to my previous comment: the upload storm also happens without this PR, so I'm withdrawing the request to hold it. Two corrections to that comment are at the end. I built the same CPU-upload log on the stack + #6917 build, which doesn't contain this PR (
In every storm, the two 2560x1440 storage images upload alternately dozens of times through the CPU path, whichever build it is. So the storm doesn't need this PR. The log itself changes how often it happens: at each CPU-path upload it also calls Corrections to my previous comment:
AI-assisted: yes (Claude Code) |
|
One more data point on the storm: with the block-mark check from my earlier comment and the CPU-upload log in the same build (stack + #6917 + this PR, local AI-assisted: yes (Claude Code) |
|
AMD / Windows run of this PR at 8a2c242: 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 (14):
AI-assisted: yes (runs made on the device above). |
|
Thanks @keravyn-ops, that covers the AMD/Windows side of every test target this PR touches. I added your run to the Tested section. AI-assisted: yes (Claude Code) |
8a2c242 to
fde3948
Compare
github-actions
Bot
added
perf
fde3948 to
f05f64a
Compare
|
Rebased onto AI-assisted: yes (Claude Code) |
DescribeCommitted on a descriptor bound over a whole GPU heap walked the page-state table one byte per page (eight per load), hundreds of MiB per dispatch. Each table now keeps a byte per 64 pages with their common value; runEnd skips a marked block in one load and eight of them in one 64-bit load. Stores and forgets clear the marks they touch after their pages, and a mark is checked again after a fence, so a racing change cannot leave a stale mark. On Windows the page table is a directory of lazily allocated 64Ki-page chunks, so each chunk carries the marks of its own pages: a block never spans two chunks, and an absent chunk has no marks to keep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two threads describe a 1536-page arena range in a loop while the test makes one page of a uniform (marked) block read-only; after each scanner has finished a pass that started after the change, the writable ranges must stop at that page. 256 rounds cover every page offset of a block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
agc_page_state_runs commits 1536 pages across a sparse chunk boundary and protects single pages at block, group and chunk edges, checking the committed and writable ranges around each and that a decommit clears the summaries; two threads keep scanning while pages flip. A 512-page run ending where an unallocated chunk begins must stop at its last page. Co-Authored-By: Izamaru <machadochristiancris@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f05f64a to
b50a5d5
Compare
|
Reworked for #8139's sparse Windows page table and rebased onto
The description's Tested section has the details. AI-assisted: yes (Claude Code) |
|
@milescoviello I revalidated the published head after the sparse-table rework, and also tested a local integration with a newer main. These results support merging from the Windows/RTX 4090 compatibility side. Pinned sources and results (3 runs of each matrix):
The head uses an older main base, so the third matrix checks the PR together with the newer main's code and tests. The local merge was conflict-free; its Environment: Windows 11 build 26200, NVIDIA GeForce RTX 4090, driver 617.14, WinLibs GCC 15.2.0 r7, Release, Khronos SDK 1.4.363.0; The 11 common tests were Negative control: on the exact PR head, skipping Metadata: the initialization fixture reported 102,400 bytes of private commit in all three sources, in all three runs. This preserves the sparse-table result in that fixture; it is not a whole-game RAM measurement or a claim that summaries have no cost (they add 1,024 bytes per allocated chunk). Scope/limitations: page-state, metadata and run fixtures are CPU checks. Thanks for adapting the summaries to #8139 and including our compatibility fixture with credit, plus your missing-chunk boundary case. No blocker appeared in this focused validation, so I support the merge within the scope above. AI-assisted: yes (Codex; local builds and runs on the hardware listed above). |
|
@Izamaru thank you, that's the Windows check this needed, and on the exact head with the layer insertions logged. Your negative control is the same one I ran on the VM (skipping the summary settle for stored zeros), so it fails your fixture the same way on two machines. I added your run to the Tested section and the GPU list. AI-assisted: yes (Claude Code) |
What
GuestMemory's page-state table keeps one byte of access flags per 4 KiB page of the guest arena (and of the main image on Windows), anddescribePagesfinds the end of a run of equal pages withPageSpan::runEnd, which reads that table eight pages per load. When a descriptor is bound over a whole GPU heap,GuestBufferMemory::addDescriptorRegioncallsDescribeCommittedon hundreds of MiB for every dispatch, so the scan reads tens of thousands of table bytes each time.In MXGP 26 (PPSA30839) on Linux,
runEndis the largest entry in aperfprofile of the process (13.8% of all samples). The run uses the PRs this title needs to get this far (#6814, #6094, #6160, #6184, #6455, #6308) plus the lease change of #6917. A DWARF call graph puts 13% underDriver::run→dispatch→ShaderResources::addGuestBuffer→addDescriptorRegion→DescribeCommitted, on the submission thread. The draw-profile counters ([guestmem] verify) count about 8.5 million range checks per 130 s run, averaging 150 MiB each and 21 s of thread time in total.Each
PageSpannow has a second table with one byte per 64 pages (256 KiB of guest memory). A byte holds the pages' common value when all 64 hold the same nonzero value, and zero otherwise. It sits next to the page entries, at 1/64 of their size:calloc, and the summaries follow the pages in the same block (3.9 MiB for the 1000 GiB arena; only the pages that are touched get backed).Chunknow also holds the 1,024 summaries of its own pages. A block never spans two chunks, and a missing chunk has no summaries:runEndends a run there as before, and stores of zero orPageInaccessiblestill don't allocate a chunk.runEndskips a marked block in one load, and at a 512-page boundary it compares eight marks in one 64-bit load (2 MiB per load). Unmarked blocks and run edges are read page by page as before.store(first, last, value)(used byrecord,forgetInaccessibleand the single-pagestore) stores the pages chunk by chunk and then settles the marks of the blocks it touched: a block whose 64 pages now agree is marked, and any other is cleared.forgetgoes through the samestorewith zero, so it clears the pages, then the marks of the blocks it touched.Marks are kept right under concurrency with fences, not locks. A store or forget writes its pages, issues a
seq_cstfence, then reads and fixes the marks. Marking writes the mark, issues a fence, then reads the 64 pages again and clears the mark if they no longer agree. So when a page change and a mark race on one block, either the change's settle sees the mark and replaces it, or the mark's second read sees the change and takes the mark back. A stale mark therefore cannot outlive a change; while a change is in flight, readers see old or new state, as they already did with the page bytes. Writing zero is always safe, so only nonzero marks get the second check. Therecordgeneration check is unchanged and now also clears the marks throughforget.Tested
Current head (
b50a5d5cea, rebased ontomain29896371ed, after #8139):Chunkas agreed with @Izamaru on fix(agc): allocate Windows page permissions on demand #8139. The third commit adds @Izamaru'sagc_page_state_runsfixture from lab/compat-8139-6921 (credited as co-author). It commits 1,536 pages across a chunk boundary, protects single pages at block, 512-page group and chunk edges, checks a decommit, and keeps two scanners running while pages flip. I added one case to it: a 512-page run that ends where an unallocated chunk begins must stop at its last page. The Linux table layout is the same as before the rework, so the MXGP measurements below still describe the Linux path.main999e612 passes its 11 page-state, metadata, graphics, BDA and memory tests 3 of 3; this head (b50a5d5cea) and a local merge with thatmainpass the same 11 plusagc_page_state_runs, 12 of 12, 3 of 3. Skipping the summary settle when pages are stored as zero failsagc_page_state_runs3 of 3. The metadata fixture reports the same 102,400 bytes of private commit on all three.70740fcffb(the third commit only adds Windows tests):agc_driver_graphics(withRunPageBlockTestsand the 256-round flip test),agc_driver_bda_deviceandagc_driver_store_over_untrackedpass 3 of 3.mainat the merge base against the head, 3 runs each:agc_page_states,agc_page_state_metadata,agc_driver_graphics,agc_driver_bda_deviceandagc_driver_store_over_untrackedpass 5 of 5 onmain; on the head (at41468e10e9, the same three commits before the last rebase) those andagc_page_state_runspass 6 of 6. As a negative control, a branch that leaves the summaries alone when pages are stored as zero (forgetwithout the summary clear, test/6921-mut) failsagc_page_state_runs(a protected page retained writable access in a summarized run) andagc_driver_graphicsin 3 of 3 runs.Earlier heads (Ubuntu 24.04, GCC 13.3, Release, RTX 4070 Ti, NVIDIA 595.99.02):
Rebased onto
main6e27fc8 (f05f64a). Tamalero's 13a3902 and 4a96406 had changedPageStates::recordmeanwhile: inaccessible pages are now cached asPageInaccessible, an access serial forgets pages recorded across an arena access change, andforgetInaccessibleclears inaccessible pages. The resolution keeps all of that and writes through this PR's rangestore, so the block summaries follow, including whenforgetInaccessibleclears a page. On the rebased headagc_driver_graphics(withRunPageBlockTests) passes 3 of 3. The 1,013 tests matching the regex below pass, exceptagc_driver_bda_device, which fails onmainon this driver too (fix(shader): leave the BDA lookup functions through one return #7571).New
RunPageBlockTestsinagc_driver_graphics(Linux, 1,536 arena pages aligned to 2 MiB). It changes one page through the registry in the middle of a marked block (700), at the start of a group of eight blocks (512), and at the last page (1535). Each time it checksAccessible,CommittedRanges(readable and writable) andDescribeCommitted(...).wholebefore and after the page is restored. Two threads then callDescribeCommittedon the whole run while the main thread toggles 128 pages through the registry 2,000 times, and the final state must be described exactly. Passes 3 of 3. With the mark clearing removed fromforget, it fails (a page protected inside a long run of cached pages kept its old access). In 256 further rounds (8a2c242), two threads runDescribeCommittedover the range while one page of a marked block turns read-only, moving through 22 blocks and every offset in a block. After each thread finishes a pass started after the change, the writable ranges must stop at that page. Passes 3 of 3. With the recheck removed fromsettleit still passes 3 of 3: two scanners don't hit that window.The 885 tests matching
^agc_driver_|^agc_vertex_barrier|recompiler|shader_cache|request_serializer|spirv|^agc_shader|bda|journal|^guest_|libcpass exceptagc_driver_store_over_untracked, which fails onmaintoo.Windows 11, AMD Radeon 780M (RDNA 3), AMD driver 25.10.20.01, WinLibs GCC 15.2.0 r7, Release, at 8a2c242: the 14 test targets this PR changes pass (run by @keravyn-ops).
Speed (Performance report protocol). MXGP 26, 100–110 s into a warm run with pad input, interleaved in one session. Only
APS5_TRACE_SECONDS=100:10(and the trace directory and pad input file) was set: noAPS5_PROFILE_*switch.maindoes not reach this scene, so the base is the stack above plus perf(libc): keep the guest allocation lease snapshot until the registry changes #6917. The commits are local test merges:5f469824dc57is the stack (with perf(agc): time compute pipeline creation #6896's one-line compute pipeline trace timer) plus perf(libc): keep the guest allocation lease snapshot until the registry changes #6917's library change, and84b92c0dc0ddadds this PR'sGuestMemory.cppchange. That build differs from this branch only in comments removed before pushing, and in fix(agc): cache inaccessible guest pages #6184'sforgetInaccessiblecallingstore(at, at + PageBytes, 0).5f469824dc5784b92c0dc0dd5f469824dc5784b92c0dc0ddtools/trace_digest.py:Summary lines (
aps5-summary.jsonl, base 1, PR 1, base 2, PR 2):{"commit":"5f469824dc57","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":477,"ms":10188.146},"presents":44,"frame_ms":{"p50":230.477,"p90":239.478,"p99":247.269,"max":247.269},"gpu_busy_ms_per_present":115.561,"worker_busy_pct":{"queue worker 0x0":99.417,"queue worker 0x20":99.300},"top_phases_ms_per_frame":{"Driver.Packet":459.173,"Driver.Packet.dispatch":179.724,"Driver.Dispatch":178.887,"Driver.Packet.memory_wait":169.497,"Driver.Dispatch.capture_resources":70.947,"Resources.Prepare":50.093,"Driver.Packet.draw":48.342,"Driver.Draw":48.119},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.825,"events_lost":77892,"totals_full":false}},"trace":"aps5-trace-5f469824dc57-3.json.gz"} {"commit":"84b92c0dc0dd","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":472,"ms":10136.123},"presents":53,"frame_ms":{"p50":190.164,"p90":201.774,"p99":210.060,"max":210.060},"gpu_busy_ms_per_present":122.500,"worker_busy_pct":{"queue worker 0x0":99.330,"queue worker 0x20":99.466},"top_phases_ms_per_frame":{"Driver.Packet":379.614,"Driver.Packet.memory_wait":138.089,"Driver.Packet.dispatch":136.793,"Driver.Dispatch":136.020,"Driver.Dispatch.capture_resources":72.500,"Driver.Packet.draw":44.223,"Driver.Draw":44.010,"Driver.Packet.prepare_memory":28.924},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.572,"events_lost":88121,"totals_full":false}},"trace":"aps5-trace-84b92c0dc0dd-3.json.gz"} {"commit":"5f469824dc57","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":441,"ms":10005.818},"presents":43,"frame_ms":{"p50":231.870,"p90":241.967,"p99":244.206,"max":244.206},"gpu_busy_ms_per_present":119.206,"worker_busy_pct":{"queue worker 0x0":99.442,"queue worker 0x20":99.515},"top_phases_ms_per_frame":{"Driver.Packet":462.388,"Driver.Packet.memory_wait":177.861,"Driver.Packet.dispatch":177.430,"Driver.Dispatch":176.601,"Driver.Dispatch.capture_resources":69.747,"Resources.Prepare":49.194,"Driver.Packet.draw":46.248,"Driver.Draw":46.035},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.890,"events_lost":69526,"totals_full":false}},"trace":"aps5-trace-5f469824dc57-3.json.gz"} {"commit":"84b92c0dc0dd","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":525,"ms":10127.634},"presents":51,"frame_ms":{"p50":198.376,"p90":205.886,"p99":212.263,"max":212.263},"gpu_busy_ms_per_present":123.567,"worker_busy_pct":{"queue worker 0x0":99.330,"queue worker 0x20":99.454},"top_phases_ms_per_frame":{"Driver.Packet":394.102,"Driver.Packet.dispatch":144.283,"Driver.Dispatch":143.497,"Driver.Packet.memory_wait":137.985,"Driver.Dispatch.capture_resources":77.539,"Driver.Packet.draw":48.411,"Driver.Draw":48.180,"Driver.Packet.prepare_memory":30.307},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.683,"events_lost":83184,"totals_full":false}},"trace":"aps5-trace-84b92c0dc0dd-3.json.gz"}Traces: https://gist.github.com/milescoviello/336bdc50227a65c6ea50f86e84ffbe1d, files
lease-5f469824dc57-run{1,2}.json.gz(base) andblocks-84b92c0dc0dd-run{1,2}.json.gz(this PR). They open in ui.perfetto.dev.APS5_PROFILE_DRAW=1, which costs time, so they are not the speed claim. Three interleaved 130 s runs each: the[guestmem] verifytime drops from 21.1 / 20.9 / 21.1 s (8.2–8.8 M range checks) to 9.9 / 9.5 / 9.4 s (10.0–10.6 M; more frames, more checks), about 2.4 µs to 0.9 µs per check. Inperf recordover 100–115 s,runEndgoes from 14.7% of all samples to 0.7%. All runs reached their limit.Not checked by me: macOS, AMD/Intel on Linux. On Windows,
RunPageBlockTestsis Linux-only; the chunked path is covered byagc_page_states,agc_page_state_metadataandagc_page_state_runsabove (parl0v's Windows runs of Minecraft Dungeons II on an earlier head showed no regression).VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest -R '^agc_' -Vonmaine2a2914 and onmain+ this PR (f05f64a, a local merge).main: 815 of 815 pass; with this PR: 815 of 815 pass. Neither log reports the layer missing. VUIDs onmain:VUID-VkImageViewCreateInfo-usage-02275×6,VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605×18,VUID-vkCmdCopyBuffer-srcBuffer-00118×2,VUID-vkCmdDraw-multisampledRenderToSingleSampled-07285×2,VUID-vkCmdWaitEvents-srcStageMask-01158×2,VUID-vkSetEvent-event-09543×2; with this PR:VUID-VkImageViewCreateInfo-usage-02275×6,VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605×18,VUID-vkCmdCopyBuffer-srcBuffer-00118×2,VUID-vkCmdDraw-multisampledRenderToSingleSampled-07285×2,VUID-vkCmdWaitEvents-srcStageMask-01158×2,VUID-vkSetEvent-event-09543×2. None is new with this PR.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