starknet_patricia_storage: dont cache reads in cached storage

[nimrod-starkware]

Note

Medium Risk
Changes storage caching semantics and LRU behavior, which can materially impact performance and cache hit rates; correctness risk is limited since underlying reads/writes are unchanged.

Overview
Disables populating the CachedStorage LRU cache on get/mget reads, so only existing cache entries are served and reads no longer “warm” the cache.

Read paths now use LruCache::peek (no LRU update) and remove cache.put(...) on fetch, while write paths keep optionally updating the cache via cache_on_write.

^{Written by Cursor Bugbot for commit cd93f36. This will update automatically on new commits. Configure here.}

[reviewable-StarkWare]

This change is 

[nimrod-starkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• starknet_patricia_storage: non mut get in storage trait #12123 : 2 dependent PRs (#12165 , #12252 )
• starknet_patricia_storage: dont cache reads in cached storage #12251 👈 (View in Graphite)
• starknet_patricia_storage: atomic counters for read stats #12122
• main-v0.14.2

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ArielElp]

@ArielElp reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware).

[dorimedini-starkware]

@dorimedini-starkware reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ArielElp and @nimrod-starkware).

---

a discussion (no related file):
if you merge this, without the rest of the stack, and we tag the branch and deploy a version - won't we get a huge performance hit?

[nimrod-starkware]

@nimrod-starkware made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on ArielElp and dorimedini-starkware).

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

if you merge this, without the rest of the stack, and we tag the branch and deploy a version - won't we get a huge performance hit?

Can you explain wdym?
This is a preparation to make the storage trait non-mut in get , mget so it can be shared between futures.

[dorimedini-starkware]

@dorimedini-starkware reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on ArielElp and nimrod-starkware).

---

a discussion (no related file):

Previously, nimrod-starkware wrote…

Can you explain wdym?
This is a preparation to make the storage trait non-mut in get , mget so it can be shared between futures.

this PR stops caching reads. if this is merged and the rest is not, we get a worse cache. right?

[nimrod-starkware]

@nimrod-starkware made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on ArielElp and dorimedini-starkware).

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

this PR stops caching reads. if this is merged and the rest is not, we get a worse cache. right?

ah, right.
We cache writes + upper in the stack I collect siblings and cache them.
This is over main-v0.14.2 so I wouldn't be worried

[dorimedini-starkware]

@dorimedini-starkware made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on ArielElp and nimrod-starkware).

---

a discussion (no related file):

Previously, nimrod-starkware wrote…

ah, right.
We cache writes + upper in the stack I collect siblings and cache them.
This is over main-v0.14.2 so I wouldn't be worried

how about I unblock this PR when all PRs up to and including the sibling-cache PR are green and ready to merge?

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on nimrod-starkware).

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

how about I unblock this PR when all PRs up to and including the sibling-cache PR are green and ready to merge?

also, should you be creating a separate "PatriciaCachedStorage" that makes the cache logic more patricia-specific...?
or at least rename the current CachedStorage so it's clear that this isn't a simple LRU cache anymore?

---

crates/starknet_patricia_storage/src/map_storage.rs line 232 at r3 (raw file):

    async fn get(&mut self, key: &DbKey) -> PatriciaStorageResult<Option<DbValue>> {
        self.reads.fetch_add(1, Ordering::Relaxed);
        if let Some(cached_value) = self.cache.peek(key) {

why? why not upgrade the item's LRU status?

Code quote:

self.cache.peek(key)

[nimrod-starkware]

@nimrod-starkware made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on dorimedini-starkware).

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

also, should you be creating a separate "PatriciaCachedStorage" that makes the cache logic more patricia-specific...?
or at least rename the current CachedStorage so it's clear that this isn't a simple LRU cache anymore?

I agree, PatriciaCachedStorage with proper doc is better.
Sure, you can block.

---

crates/starknet_patricia_storage/src/map_storage.rs line 232 at r3 (raw file):

Previously, dorimedini-starkware wrote…

why? why not upgrade the item's LRU status?

The motivation in the first few PR's it to replace &mut self with &self in get & mget.
This allows us to share a reference to storage between threads.
Upgrading entry in LRU requires a mutable reference.

[dorimedini-starkware]

@dorimedini-starkware resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on nimrod-starkware).