Skip to content

perf(agc): drop three per-draw allocations on the draw and materialize paths - #9177

Merged
adriadam10 merged 3 commits into
boykopovar:mainfrom
milescoviello:perf/draw-allocations
Oct 24, 2026
Merged

adriadam10 merged 3 commits into
boykopovar:mainfrom
milescoviello:perf/draw-allocations

Conversation

@milescoviello

@milescoviello milescoviello commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What

Three allocations that ran on every draw or stage materialization, in three commits:

  1. Driver::draw copies the draw programs for every draw. Since the draw finisher (perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946), the draw tail takes programs = decode->programs on every draw: one allocation, plus one userData vector and two shared_ptr reference bumps per program. Before perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946, the copy was made only for draws that write user words (the mesh index buffer words, indirect record words or relocated user patches), and every other draw read decode->programs in place.
    • DrawTail::writablePrograms now holds that copy, made on the same condition as before, and DrawTail::programs points at it or at decode->programs. Every write goes through writablePrograms, so a const view of the decode can't be written.
    • The queued commit closures move writablePrograms. When it is empty, the captured decode keeps decode->programs alive. decode is only replaced (by the relocated copy) on draws with user patches, and those take the copy.
  2. Driver::draw reserves programs.size() + 2 RecompileResults for every draw. Only three paths fill results: the rect-list pair, barycentric emulation and CPU-path indirect draws. The reserve now sits right before the first push on each of those paths, with the same bound, so &results[i] stays stable. An ordinary direct draw no longer allocates there.
  3. Three small per-materialization allocations in binding population:
    • DescriptorBindingBuilder::Populate reserves its exact shaderDataUserWords count.
    • GuestSamplersDescriptor reserves four dwords per sampler, as GuestBuffersDescriptor already does.
    • ResolveImageTablesShared returns one shared empty ResolvedImageTables when there are no tables, still calling ResolveImageTables first so its mismatch check keeps throwing.

Tested

Rebased onto main 1631964, after #6946's draw finisher, as 9e6966b121. #6946 moved the end of Driver::draw into a DrawTail that owns its own copy of the programs, and its commit closures move that copy. So the old first commit (capturing writablePrograms instead of copying a const vector) no longer applied, and the copy it avoided became one made on every draw. The new first commit makes that copy only for the draws that write user words, as before #6946. The other two commits are replayed onto the new structure. The second commit's removed reservation now sits in the tail setup.

On that head, Ubuntu 26.04.1 (kernel 7.0), GCC 15.2, NVIDIA RTX 4070 Ti, driver 615.71.09, Release:

  • The 62 tests matching mesh_draw_relocation|draw_cache_relocation|pipelined_indirect_draw|indirect_patched_stage|rect_list_indirect|rect_list_corner|pixel_barycentric_emulation|image_table|shader_disk_cache|_draw_pipeline$|draw_finisher|draw_pipeline_trace|draw_data_hit_user_words|mesh|indirect|queued_raw_shader pass 3 of 3. That's the earlier 15 plus perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946's draw finisher and pipelined-draw tests and the other mesh and indirect tests. Two of them, agc_driver_pipelined_metadata_pass_draw_pipeline and agc_driver_finisher_label_order_draw_pipeline, ran in a second pass after their executables were built.
  • Windows 11 VM, GeForce GT 1030 (R580), WinLibs GCC 15.2, Release, ANYPS5_REQUIRE_VULKAN=1: the 34 tests matching mesh_draw_relocation|draw_cache_relocation|pipelined_indirect_draw|indirect_patched_stage|rect_list_indirect|rect_list_corner|pixel_barycentric_emulation|image_table|shader_disk_cache|draw_finisher|draw_pipeline_trace|draw_data_hit_user_words|draw_cache_reuse|draw_write_ranges pass 3 of 3 on main 1631964 and 3 of 3 on this head.

CoD Black Ops II (PPSA34502), war-room menu: current main against this head, same machine and scene.

