BTreeMap: introduce marker::ValMut and reserve Mut for unique access #75200

ssomers · 2020-08-05T20:57:58Z

The mutable BTreeMap iterators (apart from DrainFilter) are double-ended, meaning they have to rely on a front and a back handle that each represent a reference into the tree. Reserve a type category marker::ValMut for them, so that we guarantee that they cannot reach operations on handles with borrow type marker::Mutand that these operations can assume unique access to the tree.

Including #75195, benchmarks report no genuine change:

benchcmp old new --threshold 5
 name                                 old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 btree::map::iter_100                 3,333        3,023                -310   -9.30%   x 1.10
 btree::map::range_unbounded_vs_iter  36,624       31,569             -5,055  -13.80%   x 1.16

r? @Mark-Simulacrum

ssomers · 2020-08-06T11:14:00Z

I wonder if we should allow copies of ValMut handles, just like for Immut handles. ValMut-based iterators (so all but IntoIter and DrainFilter) already thrive in pairs, front and back, and client code can switch between them willy-nilly, thus juggling with handles to the same or different nodes. We (probably) still don't want client code to copy iterators and reach the same element twice and pry out multiple mutable references to the same value. But inside map's implementation, since the only mutability you obtain with ValMut handles is references to values, what is the trouble copying them? And all the fuss with replace to make sure handles don't have to be duplicated.

Code is 39d212aa11e018e6edf7c33f7dbfed20429ac5fa and not a great leap forward, performance the same.

bors · 2020-08-08T00:25:17Z

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

bors · 2020-08-11T08:30:17Z

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

ssomers · 2020-08-12T10:23:56Z

Now also moved range_search down into the rest of iterator support navigate.rs, ruling out its use on Mut handlers, and emphasizing the unsafe part. And putting map.rs on a diet.

Mark-Simulacrum · 2020-08-13T22:06:18Z

library/alloc/src/collections/btree/node.rs

@@ -512,6 +525,24 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Mut<'a>, K, V, Type> {
    }
 }

+impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::ValMut<'a>, K, V, Type> {


This hands out a &'a mut [K] to the key slice despite being just ValMut, which seems wrong?

Yes, but this PR tries to only stick the ValMut label on (copies of) existing code and #73971 replaces this copy of into_slices_mut entirely. Though I did already remove the mut-ness of tbe keys in into_kv_valmut, which also gets replaced, so that's not consistent. And in the navigate methods and all the way up to the map functions, but these take on their final form.

I guess, looking at this in solation, it's not sound to call it ValMut and have mut keys. One could also argue that changing keys is not a problem, as long as you preserve order. Like we used to manually renumber lines in Basic programs, What really matters in this PR is the separation between structure altering handle types and methods versus K/V-altering tpyes and methods. So instead of ValMut, it could be KeyValMut, or ImmutLayout or so.

Anyways, fixed according to the ValMut philosophy.

Yeah I see that now, but I think if we're going to call it ValMut and not something like TreeImmut (which sounds confusing, don't use it), we should go with this.

Mark-Simulacrum · 2020-08-14T21:30:48Z

@bors r+

bors · 2020-08-14T21:30:49Z

📌 Commit 55bdf17e1bacbf681d98e0adcef89b41a1984fbb has been approved by Mark-Simulacrum

