Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BTreeMap: tag internal functions with preconditions as unsafe #68418

Closed
wants to merge 2 commits into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 21, 2020

#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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2020
@ssomers ssomers changed the title BTreeMap: declare unsafe all functions that can't handle shared roots BTreeMap: tag internal functions as unsafe or assert preconditions Jan 21, 2020
@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor

bors commented Jan 21, 2020

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

@ssomers
Copy link
Contributor Author

ssomers commented Jan 22, 2020

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
Copy link
Contributor Author

ssomers commented Jan 22, 2020

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
Copy link
Member

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
Copy link
Contributor Author

ssomers commented Jan 22, 2020

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

@ssomers ssomers closed this Jan 22, 2020
@ssomers ssomers changed the title BTreeMap: tag internal functions as unsafe or assert preconditions BTreeMap: tag internal functions with preconditions as unsafe Jan 22, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 30, 2020
…ark-Simulacrum

BTreeMap: tag and explain unsafe internal functions or assert preconditions

rust-lang#68418 concluded that it's not desirable to tag all internal functions with preconditions as being unsafe. This PR does it to some functions, documents why, and elsewhere enforces the preconditions with asserts.
@ssomers ssomers deleted the btreemap_prefer_unsafe branch March 27, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants