perf(encode): flush the sequence bitstream where upstream flushes it - #497
Conversation
The sequence loop wrote the three FSE state diffs, drained the accumulator, then wrote the three extra-bit fields and drained it again. Upstream drains once, and asks before the first one whether the extras that follow would actually fail to fit beside the diffs: ofBits + mlBits + llBits >= 64 - 7 - (LLFSELog + MLFSELog + OffFSELog), which with our accumulator logs is 31 (zstd_compress_sequences.c:350). Under it nothing has to leave, because 7 leftover bits plus 26 of state diffs plus 30 of extras is 63. The second conditional is upstream's too: past 56 bits of extras the offset field needs its own container (:355). So the common sequence now costs one drain instead of two, and the drain is a store, a length commit and a shift. Per frame on an 8 MiB access log at level 1 (i9, two prebuilt binaries alternated in one session, perf stat -r 3, three rounds, ranges not overlapping): 124.26 M cycles becomes 118.50 M, -4.6%. Retired instructions went the other way, 260.90 -> 262.57 M (+0.6%), which is the compare and the sum that replaced the unconditional drain: fewer stores, slightly more arithmetic, and the clock prefers it. Level 9 on the same fixture moved +0.36%, inside what the control arm drifted by. The control is incompressible input, whose blocks are written raw so this loop never runs; its instruction count is identical between the two binaries and its cycles moved 0.6%. Output is byte-identical over 30 fixture-and-level rows (three shapes, ten levels from --fast=5 to 22), which is the whole claim: only the flush timing changed, never a written bit.
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 reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe sequence encoder now uses conditional bit-accumulator flushes based on total extra-bit usage. The thresholds match upstream zstd logic. The emitted bit stream remains unchanged, while typical sequences use fewer flushes. ChangesSequence encoding
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Sequence encoding now flushes at upstream-compatible bit thresholds while preserving the emitted stream. Current evidence indicates no unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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: c2d767618f
ℹ️ 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".
The 31 and 56 thresholds are arithmetic on three widths: the FSE state
diffs at most 26 bits together, the literal-length and match-length extras
at most 16 each, the offset extras at most 31. Those bounds lived only in
the comment, so an encoder that widened its output would first show up as
an accumulator assertion further down, or in a release build as a corrupted
stream with nothing pointing at the cause.
They are now asserted where the arithmetic happens, including the aggregate
the no-flush branch actually rests on: 7 leftover bits plus the diffs
actually written plus all three extra fields must fit in 64. The diff widths
are tallied per sequence for it, under cfg(debug_assertions), so release
builds carry none of this. The whole suite exercises it on every fixture and
level.
Also records the upstream arm the flush-schedule commit was missing. Same
fixture, same session, same three rounds, per frame of the 8 MiB access log:
level 1 ours before 123.99 M cycles / 260.90 M insn
ours after 118.69 M cycles / 262.57 M insn
libzstd 75.22 M cycles / 149.85 M insn
so 1.648x -> 1.578x of upstream on cycles
level 9 ours before 663.00 M / 1,603.91 M
ours after 662.97 M / 1,604.81 M
libzstd 501.13 M / 1,038.51 M
1.323x either way
Bytes on the same runs: 1,467,854 against upstream's 1,469,809 at level 1,
1,161,999 against 1,158,646 at level 9.
1 participant
Summary
The sequence loop drained the bit accumulator twice for every sequence, once after the three FSE state diffs and once after the extra-bit fields. Upstream drains once, and asks before the first one whether the extras that follow would actually fail to fit beside the diffs.
Its two conditionals are adopted verbatim (
zstd_compress_sequences.c:350and:355): flush after the diffs only whenofBits + mlBits + llBits >= 64 - 7 - (LLFSELog + MLFSELog + OffFSELog), which with our accumulator logs is 31, and flush before the offset field only past 56 bits of extras. Below the first threshold nothing has to leave, because 7 leftover bits plus 26 of state diffs plus 30 of extras is 63.Only the flush timing changes. No written bit moves, which is what the byte-identity check below is for.
The three widths those thresholds are derived from are now asserted where the arithmetic happens, together with the aggregate bound the no-flush branch rests on. The diff widths are tallied per sequence for it under
cfg(debug_assertions), so release builds carry none of it, and the whole suite exercises it on every fixture and level.Measurement
i9, three arms alternated in one ssh session,
perf stat -r 3, three rounds, per frame of an 8 MiB access log:Against upstream on cycles that is 1.648x to 1.578x at level 1, and 1.323x either way at level 9. The three readings per arm do not overlap at level 1.
Instructions go up while cycles go down: a compare and a sum replaced an unconditional store plus length commit plus shift, and the clock prefers that trade.
Control arm: incompressible input, whose blocks are written raw so this loop never executes. Its instruction count is identical between the two binaries and its cycles moved 0.6%, which is what the level-9 figure sits inside.
Output is byte-identical to before over 30 fixture-and-level rows: three input shapes (decodecorpus, random, access log) against ten levels from
--fast=5to 22, comparing frame digests rather than lengths.Also in here
The issue's first lead turned out not to hold for the level it is about, and the issue body now says so: the Fast backend (levels -5..2) never calls
encode_offset_with_history, so nothing is computed twice there. The body also records the trap in doing that lead properly, that an offBase precomputed during matching is wrong for any partition later written raw, and a negative result measured on the way here (carrying FSE states in upstream's value form removes 5.07 M instructions a frame and is 5.2% slower at level 9).Testing
cargo nextest run -p structured-zstd -F hash,std,dict-builder: 1053 passedcargo nextest run -p ffi-bench -F bench-internals,dict-builder: 64 passedcargo test --doc: 23 passedcargo clippy --all-targetson both CI feature sets in debug and release,cargo fmt --check: cleanPart of #493.