perf(decoding): add shared dictionary handle#105
Merged
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 (1)
📝 WalkthroughWalkthroughAdds a cloneable, shareable DictionaryHandle (Arc/Rc-backed), integrates handle-based registration and initialization into FrameDecoder and StreamingDecoder, introduces dictionary validation and new FrameDecoder errors, adds tests and a Criterion benchmark, and re-exports DictionaryHandle from the decoding module. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant DH as DictionaryHandle
end
rect rgba(255,200,200,0.5)
participant FD as FrameDecoder
end
rect rgba(255,255,200,0.5)
participant DS as DecoderScratch
end
Client->>DH: DictionaryHandle::decode_dict(...) / from_dictionary(...)
Client->>FD: add_dict_handle(DH) / add_dict_from_bytes(...)
FD->>FD: store in shared_dicts / owned_dicts
Client->>FD: decode_all_with_dict_handle(input, out, DH)
FD->>FD: parse frame header
FD->>DH: as_dict() -> &Dictionary
FD->>DS: init_from_dict(&Dictionary)
DS-->>FD: decode bytes / status
FD-->>Client: return decoded size / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable, shared dictionary wrapper (DictionaryHandle) to avoid repeatedly parsing the same zstd dictionary across decode paths, and wires FrameDecoder to accept/store these handles.
Changes:
- Add
DictionaryHandle(cloneable shared handle around a parsedDictionary) and re-export it fromdecoding. - Update
FrameDecoderto store dictionary handles and provideadd_dict_handle, updating decode initialization to borrow viaAsRef<Dictionary>. - Update and extend dictionary-related tests to cover handle cloning / sharing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| zstd/src/decoding/dictionary.rs | Adds DictionaryHandle backed by shared ownership for reuse; adds a unit test asserting clones share the same inner allocation. |
| zstd/src/decoding/frame_decoder.rs | Switches decoder dictionary storage to handles; adds add_dict_handle and updates dict initialization to use borrowed dictionaries. |
| zstd/src/decoding/mod.rs | Re-exports DictionaryHandle from the decoding module. |
| zstd/src/tests/dict_test.rs | Updates decoding test to use DictionaryHandle and add_dict_handle. |
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/decode_dict_handle.rs`:
- Around line 29-67: Add a one-time correctness preflight in
bench_decode_dict_handle: before creating the Criterion group, decode compressed
into a buffer using FrameDecoder::new() with decode_all_with_dict_handle (and
separately with decode_all_with_dict_bytes) and assert the output equals payload
(using output_len, compressed, handle, and dict_raw) so both paths produce
correct bytes; only proceed to the benchmark groups if these equality checks
succeed.
🪄 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: e0b531f3-c99c-40b7-9b17-94a4e7239272
📒 Files selected for processing (5)
zstd/Cargo.tomlzstd/benches/decode_dict_handle.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/mod.rszstd/src/decoding/streaming_decoder.rs
- use Rc on non-atomic targets for DictionaryHandle - keep add_dict allocation-free and add dict decode tests - add benchmark preflight correctness checks
polaz
force-pushed
the
perf/#86-ddict-handle
branch
from
April 11, 2026 17:53
da01705 to
102052e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/decoding/frame_decoder.rs`:
- Around line 638-686: Extract the duplicated multi-frame decode loop from
decode_all_with_dict_handle into a single helper (e.g.,
decode_all_frames_with_optional_dict or decode_all_frames) that accepts the
input slice, output slice, and an Option<&DictionaryHandle>, and implements: the
outer loop with init_with_dict_handle/init (or a branch on the Option) including
SkipFrame handling, the inner loop calling
decode_blocks(BlockDecodingStrategy::UptoBytes(1024 * 1024)), draining via
self.read and advancing output/total_bytes_written, checking self.can_collect()
-> TargetTooSmall and self.is_finished(), and proper error propagation; then
replace the bodies of decode_all_with_dict_handle and decode_all (or
decode_all_with_dict_bytes) to call this helper (for the raw dictionary path
first decode DictionaryHandle::decode_dict then call helper) so the logic is
shared and behavior unchanged.
In `@zstd/src/tests/dict_test.rs`:
- Around line 311-329: The fixture selection currently grabs the first .zst
under dict_tests/files (variables files and file_path), which may pick a
non-dictionary frame; update load_sample_dict_frame() to explicitly choose a
dictionary-specific fixture by filtering file names or stems for a dictionary
marker (e.g., contains "dict" or "dictionary" or matches a known dict fixture
pattern) instead of relying on sort order, e.g., replace the .find closure with
a filter that checks both extension == "zst" and the
path.file_stem()/to_string_lossy() contains the dict marker before calling
expect.
🪄 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: 8964170b-548f-4a35-85cd-973724733a7e
📒 Files selected for processing (7)
zstd/Cargo.tomlzstd/benches/decode_dict_handle.rszstd/src/decoding/dictionary.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/mod.rszstd/src/decoding/streaming_decoder.rszstd/src/tests/dict_test.rs
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 619 🏁 Script executed: Length of output: 3031
Both checks fire once at benchmark-setup time, so any regression in either decode path will abort the benchmark run immediately rather than silently timing incorrect output. The fix looks good — appreciate the proactive note. 🧠 Learnings used |
- reuse shared multi-frame decode helper - ensure sample dict fixture requires a dictionary
Member
Author
|
@coderabbitai Fixed both: extracted a shared multi-frame decode helper for decode_all/with_dict_handle, and hardened the dict fixture by asserting the selected .zst frame declares a dictionary id. Replying here since I lack permission to reply on review comments. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14526 🏁 Script executed: Length of output: 3392
Fix Confirmed in pub fn decode_all_with_dict_bytes(…) -> Result<usize, FrameDecoderError> {
let dict = DictionaryHandle::decode_dict(raw_dictionary)?;
self.decode_all_with_dict_handle(input, output, &dict)
}The full multi-frame skip/decode loop lives only once inside Fix In the current branch snapshot 🧠 Learnings used |
- cover decode_all skip and target-too-small paths - exercise DictionaryHandle conversions
There was a problem hiding this comment.
♻️ Duplicate comments (1)
zstd/src/tests/dict_test.rs (1)
386-405: 🧹 Nitpick | 🔵 TrivialSelect a dictionary-backed fixture during discovery, not after first pick.
The helper still picks the first
.zstand only then asserts dictionary ID presence. If a non-dictionary.zstsorts first, this fails despite valid dictionary fixtures being present.Suggested hardening diff
- let file_path = files + let (file_path, compressed) = files .into_iter() .filter_map(Result::ok) .map(|entry| entry.path()) - .find(|path| { - path.extension() - .and_then(|ext| ext.to_str()) - .map(|ext| ext == "zst") - .unwrap_or(false) + .find_map(|path| { + let is_zst = path + .extension() + .and_then(|ext| ext.to_str()) + .map(|ext| ext == "zst") + .unwrap_or(false); + if !is_zst { + return None; + } + + let compressed = fs::read(&path).ok()?; + let mut header_src = compressed.as_slice(); + let (header, _) = crate::decoding::frame::read_frame_header(&mut header_src).ok()?; + if header.dictionary_id().is_some() { + Some((path, compressed)) + } else { + None + } }) - .expect("expected at least one .zst file in dict_tests/files"); - - let compressed = fs::read(&file_path).expect("compressed data should load"); - let mut header_src = compressed.as_slice(); - let (header, _) = crate::decoding::frame::read_frame_header(&mut header_src) - .expect("sample frame header should parse"); - assert!( - header.dictionary_id().is_some(), - "sample fixture must require a dictionary" - ); + .expect("expected at least one dictionary-backed .zst file in dict_tests/files");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zstd/src/tests/dict_test.rs` around lines 386 - 405, The code currently finds the first .zst file into file_path and only then checks header.dictionary_id(), which fails if that first file lacks a dictionary; instead, modify the discovery so the iterator selects the first .zst whose frame header reports a dictionary_id. Inside the closure used by find (or by using filter_map before find), open and read each candidate file into a buffer (as the existing compressed and header_src logic does), call crate::decoding::frame::read_frame_header and only return the path when header.dictionary_id().is_some(); skip files that fail to read or parse so non-dictionary or unreadable .zst fixtures are ignored. Ensure you still call fs::read and read_frame_header (the compressed, header_src, read_frame_header, and header.dictionary_id symbols) for each candidate during discovery rather than after picking the first file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@zstd/src/tests/dict_test.rs`:
- Around line 386-405: The code currently finds the first .zst file into
file_path and only then checks header.dictionary_id(), which fails if that first
file lacks a dictionary; instead, modify the discovery so the iterator selects
the first .zst whose frame header reports a dictionary_id. Inside the closure
used by find (or by using filter_map before find), open and read each candidate
file into a buffer (as the existing compressed and header_src logic does), call
crate::decoding::frame::read_frame_header and only return the path when
header.dictionary_id().is_some(); skip files that fail to read or parse so
non-dictionary or unreadable .zst fixtures are ignored. Ensure you still call
fs::read and read_frame_header (the compressed, header_src, read_frame_header,
and header.dictionary_id symbols) for each candidate during discovery rather
than after picking the first file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecf8cc0a-55ef-4217-8c44-cc4283ce4946
📒 Files selected for processing (1)
zstd/src/tests/dict_test.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
zstd/src/tests/dict_test.rs (1)
386-396: 🧹 Nitpick | 🔵 TrivialFixture discovery still depends on first
.zstordering.Line 386-Line 396 still selects the first
.zstfile, and only later (Line 402-Line 405) asserts dictionary usage. Adding a non-dictionary.zstthat sorts first will fail setup even when valid dictionary fixtures exist. Prefer selecting a.zstfixture that already satisfies the dictionary-id condition during discovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zstd/src/tests/dict_test.rs` around lines 386 - 396, The current discovery picks the first .zst by extension (using files.into_iter()...find(...) -> file_path) which can pick a non-dictionary fixture; change the finder to only select .zst files that already satisfy the dictionary-id condition used later by the test: inside the find closure, after verifying extension == "zst", open/peek the file and inspect its Zstd frame header or use the existing helper that extracts the dictionary ID (e.g., a read_dict_id_from_zst or parse_frame_header helper) and only return true when the dict-id matches the expected dictionary marker; keep the expect(...) if none match so setup fails clearly when no appropriate dictionary-backed .zst is present.
🤖 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/src/tests/dict_test.rs`:
- Around line 347-356: The test currently ignores the usize returned by
FrameDecoder::new().decode_all_with_dict_handle and decode_all_with_dict_bytes,
which can mask short writes; capture the returned lengths (e.g., len1 and len2)
and assert they equal original.len(), then compare the output buffer up to that
returned length (or assert full buffer equals original after verifying length)
to ensure partial writes don't produce false positives; update the assertions
around decode_all_with_dict_handle, decode_all_with_dict_bytes, output, and
original accordingly.
---
Duplicate comments:
In `@zstd/src/tests/dict_test.rs`:
- Around line 386-396: The current discovery picks the first .zst by extension
(using files.into_iter()...find(...) -> file_path) which can pick a
non-dictionary fixture; change the finder to only select .zst files that already
satisfy the dictionary-id condition used later by the test: inside the find
closure, after verifying extension == "zst", open/peek the file and inspect its
Zstd frame header or use the existing helper that extracts the dictionary ID
(e.g., a read_dict_id_from_zst or parse_frame_header helper) and only return
true when the dict-id matches the expected dictionary marker; keep the
expect(...) if none match so setup fails clearly when no appropriate
dictionary-backed .zst is present.
🪄 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: 379c5dc1-8f47-4cf8-abdc-a851eaf5f6df
📒 Files selected for processing (1)
zstd/src/tests/dict_test.rs
- cover add_dict_from_bytes and mismatched dict handle reset - exercise dictionary handle as_ref via into_handle - document decode_all skipping skippable frames
Member
Author
|
@coderabbitai Fixed — dict fixture selection now chooses dictionary-backed .zst files during discovery. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 14217
✏️ Learnings added
🧠 Learnings used |
- assert decode_all_* returns original length - guard against zero-filled false positives
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15632
All other decode, assert, and summary logic is shared. The outside-diff nitpick is fully addressed. ✅ 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1819
#[cfg(target_has_atomic = "ptr")]
{
let dict = DictionaryHandle::decode_dict(&dict).unwrap();
frame_dec.add_dict_handle(dict).unwrap();
}
#[cfg(not(target_has_atomic = "ptr"))]
{
let dict = Dictionary::decode_dict(&dict).unwrap();
frame_dec.add_dict(dict).unwrap();
}The outside-diff item is fully resolved. ✅ ✏️ Learnings added
🧠 Learnings used |
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/src/decoding/frame_decoder.rs`:
- Around line 294-322: In reset_with_dict_handle ensure consistent
DictionaryHandle accessor usage: replace dict.as_ref() with dict.as_dict() so
the call to initialize decoder_scratch uses the same accessor pattern used
elsewhere (see reset_with_dict_handle and other usages of
DictionaryHandle::as_dict) — update the call to
state.decoder_scratch.init_from_dict to pass dict.as_dict() and keep the rest of
the method unchanged.
🪄 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: 07286ab3-b60f-44fc-b7f9-668a8bdaaa35
📒 Files selected for processing (2)
zstd/src/decoding/frame_decoder.rszstd/src/tests/dict_test.rs
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/src/decoding/dictionary.rs`:
- Around line 317-323: The test
dictionary_handle_from_raw_content_supports_as_ref currently only calls
handle.as_dict(), so it doesn't exercise the public AsRef<Dictionary> impl;
update the test to directly call AsRef by asserting via handle.as_ref() (or
replace as_dict() assertions with assertions on handle.as_ref().id and
handle.as_ref().dict_content.as_slice()) after creating the Dictionary with
Dictionary::from_raw_content and into_handle(), ensuring the AsRef<Dictionary>
implementation is validated.
🪄 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: 6c0e5e43-6aec-43d5-99ee-930850568a17
📒 Files selected for processing (2)
zstd/src/decoding/dictionary.rszstd/src/decoding/frame_decoder.rs
Merged
This was referenced May 15, 2026
This was referenced May 23, 2026
This was referenced Jun 1, 2026
This was referenced Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DictionaryHandlereuse APIs and direct decode paths to avoid per-call dictionary parsingFrameDecoder/StreamingDecoderto accept prepared handles or raw dict bytesAcceptance Criteria
DictionaryHandle+ decoding module docs)FrameDecoder::reset_with_dict_handle,decode_all_with_dict_handle,StreamingDecoder::new_with_dictionary_handle)add_dict_from_bytes,decode_all_with_dict_bytes,new_with_dictionary_bytes)Testing
Closes #86
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests
Documentation