Keep track of position when deleting from a BTreeMap

[Amanieu]

This improves the performance of drain_filter and is needed for future Cursor support for BTreeMap.

cc @ssomers
r? @Mark-Simulacrum

[Mark-Simulacrum]

Looks good to me, thanks!

We should really look into caching the last_leaf_edge/first_leaf_edge I suspect, to avoid the (presumably) fairly expensive tree traversals on every non-leaf node we're visiting.

@bors r+

[bors]

📌 Commit 7365748 has been approved by Mark-Simulacrum

[Mark-Simulacrum]

Oh, actually, one nit, but then r=me @bors r-

[ssomers]

Nice, but now Stole is begging me to be split up into something like StoleFromLeft and StoleFromRight.

[Amanieu]

But then I would have to do the same with MergedWithRight and MergedWithLeft, and that makes just makes the match above more complicated.

[Mark-Simulacrum]

I think further improvements to the enum can be cleared up later, for now this seems fine. @bors r+

[bors]

📌 Commit 51357cf has been approved by Mark-Simulacrum

[ssomers]

Performance measurement of this PR against its master revision:

>cargo benchcmp old4.txt new4.txt --threshold 2
name old4.txt ns/iter new4.txt ns/iter diff ns/iter diff % speedup
btree::map::find_rand_100 18 17 -1 -5.56% x 1.06
btree::map::first_and_last_0 34 32 -2 -5.88% x 1.06
btree::map::first_and_last_100 45 40 -5 -11.11% x 1.12
btree::map::first_and_last_10k 77 64 -13 -16.88% x 1.20
btree::map::insert_rand_100 44 45 1 2.27% x 0.98
btree::map::insert_seq_100 50 51 1 2.00% x 0.98
btree::map::iter_mut_1000 4,333 3,989 -344 -7.94% x 1.09
btree::map::iter_mut_100000 489,685 467,505 -22,180 -4.53% x 1.05
btree::map::iter_mut_20 82 79 -3 -3.66% x 1.04
btree::map::range_excluded_excluded 459,417 444,490 -14,927 -3.25% x 1.03
btree::map::range_included_unbounded 1,494 1,690 196 13.12% x 0.88
btree::map::range_unbounded_unbounded 2 3 1 50.00% x 0.67
btree::set::clone_100_and_drain_all 2,568 2,950 382 14.88% x 0.87
btree::set::clone_100_and_drain_half 2,660 2,714 54 2.03% x 0.98
btree::set::clone_100_and_into_iter 1,929 1,976 47 2.44% x 0.98
btree::set::clone_100_and_pop_all 2,753 2,930 177 6.43% x 0.94
btree::set::clone_100_and_remove_half 2,791 2,935 144 5.16% x 0.95
btree::set::clone_10k_and_clear 209,983 203,808 -6,175 -2.94% x 1.03
btree::set::clone_10k_and_drain_all 260,393 302,350 41,957 16.11% x 0.86
btree::set::clone_10k_and_drain_half 298,840 292,800 -6,040 -2.02% x 1.02
btree::set::clone_10k_and_pop_all 300,640 308,470 7,830 2.60% x 0.97
btree::set::difference_staggered_100_vs_10k 2,385 2,475 90 3.77% x 0.96
btree::set::intersection_100_neg_vs_100_pos 28 26 -2 -7.14% x 1.08
btree::set::intersection_100_pos_vs_100_neg 28 27 -1 -3.57% x 1.04
btree::set::intersection_10k_neg_vs_100_pos 30 31 1 3.33% x 0.97
btree::set::intersection_10k_neg_vs_10k_pos 34 33 -1 -2.94% x 1.03
btree::set::intersection_10k_pos_vs_100_neg 30 33 3 10.00% x 0.91
btree::set::intersection_10k_pos_vs_10k_neg 33 34 1 3.03% x 0.97
btree::set::intersection_random_100_vs_100 679 652 -27 -3.98% x 1.04
btree::set::intersection_random_100_vs_10k 2,283 2,474 191 8.37% x 0.92

In short, some backlash on remove because of the (unused) tracking, but also on many benchmarks of drain_filter. Note I lowered the threshold to show a benefit in clone_10k_and_drain_half, and that is consistent over multiple runs.

[Amanieu]

I'm getting completely different results on my system:

