Repository navigation
feat(recompiler): load prebuilt shader packs before the writable cache - #5792
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the PR. Points to address:
- Conflicts with
mainincore/shader/recompiler/ShaderDiskCache.cppandShaderDiskCache.hpp; please rebase and re-runshader_cache_file_read. - The description mentions
--no-prebuilt-shaders, but nothing in the diff parses that flag (only theANYPS5_NO_PREBUILT_SHADERSenv var). Add it or fix the description. - No test is added. The pack format (
PackHeader/PackRecord, index checksum, sorted-hash lookup, rejection of truncated/invalid records, fall-through to the writable cache) can't be exercised on Linux CI as it stands, and the pack writer is not in this PR. Please add a ctest that builds a pack fromEncodeEntryoutput and checks hit, miss, key mismatch, corrupt index/record and wrong source version (it must fail without the change). EntryKeyandIsCurrentEntryHeaderare declared/defined but unused in this PR; either use them or move them to the PR that needs them.CompileShaderUncachedlikewise has no caller or test here.- The Windows buffered-read path (
ReadFileoverlapped,ANYPS5_PREBUILT_MAPPED_READ) is untested; say which CI job covers it or drop it from this PR. docs/dev/ARCHITECTURE.mdshould describe the new prebuilt-pack lookup before the writable cache.
c263048 to
8e619d0
Compare
|
Thanks for the review — all six points are addressed in the force-pushed update:
Verified locally after the rebase: |
…cache Integrate SaloQT's upstream PRs boykopovar#5792 and boykopovar#5793 against main 3242151. Preserve mapped writable entries and resource-plan caching, fix Linux tool linking and SPIR-V span access, and accept padded cache payload envelopes. Add pack corruption, lifetime, cross-process and alignment regressions. Co-authored-by: SaloQT <123214229+SaloQT@users.noreply.github.com>
|
On
A prebuilt hit also adds to the writable cache's The loose-folder path (32-character
|
|
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.
|
8e619d0 to
26d473f
Compare
…cache Integrate SaloQT's upstream PRs boykopovar#5792 and boykopovar#5793 against main 3242151. Preserve mapped writable entries and resource-plan caching, fix Linux tool linking and SPIR-V span access, and accept padded cache payload envelopes. Add pack corruption, lifetime, cross-process and alignment regressions. Co-authored-by: SaloQT <123214229+SaloQT@users.noreply.github.com>
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the update; the six earlier points are addressed (agc_shader_disk_cache passes here, and its new cases don't even build without the change). Remaining:
- The PR description still documents
ANYPS5_PREBUILT_SHADER_CACHEandANYPS5_NO_PREBUILT_SHADERS, and the reply says the child process covers the opt-out. The code no longer reads either variable and the test never sets one. Update the description to the real behaviour (shader_cache/prebuiltonly). core/shader/recompiler/ShaderCachePack.cpp:107:HashBytes(data)on every pack hit exists only to time "file read", andDecodeEntryalready checksums the payload, so each hit hashes the entry twice. Drop it (andreadNanosecondsfor packs) or measure something that is not extra work.core/shader/recompiler/ShaderDiskCache.cpp:1446and:1489: on a prebuilt missloadPrebuiltadds its duration toloadNanosecondsandload()then adds the whole lookup again, so the "total load" in the interval line double counts misses. Only add it when the prebuilt lookup is what returned.
26d473f to
f4622c4
Compare
Read-only packs of encoded cache entries are mapped once at startup from the shader cache prebuilt directory (plus ANYPS5_PREBUILT_SHADER_CACHE, opt out with ANYPS5_NO_PREBUILT_SHADERS). Disk loads try packs and loose prebuilt folders first and fall through to the writable cache and the compiler on a miss. Keys, source version, format and payload checksums are validated exactly as for writable entries, and interval logs report prebuilt hits, misses, rejections and load timings. Depends on the uncached compile entry point only through the offline tool that produces these packs; the loader itself is independent.
Build packs from EncodeEntry output and check the hit and miss paths, an entry stored under another key, a truncated entry, a damaged index, an out-of-range record, a foreign compiler source version and an unknown format. A child process covers pack discovery on a real disk load, the ANYPS5_NO_PREBUILT_SHADERS opt out and fall-through to the writable cache. ARCHITECTURE.md describes where the prebuilt lookup sits.
f4622c4 to
93ab137
Compare
|
Thanks for the careful review — all four points are addressed in the new head, squashed into the commit that owns each change:
Also addressed in the same push, from the latest re-review: the per-hit pack hash that existed only to time "file read" is gone (decode already checksums, so each hit hashed twice), the pack timings struct no longer carries a read time, prebuilt load time is only counted when the prebuilt lookup is the one that returned (no double count on misses), and the PR description now matches the real behaviour ( Build and the |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the three remaining points against the new head: the description matches the code (shader_cache/prebuilt only), the extra payload hash on pack hits is gone, and prebuilt load time/bytes no longer leak into the writable totals. agc_shader_disk_cache passes (including the new corrupt-entry and counter-separation cases, which don't build without the change); merging.
|
Thanks, nice work on the format and the Oodle decoder. Things we noticed, testing against Wolverine and GTA V:
|
|
Thanks, and thanks for testing against those titles. Point by point:
The tool PR has been rebased onto merged #5792 and reopened as #6032 with these corrections in its description (#5793 could not be reopened after the force-push). |
What
Load shader variants from read-only prebuilt storage before the writable disk cache.
ShaderDiskCache::Loadnow checks, in order:.agcscpacks and loose entry files undershader_cache/prebuiltnext to the writable cache, then the writable cache as before. With no prebuilt directory, behaviour and on-disk layout are unchanged.[shader-disk-cache]totals keep measuring only the writable cache; prebuilt activity reports under[shader-prebuilt].Tested
Windows 11:
agc_shader_disk_cacheandshader_cache_file_readpass, including pack validation and corruption cases, loose entries, counter separation, and the cross-process load path. Linux (NVIDIA, RADV, lavapipe) test evidence is in the review thread. No game performance claim: with no pack installed the path is inert.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)