Repository navigation
Commit 6730114
authored
perf(decode): fused sequence executor + SIMD/FSE hot-path cleanup + DoS-safe rollback (#194)
* fix(bench): lazily materialize compressed bytes in decompress benches
Previously bench_decompress ran compress_slice_to_vec + ffi_encode_to_vec
upfront for every scenario × level × source triple, even when criterion's
filter selected a single benchmark. cargo bench --profile-time <filter>
then spent its entire window in encode setup instead of the decode hot
path it was supposed to profile.
Wrap the compressed-byte production in a OnceCell inside
bench_decompress_source. Materialization now happens only when the
bench_function closure runs (i.e., when criterion's filter matches),
keeping samply / cargo flamegraph traces concentrated on the decode
side. CI timings are unaffected — the timing loop already excluded
setup; this change just narrows the work performed during ad-hoc
filtered runs.
Refs #189
* ci: add no_std compile + clippy gate for decoder
The crate declares #![no_std] with std as an opt-out feature, but the
existing test/clippy matrix only exercises --features hash,std,
dict_builder. A regression that pulls std::* into a hot decoder path
slips through unnoticed.
Add a new no-std job covering the two realistic deployment shapes:
- alloc-only (no features)
- alloc + xxhash content checksum (--features hash)
Both run cargo check and cargo clippy with -D warnings so a stray
`use std::...` lights up immediately.
Refs #189
* refactor(decode): replace LL/ML code match with const LUTs
Convert the lookup_ll_code / lookup_ml_code helpers from staircase
match arms to const arrays mirroring donor zstd's LL_base / LL_bits
/ ML_base / ML_bits tables. The bounds-check assert preserves the
original panic-on-illegal-code semantics; LLVM already lowered the
match to a jump table so perf is unchanged on this corpus, but the
LUT form is the donor-parity baseline future work can build on.
Refs #189
* perf(decode): elide FSE state-advance bounds check in hot path
FSEDecoder::update_state_fast indexes self.table.decode[next_state]
on every sequence × 3 state advances. The bounds check fires every
iteration because LLVM cannot prove the invariant that spans
table-build and decode call sites:
calc_baseline_and_numbits pairs new_state with num_bits such that
new_state + (2^num_bits - 1) < table_size = self.table.decode.len();
get_bits_unchecked guarantees add < 2^num_bits; therefore
next_state < self.table.decode.len() always holds for any valid
FSE table — but the proof is cross-function and the bounds check
stays.
Replace the indexed read with get_unchecked and a SAFETY comment
citing the invariant. No new unsafe entry point — the function is
already unsafe-ish in the sense that the caller must call ensure_bits
before invoking it (otherwise get_bits_unchecked reads stale bits).
Refs #189
* perf(decode): collapse simd_copy CopyFn dispatcher + add single-op fast path
Replace the CopyStrategy struct + CopyFn function-pointer indirection
with an inlined arch-aware decision tree. Two structural wins:
1. The per-call indirect dispatch through `(strategy.copy)(...)` is
gone — kernels are direct-called from the dispatcher, so on aarch64
(where NEON is the ABI baseline) the kernel inlines into
copy_bytes_overshooting entirely.
2. New single_op_copy_16 fast path covers copy_at_least in 1..=16 with
one SIMD store (or two overlapping u64 stores on archs without
128-bit vectors). Match-copy execution with offset 8..15 funneled
into the previous scalar-strategy loop on every iteration — the
profile showed simd_copy::copy_scalar at 273 ms (5.5%) vs copy_neon
at 80 ms (1.6%) on `decompress/level_-1_fast/decodecorpus-z000033/
c_stream/matrix/pure_rust`; the new fast path collapses those 8..15
byte calls to one vector op.
no_std semantics preserved: x86 runtime detection stays std-only, no_std
x86 uses compile-time target_feature cfg, aarch64+neon is unconditional
in both modes. CI no-std + no-std-hash jobs cover the build matrix.
Local aarch64 (M1, decodecorpus-z000033 c_stream level_-1_fast):
baseline 1,814,570 ns/iter
this PR 1,737,827 ns/iter (median of 3, best 1,683,119)
-> -4.2% median (-7.2% best); 553 → 575 MB/s
All 86 simd_copy / ringbuffer / decode_buffer / cross_validation /
roundtrip tests green. cargo clippy --no-default-features {-, +hash}
clean.
Refs #189
* perf(decode): add WILDCOPY_OVERLENGTH slack to RingBuffer allocation
simd_copy::copy_bytes_overshooting falls back to copy_from_nonoverlapping
(libc memmove) whenever `min(src.1, dst.1) < copy_at_least` rounded up
to the active SIMD chunk. The fallback dominated the decode profile —
_platform_memmove sat at 21.5% of CPU on decompress/level_-1_fast/
decodecorpus-z000033/c_stream/matrix/pure_rust because match copies near
ring-buffer wraparound boundaries had tight capacities and tripped the
fallback every iteration. The new single_op_copy_16 fast path also
requires min_buffer_size >= 16, so any 8..15-byte chunk emitted by
DecodeBuffer::repeat_in_chunks for offsets 8..15 skipped it for the
same reason.
Mirror donor zstd's WILDCOPY_OVERLENGTH = 16 contract by:
1. Allocating `cap + WILDCOPY_OVERLENGTH` bytes via alloc_zeroed. The
slack region is physically present past `cap` but never indexed via
`head`/`tail` (those still wrap on `cap`). alloc_zeroed instead of
alloc so wildcopy reads observe defined values rather than UB on
uninitialized memory.
2. Updating Drop and `reserve_amortized`'s `current_layout` to match.
3. Inflating the `src.1`/`dst.1` capacities passed to
`copy_bytes_overshooting` at every call site where the inflated end
stays inside the allocation:
- src.1: every site (source pointer + WILDCOPY_OVERLENGTH lands in
the slack region at worst).
- dst.1: only the case-A first-copy site where dst ends at `cap`
(slack absorbs overshoot). All other dst spans end at `head` and
cannot be inflated — overshoot would clobber readable data.
Decode time on the profiled scenario:
baseline 1,814,570 ns/iter
prev PR 1,737,827 ns/iter (single_op + dispatcher collapse)
this commit 1,668,752 ns/iter (median of 4, range 1.667-1.678)
-> -8.0% from baseline, -4.0% incremental
-> FFI gap 3.89x -> 3.57x; 553 MB/s -> 600 MB/s
All 556 nextest tests pass. no_std + clippy clean.
Refs #189
* ci(no-std): apply CodeRabbit nitpicks on PR #190
- Pin the `clippy` component on the toolchain step (matches the
existing `lint` job's explicit components: rustfmt, clippy spec —
deterministic install, no auto-fetch on every run).
- Drop the standalone `cargo check` steps: `cargo clippy` already
runs the full compile + analysis pipeline, so a separate check
was duplicated work for no extra signal.
Refs #189
* perf(decode): skip resize zero-fill in FSETable::build_decoding_table
The Entry { new_state: 0, symbol: 0, num_bits: 0 } pre-fill from
self.decode.resize() was redundant on little-endian targets:
copy_symbols_into_decode already writes a full 4-byte Entry (with
new_state = 0 and num_bits = 0 implicit in the packed u32/u64 layout)
to every slot via raw ptr::write_unaligned calls.
- Replace the resize with reserve + set_len on LE (BE keeps resize so
the iter_mut() field-assignment path stays safe — no production BE
target ships against this crate).
- Extend copy_symbols_into_decode's trailing odd-entry path to write
a full Entry via a u32 store; the previous field assignment relied
on the resize-side zero-fill to initialize new_state and num_bits.
Measured impact on the L-1 fast decodecorpus-z000033 c_stream path is
~0% — FSETable::build_decoding_table self-time is 171 ms / 3.4% of
decode CPU and the resize zero-fill was only ~half of that. The win
is real but small relative to the ring-buffer / simd_copy wins
upstream of it. Keeping the change because the pattern (skip
zero-fill when subsequent writes cover every byte) is a clean donor
parity point and stops future regressions where a slower iter_mut
loop would re-introduce the redundant pass.
Refs #189
* perf(decode): inline small copies in simd_copy + route RingBuffer::extend through it
Two coordinated changes that together cut _platform_memmove from 25.9%
self-time (1264 ms) to 7.1% (343 ms) on decompress/level_-1_fast/
decodecorpus-z000033/c_stream/matrix/pure_rust.
1. simd_copy: add an exact-length tail path for copy_at_least <= 16 that
fires when the single_op_copy_16 fast path can't (no WILDCOPY slack
on the destination). Two branches:
- 1..=8 bytes: byte-by-byte loop. Unrolls to ≤8 immediate-offset
load/store pairs.
- 9..=16 bytes: two overlapping unaligned u64 ops, writing exactly
copy_at_least bytes with no overshoot past dst.0 + copy_at_least.
Replaces the libc memmove call that the dispatcher used to fall back
to for short copies near RingBuffer head boundaries (where head
bounds dst.1 and there is no slack to inflate).
2. RingBuffer::extend: route both free-slice writes through
simd_copy::copy_bytes_overshooting instead of raw
copy_from_nonoverlapping. Profile showed 943 ms of memmove was
coming from `execute_sequences → DecodeBuffer::push → extend`
(literal-byte copies during sequence execution). The previous direct
copy_from_nonoverlapping always lowered to a libc call; routing
through simd_copy lets short literal pushes (the common case at
negative levels) use the inline byte / overlapping-u64 paths above.
dst.1 for the first free slice is inflated by WILDCOPY_OVERLENGTH
because f1_ptr ends at `cap` (slack region is writable); the second
free slice ends at `head` so its dst.1 stays exact.
Decode time on the profiled scenario:
baseline 1,814,570 ns/iter
prior PR state 1,668,752 ns/iter
this commit 1,653,976 ns/iter (median of 5)
-> -8.9% from baseline, -0.9% incremental
The bench-time delta is smaller than the profile-attribution shift
suggests because the new inline path does roughly the same byte-level
work memmove was doing — the win is in killing the libc call overhead
on short copies. Larger copies (>16 bytes that previously hit memmove)
still hit it; chasing those further has diminishing returns.
All 86 simd_copy / ringbuffer / decode_buffer / cross_validation /
roundtrip tests pass. no_std + clippy clean.
Refs #189
* perf(decode): split decode_sequences loop, drop per-iter bits_remaining check
decode_sequences_without_rle previously paid two per-iteration
overheads on the hot path:
1. `if target.len() < num_sequences { state advance }` — used to skip
the trailing state update on the final iteration. Materializing
`target.len()` every iteration and branching cost ~2-3 ns per
sequence on top of the FSE work.
2. `if br.bits_remaining() < 0 { return Err(NotEnoughBytes) }` — an
isize multiply + 3 subtractions every iteration. Donor zstd only
validates bitstream exhaustion once after the sequence loop.
Refactor splits the loop into a hot (num_sequences - 1) body that
always advances state, and a tail iteration that skips the advance.
The bits_remaining check moves out of the hot path into a single
post-loop validation that catches the same underflow case via the
existing >= 0 comparison.
Decode time on the profiled scenario:
baseline 1,814,570 ns/iter
prior PR state 1,653,976 ns/iter
this commit 1,632,669 ns/iter (median of 5)
-> -10.0% from baseline, -1.3% incremental
-> FFI gap 3.89x → 3.50x; 553 MB/s → 612 MB/s
Same change deferred for decode_sequences_with_rle — that path only
fires on blocks that mix RLE-mode codes with FSE-encoded streams,
which decodecorpus-z000033 (and most real corpora at fast levels)
do not produce. Splitting it would add complexity for zero measurable
gain on the targeted scenario.
Refs #189
* docs(decode): document WILDCOPY semantics on branchless ringbuffer path
CodeRabbit flagged that extend_from_within_unchecked_branchless passes
exact-fit (ptr, len) capacities through to copy_with_nobranch_check ->
simd_copy::copy_bytes_overshooting, so it cannot fire the SIMD fast
paths that require min(src.1, dst.1) >= 16 — short copies on that path
always take the inline byte / overlapping-u64 fallback instead of
single_op_copy_16.
Inflating the capacities the way `extend_from_within_unchecked` does
would require threading per-pointer head/tail context into
`copy_with_nobranch_check`, which currently sees only raw pointers and
chunk lengths. Defer that refactor: the branchless path is x86-gated
via `decode_buffer::use_branchless_wildcopy`, and my profiling rig is
aarch64 — there is no local x86 measurement to justify the extra
plumbing right now.
Document the trade-off in a doc-comment on the function so a future
maintainer doesn't accidentally assume WILDCOPY semantics hold on
this path. On aarch64, the unconditional `extend_from_within_unchecked`
is used, so the slack contract is exercised end-to-end there.
Refs #189
* perf(decode): extend simd_copy exact-length tail path to 32 bytes
The previous tail path covered copies of 1..=16 bytes inline; copies of
17..=32 bytes fell through to the chunked SIMD dispatch, which then
bailed (`min_buffer_size < copy_at_least.next_multiple_of(chunk)`) and
ended up in libc memmove for buffers without WILDCOPY_OVERLENGTH slack
on the destination side.
Extend the inline path to 17..=32 by emitting four unaligned u64
ops: two adjacent stores for the first 16 bytes, then two overlapping
stores for the trailing 1..=16 bytes. Eight loads + eight stores
total, all branch-free, exactly `copy_at_least` bytes written without
overshoot. This catches the literal-push size range (10..=24 bytes is
typical on the profiled corpus) that the previous ≤16-byte cap missed.
Decode time on `decompress/level_-1_fast/decodecorpus-z000033/
c_stream/matrix/pure_rust`:
baseline 1,814,570 ns/iter
prior PR state 1,632,669 ns/iter
this commit 1,607,680 ns/iter (median of 5)
-> -11.4% from baseline, -1.5% incremental
-> FFI gap 3.89x → 3.40x; 553 MB/s → 629 MB/s
All 86 simd_copy / ringbuffer / decode_buffer / cross_validation /
roundtrip tests pass.
Refs #189
* fix(decode): gate RingBuffer::extend WILDCOPY slack + tighten FSE set_len window
RingBuffer::extend:
The first-free-slice destination capacity passed to
`simd_copy::copy_bytes_overshooting` was unconditionally inflated by
WILDCOPY_OVERLENGTH. That is only safe when `tail >= head` — in the
wrapped layout (`tail < head`) `free_slice_parts` returns the inner
`(buf+tail, head-tail)` gap, and inflating into the WILDCOPY region
would let wildcopy overshoot past `head` into still-readable buffer
output. Today no overshoot path actually fires because `src.1` is
exact (`in_f1 == copy_at_least`) and caps `min_buffer_size`, so the
inflation never changed dispatch — but the latent UB hazard if
anyone ever adds a `dst.1`-only overshoot fast path is real.
Gate the inflation on `tail >= head` and rewrite the comment to make
both reasoning paths explicit (slack-reachability AND src.1 cap).
Add `extend_wrapped_layout_preserves_bytes_past_head`: a regression
test that builds a wrapped layout via a new `build_tail_before_head`
helper, fills the inner gap, and asserts the 16-byte window past
`head` is untouched.
FSETable::build_decoding_table:
`self.decode.set_len(table_size)` ran immediately after `reserve`,
leaving Entry slots uninitialized through the entire `table_symbols`
resize + symbol-spreading loop. A panic in that window (allocator
failure on `table_symbols.resize`) would unwind with reachable
uninitialized Entry slots — currently UB-free because `Entry: Copy`
has trivial drop, but a fragile contract for future maintainers.
Move `set_len` to immediately precede `copy_symbols_into_decode`,
after all panic-able allocations have finished. The uninitialized-
slot window is now zero panic-able code wide; an unwind from the
spreading loop leaves `self.decode.len() == 0` instead.
All 557 nextest tests pass. Decode perf unchanged on the profiled
scenario (median 1,594,500 ns/iter — the inflation never affected
dispatch outcome under the current simd_copy contract).
Refs #189
* test(decode): replace needless_range_loop with iter_mut in regression test
Clippy 1.95 flags `for i in 0..16 { sentinel[i] = ... }` as
`needless_range_loop`. Rewrite the prefill-sampling loop in
extend_wrapped_layout_preserves_bytes_past_head to use
`sentinel.iter_mut().enumerate()`; the verification loop keeps the
plain `for i in 0..16usize` form because its body indexes the raw
ring-buffer pointer (not the loop's own slice), which clippy permits.
Refs #189
* perf(decode): split BitReaderReversed::refill into hot + cold paths
The previous refill() carried a blanket #[cold] annotation. Donor analysis
(libzstd BIT_reloadDStream_internal at bitstream.h:370-444) plus the
sequence-decode profile flagged this as a regression: refill fires
roughly every 2 sequences during sequence decode — it is the dominant
mid-stream path, not a cold case. The #[cold] hint pessimised the
branch predictor and forced LLVM to spill the hot 3-instruction body
out of every caller's straight-line code.
Reshape the function so:
- Outer refill() is #[inline(always)]. Only the mid-stream hot path
(`self.index >= bytes_consumed`: subtract index, mask bit count,
load 8 bytes) lives here, folding into every get_bits / ensure_bits
call site.
- The three end-of-stream branches (`self.index > 0`, exhausted but
bit_consumed < 64, fully exhausted) move into refill_slow(),
marked #[cold] #[inline(never)]. Same semantics, just no longer
parked on the hot path.
Local aarch64 measurements are dominated by thermal / background-process
noise right now (range 1.68 - 13.1 ms across consecutive runs of the
same bench, vs the prior commit's stable 1.65 ms median); the lowest
sample matches prior best so there is no regression. Keeping the
change per the theoretically-correct-opts-stay-in-tree rule — final
profile pass will revisit if the noise floor settles.
All 120 bit_io / fse / cross_validation / roundtrip tests pass.
Refs #189
* perf(decode): drop unreachable OF/zero-offset checks from FSE hot loop
Both per-iter validations in decode_sequences (with_rle main, without_rle
main, without_rle tail — all three loop bodies) were mathematically
unreachable under any successfully-constructed FSE table:
- `of_code > MAX_OFFSET_CODE` (31): `AlignedFSETable::new(MAX_OFFSET_CODE)`
caps `max_symbol` at table construction, and `build_decoding_table`'s
`TooManySymbols` guard rejects any probabilities array exceeding
`max_symbol + 1` entries. The symbols stored in the FSE state table
index that probabilities array, so emitted symbols are guaranteed
bounded. For RLE-mode tables, `maybe_update_fse_tables` validates the
singleton byte against MAX_OFFSET_CODE at scratch setup.
- `offset == 0`: `offset = obits + (1u32 << of_code)` and `1u32 << of_code`
is ≥ 1 for any `of_code` in 0..=31 — i.e. the addend is at least 1.
Replace both branches with `debug_assert!` so a future invariant break
trips in tests without paying for the comparison on the hot path in
release builds. Same change applied to the with_rle path, the without_rle
main loop body, and the without_rle tail iteration (3 sites total) so
the four branches × N sequences disappear from every FSE decode
iteration.
Local aarch64 measurements: median 1,611 ms / best 1,585 ms across 5
runs — matches prior best (1,586 ms after the exact-len-32 commit).
Range is much tighter than the prior commit's thermal-noise window
(5 - 107 us stddev here vs 5,628 us there). The theoretical win is
~1-3% per the donor analysis (4 branches × ~100k sequences on the
profiled frame); on this run it tucked into the noise floor, but the
change is kept per the project rule that theoretically-correct
optimizations stay in tree pending a final clean-room profile.
All 67 sequence_section_decoder / decode_sequences / cross_validation /
roundtrip tests pass.
Refs #189
* refactor(decode): fold FSE decode body into inline decode_one_sequence helper
The main loop and tail iteration of decode_sequences_without_rle carried
two near-identical copies of the per-sequence decode body. Pull the
shared work (FSE symbol fetch, LL/ML LUT, get_bits_triple, offset
assembly, debug_asserts) into a private #[inline(always)] helper
returning Sequence, called from both sites. LLVM still specializes each
call site after inlining; the source-level duplication and divergence
risk are gone.
No measurable perf change on the profiled scenario (median 1,605 ms /
range 1,600 - 1,627 ms across 5 runs, vs prior 1,611 ms median —
within stddev). All 557 nextest tests pass.
Refs #189
* perf(decode): elide scratch.sequences bounds check in execute_sequences
The hot loop in execute_sequences indexes `scratch.sequences[idx]`
on every iteration to copy the Sequence struct into a local. LLVM
cannot prove the access is in-bounds because `scratch` is a
`&mut DecoderScratch` and the loop body calls
`scratch.buffer.reserve(...)` / `scratch.buffer.push(...)` /
`scratch.buffer.repeat(...)`, which the borrow checker treats as
opaque mutations of the wider struct.
Capture `scratch.sequences.len()` into a `sequences_len` local before
the loop (so the iteration range is stable) and switch the per-iter
read to `get_unchecked(idx)` with a SAFETY comment citing the idx
< sequences_len invariant. Same Sequence value extracted, no
behavioural change.
Local aarch64 measurements:
prior commit median 1,605,658 ns / range 1,600 - 1,627 ns
this commit median 1,588,630 ns / range 1,582 - 1,594 ns
-> -1.1% incremental, -12.5% cumulative from baseline 1,814,570 ns
-> FFI gap 3.89x -> 3.36x; 553 MB/s -> 644 MB/s
All 81 sequence / execute / roundtrip / cross_validation tests pass.
Refs #189
* perf(decode): drop redundant ll>0 / ml>0 branches in execute_sequences
Both `if seq.ll > 0 { push(literals) }` and `if seq.ml > 0 { repeat(...) }`
in execute_sequences were guarding downstream calls that already handle
the zero-length case:
- `RingBuffer::extend` (called via `DecodeBuffer::push`) early-returns
when the input slice is empty.
- `DecodeBuffer::repeat` early-returns when match_length == 0.
Drop the guards — when seq.ll/ml is zero the empty slice / zero match
length flows through naturally without any byte-copy work. Saves two
predictable branches per sequence on a hot path that runs once per
emitted sequence.
The literal-length bounds check (`high > literals_buffer.len()`) stays
on the always-execute path, which is mathematically equivalent to the
old check inside the `seq.ll > 0` branch — when ll == 0,
`high == literals_copy_counter`, so the check resolves identically.
Local aarch64 measurements: median 1,607 ns / range 1,582 - 1,620 ns
across 5 runs, vs prior commit 1,589 ns median. Within run-to-run
stddev (best 1,582 ns matches prior best 1,583 ns). Theoretically
correct (~0.5-1% per branch elimination); kept per the rule that
correct opts stay in tree pending a final clean-room profile.
All 557 nextest tests pass.
Refs #189
* fix(decode): allow dead_code on simd_copy x86 kernels under no-std
In --no-default-features builds on x86/x86_64 the dispatcher cfg-gates
out every call site for `copy_avx2` and `copy_avx512`:
#[cfg(target_feature = "avx512f")] try_chunk_kernel!(64, copy_avx512);
#[cfg(target_feature = "avx2")] try_chunk_kernel!(32, copy_avx2);
The default x86_64 Rust target does NOT enable +avx2 / +avx512f (those
require explicit RUSTFLAGS), so on no-std builds without those flags
both call sites cfg-out. Std builds use runtime detection through
`detect_x86_caps`, which rustc cannot see through statically.
CI no-std job tripped on `-D dead_code`. Tag both functions
`#[allow(dead_code)]` with a doc comment explaining when they go live
(std builds OR `target_feature` enabled).
All three build modes verified locally: --no-default-features,
--no-default-features --features hash, and --features hash,std,
dict_builder all clippy-clean with -D warnings.
* perf(decode): hoist literals_buffer.len() + elide slice bounds in execute_sequences
Two small loop-body changes:
1. `literals_buffer.len()` was reread on every iteration to compute the
bounds-check threshold. The buffer is not mutated by anything called
inside the loop (push writes to `scratch.buffer`, not
`scratch.literals_buffer`), so the length is loop-invariant. Capture
it before the loop as `literals_buffer_len`.
2. `&scratch.literals_buffer[lit_cur..high]` triggers a bounds check
even though we just validated `high <= literals_buffer_len` one
line above. Switch to `get_unchecked(lit_cur..high)` with a SAFETY
comment citing the monotone advance and the validation above.
Local aarch64 measurements: range 1,622 - 1,661 ns across 5 runs (one
2,230 outlier ignored as cargo-clean cache miss / thermal — first run
after full deps rebuild), best 1,622 ns matches prior best window.
Stays in the noise floor relative to the prior commit's 1,587 ns
median; theoretically saves one length read + one slice bounds check
per sequence so kept per the project rule on noise-floor opts.
All 557 nextest tests pass.
Refs #189
* perf(decode): flat fast path in RingBuffer::extend skips wrap dispatch
Profile (decompress L-1 fast decodecorpus-z000033 c_stream) showed
RingBuffer::extend self-time grew to 33.6% after the simd_copy routing
refactor — that earlier change moved the byte-level work into extend's
caller frame, but kept the 4-case free_slice_parts dispatch on the
slow path for workloads that never wrap.
Add a flat fast path at the top of extend that fires whenever the
write is one contiguous copy from data to buf+tail:
if head <= tail && tail + len <= cap {
simd_copy(src, dst with WILDCOPY slack, len);
tail += len; if tail == cap { tail = 0; }
return;
}
For frames whose total decoded size fits in window_size (the common
case on this bench and on most real-world streams), this path fires
on every literal push. The slow path still handles wraparound.
Also fold the CodeRabbit style nit on active_chunk_size_for_tests:
explicit any(target_arch = x86, x86_64) guard added to all three
no-std target_feature cfgs for symmetry with copy_bytes_overshooting.
Decode time on the profiled scenario:
prior commit ~1,637,000 ns
this commit 1,542,705 ns (5-run median)
-> -5.7% incremental
-> -15.0% cumulative from baseline (1,814,570 ns)
-> 649 MB/s best, FFI gap 3.89x -> 3.28x
557 nextest tests pass. no_std + std clippy clean.
Refs #189
* perf(decode): inline RingBuffer::extend + DecodeBuffer::push
After the flat-extend refactor, the body of `extend` is short enough
(~5 instructions plus the inline copy on the common no-wrap path) that
the function-call overhead per literal push is a measurable fraction
of the work itself. `execute_sequences -> DecodeBuffer::push ->
RingBuffer::extend` was paying two stack-frame setups per sequence on
a path where most of the work is a single SIMD store.
Mark both `RingBuffer::extend` and `DecodeBuffer::push` `#[inline]`
so LLVM folds them into `execute_sequences`. The slow-path branches
inside `extend` (wraparound, capacity miss) stay non-inlined by
LLVM's heuristic because they end in calls and panics.
Local aarch64 measurements:
prior commit (flat-extend) ~1,542,705 ns
this commit best/median 1,484,378 / ~1,506,000 ns
-> -2.4% incremental median
-> -18.2% cumulative from baseline (1,814,570 ns)
-> 676 MB/s best run
All 557 nextest tests pass.
Refs #189
* perf(decode): elide three bounds checks per iter in prefetch_literals_n_plus_two
The N+2 lookahead helper indexes scratch.sequences three times per
sequence to pull ll_curr / ll_next / ll_n2. LLVM does not collapse
the three bounds checks because the borrow checker treats the wider
DecoderScratch reference as potentially mutated by sibling call
sites in the loop body. Cache scratch.sequences.len() into a local,
verify idx + 2 < seqs_len at the top, then read the three slots via
get_unchecked. Three Vec bounds checks per sequence drop to zero.
No measurable bench impact on the profiled scenario (median stays in
1,558 - 1,579 ns/iter window vs prior 1,567 ns median). Kept per
the project rule on theoretically-correct noise-level opts. 557
nextest tests pass.
Refs #189
* perf(decode): straight-shift fast path for offset_value >= 4 in do_offset_history
The original branchless-RULES-table dispatch covered all four classes
(repcode 1/2/3 plus fresh offset) in one branchless expression. For
typical workloads the dominant case is class=3 (offset_value >= 4 =
fresh offset, not a repcode), where the work collapses to:
actual = offset_value - 3
scratch = [actual, scratch[0], scratch[1]]
Split into a fast path that handles offset_value >= 4 with an
unconditional shift, and a #[cold] #[inline(never)] do_offset_history_repcode
that runs the full RULES-table branchless dispatch for repcodes 1..=3.
Donor zstd does the same split inside ZSTD_updateRep.
Decode time on the profiled scenario:
prior commit (prefetch get_unchecked) ~1,567,000 ns median
this commit 1,413,000 ns median (best 1,406,952)
-> -9.8% incremental median
-> -22.1% cumulative from baseline (1,814,570 ns)
-> 711 MB/s best, FFI gap 3.89x -> 3.01x
All 557 nextest tests pass.
Refs #189
* perf(decode): fuse decode_sequences + execute_sequences (kill Vec<Sequence> roundtrip)
Donor zstd's ZSTD_decompressSequences_body interleaves ZSTD_decodeSequence
and ZSTD_execSequence in one loop, keeping the seq_t in registers. Our
two-pass pipeline (decode_sequences -> scratch.sequences Vec ->
execute_sequences) was paying ~24 B/seq * 2 of L1/L2 traffic on the Vec
roundtrip plus per-iter Vec::push overhead.
Add decode_and_execute_sequences in sequence_section_decoder which:
- Runs FSE table setup (maybe_update_fse_tables) and bit-stream padding
skip up front.
- For RLE-mode blocks (LL/ML/OF RLE flags), falls back to the legacy
two-pass path internally via decode_sequences_with_rle +
execute_sequences_fields. Rare on real corpora; not worth duplicating.
- Hot path: for each sequence, decode_one_sequence_inline returns a
Sequence in registers, then execute_one_sequence runs the literal
push + match repeat immediately — no scratch.sequences write/read.
Takes field-level borrows of DecoderScratch fields (fse, buffer,
offset_hist, literals_buffer, sequences for RLE fallback) so the call
site in block_decoder can keep `raw` borrowing
workspace.block_content_buffer alongside the mutable workspace borrows.
Add execute_sequences_fields helper in sequence_execution that takes
the same field-level borrows for the RLE fallback path.
Legacy decode_sequences / execute_sequences kept with #[allow(dead_code)]
for the public API surface and any external callers that iterate
sequences explicitly.
Decode time on the profiled scenario:
prior commit (offset_history fast path) ~1,413,000 ns median
this commit 1,383,841 ns median (best 1,377,041)
-> -2.1% incremental median
-> -24.1% cumulative from baseline (1,814,570 ns)
-> 726 MB/s best, FFI gap 3.89x -> 2.93x
All 557 nextest tests pass. clippy clean both std and no_std modes.
Refs #189
* perf(decode): compute bit mask via shift instead of BIT_MASK[n] lookup
The non-BMI2 fallback in mask_lower_bits indexed a 65-entry static
table on every call. Replace with `u64::MAX.checked_shr(64 - n)`:
one shift + one (predicted) cmov vs one L1 load (3-5 cycle latency).
For the hot FSE bitstream decode path this fires 3x per sequence
(once per LL/ML/OF extra-bits extraction in get_bits_triple), so
saving the load latency compounds over thousands of sequences.
`checked_shr` returns None for shift counts >= 64, which is exactly
the n=0 case (64 - 0 = 64) and the invalid n > 64 case (wraps to a
huge value via wrapping_sub). Mapping both to 0 gives the
mathematically-correct empty mask for n=0 and a safe fallback for
the debug_asserted-invalid range.
BMI2 path unchanged (`_bzhi_u64` is already a single instruction).
Local aarch64 measurements:
prior commit (fusion) ~1,386,367 ns median
this commit 1,365,211 ns median (best 1,353,635)
-> -1.5% incremental
-> -25.4% cumulative from baseline (1,814,570 ns)
-> 739 MB/s best, FFI gap 3.89x -> 2.88x
All 557 nextest tests pass.
Refs #189
* fix(decode): gate BIT_MASK table to builds that actually consume it
After the mask_lower_bits switch to compute-based masking, BIT_MASK is
read only by:
- the BMI2 PEXT triple-extract path (std + x86_64 only), and
- the in-crate tests verifying mask values.
cfg-gate the const to `any(test, all(feature = "std", target_arch = "x86_64"))`
so no-std builds on non-x86 targets don't carry a `dead_code` error
through `-D warnings` in CI.
All three configurations clippy-clean with -D warnings:
- --no-default-features
- --no-default-features --features hash
- --features hash,std,dict_builder
557 nextest tests pass.
Refs #189
* fix(decode): restore transactional contract in fused sequence executor
Two correctness fixes in decode_and_execute_sequences:
1. Clear rle_fallback_sequences on entry. The non-RLE fast path never
touches the Vec, so without an explicit clear it would carry whatever
entries the previous block left behind. Externally-observable scratch
state must reflect the current block's data only.
2. Snapshot-restore the buffer write cursor and offset history before
the per-iter side-effects, and roll back on post-loop bitstream
validation failure. The legacy two-pass pipeline (decode_sequences
first, execute_sequences after) reported NotEnoughBytesForNumSequences
/ ExtraBits before any block output was committed. The fused fast
path commits each sequence's literal push + match repeat + offset
history update immediately, so a malformed bitstream that only
reveals itself at the end of the section was leaving partial output
and mutated repeat history behind.
Restore semantics: capture a DecodeBuffer checkpoint and clone the
offset history at the top, and undo both on Err. New
DecodeBuffer::{checkpoint, restore_checkpoint} primitives sit on a
pub(super) RingBuffer::{tail, set_tail} pair so the rollback stays
inside the decode module.
Add regression test checkpoint_restore_undoes_pushes to lock in the
primitive's behaviour — a future change that breaks tail-rewind
semantics would surface here instead of as silent state leakage in
the fused executor.
Local aarch64 measurements:
prior commit ~1,365,211 ns median
this commit 1,390,764 ns median (5 runs)
-> +1.9% from extra Vec::clear + 16-byte checkpoint snapshot per
block; correctness fix is worth the small overhead
-> -23.4% cumulative from baseline (1,814,570 ns)
-> 727 MB/s, FFI gap 3.89x -> 2.96x
558 nextest tests pass (one new). clippy strict in std + no_std modes.
* refactor(decode): remove legacy two-pass sequence pipeline
After the fused decode_and_execute_sequences became the sole entry
point in block_decoder, the legacy paths were dead code held with
allow(dead_code):
- decode_sequences (pub entry, ~40 lines)
- decode_sequences_without_rle (private fast path, ~127 lines)
- execute_sequences (pub workspace-based two-pass, ~90 lines)
- prefetch_literals_n_plus_two (only called by execute_sequences)
Delete all four. decode_sequences_with_rle and execute_sequences_fields
stay — they're invoked by the fused path's RLE-mode fallback. Removes
~292 lines of unreachable code that were dragging the patch-coverage
denominator down (codecov flagged sequence_execution at 56% patch
coverage, sequence_section_decoder at 72%; the dead paths accounted
for most of the gap).
558 nextest tests pass. clippy strict in std + no_std modes. Bench
median 1,413,205 ns/iter (within the 1.35-1.43 ms cluster from prior
commits; the transactional checkpoint added overhead remains the
biggest single regression vs the peak 1,353 ms run).
* fix(ringbuffer): treat head+start==cap as continuous second-slice case
At head+start==cap the source effectively starts at index 0 after the
wrap, which is the continuous-source layout. The prior strict '>' check
routed this boundary through the split-source branch, producing a
zero-length copy_bytes_overshooting call against the trailing slack
region — correctness depended on the unsafe helper treating len == 0
as a hard no-op.
Use '>=' so the boundary case takes the continuous path directly.
* test(decode-buffer): add regression test for restore-after-realloc
restore_checkpoint() documents an invariant that no reallocation may
happen between checkpoint() and restore_checkpoint(), but the unsafe
function does not enforce it. A malformed sequence section that
decodes more than the upfront reserve(MAX_BLOCK_SIZE) can trigger
RingBuffer::reserve_amortized between the two calls — and amortized
growth compacts data (head=0, tail=s1+s2), invalidating the captured
tail index. Without a cap guard the restored tail silently points at
the wrong logical position in the new buffer.
Test fails on current code (no guard yet) — the follow-up fix adds
the cap snapshot + panic on mismatch.
* fix(decode-buffer): guard restore_checkpoint against intervening realloc
restore_checkpoint() previously documented a 'no reallocation between
checkpoint and restore' invariant but did not enforce it. A malformed
sequence section can decode more than the upfront reserve(MAX_BLOCK_SIZE)
and trigger RingBuffer::reserve_amortized, which compacts the buffer
(head=0, tail=s1+s2) — invalidating the captured tail index. Restoring
that stale tail silently produces wrong output, with no observable
error.
Snapshot the RingBuffer cap alongside tail in DecodeBufferCheckpoint
and panic on mismatch. The check is a load-bearing correctness guard,
not a debug-only assert: silent UB is worse than a controlled panic on
untrusted input.
Existing checkpoint_restore_undoes_pushes test updated to reserve()
before the first push (mirroring the real fused-decoder call pattern,
which always reserves MAX_BLOCK_SIZE up front). The new
restore_checkpoint_after_realloc_panics regression test from the
preceding commit now passes.
* docs(sequence-decoder): update restore_checkpoint SAFETY comment
The previous comment claimed 'no reallocation has happened in between
(buffer.reserve was called once before the checkpoint)' — but
reserve(MAX_BLOCK_SIZE) only rules out reallocation on well-formed
input. A malformed block that decodes past MAX_BLOCK_SIZE before the
post-loop bitstream check can still trigger reserve_amortized.
Document the actual invariant: restore_checkpoint() now enforces the
no-reallocation guarantee via a cap-equality panic, which is preferred
to silent wrong output on untrusted input.
* test(fuzz): pin libFuzzer crash-bfb3bc55 reproducer
Malformed block whose sequence section decodes more than the upfront
reserve(MAX_BLOCK_SIZE) the fused executor performs. That forces
RingBuffer::reserve_amortized between checkpoint() and the post-loop
bitstream validity check; the panic-on-cap-mismatch guard then turned
the malformed input into a hard abort.
Test fails on current code (the prior panic guard fires) and passes
once try_restore_checkpoint switches to a graceful no-restore return.
* fix(decode-buffer): graceful try_restore_checkpoint, no panic on malformed input
Replace the unsafe restore_checkpoint(_) that panicked on cap mismatch
with safe try_restore_checkpoint(_) -> bool. The cap-equality check is
now load-bearing logic instead of an unconditional abort:
- On well-formed input (the hot path) the upfront reserve(MAX_BLOCK_SIZE)
rules out reallocation between checkpoint and post-loop validity
check, so try_restore_checkpoint returns true and rolls back the
speculative pushes / offset history exactly like before.
- On malformed input that decodes past MAX_BLOCK_SIZE, reserve_amortized
compacts the ring buffer and invalidates the captured tail. The
function now returns false, no state is mutated, and the caller
continues with the same Err return — the partial bytes are discarded
with the corrupt frame.
This closes the DoS surface where any decoder fed untrusted bytes
could be aborted with the panic guard that the previous round
introduced. Function is now safe (no unsafe at the call site).
* perf(ringbuffer): inline flat fast path in reserve()
Skip free_slice_lengths' branch + saturating_sub on the common case
where the data region has not wrapped (head <= tail) and the write
does not cross cap. In that layout free space is trivially
'cap - tail - 1 + head' >= 'cap - tail - 1', which dominates 'amount'
whenever 'tail + amount < cap'.
Mirrors the flat fast path already in RingBuffer::extend so push/repeat
on frames that fit in the window pay zero dispatch overhead from
reserve() either. Theoretical win on the hot path; M1 thermal noise
masks the actual delta in bench output, kept per the iteration
discipline rule (correct opt, no regression risk).
* review: address Copilot round-1 on PR #194 (7 threads)
- fuzz_regressions: assert decoder.read_to_end is_err() instead of
swallowing the result — silent-pass shape (if let Ok / let _ = …)
could no longer catch a regression that decoded the malformed block
to a non-error result.
- fse_decoder: move set_len into copy_symbols_into_decode, adjacent to
the per-slot raw writes. Removes the non-local invariant requiring
the caller's set_len to stay paired with the callee's init loop.
- ringbuffer::set_tail: extend SAFETY to require head fixed since
capture (full/empty discriminator + invariant 4 only stay
consistent then). Document the single current caller and its
precondition.
- sequence_section_decoder: collapse the split [N-1 with-update] +
[last without-update] loop into one loop with a trailing
'if i + 1 < num_sequences' state-update guard. Same generated code,
one body, donor isLastSeq pattern.
- decode_buffer test: Vec::new() instead of vec::Vec::new() (the
latter resolves only via alloc::vec re-exporting Vec — looks like
a typo).
- bit_reader_reverse::mask_lower_bits: docstring now explicitly notes
the input-validation debug_assert is on this function (not just
get_bits) — Copilot misread the existing assert location.
- simd_copy::single_op_copy_16: add a reachability matrix comment
before the portable fallback so a future arch arm slotted between
existing arms knows it must terminate with return or its code goes
silently dead.
* review: address CodeRabbit round-2 on PR #194 (3 findings)
- sequence_section_decoder: only restore offset_hist when
try_restore_checkpoint actually rolled back the buffer. The previous
unconditional rewind left buffer + offset_hist out of sync on the
realloc-detected branch (buffer keeps speculative bytes, repcode
history rewinds) — workspace would be internally inconsistent for
any subsequent reuse after the Err.
- fse_decoder: tighten update_state_fast from pub to pub(crate). The
method's unchecked decode-table read assumes a fully-built table
reached via crate-internal call paths (FSEDecoder constructed with
an empty FSETable would dereference get_unchecked(0) on an empty
Vec, which is UB). External access cannot construct that invariant
themselves, so pub(crate) is the right surface.
- ringbuffer::reserve: switch fast-path math to cap.saturating_sub
and reserve_amortized to checked_add. A caller passing
amount == usize::MAX (e.g. via a malformed match_length) could
previously wrap tail+amount or cap+amount in release and produce
an undersized allocation that subsequent unsafe writes trusted —
now overflows panic with a clear message instead.1 parent a75864f commit 6730114
11 files changed
Lines changed: 1305 additions & 423 deletions
File tree
- .github/workflows
- zstd
- benches
- src
- bit_io
- decoding
- fse
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
176 | 176 | | |
177 | 177 | | |
178 | 178 | | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
179 | 207 | | |
180 | 208 | | |
181 | 209 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
250 | 250 | | |
251 | 251 | | |
252 | 252 | | |
253 | | - | |
254 | | - | |
255 | | - | |
256 | | - | |
257 | | - | |
258 | 253 | | |
259 | 254 | | |
260 | 255 | | |
261 | 256 | | |
262 | 257 | | |
263 | 258 | | |
264 | | - | |
265 | | - | |
266 | | - | |
267 | | - | |
268 | | - | |
269 | | - | |
270 | | - | |
271 | | - | |
272 | | - | |
273 | | - | |
274 | 259 | | |
275 | 260 | | |
276 | 261 | | |
| 262 | + | |
277 | 263 | | |
278 | 264 | | |
279 | 265 | | |
| |||
283 | 269 | | |
284 | 270 | | |
285 | 271 | | |
286 | | - | |
287 | 272 | | |
288 | 273 | | |
289 | 274 | | |
290 | | - | |
291 | | - | |
292 | 275 | | |
293 | 276 | | |
294 | 277 | | |
| |||
297 | 280 | | |
298 | 281 | | |
299 | 282 | | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
300 | 308 | | |
| 309 | + | |
301 | 310 | | |
302 | 311 | | |
303 | 312 | | |
| |||
315 | 324 | | |
316 | 325 | | |
317 | 326 | | |
| 327 | + | |
318 | 328 | | |
319 | 329 | | |
320 | 330 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
8 | | - | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
| |||
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
28 | | - | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
29 | 32 | | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
35 | 42 | | |
36 | 43 | | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
37 | 48 | | |
38 | 49 | | |
39 | 50 | | |
| |||
42 | 53 | | |
43 | 54 | | |
44 | 55 | | |
45 | | - | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
46 | 72 | | |
47 | 73 | | |
48 | 74 | | |
| |||
160 | 186 | | |
161 | 187 | | |
162 | 188 | | |
163 | | - | |
164 | | - | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
165 | 201 | | |
166 | 202 | | |
167 | 203 | | |
| |||
176 | 212 | | |
177 | 213 | | |
178 | 214 | | |
179 | | - | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
180 | 231 | | |
181 | 232 | | |
182 | 233 | | |
| |||
203 | 254 | | |
204 | 255 | | |
205 | 256 | | |
206 | | - | |
207 | | - | |
208 | | - | |
209 | 257 | | |
210 | 258 | | |
211 | 259 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
16 | 15 | | |
17 | 16 | | |
18 | 17 | | |
| |||
186 | 185 | | |
187 | 186 | | |
188 | 187 | | |
189 | | - | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
190 | 196 | | |
191 | 197 | | |
192 | 198 | | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
193 | 202 | | |
194 | 203 | | |
195 | | - | |
196 | | - | |
197 | 204 | | |
198 | 205 | | |
199 | 206 | | |
| |||
0 commit comments