$ cargo benchcmp old new --threshold 2
name old ns/iter new ns/iter diff ns/iter diff % speedup btree::map::find_rand_100 26 27 1 3.85% x 0.96 btree::map::find_rand_10_000 88 86 -2 -2.27% x 1.02 btree::map::find_seq_100 26 25 -1 -3.85% x 1.04 btree::map::first_and_last_0 43 42 -1 -2.33% x 1.02 btree::map::iter_20 131 118 -13 -9.92% x 1.11 btree::set::clone_100 1,833 1,870 37 2.02% x 0.98 btree::set::clone_100_and_clear 1,862 1,902 40 2.15% x 0.98 btree::set::clone_100_and_drain_all 3,037 2,960 -77 -2.54% x 1.03 btree::set::clone_100_and_into_iter 1,810 1,881 71 3.92% x 0.96 btree::set::clone_10k 222,512 232,939 10,427 4.69% x 0.96 btree::set::clone_10k_and_clear 226,932 235,056 8,124 3.58% x 0.97 btree::set::clone_10k_and_drain_all 336,353 326,820 -9,533 -2.83% x 1.03 btree::set::clone_10k_and_drain_half 334,862 324,618 -10,244 -3.06% x 1.03 btree::set::clone_10k_and_into_iter 228,999 237,989 8,990 3.93% x 0.96 btree::set::clone_10k_and_remove_half 615,486 600,998 -14,488 -2.35% x 1.02 btree::set::difference_random_10k_vs_10k 259,846 265,218 5,372 2.07% x 0.98 btree::set::intersection_100_neg_vs_100_pos 24 26 2 8.33% x 0.92 btree::set::intersection_100_pos_vs_10k_neg 28 27 -1 -3.57% x 1.04 btree::set::intersection_10k_neg_vs_100_pos 32 28 -4 -12.50% x 1.14 btree::set::intersection_10k_neg_vs_10k_pos 30 31 1 3.33% x 0.97 btree::set::intersection_10k_pos_vs_10k_neg 33 29 -4 -12.12% x 1.14 btree::set::intersection_random_100_vs_100 885 906 21 2.37% x 0.98 btree::set::intersection_random_10k_vs_100 4,383 4,029 -354 -8.08% x 1.09 

old -> c259553
new -> 2ec0e14

[ssomers]

Well, completely different... both say there's no significant change to worry about. I shouldn't have written there is backlash, that's just what the numbers seemed to suggest.

[ssomers]

I rebuilt the commits (the base and tip of your branch) and tweaked cargo-benchcmp to pick the best result for each benchmark over many runs (whereas before I hand-picked the seemingly best output file from 5 runs). Now I seem to get quite consistent output, within 2%, like:

 name old ns/iter new ns/iter diff ns/iter diff % speedup
btree::map::first_and_last_100 39 42 3 7.69% x 0.93
btree::map::first_and_last_10k 62 67 5 8.06% x 0.93
btree::map::insert_rand_100 41 43 2 4.88% x 0.95
btree::map::insert_rand_10_000 41 42 1 2.44% x 0.98
btree::map::insert_seq_100 49 50 1 2.04% x 0.98
btree::map::iter_1000 4,214 4,049 -165 -3.92% x 1.04
btree::map::iter_100000 483,375 472,160 -11,215 -2.32% x 1.02
btree::map::iter_mut_100000 475,045 456,035 -19,010 -4.00% x 1.04
btree::set::clone_100_and_drain_all 2,470 2,818 348 14.09% x 0.88
btree::set::clone_100_and_pop_all 2,734 2,814 80 2.93% x 0.97
btree::set::clone_10k_and_drain_all 251,810 288,665 36,855 14.64% x 0.87
btree::set::clone_10k_and_drain_half 291,425 283,066 -8,359 -2.87% x 1.03
btree::set::clone_10k_and_pop_all 286,456 296,030 9,574 3.34% x 0.97
btree::set::difference_random_100_vs_10k 2,550 2,473 -77 -3.02% x 1.03
btree::set::intersection_100_pos_vs_100_neg 27 26 -1 -3.70% x 1.04
btree::set::intersection_100_pos_vs_10k_neg 32 31 -1 -3.12% x 1.03
btree::set::intersection_10k_pos_vs_10k_neg 33 32 -1 -3.03% x 1.03
btree::set::intersection_staggered_100_vs_10k 2,180 2,112 -68 -3.12% x 1.03

Of course, it doesn't mean that the code tested is a wee bit faster or slower, probably just that, for this set of benchmarks, the optimizer ends up favouring other benchmarks than it did before. And I wouldn't be surprised that the software picks up on this and starts countermeasures so it can keep on doing what it wants to.