Bundle and document 6 BTreeMap navigation algorithms

[ssomers]

• Expose a function to step through trees, without necessarily extracting the KV pair, that helps future operations like drain/retain (as demonstrated in this drain_filter implementation)
• Also aligns the implementation of the 2 x 3 iterators already using such navigation:
  • Delay the moment the K,V references are plucked from the tree, for the 4 iterators on immutable and owned maps, just for symmetry. The same had to be done to the two iterators for mutable maps in fix overlapping references in BTree #58431.
  • Always explicitly use ptr::read to derive two handles from one handle. While the existing implementations for immutable maps (i.e. Range::next_unchecked and Range::next_back_unchecked) rely on implicit copying. There's no change in unsafe tags because these two functions were already (erroneously? prophetically?) tagged unsafe. I don't know whether they should be tagged unsafe. I guess they should be for mutable and owned maps, because you can change the map through one handle and leave the other handle invalid.
  • Preserve the way two handles are (temporarily) derived from one handle: implementations for immutable maps (i.e. Range::next_unchecked and Range::next_back_unchecked) rely on copying (implicitly before, explicitly now) and the others do ptr::read.
  • I think the functions that support iterators on immutable trees (i.e. Range::next_unchecked and Range::next_back_unchecked) are erroneously tagged unsafe since you can already create multiple instances of such ranges, thus obtain multiple handles into the same tree. I did not change that but removed unsafe from the functions underneath.

Tested with miri in liballoc/tests/btree, except those that should_panic.

cargo benchcmp the best of 3 samples of all btree benchmarks before and after this PR:

name old1.txt ns/iter new2.txt ns/iter diff ns/iter diff % speedup
btree::map::find_rand_100 17 17 0 0.00% x 1.00
btree::map::find_rand_10_000 57 55 -2 -3.51% x 1.04
btree::map::find_seq_100 17 17 0 0.00% x 1.00
btree::map::find_seq_10_000 42 39 -3 -7.14% x 1.08
btree::map::first_and_last_0 14 14 0 0.00% x 1.00
btree::map::first_and_last_100 36 37 1 2.78% x 0.97
btree::map::first_and_last_10k 52 52 0 0.00% x 1.00
btree::map::insert_rand_100 34 34 0 0.00% x 1.00
btree::map::insert_rand_10_000 34 34 0 0.00% x 1.00
btree::map::insert_seq_100 46 46 0 0.00% x 1.00
btree::map::insert_seq_10_000 90 89 -1 -1.11% x 1.01
btree::map::iter_1000 2,811 2,771 -40 -1.42% x 1.01
btree::map::iter_100000 360,635 355,995 -4,640 -1.29% x 1.01
btree::map::iter_20 39 42 3 7.69% x 0.93
btree::map::iter_mut_1000 2,662 2,864 202 7.59% x 0.93
btree::map::iter_mut_100000 336,825 346,550 9,725 2.89% x 0.97
btree::map::iter_mut_20 40 43 3 7.50% x 0.93
btree::set::build_and_clear 4,184 3,994 -190 -4.54% x 1.05
btree::set::build_and_drop 4,151 3,976 -175 -4.22% x 1.04
btree::set::build_and_into_iter 4,196 4,155 -41 -0.98% x 1.01
btree::set::build_and_pop_all 5,176 5,241 65 1.26% x 0.99
btree::set::build_and_remove_all 6,868 6,903 35 0.51% x 0.99
btree::set::difference_random_100_vs_100 721 691 -30 -4.16% x 1.04
btree::set::difference_random_100_vs_10k 2,420 2,411 -9 -0.37% x 1.00
btree::set::difference_random_10k_vs_100 54,726 52,215 -2,511 -4.59% x 1.05
btree::set::difference_random_10k_vs_10k 164,384 170,256 5,872 3.57% x 0.97
btree::set::difference_staggered_100_vs_100 739 709 -30 -4.06% x 1.04
btree::set::difference_staggered_100_vs_10k 2,320 2,265 -55 -2.37% x 1.02
btree::set::difference_staggered_10k_vs_10k 68,020 70,246 2,226 3.27% x 0.97
btree::set::intersection_100_neg_vs_100_pos 23 24 1 4.35% x 0.96
btree::set::intersection_100_neg_vs_10k_pos 28 29 1 3.57% x 0.97
btree::set::intersection_100_pos_vs_100_neg 24 24 0 0.00% x 1.00
btree::set::intersection_100_pos_vs_10k_neg 28 28 0 0.00% x 1.00
btree::set::intersection_10k_neg_vs_100_pos 27 27 0 0.00% x 1.00
btree::set::intersection_10k_neg_vs_10k_pos 30 29 -1 -3.33% x 1.03
btree::set::intersection_10k_pos_vs_100_neg 27 28 1 3.70% x 0.96
btree::set::intersection_10k_pos_vs_10k_neg 29 29 0 0.00% x 1.00
btree::set::intersection_random_100_vs_100 592 572 -20 -3.38% x 1.03
btree::set::intersection_random_100_vs_10k 2,271 2,269 -2 -0.09% x 1.00
btree::set::intersection_random_10k_vs_100 2,301 2,333 32 1.39% x 0.99
btree::set::intersection_random_10k_vs_10k 147,879 150,148 2,269 1.53% x 0.98
btree::set::intersection_staggered_100_vs_100 622 632 10 1.61% x 0.98
btree::set::intersection_staggered_100_vs_10k 2,101 2,032 -69 -3.28% x 1.03
btree::set::intersection_staggered_10k_vs_10k 60,341 61,834 1,493 2.47% x 0.98
btree::set::is_subset_100_vs_100 417 426 9 2.16% x 0.98
btree::set::is_subset_100_vs_10k 1,281 1,324 43 3.36% x 0.97
btree::set::is_subset_10k_vs_100 2 2 0 0.00% x 1.00
btree::set::is_subset_10k_vs_10k 41,054 41,612 558 1.36% x 0.99

