Repository navigation
fix(shader): memory amplification in ShaderDiskCache and quadratic scans in GraphBuilder and InstructionTranslator - #1523
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Good performance and correctness improvements across three areas. The fixes are well-justified with clear explanations of the problems (memory amplification, O(n²) scans, out-of-bounds read).
Strengths:
- Proper use of sorted vectors and binary search for efficient lookups
- Correct fix for the memory amplification issue by using proper minimum sizes
- Good handling of edge cases in the ELF parsing fixes
Minor notes:
- The PR is marked as draft with "needs rebase" label - appropriate given main has moved forward
- Consider whether these three fixes should be separate PRs for easier review/merging, though bundling related performance fixes is also reasonable
This looks ready to merge once rebased and CI passes.
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Excellent performance and correctness fixes across three areas of the shader recompiler. This PR addresses real issues that could impact both memory usage and compilation speed.
Strengths:
- ShaderDiskCache: Correctly identifies and fixes the memory amplification issue by passing the proper minimum size (114 bytes) to
reader.Listfor DescriptorBinding entries - GraphBuilder: Smart optimization replacing O(K×N) linear scans with sorted vector + binary search using
std::upper_bound- this will significantly improve compilation time for shaders with many indirect branches - InstructionTranslator: Good use of unordered_set and unordered_map to eliminate O(M×N) loop complexity in code table load detection
Technical notes:
- The approach of building the sorted direct branch targets vector once in
Build()and passing it through is clean and efficient - Proper handling of edge cases (empty programs, missing code table entries)
- Good documentation of the issues and solutions in the PR description
These optimizations will improve shader compilation performance without changing behavior. Ready to merge.
ea73efd to
274faad
Compare
|
Hi @klosixgaming-arch, the branch has been rebased onto main and conventions checks pass. Would you be able to merge this when you get a chance? Thanks! |
274faad to
8b94c13
Compare
…ans in GraphBuilder and InstructionTranslator ShaderDiskCache passed minimum=8 to reader.List for DescriptorBinding entries. The actual minimum serialised size is 114 bytes (5×uint32 + 11×uint64 list-count headers + 2×bool + 1×uint32 enum). A crafted cache file with a large count field could allocate ~14× the expected memory before any entry was decoded. Fix: pass 114 as the minimum. The same mistake was present in the decodeAllocation path; fixed there too. The stamp-file ofstream was also not checked before calling last_write_time; fixed. GraphBuilder::resolveJumpTable and resolveDwordJumpTable each scanned all N program instructions to find any direct branch targeting a range, called once per indirect-branch block. With K indirect branches the total cost was O(K*N). Fix: build a sorted vector of direct-branch targets once in Build and splitIntoBlocks, then use std::upper_bound for an O(log N) check per call. InstructionTranslator called isCodeTableLoad, which does std::find over codeTableLoadProgramCounters, at every instruction in two loops, giving O(M*N) total cost. Fix: build an unordered_set and an unordered_map from the cfg data once before the loops.
8b94c13 to
1b67f12
Compare
…written and drop unused isCodeTableLoad
eb4bb29 to
046a15d
Compare
|
Closing: these are three unrelated changes, and none shows a need. The |
What
Three fixes in the shader recompiler:
ShaderDiskCache — memory amplification from wrong List minimum
The invocation decoder passed
minimum=8toreader.ListforDescriptorBindingentries. The actual minimum serialised size is 114 bytes (5×uint32 + 11×uint64 list-count headers + 2×bool + 1×uint32 enum). A crafted cache file with a large entry count could cause the list allocator to reserve ~14× the expected memory before decoding a single entry. Fix: pass 114 as the minimum.GraphBuilder — O(K×N) scan in resolveJumpTable / resolveDwordJumpTable
Both functions scanned all N program instructions to find any direct branch whose target falls in a given range, called once per indirect-branch block. With K indirect branches the total work was O(K×N). Fix: build a sorted
std::vectorof direct-branch targets once inBuild(shared withsplitIntoBlocksandlinkBlocks, next to the swappc call analysis), then usestd::upper_boundfor an O(log N) check per call.InstructionTranslator — O(N×M) loop via isCodeTableLoad
isCodeTableLoadcalledstd::findovercfg.codeTableLoadProgramCounters(M entries) at every one of N instructions, in two loops, and the second loop searchedcfg.codeTableLoadsagain for each hit. Fix: build anstd::unordered_setand anstd::unordered_mapfrom the cfg data once before the loops, and remove the now-unusedisCodeTableLoad.Tested
shader_recompiler_agc_driverwith submodules;agc_shader_control_flow_tests,agc_shader_memory_tests,agc_shader_fusion_testsandagc_shader_disk_cache_testspassedpython3 tools/check_conventions.py --base origin/main)guestDescriptor(8), two bools (2), the shape enum (4), nine flag lists (72) andimageSamplers(8) is 114.g++ -O2, best of 5; not a measurement of a whole shader compile): theGraphBuilderrange check takes 8.2 ms against 0.11 ms for 100 000 instructions and 100 indirect jumps, and 77 ms against 0.16 ms at 1 000 jumps. TheInstructionTranslatorlookup takes 4.1 ms against 0.46 ms for 100 000 instructions and 256 table loads. With 16 table loads the set is no faster (0.28 ms against 0.34 ms), so the gain only shows on programs with many jump tables.eb4bb29b: passed for Linux, Windows, and the relinker tests on Ubuntu (g++), macOS (clang++) and Windows (g++) (run 37835318482). It is not in the PR checks because the upstream run for this head is waiting for maintainer approval (CI: 38 open PR heads are queued at action_required, so their PRs show no checks #1631).Changes since the first revision
main. Conflicts inGraphBuilder(the new swappc call analysis) andShaderDiskCache(bindingsremoved from the artifact and allocation structs upstream) are resolved; the 114-byte minimum now applies toinvocation.bindings, the only remaining caller.UpdateStampearly return is reverted: theerror_codeoverload oflast_write_timedoes not throw, and the early return would skip stale-cache and.tmpcleanup when the stamp cannot be written.isCodeTableLoadis removed, as nothing calls it any more.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repository