Repository navigation
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for splitting it out. Small and well tested (determinism and untouched disk-cache totals are the right checks). Two things to fix, then a reply to what you wrote on Discord, so it's all in one place.
Changes
- Reuse
unwrapEntry's validation.EntryKeyandIsCurrentEntryHeaderrepeat the header checks ofunwrapEntry(ShaderDiskCache.cpp), andEntryKeyskips one of them: the zero-padding check between key and payload. SoEntryKeycan return a key for an entry thatDecodeEntrythen rejects, which is exactly what a tool must not believe. Please move the header/size/padding validation out ofunwrapEntryinto one helper and have all three use it, so there is a single definition of "a valid current entry". - One header for both helpers.
EntryKeyis declared inShaderCachePack.hppandIsCurrentEntryHeaderinShaderDiskCache.hpp, both defined inShaderDiskCache.cpp. Neither is about packs; please declare both inShaderDiskCache.hpp.
Before merging we'll run the full Build (the PR's linux/windows jobs are skipping) and agc_shader_disk_cache / shader_cache_file_read on Linux here, since it was only tested on Windows.
About your message on Discord
I'm limited to submitting more than a few PR's due to the rules.
Not anymore: since #5792 merged you have one merged PR, so the limit is 20 open PRs, not 1 (the bot's text: "at least 1 merged can have 20"). You didn't need to close #6032. Reopen the tool as its own PR whenever you want, marked "Depends on: #6296", and send the other prerequisites in parallel too, each one small and with its own tests.
There is no shader compilation done at all... all 60,000 are done before the game opens.
That's the goal, and we want it too. The reason we keep asking for numbers is that what happens on your local copy isn't what this series does upstream:
- What upstream already removed. Since #639 the RDNA → SPIR-V translation runs at shader registration, not at the first draw. What's left at the first draw is per-draw specialization (spec constants, vertex input types, binding plan) plus the driver's pipeline compile. A pack of
CompiledVariants doesn't skip the driver compile, and nothing loadspipeline-cache.binupstream. So even with 100 % hits, "no compilation at all" doesn't follow from this series as it stands. - Exact keys. A variant's key includes runtime state: target formats, binding layout, stage configuration, vertex inputs and the host subgroup size. Variants built only from the archives have to guess that state. If they guess wrong, every lookup misses, silently. That's also why packs are per GPU class, which is likely the "it wouldn't work for all platforms" you ran into. So, for the tool PR, the number that matters is still
[shader-prebuilt]hits/misses from a real run, with a pack built bybuild --paksonly, plus load time with and without it. - Where this meets our work. Phase B of #2242 (shader objects, vertex fetch in the shader, buffer descriptors read at run time) is about removing that per-draw specialization. The less runtime state ends up in the key, the closer a statically built pack gets to always hitting. So your work and ours meet in the middle. What would help most: tell us, here or in #2242, what your fork does to make the keys predictable before the game runs (fixed targets? a capture pass? defaults for vertex formats?) and how you handle driver pipelines (pipeline cache, GPL, shader objects?). Then we can line up the upstream steps so the pack can actually reach "nothing compiled in game", instead of porting the fork's pieces one by one without the part that makes them hit.
Suggested order on your side: fix 1–2 here (we'll merge after the full Build), then reopen the tool on top with the hit-rate numbers, and the key/pipeline write-up in parallel.
CompileShaderUncached compiles one request without the process-lifetime source cache or a resource snapshot, so batch tools can compile shader archives with bounded worker lifetimes. Optional frontend and backend timings report where each compilation spent its time.
EntryKey returns the validated key stored in an encoded cache entry and IsCurrentEntryHeader checks that a file header belongs to this compiler source and format, so offline tooling can group and screen existing cache files without decoding them. Both validate against the aligned payload layout.
EntryKey and IsCurrentEntryHeader round-trip an encoded entry and reject truncated or damaged headers. CompileShaderUncached produces the same encoded entry twice without touching the disk-cache totals.
edf3c82 to
721af43
Compare
|
Both fixed, squashed into the commits that own them:
Rebased onto current main; |
|
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.
|
What
Two small recompiler additions that offline shader tooling needs, split out of #6032 as requested there:
CompileShaderUncached: compiles a request to aCompiledVariantwithout reading or writing the disk cache, with optional frontend/backend timings. Runtime behaviour is untouched; nothing in the emulator calls it yet.EntryHeaderBytes(48, static-asserted against the on-disk header),EntryKey(extract and hash-check the key stored in an encoded entry), andIsCurrentEntryHeader(cheap current-format check for a header plus file size). These let tools build and inspect cache entries in the exact on-disk format without duplicating the layout.The offline shader archive tool that consumes these will follow as its own PR once this lands and its real-run hit rate is measured.
Tested
Windows 11:
agc_shader_disk_cacheandshader_cache_file_readpass, including new cases: entry-key round trip, truncated/damaged headers rejected, uncached compile deterministic across two calls and leaving the disk-cache totals (hits, misses, writes, bytes) unchanged.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)