Skip to content

BTreeMap: clarify comments and panics around choose_parent_kv #79376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions library/alloc/src/collections/btree/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1282,30 +1282,32 @@ impl<'a, K, V> NodeRef<marker::Mut<'a>, K, V, marker::LeafOrInternal> {
/// Chooses a balancing context involving the node as a child, thus between
/// the KV immediately to the left or to the right in the parent node.
/// Returns an `Err` if there is no parent.
/// Panics if the parent is empty.
///
/// This method optimizes for a node that has fewer elements than its left
/// and right siblings, if they exist, by preferring the left parent KV.
/// Merging with the left sibling is faster, since we only need to move
/// Prefers the left side, to be optimal if the given node is somehow
/// underfull, meaning here only that it has fewer elements than its left
/// sibling and than its right sibling, if they exist. In that case,
/// merging with the left sibling is faster, since we only need to move
/// the node's N elements, instead of shifting them to the right and moving
/// more than N elements in front. Stealing from the left sibling is also
/// typically faster, since we only need to shift the node's N elements to
/// the right, instead of shifting at least N of the sibling's elements to
/// the left.
pub fn choose_parent_kv(self) -> Result<LeftOrRight<BalancingContext<'a, K, V>>, Self> {
match unsafe { ptr::read(&self) }.ascend() {
Ok(parent) => match parent.left_kv() {
Ok(parent_edge) => match parent_edge.left_kv() {
Ok(left_parent_kv) => Ok(LeftOrRight::Left(BalancingContext {
parent: unsafe { ptr::read(&left_parent_kv) },
left_child: left_parent_kv.left_edge().descend(),
right_child: self,
})),
Err(parent) => match parent.right_kv() {
Err(parent_edge) => match parent_edge.right_kv() {
Ok(right_parent_kv) => Ok(LeftOrRight::Right(BalancingContext {
parent: unsafe { ptr::read(&right_parent_kv) },
left_child: self,
right_child: right_parent_kv.right_edge().descend(),
})),
Err(_) => unreachable!("empty non-root node"),
Err(_) => unreachable!("empty internal node"),
},
},
Err(root) => Err(root),
Expand Down
12 changes: 6 additions & 6 deletions library/alloc/src/collections/btree/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,12 @@ impl<'a, K: 'a, V: 'a> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, mark
pos = unsafe { new_pos.cast_to_leaf_unchecked() };

// Only if we merged, the parent (if any) has shrunk, but skipping
// the following step does not pay off in benchmarks.
// the following step otherwise does not pay off in benchmarks.
//
// SAFETY: We won't destroy or rearrange the leaf where `pos` is at
// by handling its parent recursively; at worst we will destroy or
// rearrange the parent through the grandparent, thus change the
// leaf's parent pointer.
// link to the parent inside the leaf.
if let Ok(parent) = unsafe { pos.reborrow_mut() }.into_node().ascend() {
parent.into_node().handle_shrunk_node_recursively(handle_emptied_internal_root);
}
Expand Down Expand Up @@ -92,8 +92,8 @@ impl<'a, K: 'a, V: 'a> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Internal>,
}

impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, K, V, marker::Internal> {
/// Stocks up a possibly underfull internal node, recursively.
/// Climbs up until it reaches an ancestor that has elements to spare or the root.
/// Stocks up a possibly underfull internal node and its ancestors,
/// until it reaches an ancestor that has elements to spare or is the root.
fn handle_shrunk_node_recursively<F: FnOnce()>(mut self, handle_emptied_internal_root: F) {
loop {
self = match self.len() {
Expand Down Expand Up @@ -124,7 +124,7 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, K, V, marker::Internal> {
) -> Option<NodeRef<marker::Mut<'a>, K, V, marker::Internal>> {
match self.forget_type().choose_parent_kv() {
Ok(Left(left_parent_kv)) => {
debug_assert!(left_parent_kv.right_child_len() == MIN_LEN - 1);
debug_assert_eq!(left_parent_kv.right_child_len(), MIN_LEN - 1);
if left_parent_kv.can_merge() {
let pos = left_parent_kv.merge(None);
let parent_edge = unsafe { unwrap_unchecked(pos.into_node().ascend().ok()) };
Expand All @@ -136,7 +136,7 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, K, V, marker::Internal> {
}
}
Ok(Right(right_parent_kv)) => {
debug_assert!(right_parent_kv.left_child_len() == MIN_LEN - 1);
debug_assert_eq!(right_parent_kv.left_child_len(), MIN_LEN - 1);
if right_parent_kv.can_merge() {
let pos = right_parent_kv.merge(None);
let parent_edge = unsafe { unwrap_unchecked(pos.into_node().ascend().ok()) };
Expand Down