Skip to content

fix: remove excess vec capacity in caches #5935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hanabi1224 merged 1 commit into from
Aug 18, 2025
Merged

fix: remove excess vec capacity in caches #5935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/blocks/tipset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,15 +241,15 @@ impl Tipset {
pub fn new<H: Into<CachingBlockHeader>>(
headers: impl IntoIterator<Item = H>,
) -> Result<Self, CreateTipsetError> {
let headers = NonEmpty::new(
let mut headers = NonEmpty::new(
headers
.into_iter()
.map(Into::<CachingBlockHeader>::into)
.sorted_by_cached_key(|it| it.tipset_sort_key())
.collect(),
)
.map_err(|_| CreateTipsetError::Empty)?;

headers.shrink_to_fit();
verify_block_headers(&headers)?;

Ok(Self {
Expand Down
3 changes: 2 additions & 1 deletion src/chain/store/chain_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,8 @@ impl MsgsInTipsetCache {
}
}

pub fn insert(&self, key: TipsetKey, value: Vec<ChainMessage>) {
pub fn insert(&self, key: TipsetKey, mut value: Vec<ChainMessage>) {
value.shrink_to_fit();
self.cache.push(key, value);
}

Expand Down
7 changes: 5 additions & 2 deletions src/state_manager/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,17 @@ impl EnabledTipsetDataCache {
}

impl TipsetReceiptEventCacheHandler for EnabledTipsetDataCache {
fn insert_receipt(&self, key: &TipsetKey, receipts: Vec<Receipt>) {
fn insert_receipt(&self, key: &TipsetKey, mut receipts: Vec<Receipt>) {
if !receipts.is_empty() {
receipts.shrink_to_fit();
self.receipt_cache.insert(key.clone(), receipts);
}
}
Comment on lines +206 to 211
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Don’t skip caching empty receipts; it causes repeated recomputation and changes semantics

Gating insertion on non-empty vectors changes behavior: callers that legitimately produce empty receipts will no longer be cached and may recompute on subsequent lookups. Empty results are still results and should be cached (negative caching).

Apply this change to always cache, while still shrinking:

-    fn insert_receipt(&self, key: &TipsetKey, mut receipts: Vec<Receipt>) {
-        if !receipts.is_empty() {
-            receipts.shrink_to_fit();
-            self.receipt_cache.insert(key.clone(), receipts);
-        }
-    }
+    fn insert_receipt(&self, key: &TipsetKey, mut receipts: Vec<Receipt>) {
+        receipts.shrink_to_fit();
+        self.receipt_cache.insert(key.clone(), receipts);
+    }

To check current usage patterns and potential reliance on empty caching, run:

🏁 Script executed:

#!/bin/bash
rg -n "insert_receipt\(|get_receipt_or_else\(" -A 3 -B 3

Length of output: 4422

Always cache empty receipts for proper negative caching

Currently both the handler and its caller short-circuit empty vectors, so empty receipt results are never stored—causing repeated recomputation. We need to:

• In src/state_manager/cache.rs, remove the if !receipts.is_empty() guard in insert_receipt so it always calls insert(...) after shrink_to_fit().
• In src/state_manager/mod.rs, remove the surrounding if !receipts.is_empty() check before calling insert_receipt so empty vectors get cached.

Proposed diffs:

--- a/src/state_manager/cache.rs
+++ b/src/state_manager/cache.rs
@@ -206,7 +206,6 @@ impl TipsetReceiptEventCacheHandler for EnabledTipsetDataCache {
-    fn insert_receipt(&self, key: &TipsetKey, mut receipts: Vec<Receipt>) {
-        if !receipts.is_empty() {
-            receipts.shrink_to_fit();
-            self.receipt_cache.insert(key.clone(), receipts);
-        }
-    }
+    fn insert_receipt(&self, key: &TipsetKey, mut receipts: Vec<Receipt>) {
+        receipts.shrink_to_fit();
+        self.receipt_cache.insert(key.clone(), receipts);
+    }
--- a/src/state_manager/mod.rs
+++ b/src/state_manager/mod.rs
@@ -462,5 +462,4 @@ impl StateManager {
-        && !receipts.is_empty()
-    {
-        self.receipt_event_cache_handler
-            .insert_receipt(key, receipts);
-    }
+        // always cache receipts, even if empty
+    {
+        self.receipt_event_cache_handler.insert_receipt(key, receipts);
+    }

This ensures that even an empty Vec<Receipt> is cached once and won’t trigger redundant recomputations in get_receipt_or_else.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/state_manager/cache.rs around lines 206-211, remove the `if
!receipts.is_empty()` guard so that after `receipts.shrink_to_fit()` the
function always calls `self.receipt_cache.insert(key.clone(), receipts);`; also
update src/state_manager/mod.rs at the call site (remove the surrounding `if
!receipts.is_empty()` check) so `insert_receipt` is invoked even for empty
Vec<Receipt> values, ensuring empty receipts are cached for negative caching.

Copy link
Copy Markdown
Contributor Author

@hanabi1224 hanabi1224 Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe empty vec indicates value computation is skipped, thus should not be cached. cc @akaladarshi to confirm

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


fn insert_events(&self, key: &TipsetKey, events_data: StateEvents) {
fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
if !events_data.events.is_empty() {
events_data.events.shrink_to_fit();
events_data.roots.shrink_to_fit();
self.events_cache.insert(key.clone(), events_data);
}
}
Comment on lines +213 to 219
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Same here: cache empty event sets to avoid unnecessary re-computation

As with receipts, events can legitimately be empty. Skipping insertion changes cache semantics and may increase CPU churn.

-    fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
-        if !events_data.events.is_empty() {
-            events_data.events.shrink_to_fit();
-            events_data.roots.shrink_to_fit();
-            self.events_cache.insert(key.clone(), events_data);
-        }
-    }
+    fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
+        events_data.events.shrink_to_fit();
+        events_data.roots.shrink_to_fit();
+        self.events_cache.insert(key.clone(), events_data);
+    }

You can also scan for call sites:

🏁 Script executed:

#!/bin/bash
rg -n "insert_events\(|get_events_or_else\(" -A 3 -B 3

Length of output: 3584

🏁 Script executed:

#!/bin/bash
rg -n "events_cache" -A5 -B5 src/state_manager/cache.rs

Length of output: 1649

🏁 Script executed:

#!/bin/bash
rg -n "fn insert_receipt" -A5 -B5 src/state_manager/cache.rs

Length of output: 1390

🏁 Script executed:

#!/bin/bash
rg -n "struct TipsetStateCache" -A5 -B5 .

Length of output: 1434

Always cache empty StateEvents to avoid redundant recomputation

Skipping insertion when events_data.events is empty means callers of get_events_or_else will recompute identical (empty) results on every miss. Align with insert_receipt’s behavior by always caching—even empty—event sets.

Points of attention:

  • File: src/state_manager/cache.rs
  • Method: EnabledTipsetDataCache::insert_events (lines 213–219)

Suggested diff:

-    fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
-        if !events_data.events.is_empty() {
-            events_data.events.shrink_to_fit();
-            events_data.roots.shrink_to_fit();
-            self.events_cache.insert(key.clone(), events_data);
-        }
-    }
+    fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
+        events_data.events.shrink_to_fit();
+        events_data.roots.shrink_to_fit();
+        self.events_cache.insert(key.clone(), events_data);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
if !events_data.events.is_empty() {
events_data.events.shrink_to_fit();
events_data.roots.shrink_to_fit();
self.events_cache.insert(key.clone(), events_data);
}
}
fn insert_events(&self, key: &TipsetKey, mut events_data: StateEvents) {
events_data.events.shrink_to_fit();
events_data.roots.shrink_to_fit();
self.events_cache.insert(key.clone(), events_data);
}
🤖 Prompt for AI Agents 
In src/state_manager/cache.rs around lines 213 to 219, change
EnabledTipsetDataCache::insert_events to always cache the provided StateEvents
(including when events is empty) instead of skipping insertion; remove the if
!events_data.events.is_empty() guard, keep the shrink_to_fit() calls on events
and roots, then always call self.events_cache.insert(key.clone(), events_data)
so empty results are cached like insert_receipt does.

Copy link
Copy Markdown
Contributor Author

@hanabi1224 hanabi1224 Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe empty vec indicates value computation is skipped, thus should not be cached. cc @aatifsyed to confirm

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Contributor

@aatifsyed aatifsyed Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I have enough context to confirm or deny! Is this a Rust question, a protocol question, or a library question? :))

Copy link
Copy Markdown
Contributor Author

@hanabi1224 hanabi1224 Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aatifsyed thanks for your reply! I actually meant to tag @akaladarshi on this sorry about it and have a great day!

Expand Down
Loading