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.