Repository navigation
fix(shader): bound the bytes the recompiler's result memos hold - #9989
Conversation
|
The global budget can remain exceeded after concurrent insertions. |
MarcZX10
left a comment
There was a problem hiding this comment.
The global result memo budget can remain exceeded after concurrent insertions. trimResultMemo uses try_to_lock; when another trim owns the mutex, this call returns without trimming or recording pending work. If the holder sampled heldBytes before the concurrent insertion, it may trim only to 75% of the earlier value and exit, leaving the new bytes over budget indefinitely if no later insertion occurs. Please serialize/retry/schedule the trim and add a focused concurrent regression test. I posted the detailed interleaving in the PR discussion. Existing reported single-threaded tests and GPU measurements are useful but do not exercise this case; I could not rerun locally because the build failed on an unrelated object with / out of space.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 95955a6 against current main 9d6adeb. The concurrency issue from my earlier review is addressed: trimming now serializes on a blocking mutex, reloads the global held-byte count until it is within budget, and includes a separate eight-thread regression test registered with CTest. I also checked that the new test has its own registration and is included in the Windows runtime-path properties.
I reran the convention check against current main, the 172-script registration check, and git diff --check; all pass. I could not run the C++ budget tests locally because this workspace has no configured AnyPS5 build or test executable. The PR reports the focused CPU tests, including the concurrent budget case, passing on Linux and Windows; hosted product build jobs are skipped by repository policy.
The description calls out the remaining integration point with open #9049: if that reuse change lands after this PR, its slot removal must also update the byte/entry counters. The current branch is mergeable and the same-function overlap scan found no duplicate implementation. With that integration detail recorded and the earlier concurrency finding resolved, I found no blocking issue on this head.
|
@MarcZX10 thanks for the re-review. One correction to what the approved head shows: its eight-thread test passes with the old A version that arranges the race is on
Would you rather have that version on this PR, or keep the head as approved? I'll push it here only if you want it. AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
The author’s follow-up establishes that the current eight-thread test passes with both the new and old trim implementation, so it does not detect the reported race. The proposed deterministic race test on fix/result-memo-budget-race-test is described as 10/10 failures with the old trim and 20/20 passes with the new one, which directly covers the missing regression case. Please add that test (or an equivalent committed red/green test) to this PR and rerun the focused 41-test suite. The current head is still clean against main and policy checks pass, but I’m withdrawing my earlier approval until the committed test distinguishes the fix from the old behavior.
AI-assisted review.
95955a6 to
2fb7fb6
Compare
|
@MarcZX10 thanks. The deterministic test is on this PR now: the head is On this head:
I updated How and Tested in the description to match: the blocking trim, the hook and the new test. A Windows VM run of this head comes next. The VM is busy with another PR's run for about an hour. AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 2fb7fb6e57db6751f3815732022a8f2ce08dcaac. The prior race concern is addressed in the committed test: the trim hook starts two inserts after the active trim sampled heldBytes, waits for their accounting, then the test asserts the completed trim is within budget. The description reports the old implementation failing 10/10 and this implementation passing 20/20, plus the focused 41-test Linux suite and Windows focused tests. The implementation holds a blocking trim mutex and reloads the byte count after each eviction pass, matching the tested race.
I also ran the exact-head convention check and git diff --check; both pass. check_main_sync.py reports a clean merge, but this head is 2 commits ahead and 138 behind current upstream/main (bd4906d7), with both branches changing Recompiler.cpp, Recompiler.hpp, ShaderDiskCache.cpp, the AGC CMake registrations and ENVIRONMENT.md. The intervening main changes include recompiler fixes and new tests in those shared files. Please rebase this branch onto current main, resolve the shared-file changes, and rerun the focused result-memo tests and the matched Vulkan validation checks on the resulting main/head pair. Until those checks cover the current base, I can’t approve this stale snapshot. This is a current-base and validation request; the previously reported race itself is resolved.
2fb7fb6 to
59700b2
Compare
|
@MarcZX10 rebased onto
The description's Tested section now names this pair. AI-assisted: yes (Claude Code) |
|
@milescoviello a note on order with #10000 (ours, the sampler and sampled-pair limits): both PRs add test names to the same |
|
@EagleBearerYS thanks, agreed. If #10000 lands first instead, I'll keep both PRs' test names on that line here. AI-assisted: yes (Claude Code) |
|
Data point from The Last of Us Part I (PPSA03396), CachyOS, RX 6650 XT, 32 GB RAM. Without this PR the process's anonymous memory grew about 1 GiB every 10 s until the host ran out (RAM + 16 GB swap) and a dispatch failed with AI-assisted: yes (Claude Code) |
|
@Pacuka thank you. A second title that runs out of memory without this, with an allocation tracker pointing at the same AI-assisted: yes (Claude Code) |
59700b2 to
dbec127
Compare
|
@MarcZX10 rebased onto current On that main/head pair:
Also a cross-title look at how much the memos hold, from jemalloc heap profiles on AI-assisted: yes (Claude Code) |
|
The A/B with this head (
AI-assisted: yes (Claude Code) |
|
Follow-up on the cold Super Meat Boy 3D stop: four more cold runs (no AnyPS5 shader cache, AI-assisted: yes (Claude Code) |
|
@milescoviello the overlap check pairs this PR with #10423 (masked result memo) and #10757 (recycled results). #10423 adds a second per-source table, Proposal: this PR goes first. Once it merges, I rebase #10423 and #10757 onto it:
#10423 and #10757 then declare |
|
@meh7an yes, that order works for me: this PR first, then #10423 and #10757 rebased onto it with This head ( AI-assisted: yes (Claude Code) |
|
@milescoviello here is a data point for the rebase onto
In this scene the memos hold about 0.9 GiB, flat from 300 to 700 s. This includes #10423's masked table, which AI-assisted: yes (Claude Code); results above were run on this machine. |
Every shader source keeps up to 256 materialized results of Recompile(request, capture) in its result memo, and as many patch recipes in its masked memo, with no limit on their bytes. In Ghost of Tsushima's opening the memos held about 500 MiB after 5 minutes and kept growing, at a 13 % hit rate. Each memo entry now counts the bytes its result's vectors keep (the bindings' vectors included; the module is shared with the prepared module cache), plus its patch recipe in the masked memo. A result held by both tables counts once in each. Past a budget, 256 MiB unless APS5_RESULT_MEMO_MB sets another (0 sets none), the least recently used entries of both tables of all sources are dropped until three quarters of the budget is left. The recency stamps come from one clock for all sources and both tables so they compare across them. The profile line also reports the trimmed and held entries and their MiB. The trim takes its lock blocking and repeats, re-reading the bytes held, until they are within the budget, so the insertion that pushed them over has its trim run after any other; it stops when nothing is left to drop. SetResultMemoBudget and ResultMemoBytes let agc_shader_result_memo_budget check that a memo over its budget keeps the most recently used results and drops the oldest one, with the masked memo off. A one-shot hook inside the trim (SetResultMemoTrimHook) lets agc_shader_result_memo_budget_threads insert two results from other threads after the trim has read the bytes held: the memo must still end within its budget. agc_shader_result_memo_budget_masked, with the masked memo on, checks that a first result counts its masked entry too and that a trim empties the masked memo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@thuanquang thank you. The flat 0.9 GiB from 300 to 700 s is a useful baseline for what the memos hold in that scene. #10423 landed before this PR, so I've rebased onto
I'm running the shader tests on it now. I'll push it here once they pass and post when it's up. Then the same pair (default budget vs AI-assisted: yes (Claude Code) |
dbec127 to
8868e09
Compare
|
@thuanquang @MarcZX10 rebased onto current
The matched Khronos validation comparison ( AI-assisted: yes (Claude Code) |
|
@milescoviello here is the Persona 5 Royal pair on
In this scene the memos hold about 900 MiB without a budget and about 150 MiB with the default one, so the budget saves about 0.74 GiB of private memory. All three runs shared the GPU with other work, so I don't compare their frame rates. AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@thuanquang thank you, that's the measurement this PR needed: about 900 MiB down to about 150 MiB, 0.74 GiB of private memory saved in Persona 5's first gameplay scene, with the memos-off run as the floor. I've added it to Tested. AI-assisted: yes (Claude Code) |
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.
|
Checked the byte accounting (result, bindings, masked entry and patch recipe), the lock order (source then registry; the trim takes its own mutex and then each source's, outside the insert lock), the shared LRU clock, and that the three budget tests are registered, including the Windows PATH list. |
|
Thanks for the review and the merge, @adriadam10. AI-assisted: yes (Claude Code) |
What
Every shader source keeps up to 256 materialized results of
Recompile(request, capture)in its result memo (64 sets of 4), with no limit on their bytes. A result's bindings carry about twenty vectors each, sized by the descriptor counts, so with large image tables one result can hold tens of KiB. In Ghost of Tsushima Director's Cut (PPSA02225), whose opening runs into the 20 GiB memory cap on my host, the memos held 470 to 575 MiB after 5 minutes and kept growing, at a hit rate of about 13 %.This bounds the bytes all memos hold together.
How
APS5_RESULT_MEMO_MBsets another; 0 sets none.APS5_RESULT_MEMO_MBis neutral for the disk cache key, since it doesn't change any result.APS5_PROFILE_DRAW's[recompile] result memoline now also reports trimmed results and the results and MiB held.SetResultMemoBudgetandResultMemoByteslet the new tests set a budget and read the bytes held.SetResultMemoTrimHooksets a one-shot function that the trim calls right after it reads the held bytes, so a test can insert from other threads at exactly that point.Tested
8868e09624, rebased ontomain442c43a. perf(shader): reuse a memoized result by patching its copied words #10423 landed first, so this head counts and trims its masked memo as well (one commit now). Linux (Ubuntu 26.04.1, GCC 15.2, Release, RTX 4070 Ti, 615.71.09):^agc_shader_tests pass 3 of 3, including the three budget tests and perf(shader): reuse a memoized result by patching its copied words #10423'sagc_shader_table_snapshot_reuse/_unmasked.APS5_NO_MASKED_RESULT_MEMO=1. The newagc_shader_result_memo_budget_maskedruns with the masked memo on. It checks that a first result counts its masked entry too, and that a trim to one byte empties both tables, so the next result is a full one, not a patched one.a trim to one byte left 620 bytes.main442c43a and 37 of 37 on this head, no layer missing, noVUID-messages on either.mainand 44 of 44 pass 3 of 3 on this head.main73ad397 plus feat(libSceImeBackend): declare 22 sceImeBackend exports #10887, fix(libSceGameUpdate): answer no update instead of throwing on request creation #10614, fix(libSceVideodec2): mark returned pictures as not discarded #10913 and fix(agc): keep at most 4 GiB of the host heap from the host import budget #7844 with this PR merged). Three runs, each 780 s into the first gameplay scene, with private bytes sampled from outside the process:APS5_RESULT_MEMO_MB=0,main's behaviour): 19,408 / 19,416 MiB at 500 / 700 s, peak 18.99 GiB.APS5_NO_RESULT_MEMO=1): 18,512 / 18,514 MiB, peak 17.98 GiB.dbec127257, rebased ontomain5e6ae6e (include list conflict inRecompiler.hpp, both kept). Linux (Ubuntu 26.04.1, GCC 15.2, Release, RTX 4070 Ti, 615.71.09): the 44 tests matching^agc_shader_result_memo_budget|^agc_shader_memory$|^agc_shader_|^request_serializer$|shader_disk_cache|^agc_driver_graphics$|image_tablepass 3 of 3. Khronos layer 1.4.363 on that set:main42 of 42, this head 44 of 44, no VUID on either side. Windows 11 VM (GT 1030, 582.66):main6 of 6 and this head 8 of 8 of the memo, shader memory, serializer and disk cache tests, 3 of 3.Ubuntu 26.04.1, GCC 15.2, NVIDIA RTX 4070 Ti (615.71.09).
Rebased onto
main0a0ed72 as59700b2200.git range-diffshows both commits unchanged. The tests below marked "this head" ran on it.Ghost of Tsushima Director's Cut (PPSA02225). Main can't take this title past its first minutes, so these runs use @EagleBearerYS's
test/ghost-9742-9717stack with #8115, #8121, #8213, #8608 and #9318, and this change applied with no budget unlessAPS5_RESULT_MEMO_MBset one. Each run is 300 s warm, journal replay off, 20 GiB cap. Results and MiB are the memo line at the end of each run; hits are its last 10 s.APS5_NO_RESULT_MEMO=1)The budget halves the memo's bytes without changing its hit rate. The number of results held barely changes, so the trimmed least recently used results are the large ones. RssAnon varies by a few hundred MB between identical runs here, so the memo's own count is the measure.
The Last of Us Part I (PPSA03396), from @Pacuka (comment): CachyOS, RX 6650 XT, 32 GB. Without this change, the process's anonymous memory grew about 1 GiB every 10 s, until the host ran out of RAM and swap and a dispatch failed with
std::bad_allocafter 1.5 to 5 minutes. An allocation tracker put the never-freed bytes undermaterializePreparedMemoized: 962 MiB ofDescriptorBindingBuilder::Populatevector copies and 410 MiB undermaterializeResultafter 2 minutes.APS5_NO_RESULT_MEMO=1stopped the growth. With this PR's two commits, anonymous memory stayed between 2 and 4 GiB through a 7-minute run with no failure.Forspoken (PPSA03984), on
main895f47f with this change (an earlier head with the same code), 300 s, 16 GiB cap. Hits are over the whole run:APS5_RESULT_MEMO_MB=0)Here the memo stays near the budget, and trimming doesn't cost hits.
Two later warm runs of that build, one with the budget off, lost the device at 161 s (
VK_ERROR_DEVICE_LOSTinvkWaitForFences). With the budget off this change only counts bytes, and the eviction order within a set is the same asmain's. In a second set of runs past that point, 4 of this build (1 cold and 3 warm, budget off) and 5 ofmainat the same base (1 cold and 4 warm) all ran without a device loss. I read it as a fault of that GPU session and mention it in case anyone sees it again.Tests
agc_shader_result_memo_budget(tests/ShaderMemory.cpp, CPU only): a pixel program that reads its user data, compiled throughRecompile(request, capture)with four user data values. With a budget of three results, a fourth is trimmed. The memo stays within the budget after every insert, the two most recently used results still hit, and the oldest misses. Clearing the budget lets the memo grow past it again.agc_shader_result_memo_budget_threads(CPU only), for the race @MarcZX10 described: with three results in a budget of three, the test inserts a fourth, and the trim hook starts two threads that each insert one more. The hook waits until both insertions are counted, so they land after the running trim has read the held bytes. The test then checks that the memo ends within its budget.try_to_locktrim (the threads skip their own trims, and the running trim works from the count it read before them), it fails 10 of 10: "results inserted during a trim left the memo over its budget".agc_shader_result_memo_budget, on59700b2200and before the rebase on2fb7fb6e57.^agc_shader_result_memo_budget|^agc_shader_memory$|^agc_shader_|^request_serializer$|shader_disk_cache|^agc_driver_graphics$|image_tablepass 3 of 3 on this head (41 before the rebase, on2fb7fb6e57).main0a0ed72 against this head over^agc_driver_graphics$|image_table|^agc_driver_prepared(34 tests each): both pass, and both report onlymain'sVUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605(x14, which fix(agc): copy a linked pipeline's color sample counts only when it has color attachments #7985 fixes). Nothing is new in this branch.tools/env_reference.py --checkfinds the new row. Its one remaining error,APS5_NO_WGI, is undocumented onmaintoo.trimResultMemo),agc_shader_result_memo_budgetfails 3 of 3: "the memo holds more than its budget".ANYPS5_REQUIRE_VULKAN=1): the 7 tests matching^agc_shader_result_memo_budget$|^agc_shader_memory$|^request_serializer$|shader_disk_cachepass 3 of 3 on the first head of this branch. On this head, the 8 tests matching^agc_shader_result_memo_budget|^agc_shader_memory$|^request_serializer$|shader_disk_cache,agc_shader_result_memo_budget_threadsamong them, pass 3 of 3. The 6 of them that exist onmainpass there too, atmain0a0ed72.Checklist
main; no other open PR implements the same functions (perf(shader): reuse an evicted memo result's bindings for the next miss #9049 changes the same miss path inmaterializePreparedMemoizedto reuse an evicted result's bindings; see Overlap)NotImplemented_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)🤖 Generated with Claude Code