-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
fix overlapping references in BTree #58431
Conversation
r? @Kimundi (rust_highfive has picked a reviewer for you, use r? to override) |
Since the ordering here changed, it might be worth trying to run some benchmarks as well (just to make sure we're not obviously regressing). I'm not sure if we have any in-tree though. |
Without the patch:
With the patch:
(I discarded a previous run because it reported a huge variance of >200,000ns) Looks like no change to me. |
@bors r+ Code changes look reasonable (and, well, don't actually change anything). |
📌 Commit f0bef49 has been approved by |
fix overlapping references in BTree This fixes two kinds of overlapping references in BTree (both found by running the BTree test suite in Miri). In `into_slices_mut`, we did `k.into_key_slice_mut()` followed by `self.into_val_slice_mut()` (where `k` is a copy of `self`). Calling `into_val_slice_mut` calls `self.len()`, which creates a shared reference to `NodeHeader`, which unfortunately (due to padding) overlaps with the mutable reference returned by `into_key_slice_mut`. Hence the key slice got (partially) invalidated. The fix is to avoid creating an `&NodeHeader` after the first slice got created. In the iterators, we used to first create the references that will be returned, and then perform the walk on the tree. Walking the tree creates references (such as `&mut InternalNode`) that overlap with all of the keys and values stored in a pointer; in particular, they overlap with the references the iterator will later return. This is fixed by reordering the operations of walking the tree and obtaining the inner references. The test suite still passes (and it passes in Miri now!), but there is a lot of code here that I do not understand...
Rollup of 17 pull requests Successful merges: - #57656 (Deprecate the unstable Vec::resize_default) - #58059 (deprecate before_exec in favor of unsafe pre_exec) - #58064 (override `VecDeque::try_rfold`, also update iterator) - #58198 (Suggest removing parentheses surrounding lifetimes) - #58431 (fix overlapping references in BTree) - #58555 (Add a note about 2018e if someone uses `try {` in 2015e) - #58588 (remove a bit of dead code) - #58589 (cleanup macro after 2018 transition) - #58591 (Dedup a rustdoc diagnostic construction) - #58600 (fix small documentation typo) - #58601 (Search for target_triple.json only if builtin target not found) - #58606 (Docs: put Future trait into spotlight) - #58607 (Fixes #58586: Make E0505 erronous example fail for the 2018 edition) - #58615 (miri: explain why we use static alignment in ref-to-place conversion) - #58620 (introduce benchmarks of BTreeSet.intersection) - #58621 (Update miri links) - #58632 (Make std feature list sorted) Failed merges: r? @ghost
return (k, v); | ||
self.front = ptr::read(&kv).right_edge(); | ||
// Doing the descend invalidates the references returned by `into_kv_mut`, | ||
// so we have to do this last. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggle with this comment because there's no descend here, right? Maybe the right_edge also invalidates references, or the change is just here to match the code below where a descend happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, there's still a call to right_edge
so I wanted to be sure that wouldn't cause any problems, without actually checking in detail what that function does. The comment is probably a copy-paste mistake though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken the plunge and collapsed the two copies in each iterator in my #66747 PR
Bundle and document 6 BTreeMap navigation algorithms - Expose a function to step through trees, without necessarily extracting the KV pair, that helps future operations like drain/retain (as demonstrated in [this drain_filter implementation](https://github.com/ssomers/rust/compare/btree_navigation_v3...ssomers:btree_drain_filter?expand=1)) - ~~Also aligns the implementation of the 2 x 3 iterators already using such navigation:~~ - ~~Delay the moment the K,V references are plucked from the tree, for the 4 iterators on immutable and owned maps, just for symmetry. The same had to be done to the two iterators for mutable maps in #58431.~~ - ~~Always explicitly use ptr::read to derive two handles from one handle. While the existing implementations for immutable maps (i.e. Range::next_unchecked and Range::next_back_unchecked) rely on implicit copying. There's no change in unsafe tags because these two functions were already (erroneously? prophetically?) tagged unsafe. I don't know whether they should be tagged unsafe. I guess they should be for mutable and owned maps, because you can change the map through one handle and leave the other handle invalid.~~ - Preserve the way two handles are (temporarily) derived from one handle: implementations for immutable maps (i.e. Range::next_unchecked and Range::next_back_unchecked) rely on copying (implicitly before, explicitly now) and the others do `ptr::read`. - ~~I think the functions that support iterators on immutable trees (i.e. `Range::next_unchecked` and `Range::next_back_unchecked`) are erroneously tagged unsafe since you can already create multiple instances of such ranges, thus obtain multiple handles into the same tree. I did not change that but removed unsafe from the functions underneath.~~ Tested with miri in liballoc/tests/btree, except those that should_panic. cargo benchcmp the best of 3 samples of all btree benchmarks before and after this PR: ``` name old1.txt ns/iter new2.txt ns/iter diff ns/iter diff % speedup btree::map::find_rand_100 17 17 0 0.00% x 1.00 btree::map::find_rand_10_000 57 55 -2 -3.51% x 1.04 btree::map::find_seq_100 17 17 0 0.00% x 1.00 btree::map::find_seq_10_000 42 39 -3 -7.14% x 1.08 btree::map::first_and_last_0 14 14 0 0.00% x 1.00 btree::map::first_and_last_100 36 37 1 2.78% x 0.97 btree::map::first_and_last_10k 52 52 0 0.00% x 1.00 btree::map::insert_rand_100 34 34 0 0.00% x 1.00 btree::map::insert_rand_10_000 34 34 0 0.00% x 1.00 btree::map::insert_seq_100 46 46 0 0.00% x 1.00 btree::map::insert_seq_10_000 90 89 -1 -1.11% x 1.01 btree::map::iter_1000 2,811 2,771 -40 -1.42% x 1.01 btree::map::iter_100000 360,635 355,995 -4,640 -1.29% x 1.01 btree::map::iter_20 39 42 3 7.69% x 0.93 btree::map::iter_mut_1000 2,662 2,864 202 7.59% x 0.93 btree::map::iter_mut_100000 336,825 346,550 9,725 2.89% x 0.97 btree::map::iter_mut_20 40 43 3 7.50% x 0.93 btree::set::build_and_clear 4,184 3,994 -190 -4.54% x 1.05 btree::set::build_and_drop 4,151 3,976 -175 -4.22% x 1.04 btree::set::build_and_into_iter 4,196 4,155 -41 -0.98% x 1.01 btree::set::build_and_pop_all 5,176 5,241 65 1.26% x 0.99 btree::set::build_and_remove_all 6,868 6,903 35 0.51% x 0.99 btree::set::difference_random_100_vs_100 721 691 -30 -4.16% x 1.04 btree::set::difference_random_100_vs_10k 2,420 2,411 -9 -0.37% x 1.00 btree::set::difference_random_10k_vs_100 54,726 52,215 -2,511 -4.59% x 1.05 btree::set::difference_random_10k_vs_10k 164,384 170,256 5,872 3.57% x 0.97 btree::set::difference_staggered_100_vs_100 739 709 -30 -4.06% x 1.04 btree::set::difference_staggered_100_vs_10k 2,320 2,265 -55 -2.37% x 1.02 btree::set::difference_staggered_10k_vs_10k 68,020 70,246 2,226 3.27% x 0.97 btree::set::intersection_100_neg_vs_100_pos 23 24 1 4.35% x 0.96 btree::set::intersection_100_neg_vs_10k_pos 28 29 1 3.57% x 0.97 btree::set::intersection_100_pos_vs_100_neg 24 24 0 0.00% x 1.00 btree::set::intersection_100_pos_vs_10k_neg 28 28 0 0.00% x 1.00 btree::set::intersection_10k_neg_vs_100_pos 27 27 0 0.00% x 1.00 btree::set::intersection_10k_neg_vs_10k_pos 30 29 -1 -3.33% x 1.03 btree::set::intersection_10k_pos_vs_100_neg 27 28 1 3.70% x 0.96 btree::set::intersection_10k_pos_vs_10k_neg 29 29 0 0.00% x 1.00 btree::set::intersection_random_100_vs_100 592 572 -20 -3.38% x 1.03 btree::set::intersection_random_100_vs_10k 2,271 2,269 -2 -0.09% x 1.00 btree::set::intersection_random_10k_vs_100 2,301 2,333 32 1.39% x 0.99 btree::set::intersection_random_10k_vs_10k 147,879 150,148 2,269 1.53% x 0.98 btree::set::intersection_staggered_100_vs_100 622 632 10 1.61% x 0.98 btree::set::intersection_staggered_100_vs_10k 2,101 2,032 -69 -3.28% x 1.03 btree::set::intersection_staggered_10k_vs_10k 60,341 61,834 1,493 2.47% x 0.98 btree::set::is_subset_100_vs_100 417 426 9 2.16% x 0.98 btree::set::is_subset_100_vs_10k 1,281 1,324 43 3.36% x 0.97 btree::set::is_subset_10k_vs_100 2 2 0 0.00% x 1.00 btree::set::is_subset_10k_vs_10k 41,054 41,612 558 1.36% x 0.99 ``` r? cuviper
This fixes two kinds of overlapping references in BTree (both found by running the BTree test suite in Miri).
In
into_slices_mut
, we didk.into_key_slice_mut()
followed byself.into_val_slice_mut()
(wherek
is a copy ofself
). Callinginto_val_slice_mut
callsself.len()
, which creates a shared reference toNodeHeader
, which unfortunately (due to padding) overlaps with the mutable reference returned byinto_key_slice_mut
. Hence the key slice got (partially) invalidated. The fix is to avoid creating an&NodeHeader
after the first slice got created.In the iterators, we used to first create the references that will be returned, and then perform the walk on the tree. Walking the tree creates references (such as
&mut InternalNode
) that overlap with all of the keys and values stored in a pointer; in particular, they overlap with the references the iterator will later return. This is fixed by reordering the operations of walking the tree and obtaining the inner references.The test suite still passes (and it passes in Miri now!), but there is a lot of code here that I do not understand...