test: BeaconEntry::get_heap_size
#5914
Merged
Conversation
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
Contributor
WalkthroughThis change introduces new development and workspace dependencies ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as dhat_get_size_beacon_entry.rs
participant Allocator as dhat (global allocator)
participant Beacon as BeaconEntry
participant GetSize as get-size2
Test->>Allocator: Start heap profiling
Test->>Beacon: Create BeaconEntry instance(s)
Test->>GetSize: Call get_heap_size() on BeaconEntry
Test->>Allocator: Query HeapStats (current_bytes)
Test->>Test: Assert HeapStats ≈ get_heap_size()
Test->>Beacon: Modify vector capacity (reserve, shrink_to_fit)
Test->>Allocator: Query HeapStats again
Test->>GetSize: Call get_heap_size() on Vec<BeaconEntry>
Test->>Test: Assert HeapStats ≈ get_heap_size()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
5fcf065 to
9872c83
Compare
9872c83 to
3f435ea
Compare
elmattic
previously approved these changes
Aug 8, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
interop-tests/tests/dhat_get_size_beacon_entry.rs (1)
15-18: Avoid brittle constant (10); rely onGetSizeHardcoding 10 bytes assumes exact allocator behavior and Vec capacity. The next assertion already compares against
b.get_heap_size(). Drop the literal to reduce brittleness.let stats = dhat::HeapStats::get(); - dhat::assert_eq!(stats.curr_bytes, 10); dhat::assert_eq!(stats.curr_bytes, b.get_heap_size());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(2 hunks)interop-tests/Cargo.toml(1 hunks)interop-tests/tests/dhat_get_size_beacon_entry.rs(1 hunks)src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-07T13:39:15.080Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.080Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
interop-tests/Cargo.toml
📚 Learning: 2025-08-06T15:42:43.520Z
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: Cargo.toml:25-36
Timestamp: 2025-08-06T15:42:43.520Z
Learning: Forest project requires fvm_ipld_amt version 0.7.5 functionality which is available in the Git revision a633547ae414a333b2d076beef87d4d30cdb7fb4 of the ref-fvm repository, but version 0.7.5 is not yet published to crates.io (only 0.7.4 is available). The Git patch must remain until 0.7.5 is published.
Applied to files:
interop-tests/Cargo.tomlsrc/lib.rsCargo.toml
📚 Learning: 2025-08-06T15:42:43.520Z
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: Cargo.toml:25-36
Timestamp: 2025-08-06T15:42:43.520Z
Learning: The fvm_ipld_amt crate version 0.7.5 is required for the Forest project, but it's not yet published to crates.io, which is why the Git patch pointing to the ref-fvm repository is still necessary.
Applied to files:
interop-tests/Cargo.tomlsrc/lib.rsCargo.toml
📚 Learning: 2025-07-17T15:21:40.753Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5841
File: src/utils/get_size/mod.rs:10-10
Timestamp: 2025-07-17T15:21:40.753Z
Learning: The get_size2 crate's GetSize trait provides default implementations: get_stack_size() uses std::mem::size_of, get_heap_size() returns 0, and get_size() returns their sum. An empty impl like `impl GetSize for MyType {}` is valid and uses these defaults, making it suitable for simple wrapper types that don't allocate heap memory.
Applied to files:
interop-tests/tests/dhat_get_size_beacon_entry.rs
📚 Learning: 2025-08-04T13:36:22.993Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5835
File: src/chain/tests.rs:58-76
Timestamp: 2025-08-04T13:36:22.993Z
Learning: In the Forest codebase, `Vec<u8>` can be used as an `AsyncWrite` implementation in test contexts. The user confirmed that tests using `&mut Vec<u8>` with `export` and `export_v2` functions compile and pass both locally and on CI.
Applied to files:
interop-tests/tests/dhat_get_size_beacon_entry.rs
⏰ 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)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
Cargo.toml (2)
17-20: Workspace-scopeddhatandget-size2look goodKeeping
dhatonly as a workspace dependency and consuming it from interop-tests avoids pulling it into the main crate. Unifyingget-size2with the "derive" feature at the workspace level prevents version drift across crates.
96-96: Switch to workspace-managedget-size2Referencing
get-size2viaworkspace = truehere ensures a single feature set and version across the workspace.interop-tests/Cargo.toml (1)
16-24: Dev-only additions align with test intentAdding
dhatandget-size2as dev-dependencies is appropriate for the heap profiling test. No impact on production builds.src/lib.rs (1)
116-118: Minimal, feature-gated re-export is appropriateExposing only
BeaconEntryunderinterop_tests_private::beaconkeeps surface area tight and test-focused. Good separation.interop-tests/tests/dhat_get_size_beacon_entry.rs (1)
7-9: Ignore global allocator collision warningThe only
#[global_allocator]declarations are in two separate crates:
src/lib.rs(Jemalloc)interop-tests/tests/dhat_get_size_beacon_entry.rs(dhat::Alloc)Integration tests are built as independent binaries (separate processes), so each crate has exactly one global allocator and there’s no cross-crate or concurrent allocator interference. No changes required.
Likely an incorrect or invalid review comment.
Comment on lines
25
to
41
| let stats = dhat::HeapStats::get(); | ||
| assert!(v.capacity() >= v.len()); | ||
| dhat::assert_eq!(stats.curr_bytes, 32 * v.capacity() + 60); | ||
| dhat::assert_eq!(stats.curr_bytes, v.get_heap_size()); | ||
|
|
||
| v.reserve_exact(100); | ||
|
|
||
| let stats = dhat::HeapStats::get(); | ||
| assert!(v.capacity() >= 100 + v.len()); | ||
| dhat::assert_eq!(stats.curr_bytes, 32 * v.capacity() + 60); | ||
| dhat::assert_eq!(stats.curr_bytes, v.get_heap_size()); | ||
|
|
||
| v.shrink_to_fit(); | ||
| let stats = dhat::HeapStats::get(); | ||
| assert_eq!(v.capacity(), v.len()); | ||
| dhat::assert_eq!(stats.curr_bytes, 32 * v.capacity() + 60); | ||
| dhat::assert_eq!(stats.curr_bytes, v.get_heap_size()); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compute sizes dynamically; don’t assume BeaconEntry is 32 bytes or that shrink_to_fit is exact
- Replace the magic 32 with
size_of::<BeaconEntry>()to be arch- and layout-safe. - Replace the magic 60 with a sum of element heap sizes.
shrink_to_fitis a non-binding request; some allocators may keep spare capacity. Avoid assertingcapacity == len.
- let stats = dhat::HeapStats::get();
- assert!(v.capacity() >= v.len());
- dhat::assert_eq!(stats.curr_bytes, 32 * v.capacity() + 60);
- dhat::assert_eq!(stats.curr_bytes, v.get_heap_size());
+ let stats = dhat::HeapStats::get();
+ let entry_size = core::mem::size_of::<BeaconEntry>();
+ let inner_bytes = v.iter().map(GetSize::get_heap_size).sum::<usize>();
+ assert!(v.capacity() >= v.len());
+ dhat::assert_eq!(stats.curr_bytes, entry_size * v.capacity() + inner_bytes);
+ dhat::assert_eq!(stats.curr_bytes, v.get_heap_size());
v.reserve_exact(100);
- let stats = dhat::HeapStats::get();
- assert!(v.capacity() >= 100 + v.len());
- dhat::assert_eq!(stats.curr_bytes, 32 * v.capacity() + 60);
- dhat::assert_eq!(stats.curr_bytes, v.get_heap_size());
+ let stats = dhat::HeapStats::get();
+ let entry_size = core::mem::size_of::<BeaconEntry>();
+ let inner_bytes = v.iter().map(GetSize::get_heap_size).sum::<usize>();
+ assert!(v.capacity() >= 100 + v.len());
+ dhat::assert_eq!(stats.curr_bytes, entry_size * v.capacity() + inner_bytes);
+ dhat::assert_eq!(stats.curr_bytes, v.get_heap_size());
v.shrink_to_fit();
let stats = dhat::HeapStats::get();
- assert_eq!(v.capacity(), v.len());
- dhat::assert_eq!(stats.curr_bytes, 32 * v.capacity() + 60);
- dhat::assert_eq!(stats.curr_bytes, v.get_heap_size());
+ // Non-binding: capacity may remain >= len on some allocators.
+ assert!(v.capacity() >= v.len());
+ let entry_size = core::mem::size_of::<BeaconEntry>();
+ let inner_bytes = v.iter().map(GetSize::get_heap_size).sum::<usize>();
+ dhat::assert_eq!(stats.curr_bytes, entry_size * v.capacity() + inner_bytes);
+ dhat::assert_eq!(stats.curr_bytes, v.get_heap_size());To support this, add the import near the other use lines:
use forest::interop_tests_private::beacon::BeaconEntry;
use get_size2::GetSize;
+use core::mem;🤖 Prompt for AI Agents
In interop-tests/tests/dhat_get_size_beacon_entry.rs around lines 25 to 41,
replace the hardcoded size 32 with size_of::<BeaconEntry>() to dynamically
compute element size, and replace the fixed 60 with a calculation of the sum of
element heap sizes for accuracy. Remove or modify the assertion that capacity
equals length after shrink_to_fit, since shrink_to_fit is not guaranteed to
reduce capacity exactly to length. Also, add the necessary import for size_of
near the other use statements.
Member
|
no green checkmark, no review! |
LesnyRumcajs
approved these changes
Aug 8, 2025
Member
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Nice, I didn't know about this crate. I guess it might've been useful for profiling, though I'm not sure it adds to any other tools I used.
elmattic
approved these changes
Aug 11, 2025
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 of changes
This PR adds integration test for
BeaconEntry::get_heap_sizeto compare the result withdhat::AllocstatsChanges introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
BeaconEntryinstances using heap profiling.BeaconEntry.Chores