perf: add direct-mapped node cache to BTreeMap#416
Conversation
Add a 32-slot direct-mapped node cache to BTreeMap that avoids re-loading hot nodes from stable memory. Modeled after CPU caches: O(1) lookup via (address / page_size) % 32, collision = eviction. Read paths (get, contains_key, first/last_key_value) use a take+return pattern to borrow nodes from the cache without RefCell lifetime issues. Write paths (insert, remove, split, merge) invalidate affected cache slots. Key changes: - Switch get() from destructive extract_entry_at to node.value() - Remove unused extract_entry_at method - Change traverse() closure from Fn(&mut Node) to Fn(&Node) - Invalidate cache in save_node, deallocate_node, merge, clear_new Expected improvement: ~15-20% for random reads, ~65% for hot-key workloads, ~0% overhead for writes (cache.get_mut() bypasses RefCell).
|
|
|
|
|
|
…expose hit/miss stats
src/btreemap.rs
Outdated
| if !self.is_enabled() { | ||
| self.misses += 1; | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Just an idea, feel free to ignore: rather than disable the cache (and incur the cost of a branch), might it be preferable to have a cache of size 1?
(As a side note, I would actually be curious to see the difference in benchmark scores from adding this and the stats. Maybe it's all a storm in a teacup. But maybe not.)
There was a problem hiding this comment.
If we have a single entry that will strictly be more computationally expensive (always and is unlikely to give any benefit since we'll keep overwriting it all the time.
size-0 execution path will be correctly predicted every time and costs ~0 cycles in practice.
A size-1 cache, on the other hand, would still allocate a Node on the heap and would collide on every operation (every node maps to slot 0), producing worse miss behaviour than even a small real cache.
So I'd recommend we go with size-0 by default, in the next version. And then we can turn it on to size-32 or larger by default once we get some feedback from production runs.
There was a problem hiding this comment.
btw I now see in the benchmarks with default 0-size cache size some increase in instruction cost, up to +8%.
would it be possible to have some kind of short circuit not to do extra work when cache size is zero?
There was a problem hiding this comment.
btw I now see in the benchmarks with default 0-size cache size some increase in instruction cost, up to +8%.
This is what I was referring to. Sure, a size 1 cache is going to be more expensive than a size 0 cache. But having the option of a size 0 cache makes all other cache sizes more expensive. And the size 0 cache more expensive than not having this feature at all.
So I guess it's a trade-off between optimizing for no caching vs. optimizing for caching. I was going for the latter, since caching appears to make a positive change in performance, but we could also stick with no caching as the default.
There was a problem hiding this comment.
By inlining one function, we're now roughly in the negative range (marginally improved performance) even with cache disabled.
There was a problem hiding this comment.
re: size 1 cache
In current implementation direct-mapped cache does not prioritise top level nodes over lower level nodes, meaning that the cache with size 1 will always be overwritten on each tree traversal and will always have 0 hits, which is a pure waste of cycles.
More over any cache with the size smaller than the tree height will have 0 hits. So the rough range for the cache sizes that make it practically usable:
- lower bound must be at least the tree size, maybe x2 to account for collisions
- upper bound any cache size that reaches 90-95% hit ratio, above that the ROI is too small
(optional exploration idea) Based on that, I suppose one interesting idea to explore (not now, in the future PRs) is to store in cache also the current node height (or level) and prioritize top nodes (closer to root) over lower level nodes. Or maybe when we decide to use set-assotiative cache store cache lines of different levels with prioritization by level.
There was a problem hiding this comment.
added storing and checking depth to node cache, it showed some good performance boost:
Only significant changes:
| status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% |
|--------|-------------------------------------------------|-------|---------|---------|----|--------|-----|---------|
| - | btreemap_v2_get_vec_32_128_cached_32entry | | 257.72M | -13.20% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_contains_vec_32_128_cached_32entry | | 251.19M | -13.50% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_get_u64_u64_cached_32entry | | 140.77M | -15.96% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_contains_u64_u64_cached_32entry | | 136.11M | -16.42% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_get_blob_32_128_cached_32entry | | 206.55M | -17.49% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_contains_blob_32_128_cached_32entry | | 201.95M | -17.79% | 0 | 0.00% | 0 | 0.00% |
ins = instructions, HI = heap_increase, SMI = stable_memory_increase, Δ% = percent change
There was a problem hiding this comment.
Agreed — with depth-aware eviction the default now makes more sense as non-zero. Changed default to 16 slots, which covers the top 2 tree levels. The NodeCache::take/put/invalidate methods now early-return when disabled (0 slots), so there's no branch cost in the btreemap code itself.
src/btreemap.rs
Outdated
| pub fn cache_stats(&self) -> CacheStats { | ||
| self.cache.borrow().stats() | ||
| } |
There was a problem hiding this comment.
Might be useful to return the cache size in bytes. The end user might have a hard time figuring out the average node size or even just the page size.
There was a problem hiding this comment.
there were some requests from users to add to stable structures methods that report memory usage: heap usage, stable memory allocated and actually used. if this is implemented it also covers any cache, so maybe it should not report back the cache size. also if user configures the cache size, it's expected to be full.
I'm fine if this PR does not implement reporting cache size.
There was a problem hiding this comment.
Reporting the cache size in bytes is easy and cheap, so I just added it.
There was a problem hiding this comment.
it wasn't so easy after all, I'll look for another way to do it.
There was a problem hiding this comment.
I added a MemSize trait implementation to this branch.
It calculates heap usage with (in my opinion) an acceptable tradeoff between the cost and precision.
Please take a look and let me know what you think. It's ok to revert my changes if they don't work.
There was a problem hiding this comment.
I implemented an accurate memory calculation for heap usage and compared against the approximate one: if value sizes were small, then the difference in numbers were x2-x4, but if the values were big (eg. key is u32 and value is blob of 900 bytes) then it went x40 and above.
I also removed the whole mem_size implementation to see how much extra instructions it will provide and it was between +12% and +24% which is pretty significant.
Only significant changes:
| status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% |
|--------|-------------------------------------------------|-------|---------|---------|----|--------|-----|---------|
| - | btreemap_v2_get_vec_32_128_cached_32entry | | 296.90M | -12.42% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_contains_vec_32_128_cached_32entry | | 290.40M | -12.67% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_get_blob_32_128_cached_32entry | | 250.32M | -13.54% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_contains_blob_32_128_cached_32entry | | 245.66M | -13.76% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_get_u64_u64_cached_32entry | | 167.51M | -24.02% | 0 | 0.00% | 0 | 0.00% |
| - | btreemap_v2_contains_u64_u64_cached_32entry | | 162.86M | -24.54% | 0 | 0.00% | 0 | 0.00% |
ins = instructions, HI = heap_increase, SMI = stable_memory_increase, Δ% = percent change
now I changed my mind, and even though approximate value is not exactly accurate, it is still a possible upper bound user should take into consideration (because in some cases values can get cached).
I am fine with going with less accurate but with much simpler and faster solution. Sorry it took some time on debating & coding this topic. we can always add more accurate calculation later if we find a better way to do it, no need to get blocked on it now.
There was a problem hiding this comment.
Just a question for my own illumination:
I also removed the whole mem_size implementation to see how much extra instructions it will provide and it was between +12% and +24% which is pretty significant.
Is this on common workloads (i.e. do we call mem_size() frequently) or is this only the performance of computing the exact usage vs a (potentially quite bad) approximation?
There was a problem hiding this comment.
it's mostly because computing the exact usage turned out to be expensive, especially for keys, which are used on btreemap level in their original type, so the only way to get the size of the key is to convert it to bytes via to_bytes().len().
There was a problem hiding this comment.
Acknowledged. node_cache_size_bytes_approx is still available as a rough estimate. The exact-size MemSize approach was too expensive as you found (+12-24% instructions), so the approximate method stays as an order-of-magnitude guide.
|
FYI: I'll resolve merge conflicts. |
sasa-tomic
left a comment
sasa-tomic
left a comment
There was a problem hiding this comment.
Addressing open review comments with code changes and replies.
…ard NULL - Change DEFAULT_NODE_CACHE_NUM_SLOTS from 0 to 16 (covers top 2 tree levels) - Remove redundant cache_num_slots field from BTreeMap; NodeCache methods now self-guard via is_enabled() and callers no longer need checks - Add NULL address guard in NodeCache::take to prevent false cache hits
maksymar
left a comment
maksymar
left a comment
There was a problem hiding this comment.
Great results! 🎉 Thank you!
src/btreemap/node_cache.rs
Outdated
| // compete on pure recency: a non-root node can evict any other | ||
| // non-root node. This lets hot leaves and working-set interior | ||
| // nodes stay cached while stale upper-level nodes get displaced. | ||
| if slot.node.is_none() || depth == 0 || slot.depth > 0 { |
There was a problem hiding this comment.
FYI: I was thinking about this idea of always protecting nodes with lower depth, and realized it may not work very well for caches with bigger sizes, because it will always populate the top of the tree ignoring hot paths.
so instead I think it would be reasonable to protect only a few top levels (0-1) and let nodes on other levels compete freely. it should also work with the default size of 16.
|
[DONE] |
|
The chart shows how increasing node cache size reduces instruction count (relative to no cache) across operation categories.
From this data I'm happy to see that "the default 16" is already good enough while being reasonably small, at the same time bigger caches will provide more value. 
|
3 participants
Summary
(address / page_size) % 32, collision = eviction (no LRU tracking)get,contains_key,first/last_key_value) use a take+return pattern to avoid re-loading hot upper-tree nodes from stable memorysave_node,deallocate_node,merge, andclear_newget()from destructiveextract_entry_at(swap_remove) to non-destructivenode.value()(borrows via OnceCell)extract_entry_atmethodThis subsumes all four previous caching approaches (root-only, LRU+clone, LRU+Rc, page cache) into a single design that:
Node<K>directly (no Rc, no Clone, no heap allocation per cache entry)cache.get_mut()on write paths (zero RefCell overhead)Expected improvement: ~15-20% for random reads, ~65% for hot-key workloads, ~0% overhead for writes.