fix overlapping references in BTree #58431
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| r? @Kimundi (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive added the S-waiting-on-review 
| 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 |
bors added S-waiting-on-bors Centril added a commit to Centril/rust that referenced this pull request Feb 22, 2019
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...
bors added a commit that referenced this pull request Feb 22, 2019
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
ssomers reviewed Nov 23, 2019
| 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.
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.
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.
I've taken the plunge and collapsed the two copies in each iterator in my #66747 PR
bors added a commit that referenced this pull request Jan 31, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
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()(wherekis a copy ofself). Callinginto_val_slice_mutcallsself.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&NodeHeaderafter 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...