Repository navigation
perf(shader): validate vertex stage inputs without building them - #6611
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.
|
github-actions
Bot
added
perf
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the approach (same checks, same order, no ShaderVertexInputInfo build) is reasonable. Two things to fix before it can merge:
ValidateVertexStageInputsis out of sync with main. Since your base,BuildShaderStageInputInfovalidates the mesh configuration asoutputPrimitiveSizein {2, 3} andprovokingVertex < outputPrimitiveSize(ShaderInputInfoBuilder.cpp:225on main). Your validation (ShaderInputInfoBuilder.cpp:280in the PR) still usesprovokingVertex > 2uand ignoresoutputPrimitiveSize, so e.g.outputPrimitiveSize = 2, provokingVertex = 2oroutputPrimitiveSize = 4throw in the build but pass the validation. That defeats "same errors", and these two paths would then skip a check the build makes.agc_vertex_input_validationshould catch this class of drift: add mesh cases that varyoutputPrimitiveSize(0, 1, 2, 3, 4) andprovokingVertexagainst it, and make sure it fails when the check is removed. Ideally also make the build and the validation share the mesh/tessellation checks (one helper), so the next change to them cannot desync again.- The PR conflicts with main in
core/libs/prx/libSceAgcDriver/CMakeLists.txt(theforeach(agcTest IN ITEMS ...)list, line 1519). Please rebase and re-run the new test on the rebased tree.
Minor: the frame-time table in the description shows +6 % p50 for this change; you explain it as noise, fine, but the claim to keep is the ~1.2 % worker-frame saving only.
296b607 to
5bb72c6
Compare
|
This pull request is labeled |
github-actions
Bot
removed
perf numbers missing
|
Thanks, done, and rebased onto main 1d41645.
|
|
@adriadam10 could you take another look when you have a moment? On 5bb72c6 your three points look addressed to us: the build and the validation now share |
|
Re-checked 5bb72c6 against the previous points: the build and the validation share |
3 participants
What
PreparedShaderInvocation::TryCreateandCaptureResources(with a source handle) check a vertex-family stage's inputs every time, for the V# fields their keys do not cover. They do it by building the stage's whole input info throughRequestInputInfoandBuildShaderStageInputInfo, and use only its exceptions. For a vertex-family stage that build zeroes and fills aShaderVertexInputInfoof about 10 KB, merges the vertex buffers and copies the tessellation and mesh configuration.ValidateVertexStageInputsmakes the build's vertex-family checks with the same errors, in the same order, and builds nothing:outputPrimitiveSize2 or 3,provokingVertexbelow it). The build and the validation make these two checks through one helper each,_validateTessellationand_validateMesh, so they cannot drift apart.ValidateRequestInputsuses it for the vertex family, andRequestInputInfofor any other stage. Both vertex-only call sites now use it. The otherRequestInputInfocallers validate every stage once per request and are unchanged.On the S2 page of Sonic Origins Plus's main menu,
BuildShaderStageInputInfocost the queue worker (APS5 sampler, 15 s, samples per frame), all of it throughTryCreate:TryCreatewent from 0.83 to 0.87 down to 0.45 to 0.52.The claim is this saving only, about 1.2% of the worker's frame (34 to 37 samples). The frame times below move within run-to-run noise and show no gain.
RequestInputInfocomputes a tessellation draw's instance patches by dividing by the input control points before the build checks that count. For an instanced draw with zero control points it divides by zero. The validation reads only the checked fields, so on these two paths that draw now gets the build's error instead.The change is in the recompiler's sources, so the shader cache version changes with it.
Tested
Windows 11 (10.0.26300), WinLibs GCC 15.2.0 (UCRT), Release, NVIDIA GeForce RTX 2080 Ti, driver 617.14.
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.agc_vertex_input_validationnow also runs nine mesh configurations overoutputPrimitiveSize0 to 4, withprovokingVertexat and below it, 4095 cases in all (768 accepted)._validateMeshback atprovokingVertex > 2u, the test fails on sizes 0, 1 and 4, and on size 2 with provoking vertex 2.agc_driver_gpu_indirect_arguments_misaligned_writer_aheadis not run: main itself does not compile it with MinGW (alignas(65536)).agc_driver_multisample_fill_host_onlyfailed once ("the sentinel fill did not land"). That is main's own race, agc_driver_multisample_fill_host_only fails intermittently: the sentinel fill after an absorbed multisampled clear does not land #6512, which fix(agc): record a multisampled target's commands on its own device #6522 fixes; with fix(agc): record a multisampled target's commands on its own device #6522's line added it failed 0 of 200 runs (8 at a time).agc_vertex_input_validation: for every vertex-family stage, the build and the validation run over 7 vertex inputs, 4 mesh configurations and 9 tessellation configurations (1260 cases). They must raise the same error or none, and every error the build raises must occur at least once.APS5_TRACE_SECONDS=200:15on the same machine in one session, caches warm. Traces attached.This pair moved the other way, within the noise of a 15 s window: the four windows of the moving background I captured in that session read 112.3 to 119.3 ms at the median. The FPS shown in the title went from 9.53 to 9.58 (median of six readings), and a sampled pair taken an hour earlier went from 8.84 to 9.07.
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 a0dd42c, whose summary says e3b8d1c; this change 4e46e7e, whose summary says a0dd42c):
inputs-traces.zip
The two summary lines, with
tools/trace_digest.pyfor each trace:Baseline (summary commit e3b8d1c)
seconds 200:15 from present 2700: 132 presents, frame p50 112.3 / p90 115.7 / p99 146.1 / max 148.9 ms, GPU busy 83.0 ms per present
NVIDIA GeForce RTX 2080 Ti, NVIDIA 617.14, Windows 10.0.26300, 3840x2160, commit e3b8d1c (dirty), dropped: yes
{"commit":"e3b8d1c5245e","dirty":true,"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","resolution":"3840x2160","window":{"spec":"seconds 200:15","first_present":2700,"ms":15034.755},"presents":132,"frame_ms":{"p50":112.276,"p90":115.733,"p99":146.087,"max":148.862},"gpu_busy_ms_per_present":83.003,"worker_busy_pct":{"queue worker 0x0":98.123},"top_phases_ms_per_frame":{"Driver.Packet":111.728,"Driver.Packet.draw":100.855,"Driver.Draw":99.869,"Driver.Draw.draw_and_resource_release":47.11,"Graphics.Draw":45.996,"Graphics.Draw.draw_and_resource_release":37.284,"Driver.Draw.shader_compile_and_link":36.217,"Driver.Present":33.118},"trace_dropped":true,"trace":"aps5-trace-e3b8d1c5245e-31932.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 82.9, Driver.Present 33.1, Driver.Present.render_fence_wait 32.6, Vulkan.Retire 32.6, Vulkan.Retire.wait 32.6, Driver.Packet 19.5, Driver.Packet.draw 18.8, Driver.Draw 18.6, Driver.Draw.draw_and_resource_release 4.4, Graphics.Draw 4.3
Longest waits
This change (summary commit a0dd42c)
seconds 200:15 from present 2708: 124 presents, frame p50 119.3 / p90 133.3 / p99 151.6 / max 172.5 ms, GPU busy 84.7 ms per present
NVIDIA GeForce RTX 2080 Ti, NVIDIA 617.14, Windows 10.0.26300, 3840x2160, commit a0dd42c (dirty), dropped: yes
{"commit":"a0dd42cf9b44","dirty":true,"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","resolution":"3840x2160","window":{"spec":"seconds 200:15","first_present":2708,"ms":15078.201},"presents":124,"frame_ms":{"p50":119.277,"p90":133.323,"p99":151.553,"max":172.525},"gpu_busy_ms_per_present":84.702,"worker_busy_pct":{"queue worker 0x0":98.081},"top_phases_ms_per_frame":{"Driver.Packet":119.228,"Driver.Packet.draw":107.057,"Driver.Draw":106.006,"Driver.Draw.draw_and_resource_release":50.358,"Graphics.Draw":49.2,"Graphics.Draw.draw_and_resource_release":40.064,"Driver.Draw.shader_compile_and_link":37.71,"Driver.Present":32.538},"trace_dropped":true,"trace":"aps5-trace-a0dd42cf9b44-40532.json.gz"}Slowest frames (largest slices inside each, inclusive)
Threads (ms per frame)
Largest slices (ms per frame, inclusive, all threads): batch 84.7, Driver.Present 32.5, Driver.Present.render_fence_wait 32.0, Vulkan.Retire 32.0, Vulkan.Retire.wait 32.0, Driver.Packet 21.3, Driver.Packet.draw 20.4, Driver.Draw 20.2, Driver.Draw.draw_and_resource_release 5.0, Graphics.Draw 4.9
Longest waits