test(bench): expand benchmark parity matrix#43
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPrecomputes Rust- and C-compressed inputs and refactors decompression benchmarks to run per-source via a new helper; updates BENCHMARKS.md to document symmetric Rust-vs-C decompression runs and adds a single Changes
Sequence Diagram(s)sequenceDiagram
participant Bench as Criterion
participant Harness as bench_decompress
participant Helper as bench_decompress_source
participant RustEnc as structured_zstd (rust compressor)
participant CEnc as zstd (C FFI compressor)
participant RustDec as pure_rust decoder
participant CDec as c_ffi decoder
Bench->>Harness: run bench_decompress(scenario, level)
Harness->>RustEnc: compress_to_vec(level.rust_level)
Harness->>CEnc: zstd::encode_all(level.ffi_level)
Harness->>Helper: bench_decompress_source(..., "rust_stream", rust_compressed, ...)
Harness->>Helper: bench_decompress_source(..., "c_stream", c_compressed, ...)
Helper->>RustDec: assert_decompress_matches_reference(rust_compressed)
Helper->>CDec: assert_decompress_matches_reference(rust_compressed)
Helper->>Bench: create group "decompress/{level}/{scenario}/{source}/matrix"
Bench->>RustDec: run pure_rust benchmark on provided compressed bytes
Bench->>CDec: run c_ffi benchmark on provided compressed bytes
Note over Bench,Helper: emit_memory_report called with stage "decompress-{source}"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Expands the Criterion parity benchmarks to measure decompression performance symmetrically on frames produced by both the pure-Rust encoder and the C zstd encoder, and documents the resulting benchmark matrix.
Changes:
- Extend decompression benchmarks to run on two compressed-frame sources (
rust_streamandc_stream) per scenario/level. - Update
BENCHMARKS.mdto describe the new decompression matrix and map Issue #24 acceptance criteria. - Ignore an accidental root-level artifact named
=via.gitignore.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zstd/benches/compare_ffi.rs |
Refactors decompression benchmarks into a helper and runs both Rust/C decoders against both Rust/C-produced frames. |
BENCHMARKS.md |
Documents the expanded decompression matrix and adds an Issue #24 acceptance mapping section. |
.gitignore |
Ignores an accidental root artifact file named =. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zstd/benches/compare_ffi.rs`:
- Around line 135-155: The decompression benchmarks ("pure_rust" using
FrameDecoder::new() and decode_all and "c_ffi" using
zstd::bulk::Decompressor::new() and decompress_to_buffer) should wrap
inputs/outputs with std::hint::black_box to prevent unwanted compiler
optimization and match the compression benches: inside each b.iter closure call
black_box on the compressed input before passing it to
decode_all/decompress_to_buffer and black_box the result (written or
output/target) before the assert_eq! checks so the benchmark work cannot be
optimized away.
🪄 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
Run ID: cf315879-b2f2-4b33-bdb5-baf3284b7d61
📒 Files selected for processing (3)
.gitignoreBENCHMARKS.mdzstd/benches/compare_ffi.rs
- validate decoded bytes against scenario payload before timing - apply black_box to decompression input/output paths - fix flamegraph filter example for source-segmented benchmark names
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'structured-zstd vs C FFI'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 43e184f | Previous: 4857f1e | Ratio |
|---|---|---|---|
compress/default/high-entropy-1m/matrix/c_ffi |
0.384 ms |
0.298 ms |
1.29 |
compress/default/low-entropy-1m/matrix/pure_rust |
12.005 ms |
10.329 ms |
1.16 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @polaz
Summary
rust_streamandc_streaminputs for symmetric Rust vs C parity checksBENCHMARKS.md=in.gitignoreValidation
Closes #24
Summary by CodeRabbit