feat: track TipsetStateCache size with prometheus metrics #5912
Conversation
WalkthroughThis set of changes refactors and enhances cache management and memory size tracking across several modules. It replaces the LRU cache implementation in the state manager with a size-tracking variant using a new Changes
Sequence Diagram(s)sequenceDiagram
participant StateManager
participant TipsetStateCache
participant SizeTrackingLruCache
participant MetricsRegistry
StateManager->>TipsetStateCache: lookup_or_insert(key, value)
TipsetStateCache->>SizeTrackingLruCache: get_cloned(key)
alt Not in cache
TipsetStateCache->>SizeTrackingLruCache: push(key, value)
end
SizeTrackingLruCache->>MetricsRegistry: record_size(cache_name)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/state_manager/tests.rs (1)
447-452: Add newline at end of file.#[test] fn test_state_output_get_size() { let s = StateOutputValue::default(); assert_eq!(s.get_size(), std::mem::size_of_val(&s)); } +
📜 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 (13)
Cargo.toml(1 hunks)src/blocks/tipset.rs(2 hunks)src/cid_collections/mod.rs(3 hunks)src/cid_collections/small_cid_vec.rs(5 hunks)src/shim/executor.rs(3 hunks)src/state_manager/cache.rs(5 hunks)src/state_manager/mod.rs(4 hunks)src/state_manager/tests.rs(1 hunks)src/utils/cache/lru.rs(3 hunks)src/utils/cache/mod.rs(1 hunks)src/utils/get_size/mod.rs(2 hunks)src/utils/misc/mod.rs(1 hunks)src/utils/misc/type.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
src/utils/cache/lru.rssrc/shim/executor.rssrc/blocks/tipset.rssrc/cid_collections/small_cid_vec.rssrc/utils/get_size/mod.rssrc/cid_collections/mod.rssrc/state_manager/mod.rs
📚 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:
src/shim/executor.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:
src/utils/get_size/mod.rssrc/cid_collections/mod.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: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
Cargo.toml (1)
54-54: convert_case dependency is up-to-date and secure
- Cargo.toml (line 54):
convert_case = "0.8"corresponds to the latest stable version (0.8.0) as of August 2025.- No public security advisories or vulnerabilities have been reported for this crate.
No further action needed. For production assurance, periodically review the RustSec Advisory Database or crates.io for any future advisories.
src/utils/misc/mod.rs (1)
11-12: LGTM - Clean module additionThe addition of the
r#typemodule with glob re-export is well-structured. Using the raw identifierr#typeis the correct approach sincetypeis a reserved keyword in Rust.src/utils/cache/mod.rs (1)
5-5: LGTM - Appropriate API expansionAdding
LruValueConstraintsto the public exports is consistent with the trait being used as a generic bound in other modules, particularly insrc/state_manager/cache.rs.src/utils/cache/lru.rs (2)
38-40: LGTM - Better trait namingThe rename from
ValueConstraintstoLruValueConstraintsimproves clarity and specificity. The trait definition and blanket implementation are correctly updated.
46-46: LGTM - Consistent trait reference updatesAll references to the trait in generic bounds have been consistently updated to use the new
LruValueConstraintsname.Also applies to: 56-56, 169-169
src/cid_collections/mod.rs (4)
41-41: LGTM - Clean imports for GetSize integrationThe import changes support the GetSize trait integration. The glob import
super::*is reasonable given the expanded usage, and theget_size2::GetSizeimport is correctly placed.Also applies to: 44-44
48-48: LGTM - Appropriate GetSize trait derivationsThe
GetSizetrait derivations for bothCidV1DagCborBlake2b256andUncompactableare correctly implemented. These structs are suitable for the derive macro as they are simple wrapper types.Also applies to: 103-103
106-107: Good use of get_size ignore attributeThe
#[get_size(ignore)]attribute on theinner: Cidfield is appropriate sinceCidlikely doesn't implementGetSize. This ensures the size calculation focuses only on the wrapper overhead.
159-165: LGTM - Comprehensive test for GetSize implementationThe test verifies that
Uncompactable::get_size()returns the size of its innerCidfield, which is the expected behavior given the#[get_size(ignore)]attribute. The test correctly usesstd::mem::size_of_valfor comparison.src/shim/executor.rs (3)
8-8: LGTM!The import of
GetSizetrait and helper function is necessary for the size tracking implementations added below.
137-145: LGTM!The
GetSizeimplementation forReceiptcorrectly calculates heap size by focusing on thereturn_data.bytes()across all variants, which represents the main heap-allocated content in receipts.
347-358: LGTM!The
GetSizeimplementation forStampedEventcorrectly calculates heap size by summing the heap sizes ofkeyandvaluefields in each entry using the helper function. The implementation properly handles both V3 and V4 variants.src/blocks/tipset.rs (3)
9-15: LGTM!Good import organization and the
GetSizeimport is necessary for the trait derivation added toTipsetKey.Also applies to: 21-21
33-33: LGTM!Adding
GetSizederivation toTipsetKeyis correct and supports the cache size tracking functionality. The underlyingSmallCidNonEmptyVecalso implementsGetSize, ensuring proper size calculation.
104-107: LGTM!Excellent memory optimization! Calling
shrink_to_fit()before conversion reduces heap usage for cachedTipsetKeyinstances. The rationale is well-documented and the approach is sound sinceTipsetKeyis immutable.src/utils/misc/type.rs (2)
6-10: LGTM!The
short_type_namefunction implementation is well-designed with efficient regex pattern matching and appropriate use ofCow<'static, str>for memory efficiency. The regex correctly strips module paths while preserving type structure.
18-31: LGTM!Excellent test coverage! The tests validate the function works correctly for simple types, nested generics, and lifetime parameters. The expected outputs confirm the regex properly strips module paths while preserving essential type information.
src/cid_collections/small_cid_vec.rs (4)
5-5: LGTM!The
GetSizeimplementation forSmallCidNonEmptyVeccorrectly uses the helper function to calculate heap size of the underlyingNonEmpty<SmallCid>collection.Also applies to: 25-29
82-82: LGTM!Adding
GetSizederivation toSmallCidenum is correct and will properly handle heap size calculation for bothInlineandIndirectvariants.
85-85: LGTM!Excellent optimization! Removing the
Boxindirection from theIndirectvariant reduces unnecessary heap allocation while maintaining the same external behavior.
96-96: LGTM!The
Fromimplementation updates correctly reflect the removal ofBoxindirection. The conversions maintain the same external behavior while working with the optimized internal representation.Also applies to: 105-105
src/state_manager/mod.rs (2)
98-104: Verify the appropriateness of size calculation helpers for StateEvents fields.The
vec_heap_size_helperis used foreventsfield which contains nested vectors. This should correctly calculate heap size forVec<Vec<StampedEvent>>assumingStampedEventimplementsGetSize.The
vec_with_stack_only_item_heap_size_helperis used forrootsfield of typeVec<Option<Cid>>. SinceOption<Cid>is a stack-only type (Cid contains no heap allocations), this choice is appropriate.
114-120: Ignore the#[get_size(ignore)]on Cid fieldsThe Cid type itself doesn’t implement
GetSize, so omitting these fields is required for the derive to compile. If you do want their fixed stack size included, you’ll need to add aGetSizeimpl (or wrap Cid in a newtype with a default impl) rather than dropping theignorehere.• Location:
src/state_manager/mod.rslines 114–120
• Rationale: noimpl GetSize for Cidexists, so removingignorewill cause a compile errorLikely an incorrect or invalid review comment.
src/state_manager/tests.rs (1)
21-451: Well-structured and comprehensive test coverage!The tests thoroughly cover:
- State lookup scenarios with various edge cases
- Cache update behavior for both receipts and events
- Size calculation for StateOutputValue
The test utilities and setup helpers are well-organized and make the tests readable.
src/utils/get_size/mod.rs (1)
68-76: Good test coverage for heap size calculation.The test correctly verifies that the heap size calculation accounts for the full capacity of the vector, not just the length.
src/state_manager/cache.rs (3)
45-51: Well-implemented cache naming for metrics.The
cache_name()method cleverly generates a descriptive snake_case name from the type parameter, which will produce meaningful metric names liketipset_state_cache_state_output_value.
17-42: Excellent integration of size-tracking cache with metrics.The replacement of
LruCachewithSizeTrackingLruCacheand the requirement for values to implementLruValueConstraintsenables proper memory tracking. The automatic metrics registration with type-based naming is a clean solution.
335-537: Comprehensive concurrent access test coverage!The tests thoroughly validate:
- Basic cache functionality
- Concurrent access to the same key (ensuring single computation)
- Concurrent access to different keys
- Enabled vs disabled cache behavior
These tests provide confidence in the thread-safety and correctness of the cache implementation.
4753e1a to
eaa9828
Compare
| impl From<NonEmpty<Cid>> for TipsetKey { | ||
| fn from(value: NonEmpty<Cid>) -> Self { | ||
| fn from(mut value: NonEmpty<Cid>) -> Self { | ||
| // When `value.capacity() > value.len()`, it takes more heap memory. |
| #[derive(Debug, Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd, GetSize)] | ||
| #[repr(transparent)] | ||
| pub struct Uncompactable { | ||
| #[get_size(ignore)] |
There was a problem hiding this comment.
Ignore means heap size = 0, not intuitive naming
Summary of changes
Changes introduced in this pull request:
LruCachewithSizeTrackingLruCachemetrics on mainnet
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests