fix: reduce allocations in chain traversal#6009
Merged
hanabi1224 merged 3 commits intomainfrom Sep 2, 2025
Merged
Conversation
Contributor
WalkthroughRefactors CID deserialization to use a small-vector optimization via a new SmallCidVec type and updates extract_cids to return it. Adjusts benchmark command to rely on type inference when collecting CIDs from CBOR blocks. No other control flow or exported declarations are modified beyond the return type change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Bench as benchmark_cmd
participant CID as extract_cids (CBOR→SmallCidVec)
CLI->>Bench: Run benchmark (CAR streaming inspect)
Bench->>CID: extract_cids(block.data)
CID-->>Bench: SmallCidVec of CIDs
Bench->>Bench: Check codec, compute unique counts
Bench->>Sink: Write block data
Bench-->>CLI: Report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Member
|
it's a draft but I already want it |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ipld/util.rs (1)
430-460: Reduce per-iteration allocations and unify type alias usage.Good switch to SmallVec. Two follow-ups:
- Reuse the buffer across iterations to avoid re-allocating
cid_vecfor every received CID.- Prefer the shared alias to keep the inline capacity consistent across the codebase.
Apply this diff within the worker loop:
- 'main: while let Ok(cid) = extract_receiver.recv_async().await { - let mut cid_vec: smallvec::SmallVec<[_; 8]> = smallvec::smallvec![cid]; + let mut cid_vec: crate::utils::encoding::cid_de_cbor::SmallCidVec = smallvec::SmallVec::new(); + 'main: while let Ok(cid) = extract_receiver.recv_async().await { + cid_vec.push(cid); while let Some(cid) = cid_vec.pop() { if should_save_block_to_snapshot(cid) && seen.lock().insert(cid) { if let Some(data) = db.get(&cid)? { if cid.codec() == fvm_ipld_encoding::DAG_CBOR { let mut new_values = extract_cids(&data)?; cid_vec.append(&mut new_values); } // Break out of the loop if the receiving end quit. if block_sender .send_async(Ok(CarBlock { cid, data })) .await .is_err() { break 'main; } } else if fail_on_dead_links { // If the receiving end has already quit - just ignore it and // break out of the loop. let _ = block_sender .send_async(Err(anyhow::anyhow!( "[Send] missing key: {cid}" ))) .await; break 'main; } } } + cid_vec.clear(); }If you prefer a minimal change, at least switch the type on initialization:
- let mut cid_vec: smallvec::SmallVec<[_; 8]> = smallvec::smallvec![cid]; + let mut cid_vec: crate::utils::encoding::cid_de_cbor::SmallCidVec = smallvec::smallvec![cid];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/ipld/util.rs(1 hunks)src/tool/subcommands/benchmark_cmd.rs(1 hunks)src/utils/encoding/cid_de_cbor.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
📚 Learning: 2025-08-08T12:11:55.266Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/ipld/util.rs
🔇 Additional comments (4)
src/utils/encoding/cid_de_cbor.rs (3)
177-180: LGTM: zero-allocation init path maintained.
CidVec(SmallCidVec::new())aligns with the new storage and avoids upfront heap allocation.
9-13: Dependencysmallvecalready declared in Cargo.toml; no action required.
12-19: No explicit Vec expectations from extract_cids
All call sites infer or handle SmallCidVec without relying on Vec annotations—no further changes needed.src/tool/subcommands/benchmark_cmd.rs (1)
150-152: LGTM: rely on type inference forextract_cids.Clean and future-proof against container type changes.
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
LesnyRumcajs
previously approved these changes
Aug 28, 2025
akaladarshi
previously approved these changes
Sep 2, 2025
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
hanabi1224
dismissed stale reviews from akaladarshi and LesnyRumcajs
via
a41c6c2
Contributor
Author
|
I had to resolve a merge conflict. Please re-approve. @LesnyRumcajs @akaladarshi |
LesnyRumcajs
approved these changes
Sep 2, 2025
akaladarshi
approved these changes
Sep 2, 2025
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
3 participants
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 of changes
This PR tries to reduce heap allocations in chain traversal (
fn stream_chain) by replacingVecwithSmallVec. This would benefit chain export and snapshot validatation, etc.BTW, I believe it could be further improved by refactoring
fn extract_cidsinto returning an iterator of CIDsPerf gains on my laptop:
calibnet: ~4m40s -> ~3m50s
mainnet: ~47m -> ~37m
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Performance
API Changes
Refactor