Repository navigation
Conversation
|
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.
|
|
On Linux (GCC 16.2.1, Release) the head doesn't compile. Merged onto
AI-assisted: yes (analysis and text written with Claude Code; the build is from my machine). |
a2a74aa to
15bc218
Compare
|
Thanks for catching it. Fixed in 15bc218: the assert now says 784 (12 lists x 16 bytes more, as you worked out). I also compiled that Linux-only block under MinGW's libstdc++, whose layouts match: all 31 asserts hold with the change, and the old 592 fails exactly as you reported. |
|
AMD / Windows run of this PR at 15bc218: 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 (57):
AI-assisted: yes (runs made on the device above). |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the implementation reads correctly (the 64/65 boundary in push_back/set/operator[], resize shrinking back to inline, and the disk cache format are all consistent, and verifyElementFlags covers the spilled case). Two things before this can go in:
- The numbers don't show a gain. The frame table moves the wrong way (
frame_ms.p50+0.4%,mean_frame_ms+0.8%,Driver.Draw+1.1%, FPS 10.81 → 10.56), and the profile gain (Populate1.28 → 0.91, worker 32.80 → 31.04 samples) comes from one sampled pair. The cost isDescriptorBindinggrowing 592 → 784 bytes (+32%) for every binding held in results and caches. Please post repeated paired runs (several captures per side, with the spread) where the worker or frame time improves outside the noise, or drop this change. - It conflicts with
main:DescriptorBindinggainedsamplerCoordinatesIgnoredandbufferExtent, and thesizeofasserts inShaderDiskCache.cppchanged (DescriptorBindingis 656 there now). After rebasing, the newsamplerCoordinatesIgnoredlist should be anElementFlagstoo (and its users checked), and the asserts recomputed.
15bc218 to
25f8843
Compare
|
This pull request is labeled |
|
Thanks. Rebased onto main 1d41645 and re-measured.
|
|
@meh7an a Linux follow-up on the rebased head 25f8843. Merged with I also tried the commit on Stray (PPSA02100), on my local stack (
The PR's run was faster in every round. I didn't measure AI-assisted: yes (text with Claude Code; the runs are from my machine). |
|
@adriadam10 you asked for repeated paired runs where the worker or frame time improves outside the noise. Here are two titles, three alternating pairs each. Repeated paired runs on two titles: this change is faster outside the noise on both. Linux (Ubuntu 26.10, kernel 7.3), Ryzen 9 5950X, RTX 3080 Ti (NVIDIA 615.71.09), Release. Base Sonic Origins Plus (PPSA05325, 02.002.000), main menu, Sonic 2 page. Scripted: X on the autosave notice, X on the title, Right twice, 30 s to settle, then 60 s measured with no input. Every run's last screenshot is on the Sonic 2 page.
About +8%, and no PR window is as low as any Castlevania: Belmont's Curse (PPSA35399, 01.000.000), the boat scene, rendered by the engine and waiting on a dialogue line, no input (about 2,800 draws per frame, CPU-bound). Last 4 windows of 70 s,
About +20%. Capture and the committer stay the same (~12 µs and ~29–31 µs per draw), so the time comes out of the recompile phase, where
One aps5-summary.jsonl row (10 s) per side, pair 2 of each titleSonic Origins Plus, {"commit":"7e95bde794ae","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_PROFILE_DRAW":"1","APS5_PROFILE_GPU":"1","APS5_TRACE_DIR":"<path>","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 3080 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.3.0-9-generic","cpu":"AMD Ryzen 9 5950X 16-Core Processor","cpu_threads":32,"memory_mib":32003,"title_id":"PPSA05325","title":"Sonic Origins","content_version":"02.002.000","resolution":"3840x2160","window":{"spec":"summary_every 10","first_present":4019,"ms":10060.191},"interval_start_s":140.358,"interval_end_s":150.418,"presents":135,"frame_ms":{"p50":72.726,"p90":84.376,"p99":93.785,"max":97.149},"gpu_busy_ms_per_present":45.323,"worker_busy_pct":{"queue worker 0x0":97.460},"top_phases_ms_per_frame":{"Driver.Packet":72.603,"Driver.Packet.draw":59.211,"Driver.Draw":58.275,"Graphics.Draw":47.942,"Graphics.Draw.draw_and_resource_release":39.474,"Driver.Draw.shader_compile_and_link":36.951,"Shader.DrawStage":29.256,"VideoOut.Flip":20.841},"trace_dropped":false,"trace":""}{"commit":"5df59fe43bac","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_PROFILE_DRAW":"1","APS5_PROFILE_GPU":"1","APS5_TRACE_DIR":"<path>","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 3080 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.3.0-9-generic","cpu":"AMD Ryzen 9 5950X 16-Core Processor","cpu_threads":32,"memory_mib":32003,"title_id":"PPSA05325","title":"Sonic Origins","content_version":"02.002.000","resolution":"3840x2160","window":{"spec":"summary_every 10","first_present":4077,"ms":10031.033},"interval_start_s":140.273,"interval_end_s":150.304,"presents":142,"frame_ms":{"p50":67.036,"p90":87.445,"p99":95.318,"max":97.451},"gpu_busy_ms_per_present":45.373,"worker_busy_pct":{"queue worker 0x0":96.976},"top_phases_ms_per_frame":{"Driver.Packet":68.479,"Driver.Packet.draw":55.188,"Driver.Draw":54.217,"Graphics.Draw":45.574,"Graphics.Draw.draw_and_resource_release":37.521,"Driver.Draw.shader_compile_and_link":32.619,"Shader.DrawStage":25.305,"VideoOut.Flip":21.288},"trace_dropped":false,"trace":""}Castlevania, {"commit":"7e95bde794ae","dirty":false,"env":{"APS5_HOST_IMPORT_MIB":"16384","APS5_PAD_INPUT_FILE":"<path>","APS5_PROFILE_DRAW":"1","APS5_PROFILE_GPU":"1","APS5_TRACE_DIR":"<path>","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 3080 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.3.0-9-generic","cpu":"AMD Ryzen 9 5950X 16-Core Processor","cpu_threads":32,"memory_mib":32003,"title_id":"PPSA35399","title":"Castlevania: Belmont's Curse","content_version":"01.000.000","resolution":"3840x2160","window":{"spec":"summary_every 10","first_present":8239,"ms":10112.341},"interval_start_s":210.626,"interval_end_s":220.738,"presents":68,"frame_ms":{"p50":148.162,"p90":154.131,"p99":158.655,"max":158.655},"gpu_busy_ms_per_present":48.046,"worker_busy_pct":{"queue worker 0x0":90.510},"top_phases_ms_per_frame":{"Driver.Packet":134.147,"Driver.Packet.draw":125.540,"Driver.Draw":124.340,"Driver.Draw.shader_compile_and_link":105.502,"Shader.DrawStage":93.293,"Graphics.Draw":87.506,"Graphics.Draw.draw_and_resource_release":74.461,"Shader.DrawStage.materialize":58.900},"trace_dropped":false,"trace":""}{"commit":"5df59fe43bac","dirty":false,"env":{"APS5_HOST_IMPORT_MIB":"16384","APS5_PAD_INPUT_FILE":"<path>","APS5_PROFILE_DRAW":"1","APS5_PROFILE_GPU":"1","APS5_TRACE_DIR":"<path>","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 3080 Ti","driver":"NVIDIA 615.71.09","os":"Linux 7.3.0-9-generic","cpu":"AMD Ryzen 9 5950X 16-Core Processor","cpu_threads":32,"memory_mib":32003,"title_id":"PPSA35399","title":"Castlevania: Belmont's Curse","content_version":"01.000.000","resolution":"3840x2160","window":{"spec":"summary_every 10","first_present":8375,"ms":10044.415},"interval_start_s":210.816,"interval_end_s":220.861,"presents":81,"frame_ms":{"p50":123.522,"p90":128.179,"p99":133.216,"max":133.216},"gpu_busy_ms_per_present":40.417,"worker_busy_pct":{"queue worker 0x0":90.611},"top_phases_ms_per_frame":{"Driver.Packet":111.658,"Driver.Packet.draw":102.731,"Driver.Draw":101.658,"Graphics.Draw":83.382,"Driver.Draw.shader_compile_and_link":80.116,"Graphics.Draw.draw_and_resource_release":70.212,"Shader.DrawStage":67.897,"Shader.ResolveAbi":43.610},"trace_dropped":false,"trace":""}AI-assisted; the runs are from my machine(s). |
25f8843 to
a9a338c
Compare
|
@adriadam10 Rebased onto main 53a0a27 (a9a338c). The only conflict was the size asserts: main's Since your review, two independent sets of runs came in:
Thanks @teepean and @mrfelek for the runs. Could you take another look? |
a9a338c to
438978e
Compare
boykopovar#6970 turns the binding flag lists into ElementFlags; ResultHeapBytes (our memo cap, 6700440) still called the std::vector overload on them and no longer compiled. A flag list's heap is its spill past the inline word, (capacity + 7) / 8 bytes as for vector<bool> before.
|
A memory data point from Persona 5 Royal (PPSA05110, own dump): Windows 11 26200, RX 6700 XT, WinLibs GCC 15.2.0 r7, Release. The base is To attribute heap memory, both builds carried a local diagnostic that is not part of any PR: a
So in this title the change does what it says for allocations: about 2.5 million fewer heap blocks at 120 and 180 s. Memory stays about the same, though. The larger |
Review of PR #6970: perf(shader): keep a binding's element flags inlineVerdict: stale+rebase Findings
Freshness Envelope
Cross-PR Ledger
|
# Conflicts: # core/shader/recompiler/Recompiler.hpp # core/shader/recompiler/ShaderDiskCache.cpp Also adds vectorBytes(const ElementFlags&) to Recompiler.cpp for main's memo accounting (8868e09): the PR turns the DescriptorBinding flag vectors into ElementFlags, which main's descriptorBindingBytes counted as std::vector<bool>; flags held inline cost no heap, spilled flags cost (size+7)/8 bytes.
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 525 tests)Skipped (49)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. |
# Conflicts: # core/shader/recompiler/Recompiler.hpp
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Independent repeated paired runs on AMD, for the review's request for "several captures per side, with the spread". Setup: Windows 11 22621, Ryzen 9 5950X, Radeon RX 6700 XT, AMD driver 26.10.1, MinGW GCC 15.2 UCRT, Release.
Evidence (summaries, traces, stderr, run records, the pair script and its output): release AI-assisted: yes. |
|
Addendum to my A/B above. Two controls on the same machine, scene and window, run after it, check whether the PR runs' position (always second in each pair) could explain the result. A/A control: three pairs of
The second of two identical runs is not systematically lower: it was higher in 2 of 3. So the PR's 5/5 lower pairs are not an order artifact. If anything, the second slot tends to read higher. Unrelated candidate in the second slot: a separate one-line allocator change, also always run second, came out lower in only 2 of 5 pairs (−5.0% to +4.0%). The caveat this adds: identical builds differ by up to about 10% between back-to-back runs here. So the direction (5/5 pairs, sign-test p = 1/32) is the meaningful part of my result, not the size of each delta. Evidence for both controls is with the earlier logs; I can add it to the release if useful. AI-assisted: yes. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (48)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. |
What
DescriptorBindingBuilder::Populateturns a stage's binding plan into the bindings of a materialized result, for every draw stage the memos miss and for every dispatch. It copies each plannedDescriptorBinding(auto physical = entry.descriptor). A binding keeps 12 per-element flag lists asstd::vector<bool>:samplerDepthCompare,imageWritten,imageDepthCompare,imageAtomic,imageAtomic64,bufferAtomic,bufferWritten,samplerUnnormalized,samplerIntegerBorder,imageUnnormalized,imageUnnormalizedBaseLevelandbufferRead. Every copy allocates one block per non-empty list, and releasing the result frees them again.ElementFlags(inRecompiler.hpp) replacesstd::vector<bool>for those lists:std::vector<bool>. A binding rarely has more than 64 elements, so a copy allocates nothing.std::vector<bool>does:size,empty,[](a reference proxy when written),at,back, iteration,push_back,assign,resize,clear, and==with another list or astd::vector<bool>. The code that fills and reads the lists stays as it is.ElementFlagsinstead ofstd::vector<bool>.ElementFlags.verifyElementFlags(shader disk cache tests) checks 70 flags, past the inline word. It covers reads, iteration, copies, writes through[]andback(),resizeboth ways,assign,clear, and a round trip through the disk cache.On the S2 page of Sonic Origins Plus's main menu, the queue worker spent, per frame (APS5 sampler, 15 s):
Populate: 1.28 to 0.91. In a profile of the baseline, 78 of its 151 directoperator newsamples were inlinedstd::vector<bool>copies (8 call sites), and 14 more were instd::vector<bool>'s copy constructor.RecompileResult's disposal): 0.76 to 0.57;operator new3.38 to 2.88,mi_free0.68 to 0.59;That is a few percent of the worker's frame at most. On current main, three paired runs below put the frame median about 2% lower.
Tested
Windows 11 (10.0.26300), WinLibs GCC 15.2.0 (UCRT), Release, NVIDIA GeForce RTX 2080 Ti, driver 617.14.
ShaderDiskCache.cpp's size asserts compile only on Linux (x86_64, libstdc++), so the Windows build above skips them.ElementFlagsis 56 bytes againststd::vector<bool>'s 40. Main'sDescriptorBindingnow has 13 such lists, withsamplerCoordinatesIgnored, so it grows from 656 to 864 bytes (+32%), and its assert says 864.sizeof(DescriptorBinding)is 864 with this change.ShaderInvocation(216) andRecompileResult(528).DescriptorBindingis 864. All four sizes were checked under MinGW's libstdc++ with aSize<sizeof(...)>probe: 216, 528, 864 andCompiledShaderArtifact280.agc_driver_image_array_window_past_layer_limitis not run: MinGW does not build itsalignas(65536).samplerCoordinatesIgnoredis anElementFlagsas well; main'sbufferExtentstays astd::vector<std::uint32_t>.agc_driver_image_array_window_past_layer_limitis not run: main does not compileImageArrayWindowPastLayerLimit.cppwith MinGW (alignas(65536)).agc_driver_pass_input_hazard(added in ba4571a) fails on plain main 1d41645 as well, 0 of 4 runs alone.APS5_TRACE_SECONDS=200:15capture with warm caches and the title FPS (median of six readings).Driver.Draw62.63 and 60.79 ms (-2.9%).Before this rebase:
agc_driver_gpu_indirect_arguments_misaligned_writer_aheadis not run: main itself does not compile it with MinGW (alignas(65536)).select_testsfails in its setup on Windows without the symlink privilege (WinError 1314fromsymlink_to), as on main.build_eligibilitytimed out once in the parallel run; alone it passes in 1.5 s.APS5_TRACE_SECONDS=200:15on the same machine in one session, caches warm. Traces attached.The title FPS during the two captures was 10.78 and 10.82 (median of six readings each).
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)Traces and summary lines (baseline b1909f4, whose summary says 11882cc; this change 245e355, whose summary says b1909f4):
flags-traces.zip
Traces of the six re-measured runs on main 1d41645, one zip per round with that round's main and this-change capture: flags-rebased-round1.zip, flags-rebased-round2.zip, flags-rebased-round3.zip.
The re-measured median pair on main 1d41645, with
tools/trace_digest.pyfor each trace:Main 1d41645, run 2 (summary commit 1d41645)
seconds 200:15 from present 3075: 170 presents, frame p50 86.8 / p90 92.1 / p99 115.2 / max 117.7 ms, GPU busy 69.1 ms per present
NVIDIA GeForce RTX 2080 Ti, NVIDIA 617.14, Windows 10.0.26300, 3840x2160, commit 1d41645, dropped: yes
{"commit":"1d41645527f5","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":3075,"ms":15013.344},"presents":170,"frame_ms":{"p50":86.832,"p90":92.073,"p99":115.234,"max":117.659},"gpu_busy_ms_per_present":69.058,"worker_busy_pct":{"queue worker 0x0":96.848},"top_phases_ms_per_frame":{"Driver.Packet":85.501,"Driver.Packet.draw":63.243,"Driver.Draw":61.837,"Graphics.Draw":58.045,"Graphics.Draw.draw_and_resource_release":48.446,"Driver.Draw.shader_compile_and_link":41.847,"Shader.DrawStage":32.497,"VideoOut.Flip":24.784},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":6.117,"events_lost":385724,"totals_full":false},"draw committer":{"events_full_after_s":14.471,"events_lost":10071,"totals_full":false}},"trace":"aps5-trace-1d41645527f5-40372.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 68.5, Graphics.Draw 32.5, Graphics.Draw.draw_and_resource_release 28.0, VideoOut.Flip 24.8, Driver.Present 24.7, VideoOut.Flip.present 24.7, Driver.Present.render_fence_wait 24.0, Vulkan.Retire 24.0, Vulkan.Retire.wait 24.0, Driver.Packet 19.3
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)
This change 25f8843, run 2 (summary commit 25f8843)
seconds 200:15 from present 3035: 171 presents, frame p50 85.2 / p90 98.0 / p99 116.4 / max 116.6 ms, GPU busy 70.6 ms per present
NVIDIA GeForce RTX 2080 Ti, NVIDIA 617.14, Windows 10.0.26300, 3840x2160, commit 25f8843, dropped: yes
{"commit":"25f8843b15fe","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":3035,"ms":15056.684},"presents":171,"frame_ms":{"p50":85.159,"p90":98.001,"p99":116.412,"max":116.584},"gpu_busy_ms_per_present":70.598,"worker_busy_pct":{"queue worker 0x0":96.969},"top_phases_ms_per_frame":{"Driver.Packet":85.357,"Driver.Packet.draw":62.166,"Driver.Draw":60.792,"Graphics.Draw":57.97,"Graphics.Draw.draw_and_resource_release":48.128,"Driver.Draw.shader_compile_and_link":39.635,"Shader.DrawStage":30.398,"Driver.Present":26.857},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":6.419,"events_lost":354558,"totals_full":false},"draw committer":{"events_full_after_s":14.387,"events_lost":11435,"totals_full":false}},"trace":"aps5-trace-25f8843b15fe-45124.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 70.5, Graphics.Draw 32.7, Graphics.Draw.draw_and_resource_release 28.2, Driver.Present 26.9, VideoOut.Flip 26.9, VideoOut.Flip.present 26.7, Driver.Present.render_fence_wait 26.1, Vulkan.Retire 26.1, Vulkan.Retire.wait 26.1, Driver.Packet 20.2
Longest waits
Largest GPU batches (GPU ranges inside, and the CPU slices that recorded each, inclusive)