r? cuviper

[rust-highfive]

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

[ssomers]

Let me try that again...

r? @cuviper

[ssomers]

Checking again with miri (properly or with newer version), there are plenty of leaks.

[cuviper]

Sorry I lost the review on this.

Checking again with miri (properly or with newer version), there are plenty of leaks.

Having closed this PR, do you intend to address those issues and reopen it?

[ssomers]

Sure, reopening is my intent. But it doesn't look as straightforward as it did hours ago.

[ssomers]

Meanwhile miri says drain_filter building on this PR also works fine

[bors]

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

[ssomers]

Last 2 commits were only to rebase on a recent master branch (which includes code reformatting for the 1st commit)

[Mark-Simulacrum]

@cuviper Do you plan to pick up the review here, or do you not expect to have time to do so?

[cuviper]

I will try to review it this week, or else yield to another reviewer.

[Mark-Simulacrum]

FWIW I've been trying to get up to speed on the BTreeMap code so definitely feel free to yield to me if needed.

[ssomers]

Latest commit is just a rebase, no longer showing the benchmarks as proposed changes because they are already merged in.

[cuviper]

Expose a function to step through trees, without necessarily extracting the KV pair, that helps future operations like drain/retain (as demonstrated in this drain_filter implementation)

I do appreciate consolidating the code that steps through the tree, as that's complicated enough to be annoying to have it repeated in multiple places. I'm not totally comfortable with the abstraction achieved here though. For one, not extracting the KV right away means we have to keep multiple handles alive, which is quietly dangerous in the Mut and Owned cases, as I've noted inline below. I accept that miri is OK with this as used now, but there's a greater risk of introducing unchecked mistakes in the future.

Preserve the way two handles are (temporarily) derived from one handle: implementations for immutable maps (i.e. Range::next_unchecked and Range::next_back_unchecked) rely on copying (implicitly before, explicitly now) and the others do ptr::read.

That "(temporarily)" is quite important though -- we should not sweep this under the rug!

I think the functions that support iterators on immutable trees (i.e. Range::next_unchecked and Range::next_back_unchecked) are erroneously tagged unsafe since you can already create multiple instances of such ranges, thus obtain multiple handles into the same tree. I did not change that but removed unsafe from the functions underneath.

They're still unsafe for being unchecked, to assume that there is in fact a next item. Maybe your navigate functions should be similarly unchecked, removing that Option uncertainty and the "changes nothing" caveat.

[cuviper]

It's not really correct to say it changes nothing in the deallocate_and_ascend case -- in fact the original handle could be left dangling, no? I suppose that's currently avoided because the callers expect/know there is always another item, since they track their length.

I'm not really comfortable with quietly copying handles in the Mut and Owned cases. You did mark those unsafe, at least, but there's no indication to callers about the actual danger involved. Compare this to the reborrow_mut methods which say, "Beware, as this method is very dangerous, doubly so since it may not immediately appear dangerous."

