Repository navigation
perf(agc): order a storage flush behind its GPU writer when the write-back is a GPU copy - #15569
Open
milescoviello wants to merge 7 commits into
Open
milescoviello wants to merge 7 commits into
milescoviello wants to merge 7 commits into
Conversation
…-back is a GPU copy A label's readback store and a storage refresh flush pending storage images before going on. When every flushed image lives in a host import, its write-back is a copy the recorder records into the import, so it runs after the image's writer and before anything recorded later, including the label. The CPU wait for the writer is skipped then. A refresh whose flush skipped the wait re-uploads its layers from the import on the GPU instead of comparing guest bytes the copy has not reached yet. APS5_NO_FLUSH_GPU_ORDERED=1 and APS5_NO_REFRESH_GPU_ORDERED=1 restore the waits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dered_flush WAIT_REG_MEM is evaluated by the queue worker, so a gate in front of the store kept it from being recorded at all. The label now follows the store directly, while the store is still pending on the GPU, which is the case the ordered flush handles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
perf
|
This pull request is labeled |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A label stores pending linear and small (64 KiB or less) storage images before it; a 256 KiB image is stored there only when it was written occasionally, so the test checked an order the driver does not promise. The surface is now 128x128 R32UI, one 64 KiB block, and the test requires that size. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
milescoviello
marked this pull request as ready for review
November 3, 2026 22:12
Contributor
|
Hi, a note on how this PR and #13251 fit together. Both change the
A proposed composition, for whichever PR goes second:
A refresh with an alias plan stays ineligible, as one with an alias is here, so the two paths don't meet. Either order works. If this PR lands first, #13251 takes the composition in its next merge of |
Contributor
Author
|
@EagleBearerYS thanks, that composition looks right to me. A refresh with an alias plan stays out of the ordered path, as one with an alias is here, so AI-assisted: yes (Claude Code) |
Resolves Texture.cpp: the refresh keeps its GPU-ordered flush scope and calls main's pruneAdoptedSources() after the flush. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, 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 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. |
Contributor
✅ 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 (9, 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 517 tests)Skipped (57)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. |
XairRrR
suggested changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
The ordered flush is correct where I could check it, and the new test reaches the new path. The PR is a perf change, though, and it lacks the summary lines from main and from the change and the traces that the rules require.
What I ran
Head d3f7307afdf8752297fc8219277117a2d44e9dc5 merged into main 49021a4c907b7f493429dfe269416bbec2de96e1 with --no-ff (merged tree 13013875e03d5a12b8def62b4a51e6ebbea6e6fb). Debian 13 VM, GCC 14.2.0, Release, Ninja, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 49021a4c907b7f493429dfe269416bbec2de96e1...d3f7307afdf8752297fc8219277117a2d44e9dc5
python3 tools/check_conventions.py --base 49021a4c907b7f493429dfe269416bbec2de96e1
ctest --test-dir build -R '^agc_driver_label_ordered_flush$' --output-on-failure
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
ctest --test-dir build -j8 --test-load 32 --output-on-failure \
-R '^agc_driver_(label_ordered_flush|direct_write_completion|linear_target_direct_write_completion|fill_completion_labels|linear_target_completion_points|memory_value_fill)$'
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R '^agc_driver_label_ordered_flush$'
# negative control and probe (see below)
git show 49021a4c907b7f493429dfe269416bbec2de96e1:core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
ninja -C build -j32 -l32 tests/agc_driver_label_ordered_flush_tests
ctest --test-dir build -R '^agc_driver_label_ordered_flush$' --output-on-failure --repeat until-fail:3
git show 13013875e03d5a12b8def62b4a51e6ebbea6e6fb:core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Configure / build / libs (script) | passed |
| Selected tests (script, 2176 run) | 0 new failures, 0 timeouts; the 7 known failures also fail on main (agc_driver_division, denormal-mode tests, vector_constant_factor_tininess, one unwrap_self test) |
agc_driver_label_ordered_flush |
passes, 0.38 s |
| The 6 focused tests from the description, with the validation layer | 5 passed, agc_driver_linear_target_direct_write_completion skipped on lavapipe |
Validation, agc_driver_label_ordered_flush |
layer loaded (inserted, no "was not found but was requested"), 0 VUID- lines |
| Script validation comparison | 783/784 tests with the layer loaded. New VUID-VkSamplerCreateInfo-addressMode* lines only in agc_driver_image_gather_volume, which was not in main's baseline pass. The PR does not touch sampler creation, so I don't count them against it |
Skip actually taken in the new test (temporary probe in FlushPending) |
ordered=1 active=1 skip=1 n=1 in all 8 runs; label landed after the stored texels |
Probe output (temporary fprintf before if (orderedSkip), removed afterwards)
8 [probe] ordered=1 active=1 skip=1 n=1
1 label ordered flush tests passed
Negative control
I restored core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp from main, rebuilt agc_driver_label_ordered_flush_tests and ran it three times: it passes on main. That is expected. On main the CPU waits for the writer, so the label order already holds. The test guards against the skip breaking the order; it doesn't fail without the change. A temporary probe in the merged build showed that the test reaches the new skip in each of its 8 runs. I then restored the merged file and rebuilt; git status --porcelain is empty.
Findings
Blocking
- Perf evidence missing (project rule). The PR is titled
perf(agc)and still carriesperf numbers missing. CONTRIBUTING.md requires theaps5-summary.jsonllines ofmainand of the change from the same machine and scene, the two traces, andtools/trace_digest.pyoutput. The bot comment asks for the same. The description gives fps and p50 from a "local stack (the same change on an older base, with other GoT experiments)", which doesn't isolate this change. Fix: capture PPSA02225 on currentmainand on this branch the way the Performance report form describes, then paste both summary lines, attach the traces and add the digest.
Non-blocking
- The skip doesn't check the image's device against the active recorder.
core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp:4690requiresRecorder::Active() != nullptrand a host import for each flushed image. The write-back records into the recorder only whenrecorderFor(context)matches the image's device (Texture.cpp:70-72). Otherwise it goes to a standaloneCommandBatch(Texture.cpp:6783-6807), which isn't ordered behind the writer. The first range loop inStoreReadbackBeforeLabeldoesn't filter by device, and the tests create several Vulkan devices. Suggest addingtexture->context.device == Recorder::Active()->Device()to theall_ofpredicate, so a cross-device flush keeps the wait. - Checklist
Unblocksline. The rule's form isUnblocks: PPSA01325 #3232, with the Game bring-up discussion. The line here names no discussion, and it describes a speed-up rather than a stop or a wrong result. Add the discussion number, or usenoneif no bring-up entry tracks this. - Head moved since the reviewed commit. The PR head is now
f37b080(a merge of upstreammainthat resolvesTexture.cpparoundpruneAdoptedSources()). I reviewedd3f7307merged intomain, as prepared, so that conflict resolution was not reviewed here.
Not verified
- Hardware GPUs: only lavapipe here. The description's RTX 4070 Ti and GT 1030 runs, and the NVIDIA CI comment, were not reproduced.
- The GoT numbers (I can't run titles) and the claim that frames are unchanged.
- A refresh where the skip is taken over memory outside the write watch. There, the skipped write-backs' CPU-side checks (
cpuLeftUnchanged,ChangedBlocks) may read guest bytes that an earlier recorded write-back is still copying into. I found no test covering this. - The negative control can't show that the test detects a misordering, because
mainis ordered by the CPU wait.
To get this to a good state
- Add the two summary lines (
mainand this change, same scene and machine), the traces and thetrace_digest.pyoutput, so theperf numbers missinglabel clears. - Optionally restrict the skip to images on the active recorder's device, and fix the
Unblocksline. - Rerun the focused tests on the new head
f37b080after the merge resolution.
…der's device The write-backs are recorded into the active recorder, so an image of another device keeps the CPU wait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
XairRrR
suggested changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — changes still requested
Since my last review, the author merged upstream main into the branch (f37b080). That merge resolves the Texture.cpp conflict around pruneAdoptedSources() correctly, and the tests pass. The perf evidence is still missing. The newest head, 2da1597 (a device check on the ordered skip), was not part of the tree prepared for this review.
What changed since my last review
f37b080: a merge of upstreammain. InStorageTexture::refresh, the ordered-flush scope wrapsFlushPending, andpruneAdoptedSources()follows the scope. That is the same position as onmain, where it directly followsFlushPending(core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp:2339). The four earlier commits are unchanged (delta.patch).2da1597("take the ordered flush only for images on the active recorder's device") is listed in pr.md as the current head. It was not in the prepared tree, and its object is not in the repository here, so I could not review it.- No author reply to my review. The description is unchanged in the parts that matter: there are still no summary lines and no traces.
What I ran
Head f37b080fed2a31fe64cf1ecd3e88778a3ba6593f merged into main 1c8c0efc8c41314402e6966da5bef6ea3d639d10 with --no-ff (merged tree 16285379dde5714fadf0c8bb93a6248ad1222d13). Debian 13 VM, GCC 14.2.0, Release, Ninja, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 1c8c0efc8c41314402e6966da5bef6ea3d639d10...f37b080fed2a31fe64cf1ecd3e88778a3ba6593f
python3 tools/check_conventions.py --base 1c8c0efc8c41314402e6966da5bef6ea3d639d10
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R '^agc_driver_label_ordered_flush$'
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
ctest --test-dir build -j8 --test-load 32 --output-on-failure \
-R '^agc_driver_(label_ordered_flush|direct_write_completion|linear_target_direct_write_completion|fill_completion_labels|linear_target_completion_points|memory_value_fill)$'
git show 1c8c0efc8c41314402e6966da5bef6ea3d639d10:core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
ninja -C build -j32 -l32 tests/agc_driver_label_ordered_flush_tests
ctest --test-dir build -R '^agc_driver_label_ordered_flush$' --output-on-failure --repeat until-fail:3
git show 16285379dde5714fadf0c8bb93a6248ad1222d13:core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
ninja -C build -j32 -l32 tests/agc_driver_label_ordered_flush_tests
ctest --test-dir build -R '^agc_driver_label_ordered_flush$'
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Configure / build / libs (script) | passed |
| Selected tests (script, 2177 run) | 0 new failures, 0 timeouts. The 7 known failures also fail on main. agc_driver_alpha_to_coverage_sample_mask_export passed on a rerun |
agc_driver_label_ordered_flush with validation |
passes. The layer was inserted, with no "was not found but was requested" message and 0 VUID- lines |
| The 6 focused tests from the description, with validation | 5 passed, and agc_driver_linear_target_direct_write_completion was skipped on lavapipe |
| Script validation comparison | 784 of 785 tests loaded the layer. New VUID-VkSamplerCreateInfo-addressMode* lines appear only in agc_driver_image_gather_volume, which main's baseline did not run. The PR doesn't touch sampler creation, so I don't count them against it |
| Negative control | with Texture.cpp restored from main, the test passes 3 times out of 3, as expected (see last review). After restoring the merged file it passes again, and git status --porcelain is empty |
Previous findings
- Blocking: perf evidence missing. Still open. The description still has no
aps5-summary.jsonllines frommainand from the change, no traces and notrace_digest.pyoutput, and the PR still carriesperf numbers missing. - Non-blocking: the skip doesn't check the image's device against the active recorder. Still open in the reviewed tree: the
all_ofpredicate atcore/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp:4696tests onlyHostImportFor. The author pushed2da1597, whose message says it adds this check. I couldn't see that commit, so I can't confirm it. - Non-blocking: checklist
Unblocksline. Still open. It names no Game bring-up discussion, and it describes a speed-up rather than a stop or a wrong result. - Non-blocking: head moved, merge resolution not reviewed. Resolved for
f37b080. The resolution keepspruneAdoptedSources()right after the flush, as onmain, and the focused tests pass on the merged tree.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- Commit
2da1597: it was not available in the prepared tree. - Hardware GPUs: only lavapipe here.
- The GoT numbers, and the claim that frames are unchanged.
- Whether the test catches a misordering: on
mainthe CPU wait already provides the order, so the negative control passes there.
To get this to a good state
- Capture PPSA02225 on current
mainand on this branch, on the same machine and scene, following the Performance report form. Paste both summary lines, attach the traces and add thetrace_digest.pyoutput. - Add the Game bring-up discussion to the
Unblocksline, or writenone. - I'll check the device-check commit
2da1597in the next pass.
github-actions
Bot
removed
the
unblocks title
Contributor
Author
|
Thanks for the re-check.
AI-assisted: yes (Claude Code) |
XairRrR
suggested changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — changes still requested
2da1597 adds the device check on the ordered skip, and Unblocks now reads none. Both earlier non-blocking findings are resolved. The perf evidence is still missing.
What changed since my last review
2da1597:FlushPendingkeeps the active recorder inorderedRecorderand addstexture->context.device == orderedRecorder->Device()to the skip'sall_ofpredicate (core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp:4696-4697).Recorder::Device()returns the recorder'scontext.device(core/libs/prx/libSceAgcDriver/Graphics/include/Recorder.hpp:457). The four earlier commits and the merge are unchanged (delta.patch).- Description: the
Unblocksline is nownone. - Author comment: the captures are queued on an RTX 4070 Ti. The base is
main1c8c0efc8cplus #9742, and the plan is three base runs alternating with three runs of the change. The summary lines, traces and digest are not in the description yet.
What I ran
Head 2da15973ce0a3fa5e4d9e574d6ee1df640544dad merged into main 1c8c0efc8c41314402e6966da5bef6ea3d639d10 with --no-ff (merged tree 8299001b2297bfe2faec19be0c646a7b41e71798). Debian 13 VM, GCC 14.2.0, Release, Ninja, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 1c8c0efc8c41314402e6966da5bef6ea3d639d10...2da15973ce0a3fa5e4d9e574d6ee1df640544dad
python3 tools/check_conventions.py --base 1c8c0efc8c41314402e6966da5bef6ea3d639d10
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -j8 --test-load 32 -V \
-R '^agc_driver_(label_ordered_flush|direct_write_completion|linear_target_direct_write_completion|fill_completion_labels|linear_target_completion_points|memory_value_fill)$'
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R '^agc_driver_label_ordered_flush$'
env APS5_TRACE_FLUSH=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R '^agc_driver_label_ordered_flush$'
env APS5_NO_FLUSH_GPU_ORDERED=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -R '^agc_driver_label_ordered_flush$' --output-on-failure
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Configure, build and libs (script) | passed |
| Selected tests (script, 2177 run) | 0 new failures and 0 timeouts. The 7 known failures also fail on main. agc_driver_alpha_to_coverage_sample_mask_export passed on a rerun |
| The 6 focused tests, with validation | 5 passed. agc_driver_linear_target_direct_write_completion was skipped on lavapipe. 0 VUID- lines, and no "was not found but was requested" message |
agc_driver_label_ordered_flush, with validation |
passed. The loader added VK_LAYER_KHRONOS_validation, with 0 VUID- lines |
APS5_TRACE_FLUSH=1 |
each run logs a [flush] image …+0x10000 for readback before label, so the test reaches the ordered path |
APS5_NO_FLUSH_GPU_ORDERED=1 |
passed (the CPU-wait path) |
| Script validation comparison | 784 of 785 tests loaded the layer. New VUID-VkSamplerCreateInfo-addressMode* lines appear only in agc_driver_image_gather_volume. main's baseline did not run that test, and the PR doesn't touch sampler creation, so I don't count them against it (same as last time) |
git status --porcelain |
empty |
The test is unchanged since my last review, so I did not repeat the negative control. That control passed with main's Texture.cpp, as expected, because there the CPU wait already provides the order.
Previous findings
- Blocking: perf evidence missing. Still open. The description has no
aps5-summary.jsonllines from a base and from the change, no traces and notrace_digest.pyoutput, and the PR still carriesperf numbers missing. The author says the captures are in progress. - Non-blocking: the skip doesn't check the image's device against the active recorder. Resolved by
2da1597. The predicate now requirestexture->context.device == orderedRecorder->Device(). An image of another device falls through to the old wait loop, and the refresh'sorderedSkippedis only set whenFlushPendingactually skipped. - Non-blocking: checklist
Unblocksline. Resolved. It now readsnone.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- Hardware GPUs: only lavapipe here. The author reports that
2da1597passes on an RTX 4070 Ti; the GT 1030 run was still in progress. - The GoT frame-rate numbers, and the claim that frames are unchanged.
- Whether the test catches a misordering: on
mainthe CPU wait already provides the order, so the test passes there too.
To get this to a good state
- Add the PPSA02225 summary lines for the base and the change to the description, from the same machine and scene, as the Performance report form asks. Attach the traces and add the
trace_digest.pyoutput. The base the author describes (main+ #9742, with only this PR's 3 files differing) is fine, as long as the two lines carry different commits.
12 tasks done
Contributor
|
Windows/NVIDIA data point: ASTRO BOT (PPSA21567 01.019.000), Crash Site. Setup
What changes underneath (one 200 s counter run per arm,
Which half helps here (one 240 s run each on B, same method)
Confirmation on the current head 2da1597 (
Tests (Windows, RTX 5090, Khronos validation 1.4.363, head 2da1597)
No crashes, device losses or On the AI-assisted: yes (Claude Code) |
Contributor
✅ 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)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 527 tests)Skipped (50)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. |
Contributor
Author
|
@TripyFox thank you, this is very thorough, and the split by half is useful too. Yes, please record the Performance-form captures on both arms ( @XairRrR here's an update on the GoT captures I promised. PPSA02225 doesn't run on
On the "not verified" refresh case, where the skip is taken over memory outside the write watch, I traced the CPU-side reads that follow the skip:
AI-assisted: yes (Claude Code) |
Collaborator
|
Hi milescoviello, Thanks, the idea is sound: the label is stored on the GPU after the recorded write-back copy, so the CPU wait adds no order there. A few points before merge, reviewed at
|
Contributor
✅ AMD 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 (7, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 527 tests)Skipped (60)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. |
Contributor
|
Performance-form captures on both arms, as requested. The traced runs and the throughput runs are separate launches. Title and scene
Revisions
Stack and configuration
Capture
Traced runs (diagnostic, 50 presents each)
Summary lines: {"commit":"1c8c0efc8c41","dirty":false,"env":{"APS5_TRACE_FRAMES":"1200:50"},"gpu":"NVIDIA GeForce RTX 5090","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":16,"memory_mib":65140,"title_id":"PPSA21567","title":"ASTRO BOT","content_version":"01.019.000","resolution":"3840x2160","window":{"spec":"frames 1200:50","first_present":1200,"ms":4502.648},"presents":50,"frame_ms":{"p50":89.365,"p90":95.693,"p99":98.460,"max":98.460},"gpu_busy_ms_per_present":71.899,"worker_busy_pct":{"queue worker 0x0":97.293,"queue worker 0x40":98.621,"queue worker 0x48":98.148,"queue worker 0x46":1.588},"top_phases_ms_per_frame":{"Driver.Packet":265.310,"Driver.Packet.memory_wait":149.718,"Driver.Packet.draw":58.025,"Driver.Draw":57.632,"Graphics.Draw":45.145,"Graphics.Draw.draw_and_resource_release":36.134,"Driver.Draw.draw_enqueued":26.902,"Driver.Draw.shader_compile_and_link":24.790},"trace_dropped":false,"trace":"aps5-trace-1c8c0efc8c41-26308.json.gz"}
{"commit":"b830a33eb5e7","dirty":false,"env":{"APS5_TRACE_FRAMES":"1200:50"},"gpu":"NVIDIA GeForce RTX 5090","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":16,"memory_mib":65140,"title_id":"PPSA21567","title":"ASTRO BOT","content_version":"01.019.000","resolution":"3840x2160","window":{"spec":"frames 1200:50","first_present":1200,"ms":4191.368},"presents":50,"frame_ms":{"p50":84.941,"p90":89.842,"p99":101.363,"max":101.363},"gpu_busy_ms_per_present":72.803,"worker_busy_pct":{"queue worker 0x0":96.799,"queue worker 0x40":98.180,"queue worker 0x48":98.010,"queue worker 0x46":2.133},"top_phases_ms_per_frame":{"Driver.Packet":246.538,"Driver.Packet.memory_wait":139.971,"Driver.Packet.draw":52.793,"Driver.Draw":52.400,"Graphics.Draw":46.792,"Graphics.Draw.draw_and_resource_release":37.914,"Driver.Packet.prepare_memory":25.616,"Driver.Draw.shader_compile_and_link":25.597},"trace_dropped":false,"trace":"aps5-trace-b830a33eb5e7-28056.json.gz"}Traces: Throughput runs (untraced, 240 s each)
None of the six runs (two warm-up runs, two traced, two throughput) crashed or ended early. Each ran to its time cap. AI-assisted: yes (Claude Code) |
…without a live shadow or lease; cover each branch Review of 2da1597 (oneandonlydean): - The refresh's ordered scope is a destructor-restored guard, so a FlushPending that throws leaves no armed flag on its thread, and a refresh sets the flag to its own eligibility instead of inheriting an outer label scope. - FlushPending takes the ordered skip only when its publish scope is None or no live shadowed range or pending lease covers the range, since that wait also orders PublishShadow. - StorageTexture::TakeOrderedSkipCounts counts the skips taken. agc_driver_label_ordered_flush checks the label half and a storage refresh of a linear view over a pending tiled store, and its _disabled and _unimported runs check that neither skip is taken with the switches set or without a host import, with the same results. - agc_driver_ordered_refresh_failure injects a flush failure inside an armed refresh (StorageTexture::SetFlushFailureForTest) and checks that the flag is restored and that the next flush waits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@oneandonlydean thank you, all five points hold. Head
Focused tests on the new head (Linux, RTX 4070 Ti, 615.71.09): all 9 pass in each of 3 runs: AI-assisted: yes (Claude Code) |
Contributor
✅ NVIDIA 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 (3, not counted against this PR)Validated but not compared (2, not in the main baseline's validation pass)Validation IDs already present on main (13 IDs in 542 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. |
Contributor
✅ 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)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (2, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 542 tests)Skipped (60)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. |
Contributor
|
Main-vs-change data for the current head
The change lowers frame p50 by 7.2 ms in round 1 and by 6.7 ms in round 2, and lowers p99 by 14–17 ms. GPU busy per present does not change (±0.1 ms), so the gain is on the CPU side. Averaged over both rounds, the largest phase changes are Summary lines (interval starting at 100 s, round 1)main: change:
AI-assisted: yes (Claude Code) |
12 tasks done
Contributor
Author
|
@TripyFox thank you. I've put both summary lines, the table and the @oneandonlydean @XairRrR the description now has the two summary lines from the same machine and scene: AI-assisted: yes (Claude Code) |
github-actions
Bot
removed
the
perf numbers missing
This was referenced Nov 4, 2026
Contributor
|
Another ASTRO BOT data point, in a heavier scene than Crash Site: the opening desert level, where on Setup. PPSA21564 (ASTRO BOT 01.007.000), opening level from a new game, scripted pad input (
(ms) Frame p50 83.0 -> 77.0 ms (-7 %), FPS over minutes 20-30 13.6 -> 14.9 (+9 %), and GPU busy per present goes up (62.8 -> 69.2 ms), so the GPU is fed more. The p99 of the first change run comes from one stretch (max 205.5 ms); the second change run has p99 93.1 ms. None of the four runs logged a FATAL or a device loss. Why it shows here: a sampled run on |
Contributor
|
Cross-test on GTA V (PPSA04264 01.005.000), North Yankton prologue, Windows 11 26300, RTX 5080 (NVIDIA 621.01), 3840x2160, warm caches.
Traces available on request. |
Contributor
Author
|
@LeoFeitosa thanks for the desert-level runs. Frame p50 83.0 -> 77.0 ms with more GPU busy per present is the direction the committer samples pointed to. @dantdmdarealguy thanks for the GTA V cross-test. That matches what the head does: the skip is refused when a live unit shadow covers the flushed range ( AI-assisted: yes (Claude Code) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A label's readback store (
StoreReadbackBeforeLabel) and a storage refresh (StorageTexture::refresh) flush the pending storage images over their range before they go on.FlushPendingwaited on the CPU for the images' GPU writers first. When every flushed image lives in a host import, its write-back is a copy the recorder records into the import. That copy runs after the image's writer and before anything recorded later, including the label, so the wait adds nothing to the order. Ghost of Tsushima (PPSA02225) paid about 55 ms for it once a frame. A sampled 4K storage surface overlapped six pending images of a transient heap, and its refresh drained the whole recorder.How
StoreReadbackBeforeLabeland an eligiblerefreshset a thread-local scope. Inside it,FlushPendingskips the wait for the GPU writer when the recorder is active, the call is not in a completion, and every image it flushes has a host import (HostImportFor). The write-backs are then recorded behind their writers as before.APS5_NO_FLUSH_GPU_ORDERED=1andAPS5_NO_REFRESH_GPU_ORDERED=1restore the waits.Tested
agc_driver_label_ordered_flush: a computeimage_storeinto an imported 128x128 R32UI surface, a small storage image (64 KiB, the size a label stores before it), followed directly by aRELEASE_MEMlabel while the store is still pending on the GPU. When the label lands, the stored texels must be in guest memory. 8 runs per test; the test checks that the surface is small.b6007f69a9,StorageTexture::TakeOrderedSkipCounts()counts the skips taken, and the tests check them:agc_driver_label_ordered_flushcovers the label half (at least one flush skip) and the refresh half. For the refresh half, a 256x64 linear R32UI view of the block reads after a 128x128 tiled store over the whole block is left pending. The view isn't an alias, so its refresh flushes the store, and every texel read must equal the stored value and guest memory. The half requires at least one refresh skip.agc_driver_label_ordered_flush_disabled(APS5_NO_FLUSH_GPU_ORDERED=1 APS5_NO_REFRESH_GPU_ORDERED=1) and_unimported(APS5_NO_HOST_IMPORT=1) run the same scenarios. They require zero skips and the same results.agc_driver_ordered_refresh_failureinjects aFlushPendingfailure inside an armed refresh (SetFlushFailureForTest). It checks that the flag is cleared on the thread and that the next flush waits.Recorder::Syncper frame fromStorageTexture::refresh -> FlushPending(APS5_TRACE_LONG_SYNCS).main086c0686c8against the samemain+ this PR, measured by @TripyFox (comment). Windows 11, RTX 5090 (NVIDIA 621.01), 3840x2160, warm shader cache, three interleaved 240 s pairs withAPS5_TRACE_SUMMARY_EVERY=10 APS5_TRACE_BENCH=1:mainrun is slower than the slowest PR run.APS5_NO_FLUSH_GPU_ORDERED=1gives back most of it;APS5_NO_REFRESH_GPU_ORDERED=1gives back about 1 ms.2da15973cegives the same size: p50 -6 ms in every compared interval.mainb11c7bf84fagainstb6007f6merged onto it (comment). OnlylibSceAgcDriver.prxdiffers between the builds. The setup was 240 s runs with a warm cache, two interleaved rounds, and medians of the 10 s intervals from 40 s to 240 s, withAPS5_TRACE_SUMMARY_EVERY=10 APS5_TRACE_BENCH=1:main, round 1main, round 2tools/performance_compare.pyon the two summary lines below (interval from 100 s, round 1): frame p50 -8.3 ms (-9.1 %), p99 -22.9 ms, GPU busy -0.6 ms. The only configuration difference it lists iswindow.first_present.main:{"commit":"b11c7bf84f5d","dirty":false,"env":{"APS5_NO_PAD_INPUT":"1","APS5_TRACE_BENCH":"1","APS5_TRACE_DIR":"<path>","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 5090","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":16,"memory_mib":65140,"title_id":"PPSA21567","title":"ASTRO BOT","content_version":"01.019.000","resolution":"3840x2160","window":{"spec":"summary_every 10","first_present":1641,"ms":10040.788},"interval_start_s":100.904,"interval_end_s":110.945,"presents":110,"frame_ms":{"p50":90.588,"p90":100.512,"p99":114.130,"max":118.423},"gpu_busy_ms_per_present":70.925,"worker_busy_pct":{"queue worker 0x0":97.787,"queue worker 0x40":99.954,"queue worker 0x48":100.058,"queue worker 0x46":2.206},"top_phases_ms_per_frame":{"Driver.Packet":272.861,"Driver.Packet.memory_wait":159.319,"Driver.Packet.draw":55.195,"Driver.Draw":54.820,"Graphics.Draw":46.451,"Graphics.Draw.draw_and_resource_release":37.184,"Driver.Draw.shader_compile_and_link":29.343,"Shader.DrawStage":24.561},"trace_dropped":false,"trace":""}{"commit":"78693efa462b","dirty":false,"env":{"APS5_NO_PAD_INPUT":"1","APS5_TRACE_BENCH":"1","APS5_TRACE_DIR":"<path>","APS5_TRACE_SUMMARY_EVERY":"10"},"gpu":"NVIDIA GeForce RTX 5090","driver":"NVIDIA 621.01","os":"Windows 10.0.26200","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":16,"memory_mib":65140,"title_id":"PPSA21567","title":"ASTRO BOT","content_version":"01.019.000","resolution":"3840x2160","window":{"spec":"summary_every 10","first_present":1452,"ms":10065.337},"interval_start_s":100.957,"interval_end_s":111.022,"presents":124,"frame_ms":{"p50":82.300,"p90":86.826,"p99":91.225,"max":94.329},"gpu_busy_ms_per_present":70.315,"worker_busy_pct":{"queue worker 0x0":97.584,"queue worker 0x40":99.964,"queue worker 0x48":99.992,"queue worker 0x46":1.450},"top_phases_ms_per_frame":{"Driver.Packet":241.869,"Driver.Packet.memory_wait":142.862,"Graphics.Draw":47.769,"Driver.Packet.draw":47.312,"Driver.Draw":46.912,"Graphics.Draw.draw_and_resource_release":38.622,"Driver.Draw.shader_compile_and_link":26.900,"Driver.Packet.prepare_memory":22.820},"trace_dropped":false,"trace":""}.json.gztraces are attached.b6007f69a9: the four tests above plusagc_driver_direct_write_completion,agc_driver_linear_target_direct_write_completion,agc_driver_fill_completion_labels,agc_driver_linear_target_completion_pointsandagc_driver_memory_value_fill.b6007f69a9: 8 of 9 pass in each of 3 runs, including all four new tests. The failure isagc_driver_direct_write_completion, which also fails onmain1c8c0efc8cthere in each of 3 runs (#15564, fixed by #15610)1c8c0efc8cvs headb6007f69a9VUID-Overlap: #12070 also changes
FlushPending's writer wait: it skips waiting for an image's own stamped writes. Both skip the wait on different conditions, so they compose. The second to land rebases the wait condition. No open PR changesStoreReadbackBeforeLabelor the refresh compare.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.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code