Repository navigation
Conversation
|
This pull request is labeled |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head bdca440d390c7b833a14e15ee38664e16a902033 against its stated dependency #10423. The feature-flag comparison and attached traces are useful, but I found integration and validation gaps that need resolution before this can merge:
- The performance summaries compare
APS5_NO_RESULT_RECYCLE=1with recycling enabled in the same dirty build, and both report commit2ef1dd3bebef. That is a useful feature-flag ablation, but it is not the required two-commit base/change comparison. Please run the same scene on the #10423 dependency head and this exact PR head (or the agreed current integration base and this head), attach both trace captures, and put both distinct commit IDs in the summaries. The hosted product build/test jobs are currently skipped, so they do not supply this comparison. - Please coordinate the new retention path with #9989. That PR's current budget/trim walk counts
source->memoentries; this change adds a second 256-entrysource.maskedMemotable that owns fullRecompileResults and a per-thread pool retaining up to eight more full results. Those references and their vector capacities are outside the current byte accounting, so the configured memo budget would not bound the retained memory after these changes are combined. Update the accounting/trim behavior and add a test that exercises the bound with both tables and the recycle pool, or agree on an explicit integration order that resolves this before merge. - Please also reconfirm the interaction with #9049. Its current change reuses an evicted exact entry's bindings only when
use_count() == 1; the added masked table can keep a second reference to that same result, while this PR separately recycles the complete result when an entry is evicted. This can change when #9049's reuse path is eligible, contrary to the earlier expectation that the two optimizations add up. Coordinate with that author, declare the agreed dependency/order, and validate the combined path.
I did not run the GPU/AGC tests locally: this checkout has no usable configured full build, and the latest hosted Linux/Windows product jobs are skipped. I therefore have not independently validated runtime equivalence or performance.
|
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.
|
|
@MarcZX10 Thanks, all three points hold, and the first one changed the picture.
The checklist now says |
|
A second title and a second vendor for this change: ASTRO BOT's Crash Site hub on Windows / AMD. The worker-side saving shows up clearly there too. The frame does not move, because on this title the draw committer bounds the frame rather than the queue 0 worker.
AI-assisted: yes (Claude Code). The runs were made on my machine. |
What
This PR builds on #10423 (masked result memo). On a masked hit there,
materializePreparedMemoizedmakes a fresh copy of the donor result (std::make_shared<RecompileResult>(*donor)) and puts it into the exact table. The insert evicts the set's least recently used entry, and when that drops the evicted result's last reference, the result is destroyed on the worker, under the source's lock.A
RecompileResultholds dozens of vectors, and everyDescriptorBindingin it holds about twenty more. So each patched hit costs one full allocation of a result and one full free of another.On Sonic Origins Plus's Sonic 2 menu page with #10423 applied, the result memo evicts about 31,000 results per second. Sampling the queue worker (
APS5_SAMPLE_WORKER, 80 s on the page) attributes:operator delete, mostly the Windows heap's frees;This PR keeps the storage instead:
use_count() == 1under the source's lock, followed by an acquire fence), it moves the result into a small list owned by the calling thread (ThreadOwned, at most 8 results) instead of freeing it.*result = donor) instead of allocating one. The vectors keep their capacity, so most of the copy allocates nothing.applyMemoPatchthen writes the snapshot's words as before.ResultKeyMemo's assignment already clears the draw-key memo, so nothing of the old result survives.Nothing holds a
weak_ptrto aRecompileResult, so a result whose count is 1 under the lock cannot gain a new owner while it is reused. Every other owner (a recorded draw, the other table) keeps the count above 1, and that result is freed as before when its last owner drops it.APS5_NO_RESULT_RECYCLE=1frees every evicted result, as perf(shader): reuse a memoized result by patching its copied words #10423 does.ShaderRecompiler::RecycledResults()counts the reuses.agc_shader_table_snapshot_reusenow ends by patching the program for 640 distinct output buffers, enough to fill the memo and evict.agc_shader_table_snapshot_reuse_unrecycled(new) runs the test withAPS5_NO_RESULT_RECYCLE=1. It expects no reuse, and the two existing variants (default and_unmasked) still pass.Overlap.
tools/pr_overlap.pylists perf(shader): reuse an evicted memo result's bindings for the next miss #9049, perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 and fix(shader): bound the bytes the recompiler's result memos hold #9989. Their conflicts are with lines perf(shader): reuse a memoized result by patching its copied words #10423 changes inRecompiler.cpp(the memo entries andmaterializePreparedMemoized). There is also theENVIRONMENT_MODIFICATIONtest list in the AGCCMakeLists.txt, where perf(shader): reuse a memoized result by patching its copied words #10423 and this PR add test names and perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 edits the same line.materializePreparedMemoized, which perf(shader): reuse a memoized result by patching its copied words #10423 rewrites. The result fields they add are copied by the copy-assignment like every other member.ResultMemoBytes()and trimmed with the memo, with a test.Tested
Windows 11 Pro 10.0.26300, NVIDIA GeForce RTX 2080 Ti, driver 617.14, i9-9900K.
Sonic Origins Plus (PPSA05324), Sonic 2 page of the main menu,
main's defaults. Two clean builds onmaine7f46f5: #10423's head 54d7c78 and this PR's head bdca440. Each got one warm-up boot, then two interleaved traced rounds (APS5_TRACE_SECONDS=200:15):Driver.Packet(ms/frame)Driver.Draw(ms/frame)No frame-time difference within this page's noise. The page switches between a fast mode (p90 about 87 ms) and a slow one (about 116 ms), and each build landed in each mode once. Averaged, frame p50 goes 82.6 -> 82.4 ms and
Driver.Draw63.8 -> 62.8 ms.Why: the queue worker now waits on the draw thread. A sampled profile of the queue worker (
APS5_SAMPLE_WORKER, 80 s) shows:materializePreparedMemoizedgoing from 19 % to 14 % of its samples andoperator deletefrom 15 % to 9 %;DrawPipeline::Enqueueon a full draw queue growing from 11 % to 19 %.The CPU this PR saves turns into waiting, so on this page it cannot show as frame time until the draw thread gets faster. I'm keeping the PR as a draft until then and will re-measure.
The earlier flag A/B was noise. It ran
APS5_NO_RESULT_RECYCLE=1against the default in one uncommitted build (the traces say2ef1dd3beb, dirty) and read p50 89.2 / 84.2 -> 81.8 / 82.8 ms. Its first "off" round landed in the slow mode, which this clean rerun shows is noise. Its traces: recycle-s2-round1.zip, recycle-s2-round2.zip.No mismatches.
APS5_VERIFY_MASKED_RESULT_MEMO=1with recycling on: 0 patch mismatches over about 440,000 patched results.Tests
ctest(2064 tests) onmaine7f46f5 plus perf(shader): reuse a memoized result by patching its copied words #10423 plus this PR: everything passes exceptagc_driver_sample_order,agc_driver_sample_order_grid_referenceandagc_driver_sample_order_recipe_depth_locations, which fail the same way on plainmaine7f46f5 on this machine (as in perf(shader): reuse a memoized result by patching its copied words #10423).ctest --test-dir build -R "^agc_shader_|^agc_driver_draw_cache" -VwithANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation: 48/48 onmaine7f46f5, 49/49 with perf(shader): reuse a memoized result by patching its copied words #10423, and 50/50 with this PR (the extra one isagc_shader_table_snapshot_reuse_unrecycled). None of the logs haswas not found but was requested by env var VK_INSTANCE_LAYERSor aVUID-line.The summaries of all four clean rounds follow, then the full digests of round 1. All four traces are attached:
recycle-clean-s2-round1.zip
recycle-clean-s2-round2.zip
Round 1, #10423 54d7c78
seconds 200:15 from present 3354: 182 presents, frame p50 80.1 / p90 86.8 / p99 116.5 / max 137.9 ms, GPU busy 74.6 ms per present
NVIDIA GeForce RTX 2080 Ti, NVIDIA 617.14, Windows 10.0.26300, 3840x2160, commit 54d7c78, dropped: yes
{"commit":"54d7c780a583","dirty":false,"env":{"APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"200:15"},"gpu":"NVIDIA GeForce RTX 2080 Ti","driver":"NVIDIA 617.14","os":"Windows 10.0.26300","cpu":"Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz","cpu_threads":16,"memory_mib":32684,"title_id":"PPSA05324","title":"Sonic Origins","content_version":"02.002.000","resolution":"3840x2160","window":{"spec":"seconds 200:15","first_present":3354,"ms":15050.577},"presents":182,"frame_ms":{"p50":80.127,"p90":86.84,"p99":116.533,"max":137.889},"gpu_busy_ms_per_present":74.62,"worker_busy_pct":{"queue worker 0x0":97.001},"top_phases_ms_per_frame":{"Driver.Packet":80.115,"Graphics.Draw":62.677,"Driver.Packet.draw":60.628,"Driver.Draw":59.584,"Graphics.Draw.draw_and_resource_release":52.265,"Driver.Draw.shader_compile_and_link":37.047,"VideoOut.Flip":32.487,"Driver.Present":32.397},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":6.102,"events_lost":386130,"totals_full":false},"draw committer":{"events_full_after_s":12.778,"events_lost":47537,"totals_full":false}},"trace":"aps5-trace-54d7c780a583-9420.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 74.5, VideoOut.Flip 32.5, Driver.Present 32.4, VideoOut.Flip.present 32.2, Driver.Present.render_fence_wait 31.9, Vulkan.Retire 31.9, Vulkan.Retire.wait 31.9, Graphics.Draw 31.7, Graphics.Draw.draw_and_resource_release 26.1, Driver.Packet 17.5
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
Round 1, this PR bdca440
seconds 200:15 from present 3333: 165 presents, frame p50 84.6 / p90 116.3 / p99 192.6 / max 193.5 ms, GPU busy 74.7 ms per present
NVIDIA GeForce RTX 2080 Ti, NVIDIA 617.14, Windows 10.0.26300, 3840x2160, commit bdca440, dropped: yes
{"commit":"bdca440d390c","dirty":false,"env":{"APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"200:15"},"gpu":"NVIDIA GeForce RTX 2080 Ti","driver":"NVIDIA 617.14","os":"Windows 10.0.26300","cpu":"Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz","cpu_threads":16,"memory_mib":32684,"title_id":"PPSA05324","title":"Sonic Origins","content_version":"02.002.000","resolution":"3840x2160","window":{"spec":"seconds 200:15","first_present":3333,"ms":15011.098},"presents":165,"frame_ms":{"p50":84.575,"p90":116.336,"p99":192.64,"max":193.492},"gpu_busy_ms_per_present":74.693,"worker_busy_pct":{"queue worker 0x0":97.218},"top_phases_ms_per_frame":{"Driver.Packet":88.322,"Graphics.Draw":69.74,"Driver.Packet.draw":66.729,"Driver.Draw":65.594,"Graphics.Draw.draw_and_resource_release":58.618,"Driver.Draw.shader_compile_and_link":38.525,"VideoOut.Flip":30.932,"Driver.Present":30.913},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":6.173,"events_lost":385570,"totals_full":false},"draw committer":{"events_full_after_s":13.162,"events_lost":38828,"totals_full":false}},"trace":"aps5-trace-bdca440d390c-24144.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 74.7, Graphics.Draw 38.8, Graphics.Draw.draw_and_resource_release 31.6, VideoOut.Flip 30.9, Driver.Present 30.9, VideoOut.Flip.present 30.8, Driver.Present.render_fence_wait 30.4, Vulkan.Retire 30.4, Vulkan.Retire.wait 30.3, Driver.Packet 22.3
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
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)