I understand the original iterators had this effect too with their ptr::read duplication, but that was local, and it really should have been commented as well. You mention possibly passing in the handle by value, where I guess we could keep the responsibility for that copy on the caller's side -- how significant was your observed slowdown? It seems like that shouldn't impact inlined performance at all, just moving which side of the call does the copy.

[ssomers]

It's not really correct to say it changes nothing in the deallocate_and_ascend case

Indeed, that's an awkward case.

I suppose that's currently avoided because the callers expect/know there is always another item, since they track their length.

Yes, and in addition, the current callers (apart from the Drop implementation) are double-ended iterators. They could stop when front and back pointer are equal instead of tracking length, but they cannot use this "end-of-tree" return value. The only code actually using it is my drain_filter implementation, and there it's just a wee bit more elegant than tracking length. I mostly just liked the idea of having a totally safe function on immutable trees.

there's no indication to callers about the actual danger involved.

I'm not really sure what the danger is. To me, it's that these hopping functions return two mutable handles pointing inside the same structure, so once the caller uses either to change something, the other one is on fire.

You mention possibly passing in the handle by value, where I guess we could keep the responsibility for that copy on the caller's side -- how significant was your observed slowdown?

Between 5 and 10%. Normally within the range of noise, but so consistent over various changes back and forth that I guess it's really there. I tried in vain to reproduce under laboratory conditions and asked around, but no real conclusion.

[cuviper]

I'm not really sure what the danger is. To me, it's that these hopping functions return two mutable handles pointing inside the same structure, so once the caller uses either to change something, the other one is on fire.

Yes, invalidation like that is obviously bad. A simpler example is just the existence of safe Handle::into_kv_mut -- then separate handles could give you aliasing &mut, which is UB. We can admonish btree developers not to do that, but the point is that we've bypassed the compiler's usual protections against these hazards.

[ssomers]

not extracting the KV right away means we have to keep multiple handles alive, which is quietly dangerous in the Mut and Owned cases

I'm relatively new to Rust and don't grasp much of unsafety. I think I understand why extracting KV right away would be better: to reduce the attack surface of mistakes or abuse.

But the initiative to keep multiple handles alive in the Mut case was #58431, specifically done to make things, well, safer? better? possible? compatible with stacked borrows (just showing off I learnt a new term today). I don't really understand what the problem was, nor why it wasn't a problem in the Owned case, so definitely not how to extract KV right away in the Mut case and avoid #58431.

[cuviper]

@Mark-Simulacrum since you're also interested, maybe you can weigh in on the danger of duplicate handles too. I could be overreacting, but I think that at least needs to be better documented.

[ssomers]

How about this:

• 3 simpler macros Instead of 1 with weird support functions
• The functions dangerously duplicating handles still exist, but they are private in module navigate and wrapped by less unsafe functions.
• Which means more of the juicy stuff juggling with handles gets moved over to module navigate and fat module map sheds more weight.
• With more documentation.

Performance comparison with master code:

>cargo benchcmp ol3.txt new3.txt --threshold 5
name ol3.txt ns/iter new3.txt ns/iter diff ns/iter diff % speedup
btree::map::find_rand_100 19 20 1 5.26% x 0.95
btree::map::find_seq_100 19 20 1 5.26% x 0.95
btree::map::first_and_last_0 39 43 4 10.26% x 0.91
btree::map::iter_1000 4,094 4,373 279 6.81% x 0.94
btree::map::iter_100000 482,150 524,020 41,870 8.68% x 0.92
btree::map::iter_20 66 79 13 19.70% x 0.84
btree::map::iter_mut_1000 4,723 4,391 -332 -7.03% x 1.08
btree::set::build_and_clear 5,037 4,537 -500 -9.93% x 1.11
btree::set::build_and_drop 4,904 4,650 -254 -5.18% x 1.05
btree::set::build_and_into_iter 5,025 4,466 -559 -11.12% x 1.13
btree::set::build_and_pop_all 6,749 6,402 -347 -5.14% x 1.05
btree::set::build_and_remove_all 8,368 7,471 -897 -10.72% x 1.12
btree::set::difference_random_100_vs_100 1,558 1,698 140 8.99% x 0.92
btree::set::difference_random_10k_vs_100 79,070 85,602 6,532 8.26% x 0.92
btree::set::difference_random_10k_vs_10k 208,150 224,872 16,722 8.03% x 0.93
btree::set::difference_staggered_100_vs_100 2,298 1,690 -608 -26.46% x 1.36
btree::set::difference_staggered_100_vs_10k 2,506 2,835 329 13.13% x 0.88
btree::set::difference_staggered_10k_vs_10k 222,852 171,746 -51,106 -22.93% x 1.30
btree::set::intersection_100_neg_vs_100_pos 29 32 3 10.34% x 0.91
btree::set::intersection_100_neg_vs_10k_pos 34 36 2 5.88% x 0.94
btree::set::intersection_100_pos_vs_10k_neg 34 36 2 5.88% x 0.94
btree::set::intersection_10k_neg_vs_10k_pos 35 37 2 5.71% x 0.95
btree::set::intersection_10k_pos_vs_10k_neg 35 41 6 17.14% x 0.85
btree::set::intersection_random_100_vs_100 773 840 67 8.67% x 0.92
btree::set::intersection_staggered_100_vs_100 791 897 106 13.40% x 0.88
btree::set::intersection_staggered_100_vs_10k 2,275 2,497 222 9.76% x 0.91
btree::set::intersection_staggered_10k_vs_10k 76,342 86,109 9,767 12.79% x 0.89
btree::set::is_subset_100_vs_100 628 768 140 22.29% x 0.82
btree::set::is_subset_10k_vs_10k 63,406 79,173 15,767 24.87% x 0.80

