BTreeMap: tag internal functions with preconditions as unsafe

[ssomers]

#67686 ended up assuming that internal functions in node.rs that perform debug_assert, should either do a hard assert or should be declared unsafe. This set of commits does the latter, except for one function insert (because it would require removing the subtle unsafe blocks in its implementation).

(at this moment the list of commits contains #67686 because I managed to start from a different master)

r? @Mark-Simulacrum

[Mark-Simulacrum]

While we're at this, is there a chance you could post the performance cost of moving the top-level "entry" functions in the node.rs module to being safe, and assert!ing their preconditions? i.e., the cost of making the map.rs file mostly or entirely safe.

[bors]

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

[ssomers]

performance cost of moving the top-level "entry" functions in the node.rs module to being safe, and assert!ing their preconditions?

If you mean really safe, as in making deallocate_and_ascend and reborrow_mut harmless, I think I'll pass for the moment. But when it comes to precondition-safety, turning debug_assert into assert in public functions, we get something like (as usual, quite consistent over 3 runs):

>cargo benchcmp master3.txt safe3.txt --threshold 3
 name                                           master3.txt ns/iter  safe3.txt ns/iter  diff ns/iter  diff %  speedup
 btree::map::find_seq_10_000                    41                   45                            4   9.76%   x 0.91
 btree::map::first_and_last_0                   38                   42                            4  10.53%   x 0.90
 btree::map::first_and_last_100                 73                   70                           -3  -4.11%   x 1.04
 btree::map::first_and_last_10k                 87                   95                            8   9.20%   x 0.92
 btree::map::insert_seq_10_000                  99                   102                           3   3.03%   x 0.97
 btree::set::difference_random_100_vs_100       1,620                1,570                       -50  -3.09%   x 1.03
 btree::set::difference_random_100_vs_10k       2,775                2,674                      -101  -3.64%   x 1.04
 btree::set::intersection_100_neg_vs_100_pos    30                   29                           -1  -3.33%   x 1.03
 btree::set::intersection_100_pos_vs_10k_neg    36                   34                           -2  -5.56%   x 1.06
 btree::set::intersection_random_10k_vs_100     2,341                2,760                       419  17.90%   x 0.85
 btree::set::intersection_staggered_100_vs_100  791                  879                          88  11.13%   x 0.90
 btree::set::intersection_staggered_10k_vs_10k  76,247               83,194                    6,947   9.11%   x 0.92
 btree::set::is_subset_10k_vs_100               2                    3                             1  50.00%   x 0.67

But there is some cheating here: public function keys (and probably others) does not have a debug_assert to begin with, but relies on the ones in internal functions. If we harden every debug_assert (including deep down in function as_leaf), we get a significant impact:

>cargo benchcmp master3.txt assert3.txt --threshold 5
 name                                           master3.txt ns/iter  assert3.txt ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_seq_100                       19                   20                              1    5.26%   x 0.95
 btree::map::first_and_last_0                   38                   16                            -22  -57.89%   x 2.38
 btree::map::first_and_last_100                 73                   157                            84  115.07%   x 0.46
 btree::map::first_and_last_10k                 87                   168                            81   93.10%   x 0.52
 btree::map::iter_1000                          3,968                9,188                       5,220  131.55%   x 0.43
 btree::map::iter_100000                        477,620              962,420                   484,800  101.50%   x 0.50
 btree::map::iter_20                            68                   188                           120  176.47%   x 0.36
 btree::map::iter_mut_100000                    530,520              566,330                    35,810    6.75%   x 0.94
 btree::set::build_and_clear                    4,983                5,290                         307    6.16%   x 0.94
 btree::set::build_and_into_iter                4,972                5,359                         387    7.78%   x 0.93
 btree::set::build_and_pop_all                  6,763                7,120                         357    5.28%   x 0.95
 btree::set::difference_random_100_vs_10k       2,775                2,922                         147    5.30%   x 0.95
 btree::set::difference_random_10k_vs_100       79,029               83,015                      3,986    5.04%   x 0.95
 btree::set::difference_staggered_100_vs_10k    2,537                2,766                         229    9.03%   x 0.92
 btree::set::intersection_100_neg_vs_100_pos    30                   33                              3   10.00%   x 0.91
 btree::set::intersection_100_pos_vs_100_neg    30                   32                              2    6.67%   x 0.94
 btree::set::intersection_10k_neg_vs_100_pos    34                   36                              2    5.88%   x 0.94
 btree::set::intersection_10k_pos_vs_100_neg    34                   36                              2    5.88%   x 0.94
 btree::set::intersection_10k_pos_vs_10k_neg    35                   37                              2    5.71%   x 0.95
 btree::set::intersection_random_10k_vs_100     2,341                2,635                         294   12.56%   x 0.89
 btree::set::intersection_staggered_100_vs_10k  2,332                2,545                         213    9.13%   x 0.92
 btree::set::is_subset_100_vs_10k               1,923                2,150                         227   11.80%   x 0.89

[ssomers]

I've sailed around Handle::new_edge and Handle::new_kv. They are simple, way down low but exposed, their debug_assert seems both vital (we want the index to be valid) and futile (are we really sure that the index will be valid when we use the handle?)

My proposal then would be to turn debug_assert into assert in public functions, except Handle::new_edge and Handle::new_kv. Few changes outside node.rs, and no loss of information about unsafe blocks.

[Mark-Simulacrum]

So looking at the diff I think @RalfJung was right that this hides enough information that I'd rather not do it. Thanks for generating the diff though!

It is I think notable that at least some of the unsafe tagged fns added by this PR do not have any documentation as to what the preconditions necessary for safely calling them are; I would like to see us add that documentation before we continue refactoring BTreeMap code as it would make reviewing it much easier.

[ssomers]

I have added "unsafe because" information in my proposal. I'll open a new PR for that.