triedb: expose node-cache counters via FFI - #2506
Open
maxkozlovsky wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds per-handle trie node-cache statistics through the Rust/C++ FFI for Prometheus metrics.
Changes:
- Adds
NodeCacheStatsandTriedbHandle::node_cache_stats(). - Exposes the C ABI reader and generated Rust bindings.
- Reports cache counters, memory usage, and entry counts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Summary | Final review comments |
|---|---|---|
rust/crates/monad-triedb/src/lib.rs |
Adds the public stats type and accessor. | [P2] nit (2 votes): Document synchronous traversal as uncached because it bypasses AsyncContext::node_cache and may report zero stats. |
rust/crates/monad-triedb/src/ffi.rs |
Exposes generated FFI bindings. | No final comments. |
rust/crates/monad-triedb/src/ffi.cpp |
Implements cache-stat extraction. | [P1] critical (2 votes): Snapshotting used_bytes() and size() can race with async cache mutations because AsyncContext is not thread-safe. Serialize snapshots or add synchronization/atomic state, and document calling discipline. |
rust/crates/monad-triedb/include/ffi.h |
Declares the C ABI contract. | [P2] nit (2 votes): Clarify that synchronous traversal is not cache-backed; only the async traversal paths use the cache. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+70
to
+72
| // Covers triedb_async_read and the traverse calls only. triedb_read is a | ||
| // blocking path that consults no cache, so a caller using it exclusively sees | ||
| // zeros here — which is not the same as an unused cache. |
Comment on lines
+208
to
+209
| out->used_bytes = static_cast<uint64_t>(cache.used_bytes()); | ||
| out->entries = static_cast<uint64_t>(cache.size()); |
Comment on lines
+323
to
+324
| /// Only the async read and traverse paths consult the cache; [`Self::read`] | ||
| /// is blocking and uncached, so a caller using it exclusively sees zeros. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
+74
to
+86
| // Returns false without writing `out` if no counters are available; all-zero | ||
| // is a legitimate reading for an idle cache, so it cannot double as an error. | ||
| typedef struct triedb_node_cache_stats | ||
| { | ||
| uint64_t hits; | ||
| uint64_t misses; | ||
| uint64_t evictions; | ||
| uint64_t used_bytes; | ||
| uint64_t entries; | ||
| } triedb_node_cache_stats; | ||
|
|
||
| bool triedb_node_cache_stats_read( | ||
| TriedbRoInner *, triedb_node_cache_stats *out); |
Contributor
There was a problem hiding this comment.
[P2] The header advertises a "no counters available" false-return, but the implementation in ffi.cpp only returns false when db == nullptr or out == nullptr — a well-formed caller can never observe a non-null false. Cascading to TriedbHandle::node_cache_stats -> Option<NodeCacheStats>, the None branch is unreachable (self.db_ptr is non-null for any TriedbHandle produced by try_new), which defeats the commit-message intent to mirror triedb_storage_stats_read. Either drop the boolean return (and the Rust Option) to match the storage-stats shape, or tighten the doc to say the boolean signals only null-pointer misuse.
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
maxkozlovsky
force-pushed
the
max/node-cache-ffi
branch
from
August 20, 2026 23:18
8eba0cd to
69c8988
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.