round base presents per 10 s (menu mean) with this PR base mean frame in the window (ms) with this PR PR minus base
1 (both cold) 11.3 9.0 16.7 (the window fell in the intro) 1,079 not a usable pair
2 (PR first) 9.9 10.4 1,108 976 -12.0 %
3 14.0 12.1 716 865 +20.9 %

On current main this still shows no measurable difference.

  • Between rounds 2 and 3, main alone went from 9.9 to 14.0 presents per 10 s, and the two usable pairs differ by -12 % and +21 %.
  • A frame takes 0.7 to 1.1 s, and 570 to 920 ms of that is Driver.Draw, mostly Graphics.Draw.draw_and_resource_release. Three small allocations per draw don't show next to that.
  • Round 1 was each side's first run on its own shader cache version, and in the base run the intro lasted until present 7333, so its window caught 60 FPS intro frames. The menu intervals of round 1 are listed for completeness only.

The summaries, traces and comparator reports of all six runs are in this release. Round 3's window lines:

Base:

{"commit":"f1a1b4698721","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_FRAMES":"7270:10","APS5_TRACE_SUMMARY_EVERY":"10"},"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":"PPSA34502","title":"Call of Duty\u00c2\u00ae: Black Ops II","content_version":"01.010.000","resolution":"1920x1080","window":{"spec":"frames 7270:10","first_present":7270,"ms":7159.304},"presents":10,"frame_ms":{"p50":717.667,"p90":789.638,"p99":789.638,"max":789.638},"gpu_busy_ms_per_present":104.532,"worker_busy_pct":{"queue worker 0x0":87.774},"top_phases_ms_per_frame":{"Driver.Packet":600.534,"Graphics.Draw":579.031,"Graphics.Draw.draw_and_resource_release":573.217,"Driver.Packet.draw":568.397,"Driver.Draw":567.372,"Driver.Draw.draw_enqueued":502.342,"Shader.ResolveAbi":102.387,"Shader.ResolveGraphicsStagesAbi":79.292},"trace_dropped":false,"trace":"aps5-trace-f1a1b4698721-3.json.gz"}

This PR:

{"commit":"dc10dbdc15d0","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_FRAMES":"7270:10","APS5_TRACE_SUMMARY_EVERY":"10"},"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":"PPSA34502","title":"Call of Duty\u00c2\u00ae: Black Ops II","content_version":"01.010.000","resolution":"1920x1080","window":{"spec":"frames 7270:10","first_present":7270,"ms":8652.715},"presents":10,"frame_ms":{"p50":872.987,"p90":912.655,"p99":912.655,"max":912.655},"gpu_busy_ms_per_present":116.45,"worker_busy_pct":{"queue worker 0x0":89.515},"top_phases_ms_per_frame":{"Driver.Packet":739.23,"Graphics.Draw":713.162,"Graphics.Draw.draw_and_resource_release":707.611,"Driver.Packet.draw":703.764,"Driver.Draw":702.804,"Driver.Draw.draw_enqueued":639.762,"Shader.ResolveAbi":101.288,"Shader.ResolveGraphicsStagesAbi":75.025},"trace_dropped":false,"trace":"aps5-trace-dc10dbdc15d0-3.json.gz"}

Before the rebase onto #6946.

Rebased onto main cc00785 as 07e939750f, with no conflicts. git range-diff shows the three commits unchanged. On that head, on Ubuntu 26.04.1 (kernel 7.0), GCC 15.2, NVIDIA RTX 4070 Ti, driver 615.71.09, Release: the 15 tests matching mesh_draw_relocation|draw_cache_relocation|pipelined_indirect_draw|indirect_patched_stage|rect_list_indirect|rect_list_corner|pixel_barycentric_emulation|image_table|shader_disk_cache pass 3 of 3.

CoD Black Ops II (PPSA34502), war-room menu: main 4777cac against the pre-rebase head, same machine and scene.

