Skip to content

Conversation

wlawt
Copy link
Collaborator

@wlawt wlawt commented Jul 23, 2025

Overview

Introduces a shared cache to benefit Readers whenever caching an account to speed up reading the contract state, as it'd use the same state consistently.

This is done by introducing a versioned LRU cache, which is a doubly-linked-list of entries and a mapping of keys to entries. The two main operations for this versioned LRU cache are:

  • get(key, target_snapshot_id) => value, which finds the entry matching the key (via the HashMap) with the largest snapshot_id <= target_snapshot_id, and moves the entry to the front.

  • set(key, snapshot_id, value) creates a new entry, puts it at the front of the linked list, and inserts into the HashMap. If the cache is full, it then finds the tail entry, removes it from its HashMap, and then pops it off the list.

The LRU always evicts the smallest snapshot_id. It also proactively purges entries that are marked as None (aka obsolete) by setting a min_snapshot_id.

The versioned LRU cache attempts to unify the contract_account_loc_cache and not have it at a per-transaction but instead managing it at the StorageEngine level.

Tests

  • Wrote unit tests for CacheManager
  • All current tests pass
  • CI

Copy link

linear bot commented Jul 23, 2025

CHAIN-1726 introduce CacheManager

be able to centrally manage the contract_account_loc_cache mapping at the Database level instead of a per-transaction level

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jul 23, 2025

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@wlawt wlawt force-pushed the chain-1726-cachemanager branch from d89de7e to c78c825 Compare July 23, 2025 14:00
@wlawt wlawt changed the title feat: have a shared contract_account_loc_cache refactor: have a shared contract_account_loc_cache Jul 23, 2025
@wlawt wlawt force-pushed the chain-1726-cachemanager branch from c78c825 to 2120dc8 Compare August 2, 2025 15:44
@wlawt wlawt changed the title refactor: have a shared contract_account_loc_cache feat: introduce versioned LRU cache Aug 2, 2025
@wlawt wlawt force-pushed the chain-1726-cachemanager branch 5 times, most recently from dfb94d5 to bba089a Compare August 4, 2025 13:54
@wlawt wlawt changed the title feat: introduce versioned LRU cache feat: introduce versioned LRU cache for contract_account_loc_cache Aug 4, 2025
src/cache.rs Outdated

// Find smallest snapshot_id for this key
let smallest = if let Some(versions) = self.entries.get(&tail_key) {
versions.iter().map(|e| e.snapshot_id).min()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can just get the first element, assuming that these are sorted by snapshot_id

src/cache.rs Outdated
let tail_idx = self.tail.unwrap();
let tail_key = self.lru[tail_idx].key.clone();

// Find smallest snapshot_id for this key
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rationale for removing the item with the minimum snapshot_id is that this ensures that there are no version gaps in the data for a specific key/path (temporal coherence). This is what allows us to safely look up values by snapshot, and be able to fall back to not using the cache if there is definitely no match.
This invariant must hold through all permutations of get and set. For a given key/path, this means that we can only append new versions to the right side and pop old versions from the left.

  • A writer transaction will always have the latest db version and append to the right.
  • A reader transaction may be at the latest or an older db version (e.g. in a long-running RPC query). When inserting an entry into the cache, it would either already be cached (from a reader or writer), not be cached due to the key not being accessed since the db was opened, or not be cached as all versions were evicted from the LRU. Only the last case here is risky, as it may not actually be the latest version of the specific key/value pair. We can solve this by tracking the monotonically-increasing max_evicted_version for the cache, and forbidding insertion of any entry with a version less than this.

@wlawt wlawt force-pushed the chain-1726-cachemanager branch from 5e1eab0 to 8247421 Compare August 8, 2025 16:29
@wlawt wlawt force-pushed the chain-1726-cachemanager branch from 8247421 to 3e64c53 Compare August 8, 2025 16:34
@wlawt wlawt marked this pull request as ready for review August 12, 2025 13:34
@wlawt wlawt requested a review from BrianBland August 12, 2025 13:34
src/cache.rs Outdated
if let Some(min_id) = self.min_snapshot_id {
if let Some(versions) = self.entries.get_mut(key) {
if let Some(idx) = versions.iter().position(|e| e.snapshot_id >= min_id) {
versions.drain(0..idx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We actually need to keep at least one version with snapshot_id <= min_id in order to ensure that we can still query values at min_id

src/cache.rs Outdated
if let Some(min_id) = self.min_snapshot_id {
if let Some(versions) = self.entries.get_mut(key) {
if let Some(idx) = versions.iter().position(|e| e.snapshot_id >= min_id) {
versions.drain(0..idx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could end up dropping the head or tail of the LRU, which would currently cause the cache to get into a bad state. We also need to count any entries removed, and adjust the size accordingly.

My general suggestion is to call a shared remove method on any entries we're evicting

Comment on lines +264 to +265
unsafe impl Send for Entry {}
unsafe impl Send for VersionedLru {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure if there's a cleaner way to do this

@wlawt wlawt requested a review from BrianBland August 20, 2025 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants