Repository navigation
perf(vk): compress the program cache with zstd (RX_VK_PROGRAM_CACHE_ZSTD) - #1630
DFN-master wants to merge 7 commits into
Conversation
4f741c1 to
06e50c5
Compare
hckrman101
left a comment
There was a problem hiding this comment.
The record format and the read/write paths look right to me (separate file and idx, 20-byte head, kind bit 16 is free, the old cache is untouched). One build problem I think will stop this from linking, plus a couple of nits.
Main already builds zstd's decoder into the Vulkan DLL (third_party/zstd/lib/common + decompress, the vk_zstd_*.o jobs just above). third_party/zbic/zstd.c is Atmosphere's single-file zstd with the compressor and decoder both, non-static, so vk_zstd.o defines ZSTD_decompress, ZSTD_isError, the FSE/HUF/XXH64 symbols etc. a second time. I haven't built it, but I'd expect duplicate-symbol errors at the DLL link. Also, the two zstd.h files differ, so the -I third_party/zbic on vflags can shadow the other one for any vk file that includes zstd.h.
Suggestion: drop the zbic object and add the compress sources from the third_party/zstd tree (a compress/ directory, which isn't in the tree on main yet) to the existing zstd job list. Or, if you'd rather keep zbic, drop the decoder jobs and use the zbic build for everything, but then vk_imgui's font decoding needs checking.
Nits:
third_party/zbic/PROVENANCE.mdsays none of zbic is linked into the game; that stops being true.- zbic's zstd.c is GPLv2 (Atmosphere), worth checking that is fine for the DLL's licence.
| # zstd, built as C into the same DLL | ||
| zstd_src = os.path.join(kit.ROOT, "third_party", "zbic", "zstd.c") | ||
| if os.path.isfile(zstd_src): | ||
| vk_jobs.append((cc, zstd_src, os.path.join(obj_dir, "vk_zstd.o"), |
There was a problem hiding this comment.
This duplicates the decoder objects that the vk_zstd_*.o jobs above already link (ZSTD_decompress, ZSTD_isError, FSE/HUF...). Expect duplicate symbols at link; compile only the compressor sources from third_party/zstd instead.
There was a problem hiding this comment.
Fixed in 93b11c8: the third_party/zstd decoder objects are gone and zbic's single file serves both. One thing left on the new job is the missing -fPIC on Linux, see the new comment.
|
Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks! |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the quick fix. Switching everything to the one single-file zstd fixes the duplicate symbols I flagged, and it's the simpler of the two options. I also checked the things it touches: both zstd.h files are 1.5.7, so vk_imgui.cpp picking up zbic's header first does no harm, and the font reader's calls (ZSTD_getFrameContentSize, the DCtx/DDict ones) are all in zbic's zstd.c. ZSTD_ZBIC_SUPPORT defaults to 0 there, so the GPLv2 Atmosphere ZBIC code isn't compiled into the DLL. Only stock zstd goes in, under its BSD option, so you can drop my licence nit. tests/test_shader_stamp_cache.py still passes on this shape too: its fixture has no zbic/zstd.c, so the new job just doesn't show up.
Still open:
- Linux:
vk_zstd.ois built without-fPIC(inline). The decoder job you removed had it, and so do vflags and the fonts job. This object goes into the Linux .so, and zstd's public functions are explicitlyvisibility("default"), so I'd expect the link to fail with "recompile with -fPIC". I haven't built it. The removed job also passed-DNDEBUGand-fvisibility=hidden, and I'd keep those for consistency (DEBUGLEVEL 0 already turns off zstd's asserts, so leaving out NDEBUG changes nothing). - Conflicts with main (mergeable: dirty). Main changed the cache code in vk_shader.cpp a fair bit today:
disk_taken,program_cache_keys, the fallbacks inprogram_cache_load_some,descriptor_counts_fit. It also reworked build.py around the vk jobs. Please merge main in. CI hasn't run on this PR, probably because of the conflict, so after the merge it would be good to see it go green on both Windows and Linux. From a quick read, main's new readers all go throughDiskRecord/program_cache_read/program_cache_each, which you already handle, and bit 16 of the kind word is still free there (main uses bits 0-3).
Nits:
if os.path.isfile(zstd_src): the fonts now depend on this object, so if the file is missing the job gets skipped quietly and the link fails with undefined ZSTD_* symbols. Making it unconditional would give a clearer error.- With the third_party/zstd objects gone, the
-I third_party/zstd/libin imgui_flags and those headers in imgui_deps no longer do anything (zbic's header wins). The zstd.c job also hasb""deps, so a change to zbic's headers won't rebuild it. - Still from last time: third_party/zbic/PROVENANCE.md says none of zbic is linked into the game. That's no longer true. A line saying the DLL builds zstd.c with ZBIC support off would cover it.
| zstd_src = os.path.join(kit.ROOT, "third_party", "zbic", "zstd.c") | ||
| if os.path.isfile(zstd_src): | ||
| vk_jobs.append((cc, zstd_src, os.path.join(obj_dir, "vk_zstd.o"), | ||
| target() + CPU + ["-O2", "-g0", "-Wno-everything", "-I" + os.path.join(kit.ROOT, "third_party", "zbic")], |
There was a problem hiding this comment.
On Linux this object goes into the .so but gets no -fPIC. The removed zflags had (["-fPIC"] if LINUX else []), and vflags and the fonts job just above have it too. I'd expect a relocation error at the link (zstd's API symbols are default visibility). Suggest target() + CPU + (["-fPIC"] if LINUX else []) + ["-O2", "-g0", "-DNDEBUG", "-Wno-everything", "-fvisibility=hidden", ...].
There was a problem hiding this comment.
Still missing after the rebase: the job at line 994 still has no (["-fPIC"] if LINUX else []) (nor -DNDEBUG/-fvisibility=hidden). The appimage job should show it.
93b11c8 to
70417e3
Compare
afcb00e to
7c6f212
Compare
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the rebase. It merges cleanly now, and the Windows llvm-mingw link check is green, so the single-zstd build links on Windows. The rest of CI is still running.
Still open from last time:
-fPICon Linux forvk_zstd.o(build.py:994, replied in the thread). The appimage job should show whether it links.- Nits, all still there: the
os.path.isfile(zstd_src)guard (if the file is missing, the fonts fail later with undefined ZSTD_* symbols);-I third_party/zstd/libin imgui_flags and those headers in imgui_deps don't do anything now; the zstd.c job'sb""deps; and third_party/zbic/PROVENANCE.md still says none of zbic is linked in.
New:
seed_computereads the cache records raw. The rebase brought in a third reader of the records, and it doesn't unpack them. The "variants kept already" loop inseed_compute(vk_shader.cpp around line 2681) readsdisk_records[key]straight fromdisk_inand parsesrd.get<uint32_t>(); rd.list(q.answers)from it. With the zstd cache, a compressed compute record gives the low word of the unpacked size, then frame bytes. Reader's bounds checks keep this from crashing, but the kept variants aren't recognised. So I'd expect every prepare to translate the compute variants again and append them again, and the .zst.bin grows each time. The unpack step now exists twice (program_cache_read,program_cache_each). Could you make it one helper (record bytes in, payload out) and call it here as well? Then the next reader can't miss it. A test that writes and reads back one compressed record (with real zstd, or a stub that copies) would also catch this. The cleanup test only runs withdisk_zstd = false.
How this fits with #2309: the two PRs choose opposite zstd libraries for the same DLL. #2309 keeps the third_party/zstd decoder and adds its compress/ sources. This PR removes those and builds zbic's zstd.c instead. With both merged as they are, the DLL gets the same duplicate symbols again, so whichever lands second has to switch to the other's library. If #2309 goes first, the simplest route here is to drop the zbic job and the -I third_party/zbic and use its objects. That also fixes the -fPIC, visibility and PROVENANCE points above. The formats don't clash: #2309 packs the SPIR-V only in memory and leaves the disk record unchanged, and this PR compresses the record on disk. Its stamp bump only means the .zst.bin is rebuilt once too. @NotRexed, which order you take them in is your call.
73240c8 to
8222ead
Compare
Joey-Magnus
left a comment
There was a problem hiding this comment.
The seed_compute point from the last review is still open, and I confirmed it on the merge with main. seed_compute (vk_shader.cpp:2110) reads each disk_records entry straight from disk_in and parses rd.get<uint32_t>(); rd.list(q.answers). A record written by program_cache_write with kCompressedKind starts with the 8-byte unpacked size and then the zstd frame. So with RX_VK_PROGRAM_CACHE_ZSTD=1, the compute variants already kept are never recognised: each prepare translates them again and appends them, and hookshot_programs.zst.bin grows every time. Please move the unpack into one helper (record bytes in, payload out), use it in program_cache_read, program_cache_each and here, and add a test that writes and reads back one compressed record.
The appimage job links, so the missing -fPIC on vk_zstd.o (build.py:993) isn't fatal, but I'd still match the old job's flags. The os.path.isfile(zstd_src) guard and the PROVENANCE line are still open.
#2309 picks the other zstd library (third_party/zstd plus compress/). Whichever lands second has to switch, and that ordering is the maintainer's call.
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for following main's move of the cache code into vk_progcache.cpp. The 20-byte head now carries through the index check, the scan and the read path, the tests follow it, and CI is all green (appimage included). It also merges cleanly with main right now.
Still open, same as Joey-Magnus's review on this head:
seed_computestill reads the records raw (vk_shader.cpp:2110). It readsdisk_records[key]straight fromdisk_inand parsesrd.get<uint32_t>(); rd.list(q.answers). A compressed record starts with the 8-byte unpacked size and then the frame, so withRX_VK_PROGRAM_CACHE_ZSTD=1the kept compute variants aren't recognised, every prepare translates them again, and the .zst.bin grows each time. The unpack is still copied inprogram_cache_readandprogram_cache_each. One helper (record bytes in, payload out) used by all three, plus a test that writes and reads back one compressed record, is what I'd want before this goes in. The cleanup test only coversdisk_zstd = false.
Smaller, still there:
vk_zstd.ohas no-fPIC/-DNDEBUG/-fvisibility=hidden(build.py:994). The appimage job links, so it isn't fatal, but I'd still match the old job's flags.- The
os.path.isfile(zstd_src)guard: if the file is missing, the fonts fail later with undefined ZSTD_* symbols. third_party/zbic/PROVENANCE.mdstill says none of zbic is linked into the game.- Nits:
-I third_party/zstd/libin imgui_flags and those headers in imgui_deps no longer do anything, and the zstd.c job hasb""deps.
#2309 (still open) picks the other zstd library, so whichever lands second has to switch; that ordering stays with @NotRexed.
8222ead to
7f33ccf
Compare
|
Thanks — the open points are addressed in the new head (
Local checks: On #2309: whichever lands second switches libraries — agreed. If #2309 goes first, I'll drop the zbic job and the |
Test report — zstd program cacheMachine
Build and settings
Checks
Cache and prepare
The 53bc translator update (not zstd) prepared 50k+ pipelines in 1,192 s in a separate test; that is a translator change, not this cache. In game (4a53 + zstd + BC7, 4 GB texture budget, pct 90)
Caveat: one Windows/NVIDIA machine at 1080p; no AMD/Intel run. The program cache includes programs the play tests met, so its size grows with play. On #2309: if it lands first I will drop the zbic job and use its compressor objects; the record format is independent of the library. |
|
Cache numbers on the newest main, same machine (16 GB/s-class RAM, 10 GB NVIDIA, 1080p), with this PR plus the readback and BC7 changes in one build (
One note for anyone testing with the force-kill harness: an earlier |
…STD) The translated SPIR-V program cache is 6.9 GB. With RX_VK_PROGRAM_CACHE_ZSTD=1 each record is compressed with zstd level 3 into hookshot_programs.zst.bin: the reference cache went from 6.86 GB to 1.83 GB (3.75x). Records are unpacked when read; a 20-byte record head keeps the uncompressed cache format untouched. Off unless set. The build uses third_party/zbic's single-file zstd (encoder and decoder) for the program cache and the game's fonts: the decompression-only third_party/zstd is not built as well, so the two cannot define the same symbols.
The record head is 20 bytes with the zstd cache (key, stored size, kind) and 16 without it; the reader is bounded to the record's payload, compressed or not. The cleanup fixture gets the zstd switch, the head constants and the compressor's stubs.
The scan reads a record's 16-byte head (key, stored size) and the 4-byte kind after it; the zstd record's kind is in its 20-byte head. The payload offset and the sizes use head_size, but the scan's loop still needs 20 bytes at the end of the file.
The prediction's reader reads the unpacked payload through a Reader named r, as test_program_record_head.py checks; the decompression's plain buffer is what it points at.
seed_compute read a record's stored bytes as the program's own: a compressed record begins with the 8-byte unpacked size and a zstd frame, so the compute variants already kept were never recognised and every prepare translated and appended them again. program_cache_payload now takes the stored bytes and a kind and returns the payload, and program_cache_read, program_cache_each and seed_compute all use it. tests/program_cache_cleanup_test.cpp writes and reads back one compressed record through the production loader, with a copy stub for zstd.
vk_zstd.o is built for the Linux .so too (-fPIC), with the decoder job's -DNDEBUG and -fvisibility=hidden; its zbic headers are its dependencies, so a header change rebuilds it. The job is unconditional (a missing source fails at the compile that names it, not later at the link), imgui's include and deps name the zbic headers now that they are the ones read, and PROVENANCE.md says the DLL builds zstd.c with ZSTD_ZBIC_SUPPORT off (stock zstd, BSD; not the GPLv2 ZBIC parts).
The zstd job is unconditional now, so the fixture that runs build.py's Vulkan branch needs third_party/zbic's source and headers; its plain C objects are not shader-stamp objects, as third_party/zstd's already were not.
d9ec233 to
d61c65c
Compare
|
Rebased onto main ( On the newest main, with BC7 (
No cache errors, no device loss. Config: 4 GB texture budget, One note for the 10 GB card: main now preallocates 15% of VRAM as 128 MB image-pool blocks ( |
Joey-Magnus
left a comment
There was a problem hiding this comment.
Everything from my last review is in. seed_compute (vk_shader.cpp:2211), program_cache_read and program_cache_each all go through program_cache_payload now, so compressed compute records are recognised instead of being translated and appended again. The cleanup test writes a compressed record and reads it back. The zstd job has -fPIC, -DNDEBUG and -fvisibility=hidden, and PROVENANCE.md says what is linked. I merged d61c65c into current main (clean) and ran the program_cache, program_record and shader_stamp checks: 7 pass. CI is green on this head.
I also checked that nothing changes without the switch. The 16-byte head, the HSP1 magic and hookshot_programs.idx are as before. The zstd cache has its own .zst.bin and .zst.idx, so turning it on or off never reads the other file. zbic's zstd.c defaults ZSTD_ZBIC_SUPPORT to 0, so the Atmosphere code stays out.
One thing for maintainers: this replaces the third_party/zstd decoder objects that the fonts used with zbic's zstd.c, for everyone. #2309 uses third_party/zstd, so whichever lands second has to switch.
The translated program cache is 6.86 GB on the reference OoT install (46,587 programs). With
RX_VK_PROGRAM_CACHE_ZSTD=1each record is compressed with zstd level 3 intohookshot_programs.zst.bin: the same cache is 1.83 GB (3.75x smaller). Records are unpacked when read; the precompile opens 46,587 keys and builds 13,109 programs in 1.8-2.7 s, with no "not whole"/"did not unpack" records in a 300 s run.The compressed cache is a separate file with a 20-byte record head (key, stored size, kind); an existing
hookshot_programs.binis untouched, and the uncompressed format is unchanged. Off unless set. The zstd implementation is the repository'sthird_party/zbic/zstd.c(already used for NUP decompression), built into the Vulkan DLL as C.Tested on an RTX 3080 / i7-11700 at 1080p: no fps regression (59-60 fps in the same scenes as the uncompressed cache), and the cache is 5.0 GB smaller on disk.