refactor: no Arc wrapper around ChainIndex and ZstdFrameCache#6889
Merged
hanabi1224 merged 3 commits intomainfrom Apr 13, 2026
Merged
refactor: no Arc wrapper around ChainIndex and ZstdFrameCache#6889hanabi1224 merged 3 commits intomainfrom
hanabi1224 merged 3 commits intomainfrom
Conversation
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRemoved nested Arc wrappers around ChainIndex, introduced a ShallowClone trait and implementations, switched many .clone() uses to .shallow_clone(), changed various fields/parameters from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/chain/store/index.rs (1)
82-84: Avoid leakingArcthrough the public accessor.Returning
&Arc<DB>forces every caller to couple to the new ownership strategy. Keepingdb()as&DBand adding a separate owned/Arc-returning helper would contain this refactor to the few sites that actually need to clone the store.♻️ Possible API shape
- pub fn db(&self) -> &Arc<DB> { - &self.db - } + pub fn db(&self) -> &DB { + self.db.as_ref() + } + + pub fn db_owned(&self) -> Arc<DB> { + Arc::clone(&self.db) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/index.rs` around lines 82 - 84, The public accessor db(&self) -> &Arc<DB> leaks Arc ownership semantics; change db(&self) to return &DB (i.e., &*self.db) so callers remain uncoupled from Arc, and add a new explicit helper like db_arc(&self) -> Arc<DB> or clone_db(&self) -> Arc<DB> that returns a cloned Arc for callers that actually need owned/shared access; update only the call sites that require an Arc to use the new helper and leave all other uses calling db() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/chain/store/index.rs`:
- Around line 82-84: The public accessor db(&self) -> &Arc<DB> leaks Arc
ownership semantics; change db(&self) to return &DB (i.e., &*self.db) so callers
remain uncoupled from Arc, and add a new explicit helper like db_arc(&self) ->
Arc<DB> or clone_db(&self) -> Arc<DB> that returns a cloned Arc for callers that
actually need owned/shared access; update only the call sites that require an
Arc to use the new helper and leave all other uses calling db() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f43b4985-2c9d-4c75-9605-3975facf576b
📒 Files selected for processing (16)
src/chain/store/chain_store.rssrc/chain/store/index.rssrc/db/car/any.rssrc/db/car/forest.rssrc/db/car/many.rssrc/db/car/mod.rssrc/interpreter/fvm2.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/interpreter/vm.rssrc/rpc/mod.rssrc/state_manager/chain_rand.rssrc/state_manager/mod.rssrc/tool/subcommands/archive_cmd.rssrc/tool/subcommands/benchmark_cmd.rssrc/tool/subcommands/snapshot_cmd.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/rpc/methods/state.rs (1)
2727-2727: Movetsintocur_tshere.
tsis not used after the address checks, so this can take ownership directly and skip one extra shared clone in the walk.♻️ Proposed change
- let mut cur_ts = ts.shallow_clone(); + let mut cur_ts = ts;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/state.rs` at line 2727, The code currently does let mut cur_ts = ts.shallow_clone(), which unnecessarily clones ts; since ts is not used after the address checks, change this to move ownership of ts into cur_ts (i.e., assign cur_ts from ts directly) right after the address checks so the subsequent walk uses the owned value and avoids the extra shallow_clone; update any references relying on cur_ts accordingly (look for ts, cur_ts, shallow_clone, and the walk call).src/rpc/methods/f3.rs (1)
535-541: Consume the fetched header vector instead of cloning its first tipset.This branch already owns the
Vec<Tipset>, sointo_iter().next()avoids one extra clone before the vector is dropped.♻️ Proposed change
- None => ctx - .sync_network_context - .chain_exchange_headers(None, &tsk, nonzero!(1_u64)) - .await? - .first() - .map(ShallowClone::shallow_clone) - .with_context(|| format!("failed to get tipset via chain exchange. tsk: {tsk}"))?, + None => ctx + .sync_network_context + .chain_exchange_headers(None, &tsk, nonzero!(1_u64)) + .await? + .into_iter() + .next() + .with_context(|| format!("failed to get tipset via chain exchange. tsk: {tsk}"))?,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/f3.rs` around lines 535 - 541, The code is unnecessarily cloning the first Tipset from the owned Vec returned by ctx.sync_network_context.chain_exchange_headers; change the None branch to consume the Vec by using into_iter().next() (or into_iter().next().ok_or_else(...)) instead of .first().map(ShallowClone::shallow_clone) so you take ownership of the first element directly; update the error/context handling to match the existing with_context(...) usage and keep references to tsk and the original chain_exchange_headers call the same.src/utils/shallow_clone.rs (1)
6-8: Document what “shallow” guarantees here.This trait now carries core ownership semantics for the refactor. A short contract about preserving shared backing state would make future impls much harder to misuse.
📝 Suggested doc comment
+/// Cheaply duplicates a handle while preserving shared backing state. +/// +/// Implementations should not deep-copy caches or other shared resources. pub trait ShallowClone { + /// Returns another handle to the same underlying state. fn shallow_clone(&self) -> Self; }As per coding guidelines, "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/shallow_clone.rs` around lines 6 - 8, Add a doc comment to the public trait ShallowClone explaining the guarantee of "shallow": state that ShallowClone::shallow_clone must produce a new value that shares the same backing storage as the source (i.e., copies references/pointers only, not deep-copying heap data), is a cheap/constant-time operation, and that mutations to shared backing (if mutation is possible) will be visible through both values; also document any thread-safety or ownership expectations implementers must uphold (for example whether clones must be Send/Sync-safe or must preserve internal reference counts). Reference the trait name ShallowClone and the method shallow_clone in the comment so implementers know this contract applies to that method.src/rpc/methods/chain.rs (2)
1245-1252: Drop the extrashallow_clone()on already-owned tipsets.Both sites already own the
Tipset/Option<Tipset>they want to return, so cloning again just adds work without buying any ownership separation.♻️ Possible cleanup
let finalized = if depth >= 0 && let Ok(ts) = ctx.chain_index().tipset_by_height( (head.epoch() - depth).max(0), head, ResolveNullTipset::TakeOlder, ) { - Some(ts.shallow_clone()) + Some(ts) } else { None };Ok(ctx .chain_store() .heaviest_tipset() .chain(ctx.store()) .find(|ts| ts.parent_state() == &parent_state) - .shallow_clone()) + )Also applies to: 1381-1386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1245 - 1252, The code calls shallow_clone() on Tipset values that are already owned, causing unnecessary cloning; update the branches that construct finalized (the Some(ts.shallow_clone()) case) and the analogous branch around the other occurrence (lines indicated in the review) to return/insert the owned ts directly (e.g., use Some(ts) or move ts) instead of calling ts.shallow_clone(), leaving tipset_by_height and ResolveNullTipset usage unchanged.
1602-1616: Use the newShallowClonebound here or remove it.
PathChange<T>still clones throughClone, andarbitrary()does the same for bothinnerand the chosen enum value. The addedT: ShallowClonebound currently just narrows the generic impl surface.Also applies to: 1697-1705
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1602 - 1616, The generic bound on the manual impl currently uses T: Clone; change it to use the new ShallowClone bound instead (e.g., impl<T: ShallowClone> Clone for PathChange<T>) so the enum’s clone implementation is allowed only when the inner T implements ShallowClone; make the identical swap for the other duplicated impl at the other location (the block referenced around lines 1697–1705) to keep bounds consistent.src/chain/store/index.rs (1)
83-84: Prefer an ownership-neutraldb()accessor.Returning
&Arc<DB>leaks the storage strategy into callers. Returning&DBhere keeps the API smaller and leaves room for a separatedb_arc()only where cloning the handle is actually needed.♻️ Possible cleanup
- pub fn db(&self) -> &Arc<DB> { - &self.db + pub fn db(&self) -> &DB { + self.db.as_ref() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/index.rs` around lines 83 - 84, Change the accessor signature from pub fn db(&self) -> &Arc<DB> to an ownership-neutral pub fn db(&self) -> &DB and adjust the body to return a reference to the inner DB (e.g. by dereferencing the Arc stored in self.db). Also add a separate pub fn db_arc(&self) -> Arc<DB> that clones and returns the Arc when a caller actually needs ownership; update any callers that relied on the old signature to use db_arc() when they need an Arc and db() when they only need a reference.src/state_manager/mod.rs (1)
1248-1273: Minor style inconsistency betweenshallow_clone()andArc::clone().Line 1254 uses
self.shallow_clone()while line 1273 usesArc::clone(self)for the same pattern. While functionally equivalent, consider usingshallow_clone()consistently with the rest of this refactor.♻️ Suggested fix for consistency
- let sm_cloned = Arc::clone(self); + let sm_cloned = self.shallow_clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state_manager/mod.rs` around lines 1248 - 1273, There’s a style inconsistency: earlier you use self.shallow_clone() to clone the state manager but later you call Arc::clone(self); replace the Arc::clone(self) at the end with the same shallow_clone() call to be consistent (i.e., construct sm_cloned via self.shallow_clone()), and ensure the variable name (sm_cloned) doesn’t shadow the earlier one or change its type expectations where used (search_back_for_message, block_revert, task closures).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/chain/store/index.rs`:
- Around line 83-84: Change the accessor signature from pub fn db(&self) ->
&Arc<DB> to an ownership-neutral pub fn db(&self) -> &DB and adjust the body to
return a reference to the inner DB (e.g. by dereferencing the Arc stored in
self.db). Also add a separate pub fn db_arc(&self) -> Arc<DB> that clones and
returns the Arc when a caller actually needs ownership; update any callers that
relied on the old signature to use db_arc() when they need an Arc and db() when
they only need a reference.
In `@src/rpc/methods/chain.rs`:
- Around line 1245-1252: The code calls shallow_clone() on Tipset values that
are already owned, causing unnecessary cloning; update the branches that
construct finalized (the Some(ts.shallow_clone()) case) and the analogous branch
around the other occurrence (lines indicated in the review) to return/insert the
owned ts directly (e.g., use Some(ts) or move ts) instead of calling
ts.shallow_clone(), leaving tipset_by_height and ResolveNullTipset usage
unchanged.
- Around line 1602-1616: The generic bound on the manual impl currently uses T:
Clone; change it to use the new ShallowClone bound instead (e.g., impl<T:
ShallowClone> Clone for PathChange<T>) so the enum’s clone implementation is
allowed only when the inner T implements ShallowClone; make the identical swap
for the other duplicated impl at the other location (the block referenced around
lines 1697–1705) to keep bounds consistent.
In `@src/rpc/methods/f3.rs`:
- Around line 535-541: The code is unnecessarily cloning the first Tipset from
the owned Vec returned by ctx.sync_network_context.chain_exchange_headers;
change the None branch to consume the Vec by using into_iter().next() (or
into_iter().next().ok_or_else(...)) instead of
.first().map(ShallowClone::shallow_clone) so you take ownership of the first
element directly; update the error/context handling to match the existing
with_context(...) usage and keep references to tsk and the original
chain_exchange_headers call the same.
In `@src/rpc/methods/state.rs`:
- Line 2727: The code currently does let mut cur_ts = ts.shallow_clone(), which
unnecessarily clones ts; since ts is not used after the address checks, change
this to move ownership of ts into cur_ts (i.e., assign cur_ts from ts directly)
right after the address checks so the subsequent walk uses the owned value and
avoids the extra shallow_clone; update any references relying on cur_ts
accordingly (look for ts, cur_ts, shallow_clone, and the walk call).
In `@src/state_manager/mod.rs`:
- Around line 1248-1273: There’s a style inconsistency: earlier you use
self.shallow_clone() to clone the state manager but later you call
Arc::clone(self); replace the Arc::clone(self) at the end with the same
shallow_clone() call to be consistent (i.e., construct sm_cloned via
self.shallow_clone()), and ensure the variable name (sm_cloned) doesn’t shadow
the earlier one or change its type expectations where used
(search_back_for_message, block_revert, task closures).
In `@src/utils/shallow_clone.rs`:
- Around line 6-8: Add a doc comment to the public trait ShallowClone explaining
the guarantee of "shallow": state that ShallowClone::shallow_clone must produce
a new value that shares the same backing storage as the source (i.e., copies
references/pointers only, not deep-copying heap data), is a cheap/constant-time
operation, and that mutations to shared backing (if mutation is possible) will
be visible through both values; also document any thread-safety or ownership
expectations implementers must uphold (for example whether clones must be
Send/Sync-safe or must preserve internal reference counts). Reference the trait
name ShallowClone and the method shallow_clone in the comment so implementers
know this contract applies to that method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ddcc73d-5a51-44b3-ae6b-48a8a8f15e63
📒 Files selected for processing (36)
src/blocks/tipset.rssrc/chain/mod.rssrc/chain/store/chain_store.rssrc/chain/store/index.rssrc/chain_sync/bad_block_cache.rssrc/chain_sync/chain_follower.rssrc/chain_sync/network_context.rssrc/chain_sync/tipset_syncer.rssrc/cli/subcommands/chain_cmd/list.rssrc/daemon/db_util.rssrc/daemon/mod.rssrc/db/car/any.rssrc/db/car/forest.rssrc/db/car/many.rssrc/db/car/mod.rssrc/dev/subcommands/state_cmd.rssrc/fil_cns/validation.rssrc/interpreter/fvm2.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/interpreter/vm.rssrc/message_pool/msgpool/mod.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/f3.rssrc/rpc/methods/gas.rssrc/rpc/methods/state.rssrc/rpc/mod.rssrc/state_manager/chain_rand.rssrc/state_manager/mod.rssrc/tool/subcommands/archive_cmd.rssrc/tool/subcommands/benchmark_cmd.rssrc/tool/subcommands/snapshot_cmd.rssrc/utils/cache/lru.rssrc/utils/mod.rssrc/utils/shallow_clone.rs
✅ Files skipped from review due to trivial changes (3)
- src/tool/subcommands/benchmark_cmd.rs
- src/chain_sync/tipset_syncer.rs
- src/tool/subcommands/archive_cmd.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/db/car/any.rs
- src/db/car/many.rs
- src/db/car/mod.rs
- src/interpreter/fvm3.rs
- src/interpreter/fvm4.rs
- src/db/car/forest.rs
- src/interpreter/fvm2.rs
- src/tool/subcommands/snapshot_cmd.rs
- src/chain/store/chain_store.rs
LesnyRumcajs
approved these changes
Apr 13, 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
2 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
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit