fix: Inaccurate GetSize impl for a few types#7129
Conversation
|
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 (1)
WalkthroughHeap-size tracking is migrated from get_heap_size() -> usize to a tracker-threaded get_heap_size_with_tracker<T: GetSizeTracker>(...) -> (usize, T) API. Vector helpers and trait impls across blocks, proofs, collections, RPC types, shims, and the state manager were updated to thread and return a tracker. ChangesGetSize trait migration to tracker-based API
Sequence DiagramsequenceDiagram
participant Caller
participant VecHelper as vec_heap_size_with_fn_helper
participant Element as Element (GetSize)
participant Tracker as GetSizeTracker
Caller->>VecHelper: (vec, tracker, callback)
loop for each vec[i]
VecHelper->>Element: callback(&element, tracker)
Element-->>VecHelper: (elem_heap_size, tracker)
end
VecHelper->>VecHelper: sum: heap + (capacity - len) * size_of
VecHelper-->>Caller: (total_heap_size, tracker)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Git: Failed to clone repository. Please run the Comment |
hanabi1224
force-pushed
the
hm/fix-get-size-impl
branch
from
7742077 to
9e80f48
Compare
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/blocks/tipset.rs`:
- Around line 192-193: The code measures Tipset.headers by calling
nunny_vec_heap_size_helper(&self.headers, &mut tracker) which deref-coerces the
Arc<NonEmpty<CachingBlockHeader>> and bypasses Arc's tracker-aware measurement,
causing shared Arc allocations to be double-counted; change the measurement to
call self.headers.get_heap_size_with_tracker(&mut tracker).0 so
Arc<T>::get_heap_size_with_tracker runs and deduplicates shared ownership;
update the expression that computes heap_size (currently adding
nunny_vec_heap_size_helper(&self.headers, ...).0 +
self.key.get_heap_size_with_tracker(...).0) to use
self.headers.get_heap_size_with_tracker(...) instead of the nunny helper.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 264f9a18-f7dc-48c7-b86a-eeaa2b244b62
📒 Files selected for processing (13)
src/blocks/header.rssrc/blocks/tipset.rssrc/blocks/vrf_proof.rssrc/cid_collections/small_cid_vec.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/types.rssrc/shim/address.rssrc/shim/econ.rssrc/shim/executor.rssrc/shim/sector.rssrc/shim/state_tree.rssrc/state_manager/mod.rssrc/utils/get_size/mod.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
hanabi1224
force-pushed
the
hm/fix-get-size-impl
branch
from
9e80f48 to
16dbcda
Compare
| + signature.get_heap_size() | ||
| + parent_base_fee.get_heap_size() | ||
| ( | ||
| miner_address.get_heap_size_with_tracker(&mut tracker).0 |
There was a problem hiding this comment.
Are you confident this is the correct use of this API? Seems in examples the tracker gets passed around by move. Not sure if there's a practical difference.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think they're equivalent. https://github.com/bircni/get-size2/blob/6d0ae91743cfba0efa29c3e11507ed0583a670aa/crates/get-size2/src/tracker.rs#L13
impl<T: GetSizeTracker> GetSizeTracker for &mut T {
fn track<A>(&mut self, addr: *const A) -> bool {
GetSizeTracker::track(*self, addr)
}
}There was a problem hiding this comment.
Can't say for sure, but I trust you checked it.
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Worth a changelog entry, given metrics were incorrect.
|
Also, please provide numbers from before and after the change. It'd be good to have them for reference. |
2 participants
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