As you see, 5 to 10% reported loss in ordinary iteration speed, which can be cured by replacing the by value interface with references (which looks way less ugly in the Immut case, because copies are implicit). But a similar reported boost in mutable iteration, which doesn't make much sense because the code is the same, just chopped up in functions.

[ssomers]

I forgot the rest of the changes I wanted to do, i.e. revert to the order of handle manipulation that is in master (which is easy once there were 3 macros). So that this PR is purely about reorganizing code and exposing functions for later use (i.e. drain_filter). Same points as above, but the performance now is on par with master:

>cargo benchcmp ol2.txt newer3.txt --threshold 5
name ol2.txt ns/iter newer3.txt ns/iter diff ns/iter diff % speedup
btree::map::insert_seq_10_000 98 107 9 9.18% x 0.92
btree::map::iter_1000 4,129 3,818 -311 -7.53% x 1.08
btree::map::iter_mut_1000 4,647 4,361 -286 -6.15% x 1.07
btree::set::build_and_remove_all 8,705 7,991 -714 -8.20% x 1.09
btree::set::intersection_random_100_vs_100 783 737 -46 -5.87% x 1.06
btree::set::intersection_random_10k_vs_10k 169,476 151,990 -17,486 -10.32% x 1.12
btree::set::intersection_staggered_100_vs_100 792 845 53 6.69% x 0.94
btree::set::intersection_staggered_100_vs_10k 2,308 2,533 225 9.75% x 0.91
btree::set::is_subset_100_vs_100 628 680 52 8.28% x 0.92

It turns out that delaying the into_kv calls speeds up benchmarks for Owned access, and slows them down for Immut access. Changing by-value to by-reference semantics doesn't make any difference anymore; maybe because the macro-generated functions are wrapped by functions with reference semantics already, maybe because of changes in rustc.

[ssomers]

on the danger of duplicate handles

One danger I struggled with, is on a higher level of duplication: in RangeMut. This is much less temporary and contained then what the guts of mutable iterators did and do. I tried to explain in the precondition of hop_to_next_unchecked_deallocating how these two handle variables have to keep out of each other's hair.

[cuviper]

I like this a lot better! There's a clearer value now in having these all defined in the same focused module, and the required safety is much better explained.

[cuviper]

(bikeshed) These names are quite a mouthful. What if we just made them all inherent methods on Handle? They can even overlap names since their borrow-type markers are different. Something like:

impl<'a,K,V>Handle<NodeRef<marker::Immut<'a>,K,V, marker::Leaf>, marker::Edge>{pubunsafefnnext_unchecked(&mutself) -> (&'aK,&'aV){ ...}pubunsafefnnext_back_unchecked(&mutself) -> (&'aK,&'aV){ ...}}impl<'a,K,V>Handle<NodeRef<marker::Mut<'a>,K,V, marker::Leaf>, marker::Edge>{pubunsafefnnext_unchecked(&mutself) -> (&'aK,&'amutV){ ...}pubunsafefnnext_back_unchecked(&mutself) -> (&'aK,&'amutV){ ...}}impl<K,V>Handle<NodeRef<marker::Owned,K,V, marker::Leaf>, marker::Edge>{pubunsafefnnext_unchecked(&mutself) -> (K,V){ ...}pubunsafefnnext_back_unchecked(&mutself) -> (K,V){ ...}}