… r=Mark-Simulacrum BTreeMap: purge innocent use of into_kv_mut Replace the use of `into_kv_mut` into more precise calls. This makes more sense if you know that the single remaining use of `into_kv_mut` is in fact evil and can be trialled in court (rust-lang#75200) and sent to a correction facility (rust-lang#73971). No real performance difference reported (but functions that might benefit a tiny constant bit like `BTreeMap::get_mut` aren't benchmarked): ``` benchcmp old new --threshold 5 name old ns/iter new ns/iter diff ns/iter diff % speedup btree::map::clone_fat_100 63,073 59,256 -3,817 -6.05% x 1.06 btree::map::iter_100 3,514 3,235 -279 -7.94% x 1.09 ```

bors · 2020-08-15T01:03:22Z

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

… access

crlf0710 · 2020-09-04T16:06:33Z

@Mark-Simulacrum seems this needs to be re r+'ed after the rebase

Mark-Simulacrum · 2020-09-04T16:13:06Z

@bors r+

bors · 2020-09-04T16:13:07Z

📌 Commit e5f9d7f has been approved by Mark-Simulacrum

@Mark-Simulacrum

…crum BTreeMap: introduce marker::ValMut and reserve Mut for unique access The mutable BTreeMap iterators (apart from `DrainFilter`) are double-ended, meaning they have to rely on a front and a back handle that each represent a reference into the tree. Reserve a type category `marker::ValMut` for them, so that we guarantee that they cannot reach operations on handles with borrow type `marker::Mut`and that these operations can assume unique access to the tree. Including rust-lang#75195, benchmarks report no genuine change: ``` benchcmp old new --threshold 5 name old ns/iter new ns/iter diff ns/iter diff % speedup btree::map::iter_100 3,333 3,023 -310 -9.30% x 1.10 btree::map::range_unbounded_vs_iter 36,624 31,569 -5,055 -13.80% x 1.16 ``` r? @Mark-Simulacrum

@Mark-Simulacrum

…crum BTreeMap: introduce marker::ValMut and reserve Mut for unique access The mutable BTreeMap iterators (apart from `DrainFilter`) are double-ended, meaning they have to rely on a front and a back handle that each represent a reference into the tree. Reserve a type category `marker::ValMut` for them, so that we guarantee that they cannot reach operations on handles with borrow type `marker::Mut`and that these operations can assume unique access to the tree. Including rust-lang#75195, benchmarks report no genuine change: ``` benchcmp old new --threshold 5 name old ns/iter new ns/iter diff ns/iter diff % speedup btree::map::iter_100 3,333 3,023 -310 -9.30% x 1.10 btree::map::range_unbounded_vs_iter 36,624 31,569 -5,055 -13.80% x 1.16 ``` r? @Mark-Simulacrum

bors · 2020-09-04T23:16:28Z

⌛ Testing commit e5f9d7f with merge 70c5f6e...

bors · 2020-09-05T01:17:37Z

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 70c5f6e to master...

ecstatic-morse · 2020-09-08T19:40:22Z

Final perf results are here. This is a small but steady increase to instruction counts. @ssomers, this seems like part of a larger effort to eliminate UB in BTreeMap. Is this regression worth it in your opinion?

ssomers · 2020-09-08T20:40:05Z

I doubt this PR alone, which is indeed a ramp up to others, has any discernable effect on performance, but I'm beyond caring.

…lacrum BTree: remove outdated traces of coercions The introduction of `marker::ValMut` (rust-lang#75200) meant iterators no longer see mutable keys but their code still pretends it does. And settle on the majority style `Some(unsafe {…})` over `unsafe { Some(…) }`. r? `@Mark-Simulacrum`

rust-highfive assigned Mark-Simulacrum Aug 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2020

ssomers mentioned this pull request Aug 8, 2020

BTreeMap: purge innocent use of into_kv_mut #75195

Merged

ssomers force-pushed the btree_valmut branch 2 times, most recently from 1550be0 to 160cdff Compare August 8, 2020 12:21

ssomers marked this pull request as ready for review August 8, 2020 12:25

ssomers force-pushed the btree_valmut branch from 160cdff to 4fc82b5 Compare August 9, 2020 12:22

ssomers mentioned this pull request Aug 10, 2020

BTreeMap mutable iterators should not take any reference to visited nodes during iteration #73971

Merged

ssomers force-pushed the btree_valmut branch 2 times, most recently from 553756c to f611130 Compare August 12, 2020 10:17

Mark-Simulacrum reviewed Aug 13, 2020

View reviewed changes

ssomers force-pushed the btree_valmut branch from f611130 to 55bdf17 Compare August 14, 2020 21:08

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 15, 2020

ssomers force-pushed the btree_valmut branch from 55bdf17 to b6c0ec9 Compare August 15, 2020 08:59

BTreeMap: introduce marker::ValMut and reserve marker::Mut for unique…

e5f9d7f

… access

ssomers force-pushed the btree_valmut branch from b6c0ec9 to e5f9d7f Compare August 19, 2020 18:16

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2020

Dylan-DPC-zz mentioned this pull request Sep 4, 2020

Rollup of 14 pull requests #76348

Closed

Dylan-DPC-zz mentioned this pull request Sep 4, 2020

Rollup of 13 pull requests #76350

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2020

bors merged commit 70c5f6e into rust-lang:master Sep 5, 2020

rustbot added this to the 1.48.0 milestone Sep 5, 2020

ssomers deleted the btree_valmut branch September 5, 2020 19:03

ssomers mentioned this pull request Feb 10, 2021

BTree: remove outdated traces of coercions #81956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BTreeMap: introduce marker::ValMut and reserve Mut for unique access #75200

BTreeMap: introduce marker::ValMut and reserve Mut for unique access #75200

ssomers commented Aug 5, 2020 •

edited

Loading

ssomers commented Aug 6, 2020 •

edited

Loading

bors commented Aug 8, 2020

bors commented Aug 11, 2020

ssomers commented Aug 12, 2020

Mark-Simulacrum Aug 13, 2020

ssomers Aug 14, 2020

ssomers Aug 14, 2020

Mark-Simulacrum Aug 14, 2020

Mark-Simulacrum commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 15, 2020

crlf0710 commented Sep 4, 2020

Mark-Simulacrum commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 5, 2020

ecstatic-morse commented Sep 8, 2020

ssomers commented Sep 8, 2020

BTreeMap: introduce marker::ValMut and reserve Mut for unique access #75200

BTreeMap: introduce marker::ValMut and reserve Mut for unique access #75200

Conversation

ssomers commented Aug 5, 2020 • edited Loading

ssomers commented Aug 6, 2020 • edited Loading

bors commented Aug 8, 2020

bors commented Aug 11, 2020

ssomers commented Aug 12, 2020

Mark-Simulacrum Aug 13, 2020

Choose a reason for hiding this comment

ssomers Aug 14, 2020

Choose a reason for hiding this comment

ssomers Aug 14, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Aug 14, 2020

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 15, 2020

crlf0710 commented Sep 4, 2020

Mark-Simulacrum commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 5, 2020

ecstatic-morse commented Sep 8, 2020

ssomers commented Sep 8, 2020

ssomers commented Aug 5, 2020 •

edited

Loading

ssomers commented Aug 6, 2020 •

edited

Loading