refactor: purge reference-impl terminology + isolate C bindings in ffi-bench#430
Conversation
…omments Rename comments and identifiers that referenced the reference implementation by name to describe behaviour directly (inline-sequence path, expected-skippable oracle) or cite it neutrally as upstream zstd. First batch: errors, ringbuffer, decode_buffer, scratch, sequence_capture, skippable, lib.
…ec comments Comment-only files: fast kernel, row, hc, bt, dfast, cost model, ldm search, huf cstream, fse/sequence/literals decoders, huff0 decoder, buffer backend, exec-sequence-inline. Source citations now read upstream zstd; no behaviour change.
…r comments Comment-only files: fastcover, fast-kernel count, match-table helpers, streaming encoder, simple backend, encoding mod, fuzz regressions, histogram, cpu kernel, frame decoder.
…omment-only files
…cher/ldm comments Test names and the opt-start-cursor helper renamed to describe their contract (matches_expected_value, opt_start_cursor_and_litlen, applies_gate, etc.); LDM, fast-matcher, strategy, storage, hash-table, blocks comments cite upstream zstd. No behaviour change.
… asserts Replace the libzstd ZSTD_getCParams comparisons with assertions against the constant clevels table[0]/table[3] values transcribed inline. Removes the FFI dependency from these match-generator unit tests; coverage unchanged.
Rename the four files whose names embedded the legacy term to describe their role (block_splitter_parity, cparams_check, cparams_range, ffi_compress_*); update the test name entry and README prose to cite upstream zstd.
…e example identifiers
…ments Test/helper names describe their contract (matches_default_table, uses_btultra2_tiers, merge_block_delimiters, level22_block_ranges, reference_*); comparison variables named reference; comments cite upstream zstd.
Sweep the last C-binding test files (huff0, frame_compressor, block_splitter parity) plus CI/dashboard/docs prose. Identifiers describe their contract (reference_decision, build_limited_weights, encode4x_reference); prose cites upstream zstd. A repo-wide search for the term is now empty (CHANGELOG intentionally untouched).
…I re-check The dictionary and frame-compressor unit tests now decode their own compressed output with FrameDecoder instead of libzstd's bulk decoder. Cross-implementation interop stays covered by tests/cross_validation.rs. Removes the zstd dev-dep import from these two source files.
The lib crate no longer imports zstd_sys. Two white-box conformance clusters (level-22 sequence parity vs ZSTD_generateSequences in match_generator; HUF reader acceptance in huff0_encoder) are removed from src; their coverage stays in the separate surfaces where C bindings belong: tests/cross_validation.rs roundtrips and the compare_ffi / compare_ffi_sequences benches. zstd remains a dev-dependency used only by those benches and integration test crates.
This reverts commit a77075c.
… drop FFI re-check" This reverts commit 8d00b54.
…rate New non-published workspace member ffi-bench depends on structured-zstd plus the C zstd bindings; it owns every target that links libzstd (the compare_ffi* benches, the cross-validation / splitter-parity integration tests, and the FFI diagnostic examples), referencing the in-tree source files via path so their fixtures and the shared benches/support module resolve unchanged. structured-zstd disables auto-discovery for those and drops criterion/dhat. A few white-box unit tests still cross-check against libzstd, so zstd stays a structured-zstd dev-dep until those move to ffi-bench too.
The library crate now links zero C bindings. Every test that cross-checks our output against libzstd lives in the sibling ffi-bench crate: - frame compressor FFI roundtrips (+ fuzz-artifact interop replay) - streaming encoder FFI roundtrips - huff0 weight-description / 4-stream / level-22 literal-section conformance - match-generator level-22 sequence-stream conformance - dictionary roundtrip through the C decoder - corpus roundtrip through the C decoder White-box cases keep their pure-Rust capture in the library behind a testing facade (bench_internals) and feed only the result to the C side in ffi-bench. The our-decoder corpus roundtrips stay in the library. Removed the zstd dev-dependency from the library crate; fixed the moved integration tests to resolve the corpus fixture from either crate root.
📝 WalkthroughWalkthroughThis PR adds an ChangesFFI bench extraction and upstream parity
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…fair The libzstd-linking benches and tests now live in ffi-bench, so the CI references had to follow them. The rust-vs-C parity benches must not link bench_internals (it widens the API surface and biases the comparison). - ffi-bench: drop bench_internals / hash / copy_shape_stats from default and from the base structured-zstd dependency; gate each target that needs them via required-features. compare_ffi / compare_ffi_memory carry none, so building them in isolation links structured-zstd in its plain consumer configuration (dict_builder only). - bench-build: build compare_ffi / compare_ffi_memory from ffi-bench. - lint: clippy the testing facade on structured-zstd and clippy the ffi-bench targets; the parity gate now reads ffi-bench/Cargo.toml and also rejects bench_internals on the dependency and on default features. - test: run the ffi-bench parity / cross-validation / corpus suite (these moved out of the library crate and would otherwise drop out of CI). - run-benchmarks.sh: point the local fallback at ffi-bench.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
zstd/src/decoding/errors.rs (1)
1013-1050:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse backend-agnostic wording for
OutputBufferOverflow.
zstd/src/decoding/flat_buf.rs::exec_sequence_inlinealso raises this variant, so "fixed-size buffer" /UserSliceBackendis too narrow and will misreport valid capacity failures. Please keep the enum doc andDisplaytext aligned with the actual callers.💡 Suggested text
- Inline sequence would write past fixed-size buffer: tail={tail}, requested={requested}, capacity={capacity} + Inline sequence would write past the output buffer: tail={tail}, requested={requested}, capacity={capacity}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/decoding/errors.rs` around lines 1013 - 1050, The error message for the OutputBufferOverflow variant in the Display implementation for ExecuteSequencesError uses backend-specific language ("fixed-size buffer" and references to UserSliceBackend) that does not accurately reflect all callers of this error. Update the write! macro invocation for the OutputBufferOverflow match arm to use backend-agnostic terminology that describes the overflow condition without assuming a specific buffer type, and ensure the enum documentation comment for OutputBufferOverflow is also updated to match this broader scope.zstd/src/encoding/match_generator.rs (1)
4078-4093:⚠️ Potential issue | 🟡 MinorAdd
feature = "std"to the cfg gate for x86 feature detection code.The
std::is_x86_feature_detected!macro requiresstd, but the test block at line 4078 only gates on target architecture. Other modules in the codebase (fastpath/mod.rs, sequence_section_decoder.rs) consistently gate the same macro with#[cfg(all(feature = "std", ...))]. Update this test to follow the established pattern and ensure it compiles in no_std builds.Suggested fix
- #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + #[cfg(all(feature = "std", any(target_arch = "x86", target_arch = "x86_64")))] { if std::is_x86_feature_detected!("sse4.2") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/encoding/match_generator.rs` around lines 4078 - 4093, The cfg gate for the x86 feature detection block starting with `#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]` is missing the required `feature = "std"` condition. The `std::is_x86_feature_detected!` macro depends on the std feature, so update the cfg attribute to include `feature = "std"` alongside the existing target architecture check using an all() wrapper to match the established pattern used in other modules like fastpath/mod.rs and sequence_section_decoder.rs. This ensures the code compiles correctly in no_std builds.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ffi-bench/Cargo.toml`:
- Line 11: The edition field in the Cargo.toml is set to an invalid value of
"2024". Rust only supports editions 2015, 2018, and 2021. Change the edition
field from "2024" to "2021" to use the most recent stable edition.
- Around line 27-38: The features section in the Cargo.toml file is missing the
required `std` feature that coding guidelines mandate. Add a new `std` feature
line to the [features] section and include it in the default features array
alongside the existing features like hash, dict_builder, bench_internals, and
copy_shape_stats. The `std` feature should also forward to the corresponding
feature in the structured-zstd dependency, following the same pattern as the
other features already defined.
In `@ffi-bench/tests/encode_corpus_ffi.rs`:
- Around line 18-24: The corpus_files function collects all directory entries
from both CORPUS_DIR and LOCAL_CORPUS_DIR without filtering by file type, which
causes a panic when the test later attempts to read non-file entries like
subdirectories or symlinks with fs::read(). Filter the collected entries to only
include regular files by checking the metadata of each directory entry before
adding its path to the files vector. This filtering should be applied to both
the initial read_dir(CORPUS_DIR) iteration and the extension from
read_dir(LOCAL_CORPUS_DIR) to ensure only actual files are included.
In `@ffi-bench/tests/huff0_ffi.rs`:
- Around line 197-202: The literal compressed size field extraction in the
lhl_code=2 case is missing a bit mask. In the fourth tuple element where
lit_c_size is calculated as (header >> 18) as usize, apply a 14-bit mask
(0x3fff) to ensure only bits 18-31 are captured, similar to the pattern used in
the lhl_code=1 case above it. Change (header >> 18) as usize to ((header >> 18)
& 0x3fff) as usize to prevent upper bits from being included in the result.
In `@zstd/src/encoding/blocks/compressed.rs`:
- Around line 531-544: The all-identical literal shortcut is currently emitting
RLE for every non-empty run, including 1-5 byte inputs where upstream zstd would
emit raw format instead. This breaks byte-for-byte parity with upstream zstd.
Add a minimum size gate before the RLE emission logic to ensure small
all-identical runs (below the threshold where RLE becomes beneficial) fall
through to raw encoding instead. Apply the same size check logic to the
estimate_literals_section_bytes function to keep the byte estimation aligned
with actual encoding behavior. Reference the comment's explanation about when
RLE equals raw at len==1 and becomes smaller only for len>=2 to determine the
appropriate gate condition.
---
Outside diff comments:
In `@zstd/src/decoding/errors.rs`:
- Around line 1013-1050: The error message for the OutputBufferOverflow variant
in the Display implementation for ExecuteSequencesError uses backend-specific
language ("fixed-size buffer" and references to UserSliceBackend) that does not
accurately reflect all callers of this error. Update the write! macro invocation
for the OutputBufferOverflow match arm to use backend-agnostic terminology that
describes the overflow condition without assuming a specific buffer type, and
ensure the enum documentation comment for OutputBufferOverflow is also updated
to match this broader scope.
In `@zstd/src/encoding/match_generator.rs`:
- Around line 4078-4093: The cfg gate for the x86 feature detection block
starting with `#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]` is
missing the required `feature = "std"` condition. The
`std::is_x86_feature_detected!` macro depends on the std feature, so update the
cfg attribute to include `feature = "std"` alongside the existing target
architecture check using an all() wrapper to match the established pattern used
in other modules like fastpath/mod.rs and sequence_section_decoder.rs. This
ensures the code compiles correctly in no_std builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f74e9d73-54ce-48e0-97f9-e75cc108f03d
📒 Files selected for processing (97)
.github/bench-dashboard/index.html.github/workflows/ci.ymlCargo.tomlREADME.mddocs/SKIPPABLE_MAGIC_ALLOCATIONS.mdffi-bench/Cargo.tomlffi-bench/src/lib.rsffi-bench/tests/dictionary_ffi.rsffi-bench/tests/encode_corpus_ffi.rsffi-bench/tests/frame_compressor_ffi.rsffi-bench/tests/fuzz_interop.rsffi-bench/tests/huff0_ffi.rsffi-bench/tests/match_generator_ffi.rsffi-bench/tests/streaming_encoder_ffi.rszstd/Cargo.tomlzstd/benches/compare_ffi.rszstd/benches/compare_ffi_sequences.rszstd/benches/support/mod.rszstd/examples/cparams_check.rszstd/examples/cparams_range.rszstd/examples/encode_loop_z000033.rszstd/examples/ffi_compress_z000033.rszstd/examples/trace_fast_kernel.rszstd/src/bit_io/bit_reader_reverse.rszstd/src/bit_io/bit_writer.rszstd/src/cpu_kernel.rszstd/src/decoding/buffer_backend.rszstd/src/decoding/decode_buffer.rszstd/src/decoding/errors.rszstd/src/decoding/exec_sequence_inline.rszstd/src/decoding/flat_buf.rszstd/src/decoding/frame.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/literals_section_decoder.rszstd/src/decoding/prefetch.rszstd/src/decoding/ringbuffer.rszstd/src/decoding/scratch.rszstd/src/decoding/seq_decoder_avx2.rszstd/src/decoding/seq_decoder_scalar.rszstd/src/decoding/sequence_execution.rszstd/src/decoding/sequence_section_decoder.rszstd/src/decoding/simd_copy.rszstd/src/decoding/streaming_decoder.rszstd/src/decoding/user_slice_buf.rszstd/src/dictionary/fastcover.rszstd/src/dictionary/mod.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/bt/mod.rszstd/src/encoding/cost_model/mod.rszstd/src/encoding/dfast/mod.rszstd/src/encoding/dict_attach.rszstd/src/encoding/fastpath/mod.rszstd/src/encoding/fastpath/scalar.rszstd/src/encoding/fastpath/sse42.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/frame_emit_info.rszstd/src/encoding/frame_header.rszstd/src/encoding/hc/mod.rszstd/src/encoding/incompressible.rszstd/src/encoding/ldm/gear_hash.rszstd/src/encoding/ldm/mod.rszstd/src/encoding/ldm/params.rszstd/src/encoding/ldm/search.rszstd/src/encoding/ldm/table.rszstd/src/encoding/match_generator.rszstd/src/encoding/match_table/helpers.rszstd/src/encoding/match_table/storage.rszstd/src/encoding/mod.rszstd/src/encoding/opt/ldm.rszstd/src/encoding/opt/mod.rszstd/src/encoding/opt/types.rszstd/src/encoding/parameters.rszstd/src/encoding/row/mod.rszstd/src/encoding/sequence_capture.rszstd/src/encoding/simple/fast_kernel/count.rszstd/src/encoding/simple/fast_kernel/hash_table.rszstd/src/encoding/simple/fast_kernel/kernel.rszstd/src/encoding/simple/fast_kernel/mod.rszstd/src/encoding/simple/fast_matcher.rszstd/src/encoding/simple/mod.rszstd/src/encoding/strategy.rszstd/src/encoding/streaming_encoder.rszstd/src/fse/fse_decoder.rszstd/src/fse/fse_encoder.rszstd/src/fse/mod.rszstd/src/histogram.rszstd/src/huff0/huf_cstream.rszstd/src/huff0/huff0_decoder.rszstd/src/huff0/huff0_encoder.rszstd/src/lib.rszstd/src/skippable.rszstd/src/tests/encode_corpus.rszstd/src/tests/fuzz_regressions.rszstd/src/tests/roundtrip_integrity.rszstd/tests/block_splitter_parity.rszstd/tests/corpus_level22_gap.rszstd/tests/cross_validation.rs
💤 Files with no reviewable changes (1)
- zstd/src/tests/encode_corpus.rs
… collision The C ABI crate names its lib target `structured_zstd` so the cdylib becomes `libstructured_zstd.so` (SONAME rewritten to `libzstd.so.1`). Its crate-type also carried `lib`, emitting `libstructured_zstd.rlib` — the same filename the `structured-zstd` library crate produces, so a full workspace build warned on the output-filename collision (cargo #6313). Nothing depends on this crate from Rust and its 57 unit tests compile from source via `--test` (no library artifact needed); there are no doctests. Drop `lib`, keeping only cdylib + staticlib. One rlib remains, owned unambiguously by the library crate, and the collision warning is gone.
|
…ks raw Two coupled encoder fixes so small inputs are never larger than the C reference (they were by up to 4 bytes on sub-256-byte payloads): - Frame header: drop the 512-byte floor on single-segment selection. A known-size payload that fits the window is now single-segment regardless of size, matching upstream `ZSTD_writeFrameHeader`. A windowed frame cannot encode a content size below 256 in fewer than 4 FCS bytes (the 1-byte FCS class is single-segment-only), so sub-256 inputs paid a 4-byte FCS plus a window descriptor; single-segment stores the size in one byte and omits the descriptor. - Block emit: fall back to a raw block whenever the compressed payload is not smaller than the source (`payload >= block_size`), for every block rather than only dictionary-primed ones. A non-shrinking compressed block wastes bytes and, critically, can reference past a single-segment frame's window (which equals the content size), failing to decode in a strict decoder. The pre-compression raw-fast-path only catches blocks that already look incompressible, so small inputs that slip past it but fail to shrink still need this post-hoc store-raw. Applied on both the owned and borrowed one-shot block loops. Regression tests assert our one-shot frame is never larger than C across small sizes / content shapes and round-trips through the C decoder, and that sub-512 known-size frames are single-segment.
- encode_corpus_ffi: skip non-file directory entries before fs::read, and assert the corpus fixture is present (was a bare unwrap). - huff0_ffi: mask the lhl_code=2 compressed-size field to 14 bits, matching the regenerated-size field and the lhl_code=1 arm. - errors.rs: reword OutputBufferOverflow (doc + Display) backend-agnostically; the flat buffer raises it too, not only the user slice. - match_generator: gate the x86 feature-detection test block on feature = "std" (the macro needs std), matching the rest of the codebase.
- The FFI / diagnostic examples are built from the `ffi-bench` crate now, so their header build/run commands use `-p ffi-bench --example` instead of the stale `-p structured-zstd` form. - Document in zstd/Cargo.toml that with `autotests = false` a pure-Rust integration test in `zstd/tests/` would not run under `cargo test -p structured-zstd`: add such tests as `src/` unit tests or an explicit `[[test]]` entry; C-binding tests go to `ffi-bench`.
… framing The literals-section RLE shortcut only runs inside a compressed block that already carries sequences; a fully all-identical block takes the block-level RLE path instead, and the small-input framing economy lives in the frame header / block-type selection. Cross-reference both so the size math here is not mistaken for the whole small-input story.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zstd/src/encoding/frame_compressor.rs (1)
1085-1091:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the remaining legacy “reference implementation” wording.
The PR objective says this terminology was removed from comments, but Lines 1090 and 1312 still use it. Prefer “upstream zstd” here for consistency.
📝 Proposed terminology cleanup
- // MiB, where the reference implementation's context peaks at + // MiB, where upstream zstd's context peaks at @@ - /// costs as the reference implementation. + /// costs as upstream zstd.Also applies to: 1308-1312
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/encoding/frame_compressor.rs` around lines 1085 - 1091, Replace the legacy terminology "the reference implementation" with "upstream zstd" in the comments to maintain consistency throughout the codebase. Find the comment block that mentions "reference implementation's context" in the frame compression code and update this phrase to use "upstream zstd" instead. Also apply the same terminology change to any other occurrences of "reference implementation" in related comments (around line 1312 in the same file).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@zstd/src/encoding/frame_compressor.rs`:
- Around line 1085-1091: Replace the legacy terminology "the reference
implementation" with "upstream zstd" in the comments to maintain consistency
throughout the codebase. Find the comment block that mentions "reference
implementation's context" in the frame compression code and update this phrase
to use "upstream zstd" instead. Also apply the same terminology change to any
other occurrences of "reference implementation" in related comments (around line
1312 in the same file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d70e97cf-b80d-4468-aa03-369a677e06d7
📒 Files selected for processing (17)
.github/scripts/run-benchmarks.sh.github/workflows/ci.ymlc-api/Cargo.tomlffi-bench/Cargo.tomlffi-bench/tests/encode_corpus_ffi.rsffi-bench/tests/frame_compressor_ffi.rsffi-bench/tests/huff0_ffi.rszstd/Cargo.tomlzstd/examples/cparams_check.rszstd/examples/cparams_range.rszstd/examples/ffi_compress_z000033.rszstd/examples/section_split.rszstd/src/decoding/errors.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/levels/fastest.rszstd/src/encoding/match_generator.rs
Summary
Two related cleanups across the whole repo:
Terminology — removed the legacy reference-implementation term from every identifier, file name, comment, README, and Cargo metadata. Names now describe what a value is / what a function does, never where it came from. Reference attribution (file:line citations) stays in prose comments only.
C bindings isolated — the library crate (
structured-zstd) now links zero C bindings. Every libzstd cross-check (benches, FFI parity tests, profiling examples) lives in a new non-publishedffi-benchcrate that depends on bothstructured-zstdandzstd(the C bindings).C-binding test migration
All C-binding unit tests moved out of the library into
ffi-bench/tests/:White-box cases keep their pure-Rust capture in the library behind a
bench_internalstesting facade and feed only the result to the C side inffi-bench. The our-decoder corpus roundtrips stay in the library. Thezstddev-dependency is removed from the library crate.Verification
cargo tree -p structured-zstd --all-features -i zstd-sys→ not in the dependency graphzstd_sys/zstd::references remain inzstd/src/structured-zstd: 812 tests pass (no C dev-dependency)ffi-bench: 53 tests passcargo fmt --checkcleanSummary by CodeRabbit
Chores
ffi-benchworkspace crate to run C FFI benchmarking, conformance checks, and interoperability validation.Bug Fixes
Build