Add always-on cache hit/miss counters, and export the node cache over FFI - #2492
Add always-on cache hit/miss counters, and export the node cache over FFI#2492maxkozlovsky wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes cache hit/miss/eviction counters always-on across multiple caches on the state read path, adds a shared CacheStats/windowing mechanism for cumulative vs per-block reporting, and exposes trie node-cache stats (including used_bytes) over the TrieDB FFI for consumption by monad-node.
Changes:
- Add
CacheStats+CacheStatsWindowprimitives and integrate them intoLruCache,static_lru_cache,LruWeightCache, andNodeCache. - Plumb per-block “window start” via the new pure-virtual
Db::begin_block_stats()and call it from runloops; add varcode-cache block-window stats to VM logging. - Extend
monad-triedbFFI (C header + C++ implementation + Rust wrapper) to export trie node-cache counters and occupancy.
Verdict: NEEDS CHANGES
🤖 Generated with Claude Code
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vm/unit/lru_weight_cache_tests.cpp | Adds tests asserting hit/miss/eviction accounting and non-destructive stats reads for LruWeightCache. |
| rust/crates/monad-triedb/src/lib.rs | Adds Rust-side NodeCacheStats and TriedbHandle::node_cache_stats() wrapper. |
| rust/crates/monad-triedb/src/ffi.rs | Re-exports new bindgen symbols for node-cache stats. |
| rust/crates/monad-triedb/src/ffi.cpp | Implements triedb_node_cache_stats_read() on the C++ side. |
| rust/crates/monad-triedb/include/ffi.h | Declares triedb_node_cache_stats struct + triedb_node_cache_stats_read() for FFI consumers. |
| category/vm/vm.hpp | Adds per-block varcode-cache stats window start + logging helper. |
| category/vm/varcode_cache.hpp | Adds cumulative + windowed stats plumbing for the varcode cache. |
| category/vm/utils/lru_weight_cache.hpp | Integrates CacheStats into LruWeightCache and records evictions. |
| category/vm/compiler.hpp | Exposes varcode cache window control/stats via Compiler. |
| category/statesync/statesync_server_context.hpp | Implements empty begin_block_stats() override (no independent stats). |
| category/mpt/test/node_lru_cache_test.cpp | Adds NodeCache tests for counters and used_bytes() tracking. |
| category/mpt/node_cache.hpp | Adds eviction counting, exposes used_bytes(), and re-exports stats()/contains(). |
| category/mpt/find_notify_fiber.cpp | Fixes a false “miss” accounting by using non-counting contains() instead of find(). |
| category/execution/runloop/runloop_monad.cpp | Starts per-block stats windows and appends varcode-cache stats to block log line. |
| category/execution/runloop/runloop_monad_ethblocks.cpp | Starts per-block stats windows and appends varcode-cache stats to block log line. |
| category/execution/runloop/runloop_interface_monad.cpp | Plumbs begin_block_stats() down into the runloop TrieDB wrapper. |
| category/execution/runloop/runloop_ethereum.cpp | Starts per-block stats windows and appends varcode-cache stats to block log line. |
| category/execution/ethereum/db/trie_rodb.hpp | Adds empty begin_block_stats() override (no stats). |
| category/execution/ethereum/db/trie_db.hpp | Declares TrieDb::begin_block_stats(). |
| category/execution/ethereum/db/trie_db.cpp | Moves per-block stat reset into begin_block_stats() instead of print_stats(). |
| category/execution/ethereum/db/partial_trie_db.hpp | Adds empty begin_block_stats() override (no stats). |
| category/execution/ethereum/db/db.hpp | Makes begin_block_stats() pure virtual on the Db interface. |
| category/execution/ethereum/db/db_cache.hpp | Adds cache stat snapshots + per-block windows and formats block-window stats in log output. |
| category/execution/ethereum/db/db_cache_test.cpp | Adds tests for LRU accounting, block-window deltas, and overlay-hit non-accounting. |
| category/core/lru/static_lru_test.cpp | Adds tests for counters + contains() non-accounting + overwrite non-eviction. |
| category/core/lru/static_lru_cache.hpp | Adds CacheStats, contains(), and stats() to static_lru_cache. |
| category/core/lru/lru_cache.hpp | Removes macro-gated stats and replaces with always-on CacheStats snapshots. |
| category/core/lru/lru_cache_test.cpp | Adds comprehensive tests for LruCache stats behavior and CacheStatsWindow. |
| category/core/lru/cache_stats.hpp | Introduces shared CacheStatsSnapshot, monotonic delta, and CacheStatsWindow/CacheStats. |
| category/core/CMakeLists.txt | Adds lru/cache_stats.hpp to the core library build. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b285a8a to
f358e6e
Compare
|
@claude review fixes |
There was a problem hiding this comment.
Reviewed the two fix commits (b285a8a → f358e6e):
NodeCache::insert()reorder (category/mpt/node_cache.hpp) — resolves the overwrite-evicts-neighbors bug. New sequence (Base::insert → net byte adjustment →evict_until_under_limit) makes an overwrite a zero-net-byte replacement, so no spurious byte-budget eviction is triggered. Byte accounting stays correct across all three Base::insert outcomes (in-place replace, slot-eviction, plain insert). The signature change tovoidis safe — none of the callers (find_notify_fiber.cpp,find_request_sender.hpp,db.cpp) used the returned iterator.static_cast<uint64_t>at the FFI marshalling — makes thesize_t → uint64_twidening explicit at the ABI boundary.- New test
overwriting_a_key_does_not_evict_to_make_room— pins the invariant on a full 2-slot cache: same-key re-insert leavesused_bytes,size,evictions, and the sibling entry untouched.
No new findings.
Verdict: CORRECT
🤖 Generated with Claude Code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (1)
category/core/lru/static_lru_cache.hpp:147
static_lru_cache::clear()currently only callsmap_.clear()and leavesactive_list_/free_list_unchanged. After clearing a previously-full cache,free_list_will still be empty, so the nextinsert()falls into the “reuse the last node in active_list_” path and records an eviction even though the cache was just cleared. With always-on stats this makesevictionsmisleading (clears can look like capacity pressure). Consider resetting the lists inclear()(move all nodes back tofree_list_, emptyactive_list_, and reset node keys/values as needed) so inserts after a clear don’t count as evictions.
void clear() noexcept
{
map_.clear();
}
The LRU caches had no usable instrumentation. LruCache carried a stats
facility behind MONAD_LRU_CACHE_STATS, a macro defined nowhere in the tree
or the build, and static_lru_cache, LruWeightCache and NodeCache had none at
all, so cache sizing could not be evaluated on a running node.
Replace the dead facility with a shared CacheStats — three relaxed atomics —
embedded in all three cache templates and exposed two ways:
stats() cumulative totals, for a metrics scraper
block_stats() activity since begin_block_stats(), for the block metrics
log line
Reads no longer reset the counters. The old print_stats() cleared on read,
which gave per-block deltas at the cost of making the totals unscrapeable;
a baseline snapshot provides both. Db::begin_block_stats() is pure so that an
implementation reporting stats cannot forget to open the window and silently
print since-construction totals on a per-block line. TrieDb's own counters
move onto the same window, so the whole __exec_block line covers one interval.
find() records a lookup, so the existence assertions that used it now call
contains() instead; otherwise every completed disk read booked a miss.
NodeCache additionally reports used_bytes(), which with size() shows whether
its byte budget or its AVERAGE_NODE_SIZE-derived slot count is binding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add triedb_node_cache_stats_read -> TriedbHandle::node_cache_stats(), mirroring triedb_storage_stats_read, so monad-node can export the trie node cache as Prometheus metrics. Each triedb handle owns an independent NodeCache, so these are per-handle and not comparable across handles. used_bytes and entries accompany the hit/miss/eviction counts because the cache is bounded twice — by node_lru_max_mem and by the slot count derived from it — and only the pair shows which bound is binding. Unlike the storage-stats call it mirrors, this one returns a bool and the Rust wrapper returns Option: all-zero is a legitimate reading for an idle cache, so it cannot double as an error the way a zero disk capacity can. The counters also cover only the async read and traverse paths; triedb_read is blocking and consults no cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f358e6e to
8c3c3b4
Compare
Adds always-on hit/miss/eviction counters to the four caches on the state read
path, and exports the trie-node cache over the triedb FFI so monad-node can
publish it. Cache sizing could not be evaluated on a running node before this:
LruCachecarried a stats facility behindMONAD_LRU_CACHE_STATS, a macrodefined nowhere in the tree or the build, and
static_lru_cache,LruWeightCacheandNodeCachehad none at all.Design
A shared
CacheStats(three relaxed atomics) embedded in the three cachetemplates, exposed two ways:
stats()— cumulative totals, for a metrics scraperblock_stats()— activity sincebegin_block_stats(), for the block metricslog line
The old
print_stats()cleared the counters on read. That gave per-blockdeltas at the cost of making the totals unscrapeable, and two readers would
steal counts from each other. A baseline snapshot provides both views without
either reader disturbing the other.
Db::begin_block_stats()is pure rather than a defaulted no-op: animplementation that reports stats but forgets to open the window would print
since-construction totals on a per-block line, which looks more plausible than
the real thing. The three implementations with no stats define it empty
deliberately.
NodeCacheadditionally reportsused_bytes(). The cache is bounded twice —by
node_lru_max_memand by the slot count derived from it viaAVERAGE_NODE_SIZE— and onlyused_bytes()againstsize()shows which ofthe two is binding.
The FFI returns
bool/Option, deliberately unlike thetriedb_storage_stats_readit otherwise mirrors: all-zero is a legitimatereading for an idle cache, so it cannot double as an error the way a zero disk
capacity can. Its scope is also narrower than the name suggests and is
documented as such — only the async read and traverse paths consult the cache;
triedb_readis blocking and uncached.Bug fixed along the way
find()records a lookup, so using it as an existence predicate corrupts thecounters.
find_notify_fiber.cppassertedfind(...) == falseimmediatelybefore inserting, and
MONAD_ASSERThas noNDEBUGguard, so every completeddisk read booked a phantom miss. Added a non-counting
contains()and used itthere. Worth remembering whenever counters are added to a container.
NodeCachealso stopped re-exportingBase::clear, which leftused_bytes_stale while
size()read zero — harmless while unreachable, but the newaccessor would have published the inconsistency.
Testing
LruCachehad no test file at all; this adds one.lru_cache_test— hit/miss/eviction counting; that reads do not resetcounters (the deliberate reversal of the old behaviour); that
clear()leaves counters intact, which is the invariant keeping the window
subtraction from underflowing; that a backwards delta saturates instead of
wrapping to ~1.8e19.
static_lru_test— same counting, plus thatcontains()does not count as alookup (pins the bug above).
node_lru_cache_test— counters acrossNodeCache's own byte-budgeteviction loop, and
used_bytes()tracking.db_cache_test— that proposal-overlay hits are not charged to the LRUcounters, that cumulative and per-block views diverge correctly, and that a
second block with no reads reports zero rather than the first block's totals.
lru_weight_cache_tests— counting, non-destructive reads, and thatclear()preserves counters for the storage tier.
Not covered: the counters are not exercised concurrently, and the FFI
marshalling has no test — matching
triedb_storage_stats_read, which the cratealso leaves untested for want of a db-handle fixture.
A monad-bft PR consuming the new FFI follows, and needs this merged and the
submodule bumped first.