I think that will improve readability at the call sites too:

- let (k, v) =- unsafe { navigate::hop_to_next_back_unchecked_deallocating(&mut self.back) };- Some((k, v))+ Some(unsafe { self.back.next_back_unchecked() })

[ssomers]

I am happy either way, I don't know what's the usual or desired style is, but are you aware that first_leaf_edge/last_leaf_edge were standalone functions in map.rs, and that all impl blocks in map.rs are for types defined in itself? So I figured that in order to make for instance first_leaf_edge a method on NodeRef, like first_edge is, it would have to move to node.rs. Which would be both good (there is some similarity) and bad (node.rs is pretty fat already, though not as fat as map.rs). search.rs seemed most similar, and it has standalone functions.

Secondly, is it obvious that iterating over an owned tree deallocates it? I guess since it returns ownership of K and V, you have to realize the tree can't survive and the function has to be used in an IntoIterator. But if you're the owner of a tree and you somehow want to iterate through it (sending everyone a letter of resignation) without already destroying it?

Thirdly, if we do that, then do we remove the paper thin, private Range::next_back_unchecked function?

[ssomers]

Also, I stuck on the hop_on_ prefix specifically to distinguish them from the existing next_... functions. But since they're prefixed with navigate:: when used, that was pointless.

[cuviper]

Naming is certainly subjective, which is why I prefixed this as a bikeshed comment. 🙂

it would have to move to node.rs.

It does not have to move -- methods can be added from anywhere in the same crate. Maybe the original author of first/last_leaf_edge just didn't know you could do that. Such methods even get privacy according to where they are defined, rather than from the type definition.

These are all pub anyway, though not fully publicly reachable -- maybe they should be tightened to pub(crate) or even pub(super). (That's a possible cleanup across most of btree.)

Secondly, is it obvious that iterating over an owned tree deallocates it?

Seems ok to me, but that's a subjective point too.

Thirdly, if we do that, then do we remove the paper thin, private Range::next_back_unchecked function?

There's still a little value in discriminating the range's front or back for those that are calling self.inner.next_unchecked etc.

Also, I stuck on the hop_on_ prefix specifically to distinguish them from the existing next_... functions. But since they're prefixed with navigate:: when used, that was pointless.

Well, there aren't existing next* methods on Handle, at least, so I think it's ok. It's a nice symmetry IMO that outer next just calls the handle's next_unchecked, and next_back calls next_back_unchecked.

[ssomers]

maybe they should be tightened to pub(crate) or even pub(super).

Aren't they already effectively something like that because mod.rs does not pub-lish module node?

[cuviper]

Aren't they already effectively something like that because mod.rs does not pub-lish module node?

Right, they're not actually reachable, and the compiler knows that -- it will even tell you that with RUSTFLAGS="-W unreachable_pub" or #![warn(unreachable_pub)] in the code. That lint is allowed (silent) by default.

[ssomers]

Note that while rebasing, I noticed #66648 has added a mut to the return signature of next_unchecked. I'm doing the same in next_back_unchecked.

[cuviper]

Changing the pub visibility would be more about making sure they're never exposed by accident. But the compiler enforces stability attributes on truly public items in the standard library anyway, due to #![feature(staged_api)], so I guess that doesn't matter.

[ssomers]

Also, added the same is_empty to Range as was added to RangeMut and narrowed down the unsafe sections.

[ssomers]

These are all pub anyway, though not fully publicly reachable

Not sure what you mean with "not fully", but if they're not explicitly pub in navigate.rs, trying to call them in map.rs makes the compiler bark violently that they're private. So I'd rather say they're fully not public.

[bors]

☔ The latest upstream changes (presumably #68659) made this pull request unmergeable. Please resolve the merge conflicts.

[ssomers]

Miri is still happy, full test build with debug-assertions passes.

[ssomers]

I missed that the comments in navigate.rs had to adapt to their handle method ecosystem.

[cuviper]

Looks great!

[cuviper]

@bors r+

[bors]

📌 Commit 3cf724d has been approved by cuviper

[bors]

⌛ Testing commit 3cf724d with merge cd1ef39...

[bors]

☀️ Test successful - checks-azure
Approved by: cuviper
Pushing cd1ef39 to master...