Repository navigation
perf(agc): merge shared shader regions with a heap - #11957
Conversation
github-actions
Bot
added
perf
|
This pull request is labeled |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The draft correctly discloses that its required title performance report is still missing. I found one separate integration blocker to resolve before this can be ready: a direct merge preview of the current #11957 and #9626 heads conflicts in docs/dev/ENVIRONMENT.md. The two different environment rows were inserted at the same location, so the branches do not currently integrate automatically even though both entries should be retained.
Please agree the order with the #9626 author, state the prerequisite in the checklist (Depends on: #9626 if it must land first, or the agreed reverse order), and rebase/update so both environment rows survive. Keep the PR in draft until it also has the required same-title, same-scene, same-machine aps5-summary.jsonl lines and attached traces; the synthetic microbenchmark is explicitly not a substitute for that report.
I have not approved the draft.
|
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.
|
7b25204 to
7cbd7ee
Compare
github-actions
Bot
added
build
agc
7cbd7ee to
2ce643b
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
@Bram910 thanks for adding the matched menu summaries and traces and keeping their original revisions explicit. They support the stated menu result, with no gameplay speedup claim. The earlier review also asked which heap sizes titles actually reach: could you add the shared-block-count distribution per Regions()/TakeRecentRegions() call in that scene, with collection cost for the same workload? The synthetic 7-64-block gains do not show how often a title enters that regime. If gameplay is still blocked, please keep that limitation and the menu/microbenchmark results separate. @adriadam10, leaving the performance qualification and landing decision with you. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 2ce643be49d775cded5dc7c5453b2cbae82949b4. The required matched menu captures, summary lines, traces and digests are now present, and the author clearly states they show no FPS gain or gameplay result. The current pairwise merge preview with #9626 is clean and the body records both merge orders, so my earlier integration concern is resolved.
One performance question remains before I can approve: the synthetic benchmark shows gains starting at two shared blocks, but the title/menu trace does not report the shared-block-count distribution or collection cost per Regions()/TakeRecentRegions() call. The gameplay capture cannot reach gameplay even on clean main, so it does not establish that real workloads spend meaningful time in the multi-block cases. Please add that distribution and same-workload collection cost for the captured scene as requested in the latest review, or keep the PR explicitly limited to the synthetic complexity result and provide another supported workload that exercises the affected path. The exact-head Windows focused tests pass; Linux/macOS and the full project suite were not run locally.
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 436 tests)Skipped (39)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Checked the heap logic in |
What
Collecting a shader's captured memory regions repeatedly scans every supplied shared block to choose the next run, including while emitting interspersed initial regions. Use a min-heap when multiple shared blocks participate, advancing only the block whose next run was consumed. Keep the linear zero/one-block path and the existing merge rules for initial regions and captured pages. Shared blocks are already validated as ordered and disjoint by supplyWords; that validation and guest-write tracking stay in place.
The regression captures real resource tables with 0, 1, 2, 3, 7, 8, 16 and 64 supplied blocks, contiguous and sparse multi-run layouts, reverse insertion, captured descriptor pages and interspersed initial regions. It checks order, byte contents, complete record coverage, initial-region visibility and recent-read reset behavior.
Performance scope: the attached matched main/PR GTA Story-menu captures are explicitly identified by their original commit IDs below. This vblank-limited menu establishes no gameplay FPS benefit or 30 FPS opening-gameplay result. The synthetic microbenchmark measures collection cost only.
Tested
Publication validation on main
3a40945e62b7e2543031d5da2b8e351ab603bc1aand this PR2ce643be49d775cded5dc7c5453b2cbae82949b4: the same 12 focused shader-memory, SRT, table-snapshot, frontend/layout and prepared-graphics tests pass on both; this PR also passes its region regression (13/13 total) and 20 repeated runs. Both runs used the same RTX 3080,ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. Neither verbose log reports a missing layer, VUID/validation error or skipped/unrun selected test. The benchmark CLI also passes. These are focused Windows tests, not a full-suite or cross-platform run.The rebase changes the surrounding upstream code, so the attached menu pair remains labeled with its actual original revisions (
2ee8e9751a06/7cbd7eeb345c); it is not a fresh frame-rate measurement of the rebased head.ShaderMemory::collectis byte-identical between each original trace revision and its corresponding refreshed main/PR version. No gameplay speedup is claimed.Windows 11 Pro 10.0.26200, AMD Ryzen 9 7950X, MinGW GCC 15.2 Release. The original matched validation built shader-memory, AGC runtime and prepared-graphics test targets on main
75f3702aed32762d0efcde60e998e7edb93c8ae2and the original candidate7b25204cee7ee6f55ad248a5a0b7e1f99f7c94c5.The same 12 focused shader-memory, SRT patching, table-snapshot, frontend/layout and prepared-graphics GPU tests pass on both versions. This branch also passes
agc_shader_region_merge(13/13 total), plus 20 consecutive runs of the new regression.Both versions used
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_GPU=NVIDIA,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationandVK_LAYER_PATH=C:/VulkanSDK/1.4.363.0/Bin. Neither verbose validation log contains missing-layer messages, VUID errors or skipped/unrun selected tests. These are focused tests, not the full project suite; Linux/macOS were not run.Those original main and candidate versions were also measured with identical synthetic collection fixtures and Release test executables: ABBA order repeated three times, six processes per version, 1,000 Regions() calls per size per process. Request capture and correctness checks are outside the timed section. Median microseconds per collection:
This workload includes numerous interspersed initial regions. It is not a title frame-rate benchmark. No shader semantics, rendering quality, GTA saves/runtime or launch safeguards were changed by this submission.
Conventions (
python tools/check_conventions.py --base origin/main) andgit diff --check origin/main...HEADpass. The test benchmark usesagc_shader_memory_tests --region-merge-bench; ordinary--region-mergeruns assertions without timing. No environment-documentation row is added.Integration checks: the new test command is dispatched away from the insertion points used by #12051 and #6959. Both direct merge orders succeed and produce identical trees with #9626 (
7301d0241c1421d0fb37e5773c679629980e06ff), #12051 (f5cf0db16c28ee4a7575c7cd1dda342fd01a9fe6) and #6959 (bf38500c623a21a8a3938937944ee6e89938778a). No code from those PRs is required and no ordering agreement or reviewer approval is claimed.The refreshed project checker examined 1,303 open PRs and found no direct conflicting hunks or duplicate exports/new files among branches it could compare. It still lists older branches that conflict with main and cannot be compared automatically. Their relevant shared-file patches were inspected: #6188 changes speculative reads and #9898 changes capture profiling, neither implements the collection heap; CMake/test neighbors add other fixtures. Refreshed versions of those stale branches may require another integration review. This is not an unconditional all-PR conflict-free claim.
GTA V Story-menu capture pair
Captured on the same PC in one session, using unmodified main
2ee8e9751a06bd4cb7edb8d17f433d56a39a7025and this PR7cbd7eeb345c1f06346a567cd9fd6dca5a165406. Both full runtime builds succeeded and were scanned. GTA V PPSA04263, content 01.010.002, 3840x2160 guest rendering, same 1484x835 client window and save seed. Each runtime reached the Story menu once and was closed and restarted before capture. Controls were released before the window. The Story menu's artwork cycles; this is the same menu state, not a frame-identical frozen image.Both captures use
APS5_TRACE_SECONDS=360:5, withAPS5_TRACE_DIRfor output andAPS5_PAD_INPUT_FILEfor navigation. No other APS5_TRACE_* or APS5_PROFILE_* switch was set, and neither capture redirected the game log. Both revisions report dirty=false and trace_dropped=false. Both captures closed normally with exit code 0, no job-cap events, and no low-memory stop. Original saves and runtime were restored and hash-verified after capture.This is one five-second pair on a vblank-limited menu. Background workloads remained running at the user's request. It establishes no statistically supported speedup or regression, no gameplay FPS gain, and does not demonstrate the 30 FPS gameplay target. The optimization's isolated cost remains measured by the separately disclosed microbenchmark. A supported gameplay workload is still needed to assess frame-time benefit.
Gameplay was attempted on clean main first. Prologue loading aborts before gameplay with
reads or writes a converted float image with 16-bit data, which is not implemented. The diagnostic retry confirmed this runtime rejection with exit code 0xC0000409 and zero job-cap events; it is separate from packed-f16 arithmetic support. That run redirected logs for diagnosis and is excluded from performance evidence. No shader guard was bypassed and no extra gameplay patch was included in either capture build.main: unedited summary and trace_digest output
seconds 360:5 from present 19514: 300 presents, frame p50 16.7 / p90 16.9 / p99 17.0 / max 17.1 ms, GPU busy 2.1 ms per present
NVIDIA GeForce RTX 3080, NVIDIA 610.47, Windows 10.0.26200, 3840x2160, commit 2ee8e97, dropped: no
{"commit":"2ee8e9751a06","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"360:5"},"gpu":"NVIDIA GeForce RTX 3080","driver":"NVIDIA 610.47","os":"Windows 10.0.26200","cpu":"AMD Ryzen 9 7950X 16-Core Processor","cpu_threads":32,"memory_mib":64608,"title_id":"PPSA04263","title":"Grand Theft Auto V","content_version":"01.010.002","resolution":"3840x2160","window":{"spec":"seconds 360:5","first_present":19514,"ms":5004.96},"presents":300,"frame_ms":{"p50":16.684,"p90":16.889,"p99":16.986,"max":17.093},"gpu_busy_ms_per_present":2.124,"worker_busy_pct":{"queue worker 0x0":98.976},"top_phases_ms_per_frame":{"Driver.Packet":16.477,"VideoOut.Flip":16.304,"VideoOut.Flip.vblank_wait":15.629,"Driver.Packet.rendering_wait":13.676,"Driver.Packet.draw":2.291,"Driver.Draw":2.274,"Graphics.Draw":1.785,"Driver.Draw.shader_compile_and_link":1.781},"trace_dropped":false,"trace":"aps5-trace-2ee8e9751a06-62204.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): VideoOut.Flip 16.3, Driver.Packet 16.1, VideoOut.Flip.vblank_wait 15.6, Driver.Packet.rendering_wait 13.7, Driver.Packet.draw 2.2, Driver.Draw 2.2, batch 2.1, Driver.Draw.shader_compile_and_link 1.6, Graphics.Draw 1.5, Graphics.Draw.draw_and_resource_release 0.8
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
change: unedited summary and trace_digest output
seconds 360:5 from present 19210: 300 presents, frame p50 16.7 / p90 16.9 / p99 17.0 / max 17.1 ms, GPU busy 2.6 ms per present
NVIDIA GeForce RTX 3080, NVIDIA 610.47, Windows 10.0.26200, 3840x2160, commit 7cbd7ee, dropped: no
{"commit":"7cbd7eeb345c","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"360:5"},"gpu":"NVIDIA GeForce RTX 3080","driver":"NVIDIA 610.47","os":"Windows 10.0.26200","cpu":"AMD Ryzen 9 7950X 16-Core Processor","cpu_threads":32,"memory_mib":64608,"title_id":"PPSA04263","title":"Grand Theft Auto V","content_version":"01.010.002","resolution":"3840x2160","window":{"spec":"seconds 360:5","first_present":19210,"ms":5005.226},"presents":300,"frame_ms":{"p50":16.683,"p90":16.872,"p99":17.009,"max":17.066},"gpu_busy_ms_per_present":2.568,"worker_busy_pct":{"queue worker 0x0":98.949},"top_phases_ms_per_frame":{"Driver.Packet":16.474,"VideoOut.Flip":16.248,"VideoOut.Flip.vblank_wait":15.567,"Driver.Packet.rendering_wait":13.689,"Driver.Packet.draw":2.282,"Driver.Draw":2.266,"Driver.Draw.shader_compile_and_link":1.78,"Graphics.Draw":1.773},"trace_dropped":false,"trace":"aps5-trace-7cbd7eeb345c-39484.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): VideoOut.Flip 16.2, Driver.Packet 16.1, VideoOut.Flip.vblank_wait 15.6, Driver.Packet.rendering_wait 13.7, batch 2.6, Driver.Packet.draw 2.2, Driver.Draw 2.2, Driver.Draw.shader_compile_and_link 1.6, Graphics.Draw 1.5, Graphics.Draw.draw_and_resource_release 0.8
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
Raw traces and unedited summary files are packaged as
pr-11957-gta-story-menu-traces.zip(1,351,484 bytes; SHA-2560b5a758dc79befe09b9c401b3180dc82449d1640a21fbe5cbd74f7f940dd6deb). Download the raw trace archive. Both unmodified compressed traces, summaries, full digests and SHA-256 checksums are included.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)