feat: implement entry for BTreeMap similar to std lib#420
Open
hpeebles wants to merge 23 commits into
Open
Conversation
|
|
|
|
|
|
# Conflicts: # src/btreemap.rs
hpeebles
commented
Apr 1, 2026
| NodeType::Internal => { | ||
| match node.search(key, self.memory()) { | ||
| Ok(idx) => { | ||
| // Case 2: The node is an internal node and the key exists in it. |
Contributor
Author
There was a problem hiding this comment.
This logic is unchanged, it has just moved to remove_from_internal_node so that it can be used within entry.rs
hpeebles
commented
Apr 1, 2026
| NodeType::Leaf => { | ||
| match node.search(key, self.memory()) { | ||
| Ok(idx) => { | ||
| // Case 1: The node is a leaf node and the key exists in it. |
Contributor
Author
There was a problem hiding this comment.
This logic is unchanged, it has just moved to remove_from_leaf_node so that it can be used within entry.rs
Contributor
Author
|
The |
Contributor
Author
|
The benchmarks are failing because there are 2 new benchmarks. I'm holding off on regenerating the files though until the PR has been approved, so that reviewers have a chance to see the results of the benchmarks. |
# Conflicts: # src/btreemap.rs
`entry()` loaded the target node with `load_node`, bypassing the node cache, so the node (often the root) was re-read from stable memory on every call. Take it from the cache instead and hand it to the entry, which then reads and writes it without any further loads. The node is not put back when the entry goes away: mutating an entry saves it, and `save_node` invalidates the cache slot precisely because a just-saved node holds all of its keys and values materialized. An entry dropped without mutating costs at most one cold cache slot, refilled by the next ordinary lookup. `remove`'s slow path is the one place returning the node pays off, so it does so before re-traversing. That cache slot is not the whole story, though: `entry()` runs the insert descent, so it can split nodes and write to stable memory even when the caller only ever reads. Measured over 100k read-only probes of absent keys on a 100k-entry map, that is 19 extra nodes on the first pass and zero on every pass after. Node and slot index move into a `NodeSlot` struct shared by both entry variants, and `OccupiedEntry::get` reads the value uncached to keep the node lean. Measured with canbench, which is what matters on-chain: the `btreemap_get_and_incr_via_entry` benchmark drops from 326.00M instructions to 317.35M, i.e. 2.65% fewer. Against the get-then-insert baseline of 376.18M that widens the entry API's margin from 13.3% to 15.6%. `btreemap_get_and_incr` is unchanged at 376.18M, confirming that extracting `split_root_if_full` out of `insert` costs nothing. Also in this commit: - Drop the unreachable empty-leaf branch in `OccupiedEntry::remove`; `can_remove_entry_without_merging` already guarantees the leaf keeps at least the minimum number of entries. - Add an `EntryRemove` operation to the comprehensive proptest so entry-based removal is exercised on multi-level trees at every cache size, and use `wrapping_add` in the entry proptest to rule out an incidental u8 overflow panic. - Implement `Debug` for `LazyValue` without deserializing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two benchmarks added by this PR had no entries in canbench_results.yml, so `scripts/ci_run_benchmark.sh` would report them as new and fail the benchmark job. Generated with canbench 0.2.0 (the version CI pins) against the full benchmark binary. A full run now reports 254 unchanged, 0 new, 0 regressed, 0 improved, which also confirms that extracting `split_root_if_full` out of `insert` did not move any existing benchmark. For the record, `entry` uses 317.35M instructions against 376.18M for get-then-insert, i.e. 15.6% fewer. The 37% quoted in the PR description predates the node cache landing on main, which cut the get-then-insert baseline from 597.30M to 376.18M. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`OccupiedEntry::key` returned the key that was passed to `entry()` rather than the one held by the map. The standard library returns the stored key, and for most key types the two are byte-identical, so the difference is invisible — until `K` has an `Ord` impl that ignores part of the value, at which point the caller silently gets back their own probe key instead of the map's. The entry already owns the node, so reading the stored key costs nothing beyond the lookup that has already happened. `into_key` follows suit. `VacantEntry::key` is unchanged: there is no stored key to report, so it correctly hands back the one it was given. The new test uses a key whose ordering ignores a `tag` byte, and fails against the previous implementation with `left: 99, right: 7`. Also widen the `entry` proptest from 10 operations to 200. A node holds up to CAPACITY (11) entries, so at 10 operations the tree never had more than a single node: splitting, merging and every multi-node path went untested. This is also the entry API's only coverage of the V1 and V1-migrated-to-V2 layouts, via `run_btree_test`. Simulating the operation mix over 2000 runs, the smallest map now reached holds 105 entries; the proptest still runs in about two seconds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`entry()` runs the insert descent, splitting every full node on the way down so that a later insert finishes in one pass. It therefore writes — allocating nodes and rewriting the header — even when the key turns out to be present, and even when the returned entry is dropped unused. That makes it the one read-shaped method in the crate that mutates stable memory, and nothing said so. Worse, the comment added alongside the node-cache change claimed a dropped entry "costs at most one cold cache slot", which is true of the cache and invites precisely the wrong conclusion about stable memory. Scope that comment to the cache and point at the new section. The cost is bounded: measured over 100k read-only probes of absent keys on a 100k-entry map, the first pass allocates 19 extra nodes and every later pass allocates none, since a node splits at most once and that split was coming on the next insert anyway. Other documentation gaps this fills: - `# Panics` on both `insert` methods. They panic on oversized keys or values like `BTreeMap::insert`, but unlike `insert` — which validates before touching the tree — `entry` may already have restructured it. - `get` re-reads and re-deserializes on every call, and `and_modify` writes back unconditionally even when the closure changed nothing. - Why there is no `get_mut`/`into_mut` equivalent. Also: - `#[must_use]` on `entry`, which catches a bare `map.entry(k);` performing writes for no benefit. It does not fire on the chained forms. Note this can newly fail a downstream build that denies warnings. - `Debug` for `Entry`, `VacantEntry` and `OccupiedEntry`, matching the standard library. Keys only: printing a value would mean a stable-memory read and a deserialization inside a `Debug` impl. - A `debug_assert!` on the search after `split_child`, whose `Ok` arm is unreachable and would mean inserting a duplicate, plus reciprocal notes recording that `find_node_for_insert` and `insert_nonfull` share their descent logic and were left separate on purpose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
This allows you to lookup an entry by searching the map, then modify the value and insert it back into the map without having to search again.
The API is similar to
entryon the std libBTreeMap, but is not able to return mutable references, so you need to either useand_modifyor get the owned value, update it, then insert it back in the entry.For example in the std lib you would write this
Or using the shorthand syntax
Whereas using this new API for
StableBTreeMapyou would write it as thisOr using the shorthand syntax
I've added some benchmarks which compare using
gettheninsertvs usingentryfor 10k u32 values, from the results you can see that in this scenario usingentryis roughly 37% faster