starknet_patricia_storage: atomic counters for read stats

[nimrod-starkware]

Note

Low Risk
Small, localized change affecting only in-memory stats counters; no persistence, storage semantics, or key/value handling logic is modified.

Overview
Switches CachedStorage read statistics (reads, cached_reads) from u128 fields to AtomicU64 counters, updating get/mget to use atomic fetch_add.

Adjusts stats reporting/reset to load/store with explicit memory ordering, and removes the now-obsolete total_reads/total_cached_reads accessors (writes stats remain non-atomic).

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

[reviewable-StarkWare]

This change is 

[nimrod-starkware]

• 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
• starknet_patricia_storage: atomic counters for read stats #12122 👈 (View in Graphite)
• main-v0.14.2

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

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[nimrod-starkware]

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

---

crates/starknet_patricia_storage/src/map_storage.rs line 79 at r1 (raw file):

Previously, ArielElp wrote…

Change to:

use std::sync::atomic::{AtomicU64, Ordering};

And update via

        if found {
            self.hits.fetch_add(1, Ordering::Relaxed);
            Some(value)
        } else {
            self.misses.fetch_add(1, Ordering::Relaxed);
            None
        }

Ordering::Relaxed guarantees eventual correctness AFAIU, so shouldn't be significant compare to regular addition (Cursor says 1 vs 10-20 cycles for addition)

Done.

[ArielElp]

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

---

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

    fn reset_stats(&mut self) -> PatriciaStorageResult<()> {
        self.reads.store(0, Ordering::Relaxed);

Non blocking, consider something stricter here to make sure stats are reset

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

[dorimedini-starkware]

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

---

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

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

what is Ordering?
didn't really deep-dive into the docs, and corrupting this value is not so bad (it's just for statistics), but I would still like to understand why Relaxed is the choice here

Code quote:

Ordering::Relaxed

---

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

        }

        self.reads.fetch_add(1, Ordering::Relaxed);

why did this move down? now you only count a read if it is not cached.
if this is on purpose, you need to do what the bugbot says (change how cache hit rate is computed)

Code quote:

self.reads.fetch_add(1, Ordering::Relaxed);

---

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

        }

        let n_keys = u64::try_from(keys.len()).expect("keys length should fit in u64");

why count in every iteration?

Suggestion:

        for (index, key) in keys.iter().enumerate() {
            if let Some(cached_value) = self.cache.get(key) {
                values[index] = cached_value.clone();
            } else {
                keys_to_fetch.push(*key);
                indices_to_fetch.push(index);
            }
        }

        let n_keys = u64::try_from(keys.len()).expect("keys length should fit in u64");
        let cached_reads = u64::try_from(keys.len() - keys_to_fetch.len()).expect("usize should fit in u64");

[nimrod-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

what is Ordering?
didn't really deep-dive into the docs, and corrupting this value is not so bad (it's just for statistics), but I would still like to understand why Relaxed is the choice here

IIUC it means that the increment will happen eventually, not neccesarily right away.
There is a trade off between how quickly you want the operation to be done and the accuracy of the counter value at any point.
This is the most relaxed version - might make stats inaccurate but not block for a long time when calling fetch_add().

---

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

Previously, dorimedini-starkware wrote…

why did this move down? now you only count a read if it is not cached.
if this is on purpose, you need to do what the bugbot says (change how cache hit rate is computed)

Done.

---

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

Previously, dorimedini-starkware wrote…

why count in every iteration?

Done.

---

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

Previously, ArielElp wrote…

Non blocking, consider something stricter here to make sure stats are reset

Done

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware).

---

crates/starknet_patricia_storage/src/map_storage.rs line 296 at r4 (raw file):

    fn get_stats(&self) -> PatriciaStorageResult<Self::Stats> {
        let reads = u128::from(self.reads.load(Ordering::Acquire));
        let cached_reads = u128::from(self.cached_reads.load(Ordering::Acquire));

why are these now stricter?
does the fact that you can call get_stats several times without calling reset_stats once matter in this Acquire-Release pattern?

Code quote:

        let reads = u128::from(self.reads.load(Ordering::Acquire));
        let cached_reads = u128::from(self.cached_reads.load(Ordering::Acquire));

[nimrod-starkware]

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

---

crates/starknet_patricia_storage/src/map_storage.rs line 296 at r4 (raw file):

Previously, dorimedini-starkware wrote…

why are these now stricter?
does the fact that you can call get_stats several times without calling reset_stats once matter in this Acquire-Release pattern?

They are stricter to provide accurate stats whenever get_stats is called.

[dorimedini-starkware]

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

---

crates/starknet_patricia_storage/src/map_storage.rs line 296 at r4 (raw file):

Previously, nimrod-starkware wrote…

They are stricter to provide accurate stats whenever get_stats is called.

does the fact that you can call get_stats several times without calling reset_stats once matter in this Acquire-Release pattern?

[nimrod-starkware]

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

---

crates/starknet_patricia_storage/src/map_storage.rs line 296 at r4 (raw file):

Previously, dorimedini-starkware wrote…

does the fact that you can call get_stats several times without calling reset_stats once matter in this Acquire-Release pattern?

it should be ok, verified with cursor

[dorimedini-starkware]

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

---

crates/starknet_patricia_storage/src/map_storage.rs line 296 at r4 (raw file):

Previously, nimrod-starkware wrote…

it should be ok, verified with cursor

did cursor point you to docs...? not that docs can't lie, but cursor can lie for sure

[nimrod-starkware]

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

---

crates/starknet_patricia_storage/src/map_storage.rs line 296 at r4 (raw file):

Previously, dorimedini-starkware wrote…

did cursor point you to docs...? not that docs can't lie, but cursor can lie for sure

Went over this and this. It should be fine.

[dorimedini-starkware]

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