[mpt] count hits, misses and evictions in the trie node cache - #2504
[mpt] count hits, misses and evictions in the trie node cache#2504maxkozlovsky wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds cache hit/miss/eviction metrics and byte-occupancy tracking to the LRU and trie node caches.
Changes:
- Integrates
CacheStatsand non-mutatingcontains()checks. - Corrects
NodeCacheoverwrite accounting. - Adds tests for metrics, occupancy, and cache behavior.
Review finding: clear() leaves active_list_ populated, causing stale values and false eviction statistics on subsequent inserts. This is a moderate P2 issue with 2 votes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Summary |
|---|---|
category/mpt/test/node_lru_cache_test.cpp |
Tests NodeCache metrics and accounting. |
category/mpt/node_cache.hpp |
Tracks cache statistics and used bytes. |
category/mpt/find_notify_fiber.cpp |
Uses non-mutating cache existence checks. |
category/core/lru/static_lru_test.cpp |
Tests LRU statistics and behavior. |
category/core/lru/static_lru_cache.hpp |
Adds statistics and contains(); clear() requires correction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| CacheStatsSnapshot stats() const noexcept | ||
| { | ||
| return stats_.snapshot(); | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the wiring of CacheStats into static_lru_cache and NodeCache, the new contains() predicate, and the two ancillary fixes called out in the PR description (insert() net-change accounting, dropping the leaky using Base::clear).
Walked through all insert() cases (fresh into a free slot, overwrite of an existing key, LRU-tail reuse when the map is full, and a single node oversized vs max_bytes) — bytes accounting and eviction counting are correct on every path, with no double-counted eviction across Base::insert and evict_until_under_limit. All existing callers of NodeCache::insert ignore the returned iterator, so narrowing the return to void is safe. All remaining call sites of find() on NodeCache still use the accessor (not existence checks), so no other spot needs the contains() swap. The stats atomics are relaxed-order, which is right for stats and correctly leaves static_lru_cache non-copyable/non-movable — no code in the tree copies or moves it.
Verdict: CORRECT
🤖 Generated with Claude Code
Wire CacheStats into static_lru_cache and NodeCache. a follow-up exports them over the triedb FFI. Two related fixes in NodeCache while making its occupancy observable. insert() charged the full size of the incoming node before evicting, so an overwrite -- which replaces an entry already accounted for -- could evict entries to make room it did not need; it now applies the net change. And NodeCache no longer re-exports Base::clear, which left used_bytes_ stale while size() read zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2a08bbe to
bcbc643
Compare
Wire CacheStats into static_lru_cache and NodeCache. a follow-up exports them over the triedb FFI.
Two related fixes in NodeCache while making its occupancy observable. insert() charged the full size of the incoming node before evicting, so an overwrite -- which replaces an entry already accounted for -- could evict entries to make room it did not need; it now applies the net change. And NodeCache no longer re-exports Base::clear, which left used_bytes_ stale while size() read zero.