Repository navigation
feat(encoding): long-distance matching for every strategy - #589
Conversation
Long-distance matching only ran under the optimal parsers: the producer lived on the binary-tree backend, so below btopt the setting was silently ignored by every API, and the CLI refused `--long` under level 16. - The match driver owns the long-distance matcher for the frame, as upstream keeps `ldmState` in the context. The optimal parser still takes its sequences as candidates; every other strategy takes them outright (upstream `ZSTD_ldm_blockCompress`): the block is committed, handed back and committed again in pieces, the backend matches each gap between matches as its own block, a match body is indexed only at its tail, and the repeat offsets move as upstream moves them. - What a match may reach is fixed against the block end, one window back or the whole dictionary while the window has not passed it (`ZSTD_window_enforceMaxDist` on the chunk end). - A dictionary loaded under long-distance matching is indexed into its table (`ZSTD_ldm_fillHashTable` from `ZSTD_loadDictionaryContent`); the primed snapshot keeps that table, keyed on its parameters, so a frame restored from it writes what a freshly primed one writes. - A block is no longer written off by the raw-skip sample while the matcher is on: its matches are external to the block. - CLI: `--long` runs at every level, and `--patch-from` turns long mode on by upstream's rule (`FIO_adjustParamsForPatchFromMode`: `fileWindowLog > ZSTD_cycleLog(chainLog, strategy)`). A 61 MiB patch against a 60 MiB random reference at the default level drops from 62.6 MB to 76 KB (upstream 72 KB) and round-trips through both decoders.
A dictionary was indexed into the match-finder tables from its first byte, however long it was. Tables of that size keep at most a window of entries anyway, so the indexing of a large reference was pure cost: priming a 60 MiB `--patch-from` reference at the default level indexed all of it densely, a third of the run. Upstream indexes only the end of a dictionary the tables can hold, `MIN(MAX(hashLog + 3, chainLog + 1), 31)` bytes (`ZSTD_loadDictionaryContent`, zstd_compress.c:4987), while the whole of it stays in the window. The driver now sets that floor for the prime and every backend's fill starts from it: the copied and attached tables of the fast, double-fast, row, chain and tree finders, and the dictionary match states. A dictionary within the reach writes the same frames as before (levels 1-19 over three inputs and three dictionaries up to 125 KB, md5 equal). A 930 KB dictionary moves a few frames by a handful of bytes, smaller in all but one (+2 bytes on 449 KB). The 61 MiB patch against a 60 MiB reference is byte-identical and takes 750 ms of task-clock instead of 1170 on the x86 bench host.
The block splitter reads byte statistics only. Over input that a long match covers it still cuts, since a sparse sample of random bytes looks different from the next one, and every cut costs a block whose one sequence carries the same match on: a 61 MiB patch came out as 7605 blocks of a single sequence each. - The long-distance matches are now searched ahead of the blocks and kept between them, as upstream keeps a raw sequence store across blocks: each block takes the matches covering it, the one crossing its end cut there and carried into the next, a piece shorter than the level's minMatch left as literals (`maybeSplitSequence`, `ZSTD_ldm_skipSequences`). - Before a cut, both frame loops ask the matcher through the new `Matcher::settle_block_cut`; the driver searches the window the splitter looked at and moves a cut that falls inside a long match to the match's end. The 61 MiB patch against a 60 MiB reference drops from 76127 bytes to 5386 (upstream: 71906 by default, 13163 single-threaded) and round-trips through both decoders. Without long-distance matching nothing changes (corpus totals at levels 1-19 equal). With `--long` the corpus comes out smaller than upstream single-threaded `--long` at every level but 1, where it is 0.02% larger.
Upstream sets the decoder's window limit to the -M value itself (`ZSTD_DCtx_setMaxWindowSize(dctx, memLimit)` in fileio.c). We counted the decoder's other buffers and the dictionary against the limit too, so a frame with a 2^29 window was refused under `--memory=512MB`, and a 2^28 one under `--memory=256MB`: both are decode cases of the upstream `--long` test script. - Under -M the window ceiling is the limit, clamped to the decoder's range; the dictionary no longer enters it. - The `--memory` a refused frame names is the window rounded up, which now decodes it; the benchmark keeps its own accounting.
Replace the fork paragraph with an Origins section: the project is developed on its own, and the ruzstd code it started from stays credited to its author under the MIT License, whose notice is kept in NOTICE.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The PR changes the independent decoder --memory contract. decode_window_ceiling now treats the memory limit as the full decode window and removes dictionary and auxiliary-buffer accounting in `zst… |
Remove the --memory contract changes and their tests, and remove the unrelated README Origins change, or link those changes to separate issues. Keep the LDM implementation, patch activation, dictionary indexing, and required validation. |
| Docstring Coverage | |
Docstring coverage is 78.72% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 141 functions across 23 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the main change: enabling long-distance matching for every compression strategy. Linked Issues check Issue #569requires LDM for every strategy and levels 1–15, dictionary indexing, automatic LDM for large--patch-frominputs, and the listed round-trip and upstream test coverage. The current change…Full details: Linked Issues check
-
Explanation
Issue
#569requires LDM for every strategy and levels 1–15, dictionary indexing, automatic LDM for large--patch-frominputs, and the listed round-trip and upstream test coverage. The current changes move LDM state toMatchGeneratorDriver, process non-optimal strategies, retain matches across block cuts, fill LDM from dictionaries, and applypatch_needs_long_distanceusing effective matcher parameters. The PR summary reports the required patch-size result, low-level repeat result, decoder round-trips, and all upstream v1.5.7--longscenarios.
-
Full details: Out of Scope Changes check
-
Explanation
The PR changes the independent decoder
--memorycontract.decode_window_ceilingnow treats the memory limit as the full decode window and removes dictionary and auxiliary-buffer accounting inzstd/src/bin/structured-zstd/main.rs. Issue#569does not request this decoder behavior. The PR also replaces the README project-relationship paragraph with an unrelated Origins section.Resolution
Remove the
--memorycontract changes and their tests, and remove the unrelated README Origins change, or link those changes to separate issues. Keep the LDM implementation, patch activation, dictionary indexing, and required validation.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 378062819b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`--patch-from` turned long mode on from the bare level's chain and strategy, while a `--zstd=` override changes the matcher that actually runs. A short `clog=` at a high level left the reference out of reach with long mode off, and the patch came out as literals. The cycle check now reads the overridden chain and strategy. The reference command reads the bare level here; following the effective matcher only differs where its patch would miss the reference.
- A dictionary a reused context keeps resident between small or unsized frames had its long-distance table cleared by the reset and never filled again, so every frame after the first lost its long matches into the dictionary. The resident path now indexes the dictionary into the cleared table, as priming it would; a test runs three frames on one context against a fresh encoder under every strategy. - The primed snapshot no longer keeps a copy of the long-distance table: the restore path indexes the restored dictionary instead, as upstream fills the table each frame it loads a dictionary (`ZSTD_ldm_fillHashTable`). The context held two copies of its largest table, which the workspace estimates did not count, and copying it cost more than the fill. With no long-distance state in the snapshot its key drops the matcher's parameters, and a restore across a long-distance switch writes what a fresh compressor writes. - Gaps between long matches, borrowed scans and plain blocks report through one sink type, so each backend's scan is compiled once per caller instead of twice. The library's LLVM IR had grown by 64%; it is back within 0.3% of main, and the test build on CI's macOS runner had tripled.
The sink every owned match runs through forwarded each report as the matched value whole, which the compiler copies as one 32-byte block out of the separate stores that wrote its fields. Rebuilding the report from its fields brings the encoder's instruction count back to that of the code before the shared sink (387.13M at level 9 on z000033, from 387.51M), with output identical at every level.
The hash-chain lazy search runs only under the parse and search override, so its long-distance path (handing the block back, skipping the match body, carrying the offset into the repeat history) had no test. The new one checks that a shuffled repeat beyond the chain's reach stays literals without long-distance matching and is matched with it, and that the stream rebuilds both ways.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
structured-zstd/zstd/src/encoding/levels/config.rs
Lines 1060 to 1064 in b489c78
Budget the retained LDM sequence vectors
When LDM produces many short matches—especially with a custom ldmMinMatch=4 and a low hash-rate log—this estimate adds only the hash table, while the driver also retains capacities for ldm_found, ldm_block, and LdmSequenceStore::matches (and the optimal backend can retain another candidate vector). A 128 KiB block can therefore leave several MiB of unbudgeted sequence storage, so the CLI's benchmark -M check can approve a limit that the compression context exceeds; include worst-case LDM sequence-store capacity in both workspace estimators.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A benchmark with `--patch-from` compresses every level from `-b` to `-e`, but long mode was decided on the first level alone. Whether a level needs it is not constant across a range: at an equal chain log btlazy2 (level 13) cannot cycle the input where lazy2 (level 12) can, so `-b12 -e13` measured level 13 without long mode, as literals. Long mode now turns on when any benchmarked level needs it; the regression test runs that range.
Both workspace estimates counted the long-distance table alone, while the driver also keeps the sequences it searched, its store, the block's sequences and the optimal parser's copy of them. At a short minimum match those hold several MiB, so a benchmark `-M` limit could pass a run that then exceeds it. The estimates now add five vectors of a block's worth of sequences at the minimum match, upstream's per-store bound (`ZSTD_ldm_getMaxNbSeq`); a test checks that minMatch 4 budgets at least one such vector more than minMatch 64.
1 participant
Summary
--longat any level, turns long mode on for--patch-fromby the upstream rule, and reads--memoryas the decode window, as upstream does.Changes
LdmSequenceStorekeeps long matches between blocks;Matcher::settle_block_cutmoves a splitter cut that falls inside one to its end.--long;--patch-fromenables long mode when the file's window outgrows the match finder that runs (the level's, under any--zstd=override);-M/--memorysets the decode window ceiling.Results
--long=29: 146.8 MB at levels 1-15, smaller than upstream single-threaded at every level, task-clock within about 10% of it.Testing
Unit and CLI tests, doc tests, clippy over the CI feature sets and formatting pass on macOS; every
--longscenario of upstreamtests/playTests.sh(v1.5.7) passes against the CLI.Closes #569
Summary by CodeRabbit
-Mdecoding limit now applies directly as a ceiling on the decoding window, including when applying patches.