From 669bd8223bf159d757d0c552a4c413a137bc6b10 Mon Sep 17 00:00:00 2001 From: C Jones Date: Mon, 30 Apr 2018 15:24:59 -0400 Subject: [PATCH 1/7] Make LeafNode #[repr(C)] and put the metadata before generic items 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. --- src/liballoc/btree/node.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index d6346662314e6..3e27331aa3813 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -60,12 +60,12 @@ pub const CAPACITY: usize = 2 * B - 1; /// /// See also rust-lang/rfcs#197, which would make this structure significantly more safe by /// avoiding accidentally dropping unused and uninitialized keys and values. +/// +/// We put the metadata first so that its position is the same for every `K` and `V`, in order +/// to statically allocate a single dummy node to avoid allocations. This struct is `repr(C)` to +/// prevent them from being reordered. +#[repr(C)] struct LeafNode { - /// The arrays storing the actual data of the node. Only the first `len` elements of each - /// array are initialized and valid. - keys: [K; CAPACITY], - vals: [V; CAPACITY], - /// 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, @@ -77,10 +77,14 @@ struct LeafNode { /// The number of keys and values this node stores. /// - /// This is at the end of the node's representation and next to `parent_idx` to encourage - /// the compiler to join `len` and `parent_idx` into the same 32-bit word, reducing space - /// overhead. + /// This next to `parent_idx` to encourage the compiler to join `len` and + /// `parent_idx` into the same 32-bit word, reducing space overhead. len: u16, + + /// The arrays storing the actual data of the node. Only the first `len` elements of each + /// array are initialized and valid. + keys: [K; CAPACITY], + vals: [V; CAPACITY], } impl LeafNode { From ef6060c863c86e1422baa2cc85ae75af22feaf51 Mon Sep 17 00:00:00 2001 From: C Jones Date: Mon, 30 Apr 2018 17:34:14 -0400 Subject: [PATCH 2/7] Add a statically allocated empty node for empty maps 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. --- src/liballoc/btree/map.rs | 15 +++++++++++++-- src/liballoc/btree/node.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/liballoc/btree/map.rs b/src/liballoc/btree/map.rs index 3984379ea860d..985a41722d7cf 100644 --- a/src/liballoc/btree/map.rs +++ b/src/liballoc/btree/map.rs @@ -523,7 +523,7 @@ impl BTreeMap { #[stable(feature = "rust1", since = "1.0.0")] pub fn new() -> BTreeMap { BTreeMap { - root: node::Root::new_leaf(), + root: node::Root::shared_empty_root(), length: 0, } } @@ -544,7 +544,6 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn clear(&mut self) { - // FIXME(gereeter) .clear() allocates *self = BTreeMap::new(); } @@ -687,6 +686,10 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn insert(&mut self, key: K, value: V) -> Option { + if self.root.is_shared_root() { + self.root = node::Root::new_leaf(); + } + match self.entry(key) { Occupied(mut entry) => Some(entry.insert(value)), Vacant(entry) => { @@ -890,6 +893,10 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn entry(&mut self, key: K) -> Entry { + if self.root.is_shared_root() { + self.root = node::Root::new_leaf(); + } + match search::search_tree(self.root.as_mut(), &key) { Found(handle) => { Occupied(OccupiedEntry { @@ -910,6 +917,10 @@ impl BTreeMap { } fn from_sorted_iter>(&mut self, iter: I) { + if self.root.is_shared_root() { + self.root = node::Root::new_leaf(); + } + let mut cur_node = last_leaf_edge(self.root.as_mut()).into_node(); // Iterate through all key-value pairs, pushing them into nodes at the right level. for (key, value) in iter { diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index 3e27331aa3813..6381006e7fced 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -103,6 +103,18 @@ impl LeafNode { } } +// We need to implement Sync here in order to make a static instance +unsafe impl Sync for LeafNode<(), ()> {} + +// An empty node used as a placeholder for the root node, to avoid allocations +static EMPTY_ROOT_NODE: LeafNode<(), ()> = LeafNode { + parent: ptr::null(), + parent_idx: 0, + len: 0, + keys: [(); CAPACITY], + vals: [(); CAPACITY], +}; + /// The underlying representation of internal nodes. As with `LeafNode`s, these should be hidden /// behind `BoxedNode`s to prevent dropping uninitialized keys and values. Any pointer to an /// `InternalNode` can be directly casted to a pointer to the underlying `LeafNode` portion of the @@ -172,6 +184,24 @@ unsafe impl Sync for Root { } unsafe impl Send for Root { } impl Root { + pub fn is_shared_root(&self) -> bool { + ptr::eq( + self.node.as_ptr().as_ptr(), + &EMPTY_ROOT_NODE as *const _ as *const LeafNode, + ) + } + + pub fn shared_empty_root() -> Self { + Root { + node: unsafe { + BoxedNode::from_ptr(NonNull::new_unchecked( + &EMPTY_ROOT_NODE as *const _ as *const LeafNode as *mut _ + )) + }, + height: 0, + } + } + pub fn new_leaf() -> Self { Root { node: BoxedNode::from_leaf(Box::new(unsafe { LeafNode::new() })), From fa62eba92ad9a3d25b200835a5cd3ca48b700d75 Mon Sep 17 00:00:00 2001 From: C Jones Date: Tue, 1 May 2018 00:21:30 -0400 Subject: [PATCH 3/7] Don't drop the shared static node We modify the drop implementation in IntoIter to not drop the shared root --- src/liballoc/btree/map.rs | 8 ++++---- src/liballoc/btree/node.rs | 13 +++++++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/liballoc/btree/map.rs b/src/liballoc/btree/map.rs index 985a41722d7cf..a88631b1e6751 100644 --- a/src/liballoc/btree/map.rs +++ b/src/liballoc/btree/map.rs @@ -686,10 +686,6 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn insert(&mut self, key: K, value: V) -> Option { - if self.root.is_shared_root() { - self.root = node::Root::new_leaf(); - } - match self.entry(key) { Occupied(mut entry) => Some(entry.insert(value)), Vacant(entry) => { @@ -1301,6 +1297,10 @@ impl Drop for IntoIter { self.for_each(drop); unsafe { let leaf_node = ptr::read(&self.front).into_node(); + if leaf_node.is_shared_root() { + return; + } + if let Some(first_parent) = leaf_node.deallocate_and_ascend() { let mut cur_node = first_parent.into_node(); while let Some(parent) = cur_node.deallocate_and_ascend() { diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index 6381006e7fced..79615e11c6724 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -101,6 +101,10 @@ impl LeafNode { len: 0 } } + + fn is_shared_root(&self) -> bool { + self as *const _ == &EMPTY_ROOT_NODE as *const _ as *const LeafNode + } } // We need to implement Sync here in order to make a static instance @@ -185,10 +189,7 @@ unsafe impl Send for Root { } impl Root { pub fn is_shared_root(&self) -> bool { - ptr::eq( - self.node.as_ptr().as_ptr(), - &EMPTY_ROOT_NODE as *const _ as *const LeafNode, - ) + self.as_ref().is_shared_root() } pub fn shared_empty_root() -> Self { @@ -387,6 +388,10 @@ impl NodeRef { } } + pub fn is_shared_root(&self) -> bool { + self.as_leaf().is_shared_root() + } + pub fn keys(&self) -> &[K] { self.reborrow().into_slices().0 } From 5b94e9f053c3fecb0e29c89e453ecaf07d97218c Mon Sep 17 00:00:00 2001 From: C Jones Date: Tue, 1 May 2018 15:26:49 -0400 Subject: [PATCH 4/7] Split into_slices() to avoid making extra slices 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. --- src/liballoc/btree/node.rs | 66 +++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index 79615e11c6724..4dcc4d54eaf01 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -393,11 +393,11 @@ impl NodeRef { } pub fn keys(&self) -> &[K] { - self.reborrow().into_slices().0 + self.reborrow().into_key_slice() } - pub fn vals(&self) -> &[V] { - self.reborrow().into_slices().1 + fn vals(&self) -> &[V] { + self.reborrow().into_val_slice() } /// Finds the parent of the current node. Returns `Ok(handle)` if the current @@ -540,29 +540,37 @@ impl<'a, K, V, Type> NodeRef, K, V, Type> { } pub fn keys_mut(&mut self) -> &mut [K] { - unsafe { self.reborrow_mut().into_slices_mut().0 } + unsafe { self.reborrow_mut().into_key_slice_mut() } } pub fn vals_mut(&mut self) -> &mut [V] { - unsafe { self.reborrow_mut().into_slices_mut().1 } + unsafe { self.reborrow_mut().into_val_slice_mut() } } } impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { - pub fn into_slices(self) -> (&'a [K], &'a [V]) { + fn into_key_slice(self) -> &'a [K] { unsafe { - ( - slice::from_raw_parts( - self.as_leaf().keys.as_ptr(), - self.len() - ), - slice::from_raw_parts( - self.as_leaf().vals.as_ptr(), - self.len() - ) + slice::from_raw_parts( + self.as_leaf().keys.as_ptr(), + self.len() + ) + } + } + + fn into_val_slice(self) -> &'a [V] { + unsafe { + slice::from_raw_parts( + self.as_leaf().vals.as_ptr(), + self.len() ) } } + + fn into_slices(self) -> (&'a [K], &'a [V]) { + let k = unsafe { ptr::read(&self) }; + (k.into_key_slice(), self.into_val_slice()) + } } impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { @@ -574,20 +582,28 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { } } - pub fn into_slices_mut(mut self) -> (&'a mut [K], &'a mut [V]) { + fn into_key_slice_mut(mut self) -> &'a mut [K] { unsafe { - ( - slice::from_raw_parts_mut( - &mut self.as_leaf_mut().keys as *mut [K] as *mut K, - self.len() - ), - slice::from_raw_parts_mut( - &mut self.as_leaf_mut().vals as *mut [V] as *mut V, - self.len() - ) + slice::from_raw_parts_mut( + &mut self.as_leaf_mut().keys as *mut [K] as *mut K, + self.len() + ) + } + } + + fn into_val_slice_mut(mut self) -> &'a mut [V] { + unsafe { + slice::from_raw_parts_mut( + &mut self.as_leaf_mut().vals as *mut [V] as *mut V, + self.len() ) } } + + fn into_slices_mut(self) -> (&'a mut [K], &'a mut [V]) { + let k = unsafe { ptr::read(&self) }; + (k.into_key_slice_mut(), self.into_val_slice_mut()) + } } impl<'a, K, V> NodeRef, K, V, marker::Leaf> { From ddacf037fdc8bfb845bde2ce41ea4b9b6de445c7 Mon Sep 17 00:00:00 2001 From: C Jones Date: Tue, 1 May 2018 16:37:07 -0400 Subject: [PATCH 5/7] Make into_key_slice avoid taking out-of-bounds pointers --- src/liballoc/btree/node.rs | 48 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index 4dcc4d54eaf01..ce770d384f8d5 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -107,10 +107,12 @@ impl LeafNode { } } -// We need to implement Sync here in order to make a static instance +// We need to implement Sync here in order to make a static instance. unsafe impl Sync for LeafNode<(), ()> {} -// An empty node used as a placeholder for the root node, to avoid allocations +// An empty node used as a placeholder for the root node, to avoid allocations. +// We use () in order to save space, since no operation on an empty tree will +// ever take a pointer past the first key. static EMPTY_ROOT_NODE: LeafNode<(), ()> = LeafNode { parent: ptr::null(), parent_idx: 0, @@ -539,26 +541,39 @@ impl<'a, K, V, Type> NodeRef, K, V, Type> { } } - pub fn keys_mut(&mut self) -> &mut [K] { + fn keys_mut(&mut self) -> &mut [K] { unsafe { self.reborrow_mut().into_key_slice_mut() } } - pub fn vals_mut(&mut self) -> &mut [V] { + fn vals_mut(&mut self) -> &mut [V] { unsafe { self.reborrow_mut().into_val_slice_mut() } } } impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { fn into_key_slice(self) -> &'a [K] { - unsafe { - slice::from_raw_parts( - self.as_leaf().keys.as_ptr(), - self.len() - ) + // When taking a pointer to the keys, if our key has a stricter + // alignment requirement than the shared root does, then the pointer + // would be out of bounds, which LLVM assumes will not happen. If the + // alignment is more strict, we need to make an empty slice that doesn't + // use an out of bounds pointer. + if mem::align_of::() > mem::align_of::>() && self.is_shared_root() { + &[] + } else { + // Here either it's not the root, or the alignment is less strict, + // in which case the keys pointer will point "one-past-the-end" of + // the node, which is allowed by LLVM. + unsafe { + slice::from_raw_parts( + self.as_leaf().keys.as_ptr(), + self.len() + ) + } } } fn into_val_slice(self) -> &'a [V] { + debug_assert!(!self.is_shared_root()); unsafe { slice::from_raw_parts( self.as_leaf().vals.as_ptr(), @@ -583,15 +598,20 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { } fn into_key_slice_mut(mut self) -> &'a mut [K] { - unsafe { - slice::from_raw_parts_mut( - &mut self.as_leaf_mut().keys as *mut [K] as *mut K, - self.len() - ) + if mem::align_of::() > mem::align_of::>() && self.is_shared_root() { + &mut [] + } else { + unsafe { + slice::from_raw_parts_mut( + &mut self.as_leaf_mut().keys as *mut [K] as *mut K, + self.len() + ) + } } } fn into_val_slice_mut(mut self) -> &'a mut [V] { + debug_assert!(!self.is_shared_root()); unsafe { slice::from_raw_parts_mut( &mut self.as_leaf_mut().vals as *mut [V] as *mut V, From f3a3599e090cb6aa63a327351738d7633c934728 Mon Sep 17 00:00:00 2001 From: C Jones Date: Mon, 7 May 2018 19:42:28 -0400 Subject: [PATCH 6/7] Add debug asserts and fix some violations --- src/liballoc/btree/map.rs | 5 +++++ src/liballoc/btree/node.rs | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/liballoc/btree/map.rs b/src/liballoc/btree/map.rs index a88631b1e6751..7ed7fd204fd99 100644 --- a/src/liballoc/btree/map.rs +++ b/src/liballoc/btree/map.rs @@ -246,6 +246,9 @@ impl super::Recover for BTreeMap } fn replace(&mut self, key: K) -> Option { + if self.root.is_shared_root() { + self.root = node::Root::new_leaf(); + } match search::search_tree::(self.root.as_mut(), &key) { Found(handle) => Some(mem::replace(handle.into_kv_mut().0, key)), GoDown(handle) => { @@ -889,6 +892,7 @@ impl BTreeMap { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn entry(&mut self, key: K) -> Entry { + // FIXME(@porglezomp) Avoid allocating if we don't insert if self.root.is_shared_root() { self.root = node::Root::new_leaf(); } @@ -1026,6 +1030,7 @@ impl BTreeMap { let total_num = self.len(); let mut right = Self::new(); + right.root = node::Root::new_leaf(); for _ in 0..(self.root.as_ref().height()) { right.root.push_level(); } diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index ce770d384f8d5..17eee65178e00 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -195,6 +195,10 @@ impl Root { } pub fn shared_empty_root() -> Self { + // Ensuring that the shared node hasn't been corrupted by any mutations + debug_assert!(EMPTY_ROOT_NODE.parent == ptr::null()); + debug_assert!(EMPTY_ROOT_NODE.parent_idx == 0); + debug_assert!(EMPTY_ROOT_NODE.len == 0); Root { node: unsafe { BoxedNode::from_ptr(NonNull::new_unchecked( @@ -246,6 +250,7 @@ impl Root { /// new node the root. This increases the height by 1 and is the opposite of `pop_level`. pub fn push_level(&mut self) -> NodeRef { + debug_assert!(!self.is_shared_root()); let mut new_node = Box::new(unsafe { InternalNode::new() }); new_node.edges[0] = unsafe { BoxedNode::from_ptr(self.node.as_ptr()) }; @@ -474,6 +479,7 @@ impl NodeRef { marker::Edge > > { + debug_assert!(!self.is_shared_root()); let node = self.node; let ret = self.ascend().ok(); Global.dealloc(node.as_opaque(), Layout::new::>()); @@ -631,6 +637,7 @@ impl<'a, K, V> NodeRef, K, V, marker::Leaf> { pub fn push(&mut self, key: K, val: V) { // Necessary for correctness, but this is an internal module debug_assert!(self.len() < CAPACITY); + debug_assert!(!self.is_shared_root()); let idx = self.len(); @@ -646,6 +653,7 @@ impl<'a, K, V> NodeRef, K, V, marker::Leaf> { pub fn push_front(&mut self, key: K, val: V) { // Necessary for correctness, but this is an internal module debug_assert!(self.len() < CAPACITY); + debug_assert!(!self.is_shared_root()); unsafe { slice_insert(self.keys_mut(), 0, key); @@ -959,6 +967,7 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Edge fn insert_fit(&mut self, key: K, val: V) -> *mut V { // Necessary for correctness, but in a private module debug_assert!(self.node.len() < CAPACITY); + debug_assert!(!self.node.is_shared_root()); unsafe { slice_insert(self.node.keys_mut(), self.idx, key); @@ -1136,6 +1145,7 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::KV> /// allocated node. pub fn split(mut self) -> (NodeRef, K, V, marker::Leaf>, K, V, Root) { + debug_assert!(!self.node.is_shared_root()); unsafe { let mut new_node = Box::new(LeafNode::new()); @@ -1173,6 +1183,7 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::KV> /// now adjacent key/value pairs to the left and right of this handle. pub fn remove(mut self) -> (Handle, K, V, marker::Leaf>, marker::Edge>, K, V) { + debug_assert!(!self.node.is_shared_root()); unsafe { let k = slice_remove(self.node.keys_mut(), self.idx); let v = slice_remove(self.node.vals_mut(), self.idx); From e83c18f91d373592ecf7a0fbbc24d7597925af13 Mon Sep 17 00:00:00 2001 From: C Jones Date: Tue, 8 May 2018 13:28:49 -0400 Subject: [PATCH 7/7] Make an ensure_root_is_owned method to reduce duplication Also remove some unnecessary debug_assert! when creating the shared root, since the root should be stored in the rodata and thus be impossible to accidentally modify. --- src/liballoc/btree/map.rs | 21 ++++++++++----------- src/liballoc/btree/node.rs | 4 ---- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/liballoc/btree/map.rs b/src/liballoc/btree/map.rs index 7ed7fd204fd99..bb2c68a27ba30 100644 --- a/src/liballoc/btree/map.rs +++ b/src/liballoc/btree/map.rs @@ -246,9 +246,7 @@ impl super::Recover for BTreeMap } fn replace(&mut self, key: K) -> Option { - if self.root.is_shared_root() { - self.root = node::Root::new_leaf(); - } + self.ensure_root_is_owned(); match search::search_tree::(self.root.as_mut(), &key) { Found(handle) => Some(mem::replace(handle.into_kv_mut().0, key)), GoDown(handle) => { @@ -893,10 +891,7 @@ impl BTreeMap { #[stable(feature = "rust1", since = "1.0.0")] pub fn entry(&mut self, key: K) -> Entry { // FIXME(@porglezomp) Avoid allocating if we don't insert - if self.root.is_shared_root() { - self.root = node::Root::new_leaf(); - } - + self.ensure_root_is_owned(); match search::search_tree(self.root.as_mut(), &key) { Found(handle) => { Occupied(OccupiedEntry { @@ -917,10 +912,7 @@ impl BTreeMap { } fn from_sorted_iter>(&mut self, iter: I) { - if self.root.is_shared_root() { - self.root = node::Root::new_leaf(); - } - + self.ensure_root_is_owned(); let mut cur_node = last_leaf_edge(self.root.as_mut()).into_node(); // Iterate through all key-value pairs, pushing them into nodes at the right level. for (key, value) in iter { @@ -1165,6 +1157,13 @@ impl BTreeMap { self.fix_top(); } + + /// If the root node is the shared root node, allocate our own node. + fn ensure_root_is_owned(&mut self) { + if self.root.is_shared_root() { + self.root = node::Root::new_leaf(); + } + } } #[stable(feature = "rust1", since = "1.0.0")] diff --git a/src/liballoc/btree/node.rs b/src/liballoc/btree/node.rs index 17eee65178e00..431695c32ab68 100644 --- a/src/liballoc/btree/node.rs +++ b/src/liballoc/btree/node.rs @@ -195,10 +195,6 @@ impl Root { } pub fn shared_empty_root() -> Self { - // Ensuring that the shared node hasn't been corrupted by any mutations - debug_assert!(EMPTY_ROOT_NODE.parent == ptr::null()); - debug_assert!(EMPTY_ROOT_NODE.parent_idx == 0); - debug_assert!(EMPTY_ROOT_NODE.len == 0); Root { node: unsafe { BoxedNode::from_ptr(NonNull::new_unchecked(