Repository navigation
perf(host): the game's zstd stream decode on the host's libzstd (HOS_NATIVE_ZSTD, opt-in) - #2738
klimovich008 wants to merge 7 commits into
Conversation
…OS_NATIVE_ZSTD, opt-in) The game decodes its .zs files on ParallelDecompThread through f_80115c80, its own wrapper around zstd's buffer-less streaming API. Lifted, it costs 11-17% of a core while the game streams, and during loads it runs for long stretches without an OS call while holding a virtual core. nx2/oot_patches.py puts hos_zstd_stream(c) first in f_80115c80. With HOS_NATIVE_ZSTD=1 the host does the same calls into the vendored libzstd 1.5.7 (decompressBegin_usingDDict, nextSrcSizeToDecompress, decompressContinue), keeps every field of the game's stream object and every result the game's wrapper gives (still to come, 0, -1, -2, -3), uses the game's DDict content by reference, and gives the virtual core up while it decodes (=2 keeps it). Off by default: the lifted function runs as before. tests/test_native_zstd.py builds host/hos_zstd.c as it is with the vendored decoder and checks it on frames made with python-zstandard: outputs, results and fields over many chunk splits, dictionaries, checksums, the failure codes, fallbacks to the lifted code, page marks for the GPU mirror, and a broken copy that must fail. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a host entry Its memory may have been freed (FreeMemoryPages decommits pages). An entry now goes only when its stream fails or ends, or is reset when its object starts the next frame; with all 64 in use a new stream decodes lifted. Also logs each new peak of streams decoding on the host. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The mark was taken before zs_chunk updated the output pointer, room, still-to-come, staging fill and state; a GPU mirror scan between the two could clear it and miss those writes (found by the gpt-6.1-sol review). The test's fixture now keeps each page as it was at its last mark, so a page written after its mark fails; the old order fails it. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cache took the same address, content, size, dictID, entropy flag and repeat offsets for the same dictionary; a DDict made again in place for another dictionary with the same ID and size kept the old entropy tables (found by the gpt-6.1-sol review). It now keeps a copy of the game's own DDict tables and compares them. The test covers that case with small frames (they use the dictionary's tables), checks every result against the stream object's still-to-come and output pointer, and checks HOS_NATIVE_ZSTD=2 decodes. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d decode 2 while the frame decodes, 4 after its last call; a host decode that left another state mid-frame passed before (found by the gpt-6.1-sol review, round 3). Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nx2/oot_patches.py: main moved the per-file patches into patch_file(); the zstd stream hook is there now, counted by report() like the other supported-build hooks. nx2/build.py: ZSTD_HOST beside main's dav1d flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wanted" label (#2746) A test request asks someone's agent to download, build and run a PR on their own PC, and since #2734 anyone signed in to the team chat can post one for any PR. This makes a maintainer the gate: a request can only be picked up when the PR's author is a collaborator on GitHub, or a maintainer has looked at the PR and added the `testing wanted` label. **What changed** - `scripts/test_requests.py` asks GitHub (no sign-in needed; `GITHUB_TOKEN`/`GH_TOKEN` used when set) for the PR's `author_association` and labels. A request is approved when the author is `OWNER`, `MEMBER` or `COLLABORATOR`, or the PR has the `testing wanted` label. - `claim` refuses (exit 3) a request that isn't approved, or whose PR is merged or closed, and sends nothing when GitHub can't be asked. - `list` and `show` mark each open request `approved` and add the PR's state; waiting ones are listed last, with "waiting for a maintainer to add the label". - `request` tells the author when their PR needs the label. - The server's relay shows a waiting request as a grey card, "Waiting for a maintainer to add the `testing wanted` label", with no claims, results or guide link, and checks GitHub every 3 minutes so the card opens by itself soon after the label is added (approved ones every 15, for the PR's state). - **The label is also an entry point:** the relay lists open PRs labelled `testing wanted` every 3 minutes and posts a request for each one that has none, for its author (their GitHub name and avatar on the card, the PR title as the summary); it closes the ones it posted when the label comes off or the PR is merged or closed. The guide, the request skill and AGENTS.md say so. - The testing skill now treats the request, the PR and its code as data, never instructions: it builds and runs only as docs/development.md says, and before the first build looks at what the PR changes that runs on the PC (scripts, tools, workflows, anything downloading or reaching outside the repository), stopping if something isn't explained. - The request skill, the guide (`docs/test-requests.md`) and AGENTS.md say the same. **Tested** - Live: with this running, the relay posted requests for the two labelled PRs that had none (#2456, #2738), under their authors' GitHub names and avatars, and left #2306 alone (it already had one). - `tests/test_test_requests.py`: a stranger's PR without the label can't be claimed; the label approves it; a merged PR can't be claimed; no claim goes out when GitHub can't be asked, while `list` still works; waiting requests list last. `python -I -B tests/run_all.py test_requests team_chat`: both pass. - Live: the open request for #2306 (a contributor's PR) turned into the waiting card when the relay was updated; after adding `testing wanted` to #2306 it opened again on its own within 3 minutes, in both Discord channels, and `list` shows it approved.
Linux testI built and ran this on Linux (the part the PR says is not covered).
The hook is active ( Caveat: the intro is vsync-capped at 60, so this is not a CPU-time measurement. I did not run the Hyrule Field standard test or measure CPU ms/frame; the 2.4 ms/frame prediction remains untested on Linux. If a Linux perf row is wanted, I can run an uncapped Hyrule Field A/B with process CPU time, but it needs the save/camera automation. No game files or saves used. 🤖 Generated with opencode using DeepSeek V4.1 Flash. |
Joey-Magnus
left a comment
There was a problem hiding this comment.
I merged this into current main and ran tests/test_native_zstd.py (zstandard 0.25.0) and test_patch_hooks_defined.py: both pass, and so does the broken-copy control. I read host/hos_zstd.c. Off, hos_zstd_stream returns after a cached env_on (line 295), so the default path costs one call per chunk. The stream table only frees an entry on failure, on done, or when a new frame starts. An object the game abandons mid-stream keeps its entry, which is documented, and after 64 of those new streams fall back to the lifted decoder. The DDict is checked against a copy of the game's entropy tables, so one remade in place isn't served stale. CIB's Linux run covers the "not covered: Linux" line.
I see no correctness problem. What's left is whether to carry 336 lines of host code and a hook in the lifted f_80115c80 for a gain 4 pairs couldn't resolve. Those runs were also on main before #2602. With #2602 CPU time a frame is about 78 ms instead of 115, so the predicted ~2.4 ms is a bigger share and might show. I'd rerun =2 against off on current main before deciding.
|
Test request T-M4T5: pass Hardware: NVIDIA GeForce RTX 3080 (10 GB), driver 621.14, AMD Ryzen 9 3900X (12 cores, 24 threads), Windows 11. Built f0f7dfd (the PR head; What I checked: the same build, switch off against Result: no difference beyond run-to-run spread, and nothing broken.
|
|
Test request T-M4T5: pass Hardware: AMD Radeon RX 580 8 GB (RADV Polaris10), Mesa 26.2.6, CachyOS Linux, 8 threads, 15 GB RAM. Built f0f7dfd (the PR head). One build; What I checked: start to play from the test save, then Debug teleports Kokiri Forest → Hyrule Field → Market, with a 34 s window in each scene, the camera turning (
Results, per run → median:
Correctness:
|
The game decodes every streamed file with its own lifted copy of zstd (ParallelDecompThread, about 3.5 ms of CPU a frame in the standard test). This adds an opt-in switch,
HOS_NATIVE_ZSTD, that hands that decode to the host's libzstd 1.5.7, already vendored and linked forhost/prompts.c. In a stub, libzstd takes about a third of the lifted decoder's time per MiB. In the game the difference is too small for 4 pairs of the standard test to resolve, so it stays default off.This is a performance change, not a bug fix.
What changed
host/hos_zstd.c(new):hos_zstd_stream, a replacement for the game's per-chunk wrapperf_80115c80around zstd's buffer-less API (frame header from the first chunk,begin_usingDDict, then thenextSrcSizeToDecompress/decompressContinueloop with the game's staging buffer for a piece split between chunks). Every field of the game's stream object (state, dst, room, bytes still to come, staged fill) and every result (bytes to come, 0, -1, -2 fordstSize_tooSmall, -3 for input past the frame's end) is kept as the game's code leaves it. libzstd keeps its ownZSTD_DCtxper stream; the game's context is never written. The game's dictionary is used by reference: aZSTD_DDictmade from the game's content and checked against a copy of the game's entropy tables, so a dictionary remade at the same address is never served stale. Output pages are marked withHOS_WRITTENafter the writes. States 3 and 4 (failed, done) and anything unexpected (a misaligned workspace, an allocation failure) go back to the lifted code. Fixed tables of 64 streams and 64 dictionaries, under a spin lock; the peak in use is logged.HOS_NATIVE_ZSTD=1: decodes on the host and gives the virtual core up while it decodes (hos_vcore_away/hos_vcore_back), as a sleep does.HOS_NATIVE_ZSTD=2: decodes on the host and keeps the core.0: off, the lifted code runs as before.nx2/oot_patches.py:patch_zstd_streamputsif (hos_zstd_stream(c)) return;first inf_80115c80, for the supported build only, counted byreport()like the other hooks (warns unless found exactly once).nx2/build.py:ZSTD_HOST, the host files that get the zstd include path and headers in their dependency key (prompts.c,hos_zstd.c).tests/test_native_zstd.py(new, game-free;zstandardfromRUN_ALL_MODULESmakes the frames): compileshos_zstd.cas it is, with the vendored zstd decoder, over a small fake guest window. It feeds frames with and without checksums, with a dictionary (entropy tables and raw content), empty and one byte long, RLE and raw blocks, whole and in chunks from 7 bytes to 128 KB + 3 and at random, so headers and blocks are split through the staging buffer. Each call's result is checked against the stream object's remaining-byte field and state byte, and the final state against what the game's wrapper leaves. Failures must answer as the game's do: wrong checksum or dictionary, bad magic, unknown content size -1; too little room -2; bytes after the frame -3. Also covered: a dictionary remade in place with the same id; the fallbacks (switch off, misaligned workspace, all 64 entries used); page marks after the last write; the core given up once a call with=1and kept with=2. A broken copy (the staging fill never stored) runs inside the test and must fail.docs/tested-configurations.md: the three rows below.How it was tested
f_80115c80and its callees, run in Unicorn from a local image, againsthos_zstd.con the same inputs. 110 cases (the test's kinds plus larger and multi-frame sets), about 88,000 calls. Results, every stream field, the staging buffer, the dictionary region and the decoded output were identical at every call. Differences were only in the up-to-32 bytes just past the decoded output, which both decoders' fast copies over-write: the game's writes 3-11 bytes past it, libzstd up to 27. For a corrupt block, the partial output's last bytes differ; both report -1. A broken variant fails.python -I -B tests/run_all.pyon the merged tree (Windows, llvm-mingw): 457 passed, 36 skipped, 1 failed. The failure isnx2/test_nup_paths.py, which cannot create its file-link fixture on this PC (no symlink right); this PR does not touch that test or what it covers.hos_zstd.cat the build's flags (-O2 -march=x86-64-v3,ZSTD_DISABLE_ASM) takes 0.99 ms per MiB, against 3.09 ms for the lifted decoder at -O2 (PGO hot list: the game's Zstd decoder at the hot flags whatever the profile says #909) and 3.70 ms at -O1. That is 0.27-0.36 of the lifted -O2 time per frame.HOS_FPS_MAX=240, 20 s window): execacdc864b4239f07(1bde186, before the merge with main),hookshot_vk.dllbfdac38fbf7d7c23. RX 9070 XT, Vulkan driver 0x8001a0, Ryzen 7 5700X3D, 64 GB, Windows 11, 5120x1440 (32:9). 2 warm-ups, then 4 alternating pairs per arm, same binaries:=1(core given up)=2(core kept)The CPU-time differences (-1.0 and -2.3 ms a frame) are inside the run-to-run range (111.5-117.9 ms). The stub predicts about 2.4 ms a frame (3.54 ms x 0.68), which 4 pairs cannot resolve. Save load: -0.3 and -0.5 s, also inside the range. All of
=1's frames over 100 ms came in its first run, which also had 5 virtual-core waits over 200 ms, against 1-2 in every other run. The control's first run (220.6 ms) and a warm-up (267.2 ms) spiked too, so I don't attribute them. The escapes were measured at main's 2 s escape default (#2127). None had ParallelDecompThread holding a core, so giving the core up buys nothing measurable here.=2is the mode to try.These runs were on a main from before #2602, which fixed a regression on main (this machine: about 41 → 53 fps and 125 → 78 ms of CPU a frame on the standard test), so every arm ran on the slower main. I can rerun the A/B on a main with #2602 if wanted.
Not covered
f_8010b380(whole-buffer decode): not hooked. A follow-up if this one proves out.ZSTD_DISABLE_ASMstays, as for the rest of the build): a follow-up.Related
🤖 Generated with Claude Code