Repository navigation
fix(shader): release specialized modules that no result uses - #10473
adriadam10 merged 2 commits into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the exact head bb9c75b3546e76ae175c55cd30be9d9d2857f338, including the weak interning path, module-cache rebuild path, lifetime tests, and the reported main/change validation. I found no code-level blocker in those paths. The branch is based on db6af9e0fd8c75366dabc2066dbf90117f7490d0, while current main is 379b1dca32ecc4e3570ad0a53c69f019b70dbe59 (30 commits ahead); the repository requires PR branches to stay up to date with main. Please refresh the branch and rerun the focused shader tests on the updated main and change, then I can re-review. I did not independently run the GPU tests in this review.
|
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.
|
bb9c75b to
460029a
Compare
|
@MarcZX10 refreshed: rebased onto AI-assisted: yes (Claude Code) |
|
A caveat on the Ghost of Tsushima comparison above, from a third run. This one had both changes, plus a third that is inert on a fresh cache version, at a 19 GB cap instead of 20 GB. It was killed by the cap at 913 s. So the run-to-run spread in this title's opening is about as large as the per-minute difference I reported, and those numbers shouldn't be read as the saving. The run lengths point the same way: 633 s without the changes, 1,260 s and 913 s with them, the last at the lower cap. The count of interned modules held only by the module caches (323 to 345 MiB at 10 to 12 min) is a direct count, not a comparison. AI-assisted: yes (Claude Code) |
The specialized module caches (by variant and constants, by binding plan and by registration key) and the SPIR-V interning table held every module's words for the rest of the run, after the results that used them were gone. They now keep a module's interface (bindings, push data, specialization ID and content hash) and only a weak reference to its words; a lookup after the words were released builds them again from the same key, checks the content hash, and keeps the specialization ID that names the module's pipelines. The interning table keeps weak references and drops expired ones from the bucket it searches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
460029a to
611daf9
Compare
|
@adriadam10 thank you for the rebase and for 611daf9. Those two helpers bound a reference to the words of a temporary AI-assisted: yes (Claude Code) |
|
@MarcZX10 the branch is refreshed: @adriadam10 rebased it onto AI-assisted: yes (Claude Code) |
|
A check on @adriadam10's 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.
|
Rechecked against current main (#6010, #8632 and #10496 touch other parts of |
|
Thanks for the recheck against the newer Recompiler.cpp changes and for merging, @adriadam10. AI-assisted: yes (Claude Code) |
What
Three caches in
Recompiler.cppkeep every specialized module for the rest of the run:specializeModule's map (by variant and specialization constants), each prepared binding plan'smodules, andregistrationModule's map. Since #8340, the SPIR-V interning table holds every module's words as well. A module's words outlive the results that used them, so the memory only grows. Once the draw caches start evicting (#9129's byte budget, for example), most of it belongs to no result.In Ghost of Tsushima (PPSA02225), with a diagnostic that counts the interned modules held only by these caches (Linux, RTX 4070 Ti, the stack the title needs to reach its opening plus #8340):
The title was killed by its 20 GB cap at 633 s in its opening. The same count in lighter titles: MGS4 (PPSA18194) 24 MiB idle of 44 MiB, MXGP 26 (PPSA30839) 2 of 110 MiB, Super Meat Boy 3D (PPSA31162) 7 of 123 MiB, at 300 s each.
How
SharedSpirv::Weakis a reference that does not keep the words alive:Lock()returns them while anySharedSpirvstill holds them, and an empty module after that.InternSpirvkeeps weak references and drops the expired ones from the bucket it searches. Equal words are still shared while a holder lives, as fix(shader): share the words of identical specialized modules #8340 does.SpecializedModulekeeps the interface: its bindings, push data use,specializationId, the content hash of its words, and a weak reference to the words.buildSpecializedModulereturns the module together with the words it built.specializeModule,registrationModuleand the prepared plan's module cache return the words with the module. When the words were released, they are built again from the same key under the module's mutex. That gives the same module, because the key carries the variant and every specialization constant, including the vertex input classes. The content hash is checked (a mismatch throws), so thespecializationIdthat names the module's pipelines stays valid.Tested
Ubuntu 26.04.1, GCC 15.2, Release, RTX 4070 Ti, NVIDIA 615.71.09. The commit was tested as
09a7d25150onmain9d4fb4f and is unchanged (git range-diff) on the current head611daf97e8, which @adriadam10 rebased ontomainaeeaefa and which adds611daf97e8: two test helpers (DenormalFlushElimination.cpp,RedundantValueElimination.cpp) bound a reference to the words of a temporary result, which this change now frees. The 38 tests below pass 3 of 3 on aeeaefa and on611daf97e8; on the earlier head460029a65athey passed 3 of 3 onmain379b1dc too.verifySpecializedModuleReleaseinagc_shader_memory: a cached pixel request that gets specialized is recompiled. A weak reference to its result's words expires once the result is gone. Recompiling it then gives the samespecializationIdand the same words, and a second live result shares them. NewtestInternedSpirvReleasesUnusedWordsinagc_spirv_module: interned words are released after their last holder, and interning them again gives the same words.test/specweak-mut), both fail 3 of 3:specialized module release: the module caches kept the words of a module no result usesandthe SPIR-V interning table kept words no holder uses.^agc_shader_memory$|^agc_spirv_module$|^agc_shader_|shader_disk_cache|^agc_driver_graphics$|pipeline_journalpass 3 of 3 onmain9d4fb4f and on09a7d25150.^agc_shader_memory$|^agc_spirv_module$|^agc_shader_disk_cache$|^agc_driver_graphics$|^agc_driver_pipeline_journal$: 5 of 5 onmain9d4fb4f and on09a7d25150. Neither log reports the layer missing, and neither has a VUID.mainat the merge base and on09a7d25150.RssAnonat 4 / 6 / 8 / 10 / 12 min: 4,577 / 5,533 / 5,833 / 6,266 / 6,745 MB against 4,892 / 6,152 / 6,990 / 7,763 / 8,095 MB, up to 1.5 GB lower in that pair. A third run with both changes (19 GB cap, killed at 913 s) came out between the two at 6 to 12 min, so the per-minute difference is within this title's run-to-run spread; the run lengths and the direct count above are the firmer evidence. The two changes are measured together. NVIDIA's own shader cache was warmer for the second run, which helps its progress, not its memory.Not checked by me: AMD, Intel, macOS.
Overlaps: #6959 (JJenkx) adds a
specializationlist toSpecializedModule, read from the built words, and copies it into each result, in the same functions this PR splits. Under this PR, that list belongs with the interface the module keeps: a rebuild gives the same words, so it stays valid. Whichever lands second carries it across. #9989 (mine) adds a test next to the same line ofShaderMemory.cpp.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)🤖 Generated with Claude Code