round base presents per 10 s (menu mean) with this PR base frame p50 (ms) with this PR mean frame time, PR minus base
1 11.0 10.3 950 961 -0.2 %
2 (PR first) 10.4 9.6 1028 1107 +9.5 %
3 11.9 12.0 880 889 +0.2 %

On main this shows no measurable difference. A frame takes about 0.9 to 1.1 s, and 690 to 890 ms of it is Graphics.Draw.draw_and_resource_release, the per-draw snapshot copies that #8756 removes. Three small allocations per draw don't show next to that, and round 2's gap is within this machine's run-to-run noise. The summaries, traces and comparator reports of all six runs are in this release. Round 3's window lines:

Base:

{"commit":"9d56f02254da","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_FRAMES":"7270:10","APS5_TRACE_SUMMARY_EVERY":"10"},"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":"PPSA34502","title":"Call of Duty\u00c2\u00ae: Black Ops II","content_version":"01.010.000","resolution":"1920x1080","window":{"spec":"frames 7270:10","first_present":7270,"ms":8718.58},"presents":10,"frame_ms":{"p50":879.643,"p90":914.046,"p99":914.046,"max":914.046},"gpu_busy_ms_per_present":118.379,"worker_busy_pct":{"queue worker 0x0":90.263},"top_phases_ms_per_frame":{"Driver.Packet":757.556,"Driver.Packet.draw":713.589,"Driver.Draw":712.425,"Graphics.Draw":694.609,"Graphics.Draw.draw_and_resource_release":688.25,"Driver.Draw.draw_enqueued":627.459,"Shader.ResolveAbi":94.793,"Shader.ResolveGraphicsStagesAbi":69.168},"trace_dropped":false,"trace":"aps5-trace-9d56f02254da-3.json.gz"}

This PR:

{"commit":"5b24a5b8c3fb","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_FRAMES":"7270:10","APS5_TRACE_SUMMARY_EVERY":"10"},"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":"PPSA34502","title":"Call of Duty\u00c2\u00ae: Black Ops II","content_version":"01.010.000","resolution":"1920x1080","window":{"spec":"frames 7270:10","first_present":7270,"ms":8732.553},"presents":10,"frame_ms":{"p50":889.031,"p90":912.664,"p99":912.664,"max":912.664},"gpu_busy_ms_per_present":116.286,"worker_busy_pct":{"queue worker 0x0":90.086},"top_phases_ms_per_frame":{"Driver.Packet":751.697,"Driver.Packet.draw":717.244,"Driver.Draw":715.92,"Graphics.Draw":706.998,"Graphics.Draw.draw_and_resource_release":699.975,"Driver.Draw.draw_enqueued":627.015,"Shader.ResolveAbi":86.669,"Driver.Draw.shader_compile_and_link":71.515},"trace_dropped":false,"trace":"aps5-trace-5b24a5b8c3fb-3.json.gz"}

Supporting context only, not evidence against main: the same menu on a faster composite base. The runs below are from before the rebase: Ubuntu 24.04, GCC 13.3, NVIDIA RTX 4070 Ti, driver 595.99.02, i7-13700K.

Suites on main e072e88:

CoD Black Ops II (PPSA34502), war-room menu, on a composite base. Release libs, 12 GB cap, the scripted run, 10 s summaries from 140 s. On main alone the menu is bound by per-draw snapshot copies (about 9 presents per 10 s), and the three commits make no measurable difference there: 9.2 against 9.2 over three rounds. So the comparison runs on main f894ba6 with #8975 (its pre-merge head), #8756, #8888 and #8928 merged (base 690fd73114), and the same three commits on it, where the menu reaches about 60 presents per 10 s and these allocations are a visible part of the queue worker's time. Six alternating rounds, after one warm-up round each:

round base presents per 10 s (mean) with this PR
1 59.0 61.1
2 61.6 61.2
3 59.5 63.3
4 56.5 60.4
5 60.8 63.0
6 62.2 60.9

