Remove leaf bias from BTreeMap's BoxedNode and NodeRef #67980
Closed
Conversation
This file contains 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? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @rkruppe |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
hanna-kruppe
suggested changes
Jan 8, 2020
| /// `Box<InternalNode<K, V>>`. However, it contains no information as to which of the two types | ||
| /// of nodes is actually behind the box, and, partially due to this lack of information, has no | ||
| /// destructor. | ||
| /// `Box<InternalNode<K, V>>` or `Box<NodeHeader<(), ())` (i.e. EMPTY_ROOT_NODE). |
There was a problem hiding this comment.
The shared root is neither boxed nor uniquely owned by one BoxedNode, so although it's good to note that this pointer can refer to the shared root, wording it like this is misleading IMO. I'd suggest the following outline instead:
- This is usually an owned pointer to a LeafNode/InternalNode, but it can also be a pointer (no ownership) to EMPTY_ROOT_NODE
- No destructor in part because we don't know which of these variants it is
- All of these types have a
NodeHeader<K, V>prefix (evenEMPTY_ROOT: NodeHeader<(), ()>), so that's the pointee type we store
There was a problem hiding this comment.
I'm confused by and can't find a description of "prefix" in a pointer context. Does that mean the data it points at always has at least the size and layout of NodeHeader<K, V>? Not that I know any better word for it. Does "prefix" also imply that pointer alignment is at least as high as that of NodeHeader<K, V>?
There was a problem hiding this comment.
To be precise, I mean that all of InternalNode<K, V>, LeafNode<K, V> and NodeHeader<(), ()>:
- are at least as large as
NodeHeader<K, V> - have alignment at least as large as
NodeHeader<K, V> - store the same kinds of data, at the same offsets, as a
NodeHeader<K, V>in the firstsize_of::<NodeHeader<K, V>>
This is the reason why as_header() is unconditionally safe. Alignment in particular is probably worth calling out separately, as it's not really implied by "prefix" (e.g., you might say i32 is a prefix of #[repr(C)] struct { a: i32, b: i64 } but alignment differs).
| } | ||
|
|
||
| impl<K, V> BoxedNode<K, V> { | ||
| fn from_leaf(node: Box<LeafNode<K, V>>) -> Self { | ||
| BoxedNode { ptr: Box::into_unique(node) } | ||
| unsafe { | ||
| BoxedNode { ptr: Unique::new_unchecked(Box::into_raw(node) as *mut NodeHeader<K, V>) } |
There was a problem hiding this comment.
I see that this is consistent with from_internal but I'm tempted to suggest rewriting both into Box::into_unique(node).cast() to get rid of the gratuitous unsafe.
There was a problem hiding this comment.
Done, and alloc tests and Miri are happy
| @@ -510,7 +513,7 @@ impl<'a, K, V, Type> NodeRef<marker::Mut<'a>, K, V, Type> { | |||
| /// This also implies you can invoke this member on the shared root, but the resulting pointer | |||
| /// might not be properly aligned and definitely would not allow accessing keys and values. | |||
| fn as_leaf_mut(&mut self) -> *mut LeafNode<K, V> { | |||
| self.node.as_ptr() | |||
| unsafe { &mut *(self.node.as_ptr() as *mut LeafNode<K, V>) } | |||
There was a problem hiding this comment.
Creating a reference here risks UB and is also completely pointless. Just use self.node.as_ptr() as *mut LeafNode<K, V>.
There was a problem hiding this comment.
Done, and alloc tests and Miri are happy
| @@ -204,7 +207,7 @@ impl<K, V> Root<K, V> { | |||
| Root { | |||
| node: unsafe { | |||
| BoxedNode::from_ptr(NonNull::new_unchecked( | |||
| &EMPTY_ROOT_NODE as *const _ as *const LeafNode<K, V> as *mut _, | |||
| &EMPTY_ROOT_NODE as *const _ as *const NodeHeader<K, V> as *mut _, | |||
There was a problem hiding this comment.
Nit: can now be simplified to NonNull::from(&EMPTY_ROOT_NODE).
There was a problem hiding this comment.
The compiler and I think you're wrong there: something must be done to transmute the NodeHeader<(), ()> to NodeHeader<K, V>. Why we also have to tag it as mutable, beats me.
Earlier, I got off on a tangent to adjust the pointer in BoxedNode to this actual type of EMPTY_ROOT_NODE, which implies removing K and V altogether, or pretending to understand PhantomData, but I managed to squeeze that monster back into the box before it ate me.
There was a problem hiding this comment.
I forgot about the type parameters, mea culpa. NonNull::from(&EMPTY_ROOT_NODE).cast() should work, though.
|
Meanwhile, I managed to reproduce and hopefully fix the failing pr build. Awaiting a long build. |
ssomers
force-pushed
the
btreemap_without_leaf_bias
branch
from
53450ce
to
1d00b87
Compare
ssomers
commented
Jan 8, 2020
ssomers
commented
Jan 8, 2020
ssomers
commented
Jan 8, 2020
|
I know nothing about the gdb pretty printing, so I would prefer if the changes to it were kept to the bare minimum required by the other changes in this PR, or alternatively, if someone who knows anything about the pretty printers reviewed those parts. |
|
I'll first submit a PR fixing and testing pretty printing as is now, and they can judge whether the near-invalid memory access is cool or cripple. |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: |
|
This PR is now blocked by #68098 and won't automatically merge after it (or will need changes). As far as I know, I've addressed all change requests. |
ssomers
force-pushed
the
btreemap_without_leaf_bias
branch
from
1d00b87
to
02e9595
Compare
|
Nothing to see here, folks. I just played around in the GitHub interface and rebased on the #68098 commit. |
ssomers
force-pushed
the
btreemap_without_leaf_bias
branch
from
02e9595
to
977782b
Compare
JohnCSimon
added
S-waiting-on-review
hanna-kruppe
added
S-blocked
|
Yes, AFAIK there's nothing to be done here before #68098 is merged and once that's done this PR can be rebased and most likely merged (NB: the first commit here has a different hash than the commit in that PR, so some manual |
|
Well spotted, I think that before rebasing this one on that one, I rebased that one on master and forgot to push. |
|
Closing for now (and it looks to be a very long time) because being based on another PR's commit misguides GitHub. |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Mar 8, 2020
…, r=Mark-Simulacrum More documentation and simplification of BTreeMap's internals Salvage the documentation and simplification from rust-lang#67980, without changing the type locked down by debuginfo. r? @rkruppe
Centril
added a commit
to Centril/rust
that referenced
this pull request
Mar 8, 2020
…, r=Mark-Simulacrum More documentation and simplification of BTreeMap's internals Salvage the documentation and simplification from rust-lang#67980, without changing the type locked down by debuginfo. r? @rkruppe
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Improve readability - there is no genuine problem here, after #56648.
BoxedNodeandNodeRefcontain a pointer to a node that is typed asLeafNode. In case of EMPTY_ROOT_NODE, that pointer may not be properly aligned and does not point to anything matchingLeafNode<K, V>. OnlyNodeRef's documentation mentions this.It seemed more logical to me to give these pointers as type the least common denominator,
NodeHeader<K, V>(which only exists since #56648). That simplifies the simplest accessas_headerand levels the playing field for leaf and internal nodes.One could argue that most nodes are in fact leaves, and most access to nodes is to their leaf-portion, keys and values. Then I'd still like to fix the documentation of
BoxedNode.