Repository navigation
Conversation
XairRrR
approved these changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — approved
The change marks entries that cannot be proven unchanged as live and re-resolves them through cachedTexture on every reuse. That path does the same work as a fresh resolve. The new test step fails on main and passes with the change, and I found nothing blocking.
What I ran
Head 38e7f40892e428811467e05ede279911e15151e0, main 1e658521a0a599872612bb91949311e2956b5283, merged tree 48eb641f852164cb1e1a8b7d4575de365ac2b81c. Debian 13 VM, 2× Xeon Gold 6136, GCC 14.2.0, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 1e658521a0a599872612bb91949311e2956b5283...38e7f40892e428811467e05ede279911e15151e0
python3 tools/check_conventions.py --base 1e658521a0a599872612bb91949311e2956b5283
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R image_table_reuse
# negative control
git show 1e658521a0a599872612bb91949311e2956b5283:core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
ninja -C build agc_driver_image_table_reuse_tests
ctest --test-dir build --output-on-failure -R image_table_reuse
git show 48eb641f852164cb1e1a8b7d4575de365ac2b81c:core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
ninja -C build libSceAgcDriver agc_driver_image_table_reuse_tests
git status --porcelain
ctest --test-dir build -R image_table_reuse
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Build of the merged tree (script) | ok |
| Selected tests (script, 2151 ran) | no new failures and no timeouts. The 7 known failures also fail on main (agc_driver_division, the denormal-mode tests, vector_constant_factor_tininess and one unwrap test) |
| Validation comparison (script, 772 Vulkan tests) | the layer loaded in 771 tests and was missing in none; no new VUID- lines compared with main |
agc_driver_image_table_reuse (validation on) |
passed, 0.76 s |
agc_driver_image_table_reuse_draw_pipeline (validation on) |
passed, 0.74 s |
Host imports are available on lavapipe here, so RunViewedStore actually ran. The "host imports are unavailable" skip line does not appear in the output.
Negative control
I restored ShaderResources.cpp from main and kept the new test. Both tests then fail with the message the description quotes:
output
1/2 Test #1276: agc_driver_image_table_reuse .................***Failed 0.67 sec
AGC graphics: image table reuse: a table of storage-viewed entries, image stores leaving results pending over one between dispatches: 0 reused and 3 resolved tables, expected 3 and 0
2/2 Test #1822: agc_driver_image_table_reuse_draw_pipeline ...***Failed 0.65 sec
AGC graphics: image table reuse: a table of storage-viewed entries, image stores leaving results pending over one between dispatches: 0 reused and 3 resolved tables, expected 3 and 0
After I restored the merged file and rebuilt, both tests pass again, and git status --porcelain is empty.
Findings
Blocking
None.
Non-blocking
-
A table with no reusable entries now takes a reuse slot. In the fresh path, an entry that fails the lookup-log check is pushed as live (
core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp:5649-5651), andreusablestays true. So a table whose entries are all live is still remembered atShaderResources.cpp:5656-5665. Two examples are a table of DCC-compressed entries and a table of entries withlooked->source != nullptr.- On
mainsuch a table was never remembered. - Now every later draw runs
TableCurrentandCachedTableTexturesand then resolves every entry again anyway, so it gains nothing. - It also takes one of the
ReusedTableCount = 4thread-local slots (ShaderResources.cpp:2032), which can push out a table that does benefit from reuse.
Suggested fix: skip
RememberTablewhen no element is non-live and non-rejected (for example, whenfresh.queriesis empty). - On
-
The live branch in
revalidateFullrepeats the fallback loop. The new branch atShaderResources.cpp:3827-3836repeats the decode andcachedTexturecomparison of the fallback loop atShaderResources.cpp:3847-3855. A small shared lambda would keep the two from drifting apart, for example if the fallback later gainslayerRowsBindingthe way the non-table walk atShaderResources.cpp:3795has it.
I also checked these and found no problem:
- Reuse matches a fresh resolve.
resolveLivedoes the same steps as the fresh path (decodeEntry,prepareSampledwithCompletePendingCpuWrites,lookupSampled,bind). In the reuse path,storageTableanditem != nullptralready exclude theimageWrittenand missing-binding branches. - The cached-texture cursor stays in step.
nextadvances the same way inCachedTableTextures, in the capacity branch and in the reuse loop. - The aggregate initialiser
{{}, {}, {}, 0, firstLayer, false, {}, true}is correct. It matches the field order (livecomes aftermessage). - Every pass through the fresh loop either adds one element or clears
reusable. Sofresh.elementskeeps its size equal tobinding.count.
Not verified
- I did not check the PPSA18653 performance numbers or the attached traces, and I did not launch any title. The trace commits (
68a15ec229/12ed9524c8) also carry #15327 and are not the PR head, as the description says. - I did not check the claimed line overlap with #9508.
- I could not check behaviour on non-lavapipe GPUs. The author reports an NVIDIA RTX 5080 run.
Author
|
Thanks @XairRrR, both non-blocking points are taken in b2ca8fa:
Re-run on the same machine: |
Contributor
✅ 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. |
XairRrR
approved these changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — approved
Commit b2ca8faf64 resolves both non-blocking points from my last review. A table whose entries are all live is no longer remembered, and the two revalidateFull paths share currentTableEntry. The tests and the negative control behave as expected.
What changed since my last review
- New commit
b2ca8faf64("keep tables with nothing to reuse out of the reuse slots"). Commit38e7f40892is unchanged.ShaderResources.cpp:5677-5678addsanyKept. A table is remembered only when at least one element is not live. Rejected elements count as not live, so a table of rejected entries is still remembered, as onmain.ShaderResources.cpp:3812-3820adds thecurrentTableEntrylambda, which is now used by the live branch (:3838) and by the fallback loop (:3853).
- Test changes.
RunViewedStorenow points recordsViewed..Recordsat a second block that is not imported (plain). The table therefore mixes kept and live entries, and the expected count goes from 3 reused / 0 resolved to 4 / 0. A new step atImageTableReuse.cpp:296-301binds a table of only storage-viewed entries and expects 0 reused and 2 resolved. - Author reply in the discussion, describing the above.
What I ran
Head b2ca8faf642de09526cd5d4a7b98574d7e7a7b5f, main 086c0686c8adff3ee648fdbe34e0b0b647e1d511, merged tree 0a25af341deac165d5a9ded01ebc7082aa852e88. Debian 13 VM, 2× Xeon Gold 6136, GCC 14.2.0, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 086c0686c8adff3ee648fdbe34e0b0b647e1d511...b2ca8faf642de09526cd5d4a7b98574d7e7a7b5f
python3 tools/check_conventions.py --base 086c0686c8adff3ee648fdbe34e0b0b647e1d511
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R image_table_reuse
# negative control 1: drop only the new anyKept condition
sed -i "s/if (reusable && anyKept && CurrentTableState() == state)/if (reusable \&\& CurrentTableState() == state)/" core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
ninja -C build agc_driver_image_table_reuse_tests
ctest --test-dir build --output-on-failure -R image_table_reuse
# negative control 2: all production code from main
git show 086c0686c8adff3ee648fdbe34e0b0b647e1d511:core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
ninja -C build agc_driver_image_table_reuse_tests
ctest --test-dir build --output-on-failure -R image_table_reuse
# validation lines of agc_driver_image_gather_volume, with the PR's file and with main's file
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R "^agc_driver_image_gather_volume$"
# restore
git show 0a25af341deac165d5a9ded01ebc7082aa852e88:core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
ninja -C build libSceAgcDriver agc_driver_image_gather_volume_tests agc_driver_image_table_reuse_tests
git status --porcelain
ctest --test-dir build -R image_table_reuse
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Build of the merged tree (script) | ok |
| Selected tests (script, 2171 ran) | no new failures and no timeouts. The 7 known failures also fail on the baseline main |
agc_driver_image_table_reuse / _draw_pipeline (validation on) |
both passed, 0.80 s and 0.76 s; host imports are available, so RunViewedStore ran |
| Validation comparison (script, 783 tests, layer loaded in 782, missing in none) | one test with new lines, agc_driver_image_gather_volume. These lines come from main, not from this PR (see below) |
Negative control 1 (anyKept removed) |
both tests fail: a table of only storage-viewed entries, which has nothing to reuse: 1 reused and 1 resolved tables, expected 0 and 2 (the message the author quoted) |
Negative control 2 (ShaderResources.cpp from main) |
both tests fail: a table of storage-viewed and other entries, ...: 0 reused and 4 resolved tables, expected 4 and 0 |
| After restore | both tests pass; git status --porcelain is empty |
About the agc_driver_image_gather_volume lines. The script marks them "(test not run on main)" because the test was added to main after the validation baseline 1e65852. I ran it with validation on, once with the PR's ShaderResources.cpp and once with main's. Both runs give the same IDs, so they come from main, not from this PR:
VUID counts, the same with either file
2 VUID-VkSamplerCreateInfo-addressModeU-01079
2 VUID-VkSamplerCreateInfo-addressModeU-parameter
2 VUID-VkSamplerCreateInfo-addressModeV-parameter
2 VUID-VkSamplerCreateInfo-addressModeW-parameter
Previous findings
- A table with no reusable entries takes a reuse slot: Resolved.
ShaderResources.cpp:5677-5678now remembers a table only if some element is not live. With that condition removed, the new step fails with1 reused and 1 resolved tables, expected 0 and 2; with it, the step passes. The author chose "any element not live" instead of my suggestedfresh.queriesempty, so tables of rejected entries are still remembered as onmain. That keeps their rejection messages from being decoded again, which is reasonable. - The live branch in
revalidateFullrepeats the fallback loop: Resolved. Both paths now callcurrentTableEntry(ShaderResources.cpp:3812-3820, used at:3838and:3853). The fallback condition is equivalent to the old one: thetextureIndex >= textures.size()guard moved into the lambda, and the comparison is the same||chain. The lambda capturestextureIndexby reference, and both callers advance it after the call, as before.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- I did not check the PPSA18653 performance numbers or traces, and launched no title. They were captured on
68a15ec229/12ed9524c8, which carry #15327 and the first commit only. The second commit only narrows when a table is remembered, so it should not slow the measured scene, but I have not measured that. - I could not check behaviour on non-lavapipe GPUs. The author reports a 117/117 rerun on the NVIDIA RTX 5080, which I accept as author-reported.
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, 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. |
12 tasks done
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)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. |
A table entry viewed through a cached storage image, or otherwise not provably unchanged, no longer stops the whole table from being reused. Such entries are marked live and resolved through the texture cache on every draw; the rest of the table is reused as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A table whose entries are all live gains nothing from reuse, so it no longer takes one of the reuse slots. The live entry check in revalidateFull and its fallback now share one lambda. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
XatiX
force-pushed
the
perf/agc-image-table-live-entries
branch
from
November 4, 2026 10:49
b2ca8fa to
ec5cbbd
Compare
Author
|
Rebased onto The conflict was in Re-run on the rebased head (RTX 5080, 617.42, Windows 11, WinLibs GCC 15.2.0 Release,
|
Author
|
Paired runs for the rebased head
|
Contributor
✅ 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 (13 IDs in 549 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. |
12 tasks done
Contributor
|
One non-blocking point on the new test step. I ran it ported onto our tree (described below) on two Linux hosts, not the PR head itself.
if (!AgcDriver::GuestMemory::WriteWatched()) {
std::puts("guest memory has no write watch: a table of storage-viewed entries is not tested");
return;
}With that, the step skips on that host. It passes on a desktop with the watch (RX 9070 XT, RADV Mesa 26.2.4, Linux, run with Timing on 007 Setup:
Six 20-minute runs in one session, alternated base / port. The figure is seconds per frame over presents 5 to 1,005 after the first lit frame of the scripted opening walk:
AI-assisted: yes (Claude Code) |
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)Validation IDs already present on main (4 IDs in 556 tests)Skipped (67)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. |
Open
12 tasks done
12 tasks done
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
An image table was reused across draws only if every one of its entries was provably unchanged. One entry that failed the check, for example a sampled texture viewed through its cached storage image (
SampledFromStorageEligible: host-imported memory and a storage-capable, uncompressed format), made the whole table resolve again on every draw.Now such an entry is marked live in the remembered table. When the table is reused, a live entry is resolved through
cachedTextureon every draw, as a fresh table resolves it, and the remaining entries are reused as before.CachedTableTexturesand the revalidation path skip live entries in the same way. Entries that are rejected, written by the dispatch, or past the table capacity are handled as before.PPSA18653 binds a table of about 14,200 entries in each of about 730 draws per frame. About 1.6% of each table (about 229 entries) are storage-viewed, so no table was ever reused, and
Resources.Completetook about 5.5 s per frame.The new step in
agc_driver_image_table_reusehost-imports a guest block, so its entries are storage-viewed, and then checks three things:On
mainit fails with0 reused and 3 resolved tables, expected 3 and 0.Tested
ctest -V -R "table|texture|storage|sampled|bindless"withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1. Both runs were on the same machine, with the related execution tests built (ANYPS5_AGC_EXECUTION_TESTS):main: 115/117 passed. Onlyagc_driver_image_table_reuseandagc_driver_image_table_reuse_draw_pipelinefail;VUID-error. The same 4 tests skip in both runs (multisampled pass order and 3 packed16 storage formats). Logs attached.APS5_TRACE_SECONDS=20:60after a warm run, with no other trace or profile switch and the log not redirected.maincannot reach this screen on its own, so both builds carry fix(agc): copy GPU-side indirect draw records when the packet is processed #15327 (GPU hang) and the opt-in clock change, run withAPS5_FAIL_UNKNOWN_CLOCKS=1. The base is68a15ec229and the change is12ed9524c8, the same tree plus this commit. Traces attached.Resources.Completeper frame{"commit":"68a15ec2299e","dirty":false,"env":{"APS5_FAIL_UNKNOWN_CLOCKS":"1","APS5_TRACE_SECONDS":"20:60"},"gpu":"NVIDIA GeForce RTX 5080","driver":"NVIDIA 617.42","os":"Windows 10.0.26300","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":16,"memory_mib":32404,"title_id":"PPSA18653","title":"Age of Mythology: Retold","content_version":"01.043.000","resolution":"3840x2160","window":{"spec":"seconds 20:60","first_present":505,"ms":65172.035},"presents":9,"frame_ms":{"p50":7965.365,"p90":8422.926,"p99":8422.926,"max":8422.926},"gpu_busy_ms_per_present":396.860,"worker_busy_pct":{"queue worker 0x0":99.978},"top_phases_ms_per_frame":{"Driver.Packet":7239.496,"Driver.Packet.draw":7108.396,"Driver.Draw":7107.726,"Driver.Draw.draw_and_resource_release":5623.944,"Graphics.Draw":5599.273,"Graphics.Draw.draw_and_resource_release":5589.065,"Resources.Complete":5494.540,"Driver.Draw.shader_compile_and_link":1463.332},"trace_dropped":false,"trace":"aps5-trace-68a15ec2299e-15048.json.gz"}{"commit":"12ed9524c874","dirty":false,"env":{"APS5_FAIL_UNKNOWN_CLOCKS":"1","APS5_TRACE_SECONDS":"20:60"},"gpu":"NVIDIA GeForce RTX 5080","driver":"NVIDIA 617.42","os":"Windows 10.0.26300","cpu":"AMD Ryzen 7 9800X3D 8-Core Processor","cpu_threads":16,"memory_mib":32404,"title_id":"PPSA18653","title":"Age of Mythology: Retold","content_version":"01.043.000","resolution":"3840x2160","window":{"spec":"seconds 20:60","first_present":506,"ms":60279.308},"presents":20,"frame_ms":{"p50":3178.079,"p90":3369.582,"p99":3401.643,"max":3401.643},"gpu_busy_ms_per_present":298.053,"worker_busy_pct":{"queue worker 0x0":99.953},"top_phases_ms_per_frame":{"Driver.Packet":3012.245,"Driver.Packet.draw":2941.594,"Driver.Draw":2941.148,"Driver.Draw.shader_compile_and_link":1574.332,"Driver.Draw.draw_and_resource_release":1349.491,"Graphics.Draw":1324.282,"Graphics.Draw.draw_and_resource_release":1315.625,"Resources.Complete":1205.909},"trace_dropped":false,"trace":"aps5-trace-12ed9524c874-25392.json.gz"}tools/check_conventions.pypasses.tools/pr_overlap.py --branchreports one line overlap, with perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508 atShaderResources.cppL5466-5467: both change the capacity condition of the reuse loop, and the merge keeps both.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)aps5-trace-12ed9524c874-25392.json.gz
aps5-trace-68a15ec2299e-15048.json.gz
change-ctest.log
main-ctest.log
🤖 Generated with Claude Code