The mean is 59.9 against 61.7, +2.9 %, and this PR is ahead in 4 of 6 rounds. That's small next to the run-to-run noise on this machine, which shares its CPU with other services. Treat it as a modest gain, not a proven one. Round 4's base run lost its first intervals to host load.

Captures of round 6, the last one (summary files, traces and tools/trace_digest.py output), are in this release. The 220:5 window lines:

Base:

{"commit":"690fd731145a","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"220:5","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","cpu":"13th Gen Intel(R) Core(TM) i7-13700K","cpu_threads":24,"memory_mib":32022,"title_id":"PPSA34502","title":"Call of Duty\u00c2\u00ae: Black Ops II","content_version":"01.010.000","resolution":"1920x1080","window":{"spec":"seconds 220:5","first_present":7848,"ms":5124.782},"presents":32,"frame_ms":{"p50":158.175,"p90":175.641,"p99":179.709,"max":179.709},"gpu_busy_ms_per_present":28.208,"worker_busy_pct":{"queue worker 0x0":77.066},"top_phases_ms_per_frame":{"Driver.Packet":122.550,"Driver.Packet.draw":101.710,"Driver.Draw":100.761,"Driver.Draw.shader_compile_and_link":65.929,"Graphics.Draw":60.745,"Graphics.Draw.draw_and_resource_release":54.655,"Shader.DrawStage":54.205,"Shader.ResolveGraphicsAbi":32.483},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":4.434,"events_lost":42081,"totals_full":false}},"trace":"aps5-trace-690fd731145a-3.json.gz"}

This PR:

{"commit":"229a439d32e7","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"220:5","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","cpu":"13th Gen Intel(R) Core(TM) i7-13700K","cpu_threads":24,"memory_mib":32022,"title_id":"PPSA34502","title":"Call of Duty\u00c2\u00ae: Black Ops II","content_version":"01.010.000","resolution":"1920x1080","window":{"spec":"seconds 220:5","first_present":7836,"ms":5156.383},"presents":31,"frame_ms":{"p50":166.949,"p90":194.986,"p99":201.887,"max":201.887},"gpu_busy_ms_per_present":30.604,"worker_busy_pct":{"queue worker 0x0":74.916},"top_phases_ms_per_frame":{"Driver.Packet":123.340,"Driver.Packet.draw":101.303,"Driver.Draw":100.314,"Driver.Draw.shader_compile_and_link":66.453,"Graphics.Draw":61.821,"Shader.DrawStage":56.791,"Graphics.Draw.draw_and_resource_release":55.914,"Shader.ResolveGraphicsAbi":33.164},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":4.932,"events_lost":12130,"totals_full":false}},"trace":"aps5-trace-229a439d32e7-3.json.gz"}
  • macOS, a title bound by the queue worker: @imellag ran it on an Apple M4 Pro (macOS 27.0.1, MoltenVK 1.4.2, x86-64 Release under Rosetta 2). main doesn't build on macOS, so the base was main cecb540 plus the macOS port PRs, with this PR's three commits (pre-rebase head 401aab07d9, the same commits) cherry-picked onto it. Arms alternated, scripted input.

    • Planet of Lana (PPSA19450), about 650 draws a frame with the queue worker as the limit: 29.70 fps mean on the base, 30.09 with this PR (+1.3 %). Every PR run is above the base mean, and the busiest queue worker goes from 71-73 % to 69-70 %.
    • God of War Sons of Sparta (PPSA28997), which is GPU-bound: unchanged, no regression.
  • AMD, Linux: @JJenkx ran it on an RX 6650 XT (RADV, Mesa 26.2.4) with ASTRO BOT booted without input, the PR against its own base, alternated base, PR, PR, base. FPS 21.40, 21.42 on the base and 21.54, 21.18 with the PR: no change and no regression on that scene. Swapping in jemalloc gains about 4 % there, so allocation does show, but these three allocations are too small to.

  • Windows 11 Enterprise 26300, NVIDIA GeForce GT 1030 (driver 582.78), WinLibs GCC 15.2.0 r7 (UCRT), Release: the 28 AGC relocation, indirect, rect-list, barycentric, draw descriptor and pipelined tests pass 3 of 3 on main and 3 of 3 with this change. The pattern also matched 6 relinker relocation tests. They could not start on either side, because that run didn't build the relinker they launch.

