BTreeMap: clean up access to MaybeUninit arrays #79520

ssomers · 2020-11-28T21:04:41Z

Stop exposing and using immutable access to MaybeUninit slices when we need and have exclusive access to the tree.

Mark-Simulacrum · 2020-12-08T22:08:14Z

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

-    unsafe fn edge_area(self) -> &'a [MaybeUninit<BoxedNode<K, V>>] {
-        Self::as_internal(&self).edges.as_slice()
+    unsafe fn edge_area(mut self) -> &'a [MaybeUninit<BoxedNode<K, V>>] {
+        Self::as_internal_mut(&mut self).edges.as_slice()


It seems quite odd to require unique access to get a read-only slice of the edges -- can you say more about the use case here?

The change is not to require unique access (to the tree), we admit that the caller has it and needs to have it. We don't get you a slice of edges, but a slice of containers of edges. If you only have shared access to the tree, the only thing you could ever do with those containers is to read the initialized ones, and we already have better methods for that.

The actual use case is to unify the various methods that access MaybeUninits in another PR.

Mark-Simulacrum · 2020-12-08T22:09:10Z

stop using immutable access when we have exclusive access to the tree

Why is this good? It feels like we should not impose exclusive access just to reborrow into a shared reference?

ssomers · 2020-12-09T11:46:26Z

Why is this good? It feels like we should not impose exclusive access just to reborrow into a shared reference?

I've returned a &'a mut [_] instead now. #79347 merges the different methods to access MaybeUninit slices and then it always returns exclusiveness.

Mark-Simulacrum · 2020-12-12T16:25:18Z

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

    /// Exposes the entire key storage area in the node,
    /// regardless of the node's current length,
    /// having exclusive access to the entire node.
-    unsafe fn key_area(self) -> &'a [MaybeUninit<K>] {
-        Self::as_leaf(&self).keys.as_slice()
+    unsafe fn key_area(mut self) -> &'a mut [MaybeUninit<K>] {


Ok, so I was initially confused by this change -- it still seems unnecessary, as this PR doesn't introduce any cases where key_area is being used with the unique access being provided here. Since this PR is an attempt to split off part of #79347, I took a look there, and it looks like the change this is maybe.. not going far enough?

Can we include the next step of this refactor (but avoid the other changes)? I would expect, I guess, a PR that just switches away from using the different key_area/into_key_area etc methods to a single one, but doesn't do anything else (no introduction of e.g. slice_move). That would make it easier to judge whether the generics and such suggested in the other PR are worth it to me. As-is, it's hard to determine what the outcome is looking like quite yet.

it still seems unnecessary, as this PR doesn't introduce any cases where key_area is being used with the unique access being provided here

The compiler doesn't notice, but I think the uniqueness is being used and required. You cannot sensibly copy a MaybeUninit<K> if there might be someone else copying it too, because the key would be duplicated. The callers of key_area move ownership, so they need uniqueness, even though the ptr::copy doing the hard work for them doesn't care about the uniqueness of the source .

Can we include the next step of this refactor (but avoid the other changes)?

Not sure which next step and other changes you mean.

As-is, it's hard to determine what the outcome is looking like quite yet.

Again, not sure which outcome you mean. In my mind, the outcome looks a lot like #79347 (just needs rebase for the last tweaks).

Mark-Simulacrum · 2020-12-12T16:26:27Z

More concretely, it feels like this PR combines together many refactors in one commit, and that makes it hard for me to understand the intent behind each and evaluate it. (Or figure out when things are intended to depend on each other or be related).

ssomers · 2020-12-12T19:11:34Z

I can split off some more items from this one and present #79521 separately from any of this PR. But there are (minor) conflicts between them, so I'll just start on #79521 alone.

ssomers · 2020-12-13T12:20:00Z

Apart from the really small stuff, I see 4 changes that could be here (each repeated for the key, val and edge arrays):

Stop pretending callers of key_area could share references. Which implies all the "area" methods return exclusive access.
Stop implicitly injecting the node's current length in key_area_slice because that introduces a non-obvious order dependency with statements updating length; instead pass length explicitly. Which leaves only 2 "area" methods, taking either a single index or a range.
Introduce SliceIndex to settle on a single "area" method name.
Introduce slice functions to slightly abstract ptr::copy statements and require a source and destination size, so that we can debug-assert sizes the way -Zmiri-track-raw-pointers does.
Finally, not here ~~but in BTreeMap: respect pointer provenance rules in split_off #79347~~, convert the code around into_kv_pointers to use that infrastructure.

