Don't allocate when creating an empty BTree #50352
Merged
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
rust-highfive
added
the
S-waiting-on-review
|
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 |
|
I was having problems getting tests to run locally and got impatient, so I guess I'm going to be fixing that. In order to make this more robust for future maintainers, I'm thinking of adding some type-state to Root so that you can't do write operations without first verifying that it's not the shared static root. For example, |
|
Wow this turned out really simple! That said I think you need to be careful with the empty singleton. If any BTreeMap with non-zst K/V tries to take a pointer to the first value or last key, this will construct an out-of-allocation pointer using ptr::offset (things like array indexing all lower to that), which is UB. In addition types with a higher alignment that 8 (or 4 on 32-bit) will access out-of-allocation if they try to take a pointer to the first key. You should take a quick audit for that kinda stuff. For instance any call to keys() will create such a pointer. This is why thin-vec has this awful mess: https://github.com/Gankro/thin-vec/blob/master/src/lib.rs#L99-L113 |
Gankra
reviewed
May 1, 2018
src/liballoc/btree/node.rs
Outdated
|
|
||
| /// We use `*const` as opposed to `*mut` so as to be covariant in `K` and `V`. | ||
| /// This either points to an actual node or is null. | ||
| parent: *const InternalNode<K, V>, |
There was a problem hiding this comment.
why not hoist the parent pointer too?
There was a problem hiding this comment.
I was trying to figure out what the ideal choice for alignment/packing would be, but this is probably more general (so it can see that it's null?)
src/liballoc/btree/node.rs
Outdated
| ptr::eq( | ||
| unsafe { self.node.as_ptr().as_ref() }, | ||
| &EMPTY_ROOT_NODE as *const LeafNode<(), ()> as *const LeafNode<K, V>, | ||
| ) |
There was a problem hiding this comment.
is there any reason to use ptr::eq and not just == ?
There was a problem hiding this comment.
(Also is the as_ref() necessary..?)
There was a problem hiding this comment.
ptr::eq just to be explicit about not deref-ing, but that's not necessary if they're raw pointers, and the as_ref was because I expected to compare the the reference not the raw pointer. I'll clean that up.
There was a problem hiding this comment.
Oh wait, yes, as_ref() is necessary because as_ptr() returns a NonNull<_>, so ptr::eq lets you avoid an extra pair of coercions.
I think .as_ptr().as_ptr() is better than .as_ptr().as_ref() though since then we don't need unsafe.
src/liballoc/btree/node.rs
Outdated
| BoxedNode::from_ptr(NonNull::from( | ||
| (&EMPTY_ROOT_NODE as *const LeafNode<(), ()> as *const LeafNode<K, V>) | ||
| .as_ref() | ||
| .unwrap(), |
There was a problem hiding this comment.
I would rather use the new_unchecked constructor for NonNull.
|
Ah, I suppose the alignment issue on that is why you recommended |
|
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 |
|
I think you can probably just get away with making key()/vals() do the |
|
So, presumably the logic behind that I can check if it's pointing to the shared root and then use |
|
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 |
arthurprs
reviewed
May 1, 2018
src/liballoc/btree/map.rs
Outdated
| @@ -687,6 +686,10 @@ impl<K: Ord, V> BTreeMap<K, V> { | |||
| /// ``` | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| pub fn insert(&mut self, key: K, value: V) -> Option<V> { | |||
| if self.root.is_shared_root() { | |||
There was a problem hiding this comment.
I think this can be removed as self.entry takes care of it.
src/liballoc/btree/node.rs
Outdated
| unsafe impl Sync for LeafNode<u64, u64> {} | ||
|
|
||
| // An empty node used as a placeholder for the root node, to avoid allocations | ||
| static EMPTY_ROOT_NODE: LeafNode<u64, u64> = LeafNode { |
There was a problem hiding this comment.
I assume this can't be a <(), ()> right? Can we add a small comment explaining?
src/liballoc/btree/node.rs
Outdated
| @@ -435,7 +472,9 @@ impl<K, V> NodeRef<marker::Owned, K, V, marker::Leaf> { | |||
| > { | |||
| let node = self.node; | |||
| let ret = self.ascend().ok(); | |||
| Global.dealloc(node.as_opaque(), Layout::new::<LeafNode<K, V>>()); | |||
| if !node.as_ref().is_shared_root() { | |||
There was a problem hiding this comment.
Is this ever false? Due to the way insertion works any non empty tree will not have a shared root.
There was a problem hiding this comment.
See the Drop impl on IntoIter, it calls leaf_node.deallocate_and_ascend() on the root, so it will always try to deallocate the root node. I could move that check into the Drop impl, but then that would require making more public functions available.
There was a problem hiding this comment.
I see, thanks for the explanation.
There was a problem hiding this comment.
It might be better to move this into the IntoIter Drop though, even if it takes another public method. It looks cleaner and map.rs is already aware of the shared root anyway.
arthurprs
reviewed
May 1, 2018
src/liballoc/btree/node.rs
Outdated
| @@ -97,8 +101,24 @@ impl<K, V> LeafNode<K, V> { | |||
| len: 0 | |||
| } | |||
| } | |||
|
|
|||
| unsafe fn is_shared_root(&self) -> bool { | |||
There was a problem hiding this comment.
Doesn't need to be unsafe
|
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 |
porglezomp
force-pushed
the
btree-no-empty-alloc
branch
from
a60c8f8 to
d2ff008
Compare
|
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 |
|
Ping from triage, @gankro — looks like there are new commits to review! |
shepmaster
added
S-waiting-on-author
|
But the PR is marked as WIP, so moving back to waiting on author. |
|
I was waiting for the CI issues to be fixed first |
This way we can safely statically allocate a LeafNode to use as the placeholder before allocating, and any type accessing it will be able to access the metadata at the same offset.
This gives a pointer to that static empty node instead of allocating a new node, and then whenever inserting makes sure that the root isn't that empty node.
We modify the drop implementation in IntoIter to not drop the shared root
This splits into_slices() into into_key_slice() and into_val_slice(). While the extra calls would get optimized out, this is a useful semantic change since we call keys() while iterating, and we don't want to construct and out-of-bounds val() pointer in the process if we happen to be pointing to the shared static root. This also paves the way for doing the alignment handling conditional differently for the keys and values.
porglezomp
force-pushed
the
btree-no-empty-alloc
branch
from
7650e2c to
f3a3599
Compare
|
@gankro CI issue is cleared up, and I squashed some of the commits that went back and forth (like swapping between |
|
I believe I could fix the The first means making the The second adds no size but is a horrendous scary type abuse (maybe UB? are you allowed to cast pointers to completely unrelated types?) making the |
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit e83c18f with merge 80f68b357b7eb9e7d5df79581a2d27e15c357229... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
1 similar comment
|
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 |
|
This seems to be the problem?
Doesn't seem related to the PR... |
|
@bors retry This is odd |
bors
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
May 14, 2018
It was introduced in rust-lang#50240 to avoid an allocation when creating a new BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.
Merged
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
May 15, 2018
…ertj Remove LazyBTreeMap. It was introduced in rust-lang#50240 to avoid an allocation when creating a new BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
May 16, 2018
…ertj Remove LazyBTreeMap. It was introduced in rust-lang#50240 to avoid an allocation when creating a new BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.
francesca64
added a commit
to francesca64/rust
that referenced
this pull request
Aug 6, 2019
In rust-lang#50352, `EMPTY_ROOT_NODE` was introduced to avoid allocating when creating an empty `BTreeMap`. `EMPTY_ROOT_NODE` was distinguished from regular nodes using pointer identity. However, `EMPTY_ROOT_NODE` can have multiple addresses if multiple copies of `liballoc` are used together, i.e. when interoperating with a dynamic library. In that situation, all sorts of scary things will happen, since it's possible for `EMPTY_ROOT_NODE` to be treated as a regular node. To fix that, this PR adds a flag to every node, and simply uses that flag to distinguish `EMPTY_ROOT_NODE` from regular nodes. This only increases the node size if `keys` previously used every byte of padding after `len`, or if every byte of remaining padding was previously used to also fit `vals`. In those cases, every node will grow by 1 word. This playground link can be used to easily test the space impact of this PR: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=87850055d6049be048f9e73f6a5055d6
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.
Following the discussion in #50266, this adds a static instance of
LeafNodethat empty BTrees point to, and then replaces it oninsert,append, andentry. This avoids allocating for empty maps.Fixes #50266
r? @gankro