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)

[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> {
    pub unsafe fn next_unchecked(&mut self) -> (&'a K, &'a V) { ...  }
    pub unsafe fn next_back_unchecked(&mut self) -> (&'a K, &'a V) { ...  }
}

impl<'a, K, V> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::Edge> {
    pub unsafe fn next_unchecked(&mut self) -> (&'a K, &'a mut V) { ...  }
    pub unsafe fn next_back_unchecked(&mut self) -> (&'a K, &'a mut V) { ...  }
}

impl<K, V> Handle<NodeRef<marker::Owned, K, V, marker::Leaf>, marker::Edge> {
    pub unsafe fn next_unchecked(&mut self) -> (K, V) { ...  }
    pub unsafe fn next_back_unchecked(&mut self) -> (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...