I think (1) is a worth while clarification on its own, but it seems you're not convinced. #79347 does (2) first, but it seemed sensible to combine with (3) rather than come up with additional methods. It's possible to do (4) first ~~, but it causes long lines without all the other tweaks (i.e., #79521, #80006 and (3))~~.

ssomers · 2020-12-13T12:49:19Z

Meanwhile, a variation rebased on #79521 and #80006, that abolishes key_area (and cousins) and replaces calls to it instead.

bors · 2020-12-14T01:07:13Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-12-25T00:54:53Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ssomers · 2020-12-25T08:54:11Z

Focusing on the point discussed here, the rest is for later PRs.

PS Another way to defend my crusade against shared access to MaybeUninit slices: we get away with shared access because MaybeUninit is too stupid to remember whether it contains something or not. When we move over items that we access through key_area to another node, we do mutate the source MaybeUninit containers, we uninitialize them.

Mark-Simulacrum · 2020-12-26T14:06:13Z

@bors r+ rollup=never

Ok, this seems fine. I think I am at least somewhat convinced by the ownership argument, but ultimately it doesn't matter too much, and this seems to let us delete some unused APIs which is great.

bors · 2020-12-26T14:06:14Z

📌 Commit 0d2548a has been approved by Mark-Simulacrum

bors · 2020-12-26T16:47:38Z

⌛ Testing commit 0d2548a with merge 89524d0...

bors · 2020-12-26T19:43:02Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 89524d0 to master...

rust-highfive assigned Mark-Simulacrum Nov 28, 2020

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

This comment has been minimized.

Sign in to view

ssomers mentioned this pull request Nov 28, 2020

BTreeMap: relax the explicit borrow rule to make code shorter and safer #79521

Merged

ssomers force-pushed the btree_cleanup_1 branch from 4b16dcb to 3221438 Compare November 29, 2020 16:09

Mark-Simulacrum reviewed Dec 8, 2020

View reviewed changes

ssomers force-pushed the btree_cleanup_1 branch from 3221438 to 1f71886 Compare December 9, 2020 11:44

Mark-Simulacrum reviewed Dec 12, 2020

View reviewed changes

ssomers marked this pull request as draft December 12, 2020 20:10

ssomers force-pushed the btree_cleanup_1 branch from 1f71886 to 621869e Compare December 13, 2020 12:45

ssomers changed the title ~~BTreeMap: lightly refactor a bunch of methods~~ BTreeMap: clean up access to MaybeUninit arrays Dec 13, 2020

ssomers force-pushed the btree_cleanup_1 branch from 621869e to 9f129de Compare December 17, 2020 19:08

BTreeMap: declare exclusive access to arrays when copying from them

0d2548a

ssomers force-pushed the btree_cleanup_1 branch from 9f129de to 0d2548a Compare December 25, 2020 08:51

ssomers marked this pull request as ready for review December 25, 2020 08:51

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 Dec 26, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 26, 2020

bors merged commit 89524d0 into rust-lang:master Dec 26, 2020

rustbot added this to the 1.51.0 milestone Dec 26, 2020

ssomers deleted the btree_cleanup_1 branch December 26, 2020 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BTreeMap: clean up access to MaybeUninit arrays #79520

BTreeMap: clean up access to MaybeUninit arrays #79520

ssomers commented Nov 28, 2020 •

edited

Loading

This comment has been minimized.

Mark-Simulacrum Dec 8, 2020

ssomers Dec 9, 2020 •

edited

Loading

Mark-Simulacrum commented Dec 8, 2020

ssomers commented Dec 9, 2020

Mark-Simulacrum Dec 12, 2020

ssomers Dec 12, 2020

Mark-Simulacrum commented Dec 12, 2020

ssomers commented Dec 12, 2020

ssomers commented Dec 13, 2020 •

edited

Loading

ssomers commented Dec 13, 2020

bors commented Dec 14, 2020

bors commented Dec 25, 2020

ssomers commented Dec 25, 2020 •

edited

Loading

Mark-Simulacrum commented Dec 26, 2020

bors commented Dec 26, 2020

bors commented Dec 26, 2020

bors commented Dec 26, 2020

BTreeMap: clean up access to MaybeUninit arrays #79520

BTreeMap: clean up access to MaybeUninit arrays #79520

Conversation

ssomers commented Nov 28, 2020 • edited Loading

This comment has been minimized.

Mark-Simulacrum Dec 8, 2020

Choose a reason for hiding this comment

ssomers Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 8, 2020

ssomers commented Dec 9, 2020

Mark-Simulacrum Dec 12, 2020

Choose a reason for hiding this comment

ssomers Dec 12, 2020

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 12, 2020

ssomers commented Dec 12, 2020

ssomers commented Dec 13, 2020 • edited Loading

ssomers commented Dec 13, 2020

bors commented Dec 14, 2020

bors commented Dec 25, 2020

ssomers commented Dec 25, 2020 • edited Loading

Mark-Simulacrum commented Dec 26, 2020

bors commented Dec 26, 2020

bors commented Dec 26, 2020

bors commented Dec 26, 2020

ssomers commented Nov 28, 2020 •

edited

Loading

ssomers Dec 9, 2020 •

edited

Loading

ssomers commented Dec 13, 2020 •

edited

Loading

ssomers commented Dec 25, 2020 •

edited

Loading