Not tested: Intel.

Overlap (the generated report on this head: a textual conflict with #9049; #6804, #7369, #9387 and #9508 also share Draw.cpp but conflict with main, so they weren't compared):

Checklist

🤖 Generated with Claude Code

@github-actions github-actions Bot added the perf Performance change label Oct 21, 2026
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 6804 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp L999, L1045
PR 9049 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp L140-141
PR 9599 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp L398-401

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.

Pull request Shared files not compared
PR 907 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 1896 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 5371 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 5431 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 5441 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 5609 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
PR 5925 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 6188 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 6280 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 6412 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 6488 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 6539 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
PR 7201 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 7223 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 7369 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 7642 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 7684 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 7689 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 8077 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 8998 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 9387 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 9852 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
PR 9889 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 9897 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
PR 9898 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
PR 9969 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 10049 core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
PR 10193 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp
core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 10233 core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp
PR 10420 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp

@JJenkx

JJenkx commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

AMD/Linux data point, for the "Not tested: AMD" line. The result is neutral: no change and no regression.

Setup

  • ASTRO BOT (PPSA21564) title screen ("PRESS ANY BUTTON"), reached with no input, so the scene is the same in every run. It is not a gameplay scene.
  • Linux 7.2.9 (Arch), Ryzen 9 7950X, RX 6650 XT, RADV (Mesa 26.2.4).
  • This PR's head 401aab0 against its own base main e072e88; both builds are clean, and one relinked app.elf is shared by both.
  • ANYPS5_PRESENT_MODE=immediate, APS5_TRACE_SECONDS=150:30, APS5_TRACE_EVENTS=4194304.
  • One warm-up run, then alternated base, PR, PR, base; 240 s each.
  • Every trace reports dirty false and dropped false.
Run FPS (presents/window) frame p50 / p99 ms GPU busy ms/present
base 21.40 46.5 / 52.4 34.0
this PR 21.54 46.3 / 52.2 34.0
this PR 21.18 47.0 / 53.9 34.1
base 21.42 46.4 / 53.2 33.9

Means are 21.36 FPS for this PR and 21.41 for the base, within noise. Swapping the whole allocator for a preloaded jemalloc gained about 4 % on this scene (#357), so allocation cost is visible here, but the three allocations this PR removes are too small to show.

AI-assisted: yes (Claude Code); results above were run on this machine.

@milescoviello

Copy link
Copy Markdown
Contributor Author

Thanks @JJenkx. A neutral result on RADV fits what this PR removes: three small allocations per draw, which only show on a frame bound by the draw path, as BO2's menu is here. I added your run to the Tested section and the GPU line.

AI-assisted: yes (Claude Code)

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

I did not find a correctness defect in the three allocation changes on this head. The move capture is valid for the local writable program vector, and the scoped reservations match the result-producing paths. I’m requesting changes for the performance evidence and the current overlap state.

Performance evidence

The CONTRIBUTING performance guidance asks for main and change summary lines from the same machine and scene, with traces attached per the issue form. The only paired traces in this PR are from the composite base 690fd731145a and the candidate. Your description explicitly says that base includes #8975, #8756, #8888, and #8928, and correctly warns that the +2.9% result is small and noisy.

I ran the repository comparator on those attached summaries and traces. With --require-matching-context, it rejects the pair because window.first_present differs (7848 vs 7836). Without that requirement, it warns that the measurements do not isolate the code change. Both traces also report dropped events, so the trace detail is incomplete even though the summary data is available. The PR mentions a separate main-alone 9.2 vs 9.2 result, but does not include its matching summary/trace evidence.

Please add a same-machine, same-scene comparison of current main and this change, with the matching summary lines and traces (or explain any unavoidable context mismatch in the report). Please keep the composite-base result clearly labeled as supporting context rather than evidence of the change against main. The current head is 599 commits behind current main, so please also refresh the base and rerun the focused validation after rebasing.

Overlap

The generated overlap report now lists #6946 and #9508 in addition to #6804, #7369, and newer #9387. These are not all duplicates, but the impact should be recorded:

  • #6946’s current Draw.cpp still unconditionally reserves results for programs.size() + 2, which would erase this PR’s ordinary-draw allocation saving if that path is combined as written. Its draw-finisher also changes the ownership/capture flow, so the vector-capture optimization should be rechecked against the combined implementation.
  • #9508 is stacked on #6946 and inherits that interaction.
  • #9387 changes the barycentric-emulation branch in the same draw function, which is one of the paths whose result reservation this PR moves.

Please add the relevant PRs to the overlap notes/checklist and coordinate the combined behavior with those authors as needed. The existing #6804 and #7369 intersections appear to be neighboring, distinct changes.

Checks I ran

  • git diff --check: passed.
  • Repository conventions against current main: passed.
  • Three-way merge check against current main: clean.
  • I did not run a local AGC build or title benchmark on this head; the submitted report contains extensive hardware testing, but the missing current-main paired evidence remains.

The code changes themselves look reasonable. I’ll revisit the review when the performance comparison and overlap notes are updated.

@milescoviello

Copy link
Copy Markdown
Contributor Author

@MarcZX10 thanks.

Rebase: onto main cc00785 as 07e939750f, with no conflicts. The three commits are unchanged, and the 15 focused tests pass 3 of 3.

Current main against this PR: three alternating rounds in BO2's war-room menu on the same machine.

  • The base is main 4777cac plus fix(libSceVoice): report inactive input voice activity #9656, which the title needs to start.
  • Each run traces the same 10 presents (APS5_TRACE_FRAMES=7270:10). The windows start at the same present with the same environment and nothing dropped, and --require-matching-context accepts all three pairs.
  • The result is no measurable difference on main: mean frame time -0.2 %, +9.5 % (the round with this PR first) and +0.2 %. A menu frame there is about a second, and 690 to 890 ms of it is Graphics.Draw.draw_and_resource_release, the per-draw snapshot copies perf(agc): take a draw snapshot from a kept snapshot that contains it #8756 removes. Three small allocations per draw don't show next to that.
  • The summaries, traces and comparator reports are in Tested and in the linked release.
  • The composite-base result is now labelled as supporting context only.

Overlap: recorded under Overlap in Tested.

@Raghav314 (#6946, #9508): do you have a preferred order? If #6946 goes first, I'll rebase this onto it and move the three reservations and the writablePrograms capture into your finisher. If this goes first, the same two changes apply on your side. @Alexius0815 (#9387): I don't think we need an order. Correct me if your barycentric branch can append more than two results.

AI-assisted: yes (Claude Code)

@imellag

imellag commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

An Apple / MoltenVK data point, on a title whose frame is bound by the queue worker. A small gain there.

Setup: Apple M4 Pro, macOS 27.0.1, MoltenVK 1.4.2, x86-64 Release under Rosetta 2. main does not build on macOS, so the base is main cecb540 + #8658 + #9193 + #9263 + #8612. This PR's three commits (head 401aab0) cherry-pick onto it cleanly. Warm shader caches, APS5_TRACE_SUMMARY_EVERY, no other trace or profile switch, arms alternated, scripted input.

Scene base + this PR
Planet of Lana (PPSA19450), village, standing still, 110 to 190 s 29.40 / 30.04 / 29.83 / 29.51 fps (mean 29.70) 30.08 / 30.13 / 29.89 / 30.25 fps (mean 30.09, +1.3%)
busiest queue worker, same windows 71 to 73% 69 to 70%
God of War Sons of Sparta (PPSA28997), Cyclops fight 39.72 / 39.57 fps 39.90 / 39.88 fps (GPU-bound at 22.8 ms in both, neutral)

In Lana every run of this PR is above the mean of the base runs, and the worker is busy about 3 points less. That fits the per-draw allocations this removes: Lana records about 650 draws a frame with the queue worker as the limit. God of War is limited by the GPU, so it shows no change, and no regression.

AI-assisted: yes (Claude Code). The runs are from my M4 Pro.

@milescoviello

Copy link
Copy Markdown
Contributor Author

@imellag thanks for the M4 Pro runs. Planet of Lana, with about 650 draws a frame and the queue worker as the limit, is the kind of scene where these allocations should show. Every PR run was above the base mean, and the worker dropped about 3 points, which fits that. God of War being neutral is also useful, as it shows no regression in a GPU-bound case. Added to Tested.

AI-assisted: yes (Claude Code)

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 23, 2026
milescoviello and others added 3 commits October 23, 2026 05:44
Since the draw finisher (boykopovar#6946), Driver::draw copies decode->programs
into the draw tail for every draw: one outer vector, one userData
vector per program and two shared_ptr reference bumps per program.
Before it, the copy was made only when the draw writes user words
(mesh index buffer words, indirect record words or relocated user
patches), and every other draw read decode->programs in place.

Keep the copy in DrawTail::writablePrograms and make DrawTail::programs
a pointer to it or to decode->programs, as before. Every write goes
through writablePrograms, and the draws that write are exactly the
mesh, indirect and patched ones, so a const view of decode->programs
can't be written.

The queued commit closures now move writablePrograms. When it is empty
the captured decode keeps decode->programs alive, and decode is only
replaced (by the relocated copy) on draws with user patches, which
take the copy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Driver::draw reserved programs.size() + 2 RecompileResults for every
draw, but `results` only receives elements on three paths: the
rect-list tessellation pair (2), barycentric emulation (1 geometry
shader or 2 tessellation shaders) and CPU-path indirect draws (one copy
per compiled program). An ordinary direct draw never touches it, so the
reservation was a sizeable heap allocation thrown away each draw.

Move the reserve to just before the first push on each of those paths.
Every site keeps the old programs.size() + 2 bound, which covers the
largest combination (rect-list or barycentric emulation, then the
indirect copies; rect-list and barycentric emulation exclude each
other). The first reserve on a path therefore sets the same capacity
as before, later ones are no-ops, and the &results[i] pointers kept in
`stages` stay valid.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tion

Three allocations ran on every stage materialization:

- DescriptorBindingBuilder::Populate pushed one shaderDataUserWords
  entry per layout.userDataRegisters element without reserving, so the
  vector regrew through 1, 2, 4, ... Reserve the exact count first.
- GuestSamplersDescriptor appended each sampler's dwords without a
  reserve. Samplers are four dwords (the table path forces 4, and
  Populate indexes the result as element * 4), matching the
  resources.size() * 4 reserve GuestBuffersDescriptor already uses.
- ResolveImageTablesShared made a fresh shared ResolvedImageTables for
  every snapshot without image tables. With no tables,
  ResolveImageTables either throws (the shader expects tables) or
  returns a default-constructed result, so return one shared, immutable
  empty instance instead, still calling ResolveImageTables first so the
  mismatch keeps throwing. Its only caller reads the result through a
  const reference and never compares the pointer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@milescoviello

Copy link
Copy Markdown
Contributor Author

@MarcZX10 #6946 has landed, so I rebased onto main 1631964 and carried it as the overlap note said. The new head is 9e6966b121.

  • The first commit is new. perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946's DrawTail copies decode->programs on every draw, and its closures move that copy, so the old const-vector capture fix no longer applied. The new commit makes the copy only for draws that write user words, as before perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946. The other two commits are replayed: the reservations stay in the three paths that fill results.
  • Tests: the focused set, now with perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946's draw finisher and pipelined-draw tests (62), passes 3 of 3 on Linux. 34 of them pass 3 of 3 on the Windows VM, on main and on this head.
  • Current main against this head, same machine and scene: CoD BO2's war-room menu, three rounds with summaries, traces and comparator reports for every run (release).
    • The two usable pairs differ by -12 % and +21 % in mean frame time, and main alone ranges from 9.9 to 14.0 presents per 10 s.
    • So there's no measurable difference on main, where most of the frame is per-draw snapshot copies. Round 1 isn't a usable pair, because the base's window fell in the intro; the table says so.
  • Overlap: the report now shows a textual conflict with perf(shader): reuse an evicted memo result's bindings for the next miss #9049 (GuestSamplersDescriptor). The resolution is to keep the reserve after its clear(). I updated the description's Overlap and checklist.

AI-assisted: yes (Claude Code)

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 23, 2026
@EdouardSence

Copy link
Copy Markdown
Contributor

@milescoviello confirmed from the #9049 side: the resolution is right. With #9049, GuestSamplersDescriptor fills the caller's reused physical.guestDescriptor, so the merged body is result.clear(); then result.reserve(resources.size() * 4u);. On a reused vector the reserve does nothing once capacity is there, and it still saves the regrowth on a first fill. The two changes don't interact otherwise, so either merge order works.

Written by Claude (AI assistant) on a contributor's machine.
AI-assisted: yes

@milescoviello

Copy link
Copy Markdown
Contributor Author

@EdouardSence thanks for confirming from the #9049 side. I'll keep the reserve after your clear() if this lands second.

AI-assisted: yes (Claude Code)

@EdouardSence

Copy link
Copy Markdown
Contributor

A Linux/RADV data point for 9e6966b in a scene where the committer is the wall: level within noise, and clean.

Setup: ASTRO BOT (PPSA21567) Sky Garden (-lvl underwater_aerial_garden), 3840x2160, no input, headless gamescope. Ryzen 7 7800X3D, RX 9070 XT (RADV Mesa 26.2.4), GCC, Release.

round base p50 / p99 ms this PR p50 / p99 ms
1 206.7 / 217.6 205.0 / 215.8
2 206.9 / 220.3 204.3 / 217.0
3 205.8 / 225.7 206.5 / 221.6

The mean p50 is 206.5 against 205.3 ms (−0.6 %), and this PR won 2 of 3 rounds. Run-to-run spread here is about ±1.5 %, so this is level. The three allocations it drops don't show in this scene's committer, where preparation is spread over many smaller costs. No device loss, page fault or VUID on any run.

Measured and written by Claude (AI assistant) on a contributor's machine.
AI-assisted: yes

@milescoviello

Copy link
Copy Markdown
Contributor Author

@EdouardSence thanks. A scene where the committer is the wall is the right place to look, and level within ±1.5 % on RDNA 4 with no device loss, faults or VUIDs matches what BO2's menu shows on NVIDIA. I added the card to the description's GPU list.

AI-assisted: yes (Claude Code)

@adriadam10 adriadam10 self-assigned this Oct 23, 2026
@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 24, 2026 10:21

Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit 66de8e7 into boykopovar:main Oct 24, 2026
14 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Re-checked against current main (head unchanged, merges cleanly):

  • ResolveImageTablesShared with no tables still calls ResolveImageTables, which returns a default ResolvedImageTables when no image or sampler uses a table and throws on a table-count mismatch; the shared empty result is equivalent after the changes on main.
  • Every write to the draw programs goes through writablePrograms, and writesPrograms covers the mesh, indirect and user-patch cases.
  • MarcZX10's earlier request (same-machine traces, overlap) is answered in the description on this head.

No shader semantics change, so nothing to measure on the oracle; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* perf Performance change shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants