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.