Skip to content

Commit

Permalink
Rollup merge of rust-lang#66648 - crgl:btree-clone-from, r=Amanieu
Browse files Browse the repository at this point in the history
Implement clone_from for BTreeMap and BTreeSet

See rust-lang#28481. This results in up to 90% speedups on simple data types when `self` and `other` are the same size, and is generally comparable or faster. Some concerns:

1. This implementation requires an `Ord` bound on the `Clone` implementation for `BTreeMap` and `BTreeSet`. Since these structs can only be created externally for keys with `Ord` implemented, this should be fine? If not, there's certainly a less safe way to do this.
2. Changing `next_unchecked` on `RangeMut` to return mutable key references allows for replacing the entire overlapping portion of both maps without changing the external interface in any way. However, if `clone_from` fails it can leave the `BTreeMap` in an invalid state, which might be unacceptable.

~This probably needs an FCP since it changes a trait bound, but (as far as I know?) that change cannot break any external code.~
  • Loading branch information
Dylan-DPC authored Jan 30, 2020
2 parents 9ed29b6 + 6c3e477 commit 12c9562
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 9 deletions.
82 changes: 74 additions & 8 deletions src/liballoc/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,60 @@ impl<K: Clone, V: Clone> Clone for BTreeMap<K, V> {
clone_subtree(self.root.as_ref())
}
}

fn clone_from(&mut self, other: &Self) {
BTreeClone::clone_from(self, other);
}
}

trait BTreeClone {
fn clone_from(&mut self, other: &Self);
}

impl<K: Clone, V: Clone> BTreeClone for BTreeMap<K, V> {
default fn clone_from(&mut self, other: &Self) {
*self = other.clone();
}
}

impl<K: Clone + Ord, V: Clone> BTreeClone for BTreeMap<K, V> {
fn clone_from(&mut self, other: &Self) {
// This truncates `self` to `other.len()` by calling `split_off` on
// the first key after `other.len()` elements if it exists
let split_off_key = if self.len() > other.len() {
let diff = self.len() - other.len();
if diff <= other.len() {
self.iter().nth_back(diff - 1).map(|pair| (*pair.0).clone())
} else {
self.iter().nth(other.len()).map(|pair| (*pair.0).clone())
}
} else {
None
};
if let Some(key) = split_off_key {
self.split_off(&key);
}

let mut siter = self.range_mut(..);
let mut oiter = other.iter();
// After truncation, `self` is at most as long as `other` so this loop
// replaces every key-value pair in `self`. Since `oiter` is in sorted
// order and the structure of the `BTreeMap` stays the same,
// the BTree invariants are maintained at the end of the loop
while !siter.is_empty() {
if let Some((ok, ov)) = oiter.next() {
// SAFETY: This is safe because the `siter.front != siter.back` check
// ensures that `siter` is nonempty
let (sk, sv) = unsafe { siter.next_unchecked() };
sk.clone_from(ok);
sv.clone_from(ov);
} else {
break;
}
}
// If `other` is longer than `self`, the remaining elements are inserted
self.extend(oiter.map(|(k, v)| ((*k).clone(), (*v).clone())));
}
}

impl<K, Q: ?Sized> super::Recover<Q> for BTreeMap<K, ()>
Expand Down Expand Up @@ -1357,7 +1411,10 @@ impl<'a, K: 'a, V: 'a> Iterator for IterMut<'a, K, V> {
None
} else {
self.length -= 1;
unsafe { Some(self.range.next_unchecked()) }
unsafe {
let (k, v) = self.range.next_unchecked();
Some((k, v)) // coerce k from `&mut K` to `&K`
}
}
}

Expand Down Expand Up @@ -1736,7 +1793,14 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> {
type Item = (&'a K, &'a mut V);

fn next(&mut self) -> Option<(&'a K, &'a mut V)> {
if self.front == self.back { None } else { unsafe { Some(self.next_unchecked()) } }
if self.is_empty() {
None
} else {
unsafe {
let (k, v) = self.next_unchecked();
Some((k, v)) // coerce k from `&mut K` to `&K`
}
}
}

fn last(mut self) -> Option<(&'a K, &'a mut V)> {
Expand All @@ -1745,16 +1809,19 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> {
}

impl<'a, K, V> RangeMut<'a, K, V> {
unsafe fn next_unchecked(&mut self) -> (&'a K, &'a mut V) {
fn is_empty(&self) -> bool {
self.front == self.back
}

unsafe fn next_unchecked(&mut self) -> (&'a mut K, &'a mut V) {
let handle = ptr::read(&self.front);

let mut cur_handle = match handle.right_kv() {
Ok(kv) => {
self.front = ptr::read(&kv).right_edge();
// Doing the descend invalidates the references returned by `into_kv_mut`,
// so we have to do this last.
let (k, v) = kv.into_kv_mut();
return (k, v); // coerce k from `&mut K` to `&K`
return kv.into_kv_mut();
}
Err(last_edge) => {
let next_level = last_edge.into_node().ascend().ok();
Expand All @@ -1768,8 +1835,7 @@ impl<'a, K, V> RangeMut<'a, K, V> {
self.front = first_leaf_edge(ptr::read(&kv).right_edge().descend());
// Doing the descend invalidates the references returned by `into_kv_mut`,
// so we have to do this last.
let (k, v) = kv.into_kv_mut();
return (k, v); // coerce k from `&mut K` to `&K`
return kv.into_kv_mut();
}
Err(last_edge) => {
let next_level = last_edge.into_node().ascend().ok();
Expand All @@ -1783,7 +1849,7 @@ impl<'a, K, V> RangeMut<'a, K, V> {
#[stable(feature = "btree_range", since = "1.17.0")]
impl<'a, K, V> DoubleEndedIterator for RangeMut<'a, K, V> {
fn next_back(&mut self) -> Option<(&'a K, &'a mut V)> {
if self.front == self.back { None } else { unsafe { Some(self.next_back_unchecked()) } }
if self.is_empty() { None } else { unsafe { Some(self.next_back_unchecked()) } }
}
}

Expand Down
13 changes: 12 additions & 1 deletion src/liballoc/collections/btree/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,23 @@ use crate::collections::btree_map::{self, BTreeMap, Keys};
/// println!("{}", book);
/// }
/// ```
#[derive(Clone, Hash, PartialEq, Eq, Ord, PartialOrd)]
#[derive(Hash, PartialEq, Eq, Ord, PartialOrd)]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct BTreeSet<T> {
map: BTreeMap<T, ()>,
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: Clone> Clone for BTreeSet<T> {
fn clone(&self) -> Self {
BTreeSet { map: self.map.clone() }
}

fn clone_from(&mut self, other: &Self) {
self.map.clone_from(&other.map);
}
}

/// An iterator over the items of a `BTreeSet`.
///
/// This `struct` is created by the [`iter`] method on [`BTreeSet`].
Expand Down
20 changes: 20 additions & 0 deletions src/liballoc/tests/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,26 @@ fn test_clone() {
}
}

#[test]
fn test_clone_from() {
let mut map1 = BTreeMap::new();
let size = 30;

for i in 0..size {
let mut map2 = BTreeMap::new();
for j in 0..i {
let mut map1_copy = map2.clone();
map1_copy.clone_from(&map1);
assert_eq!(map1_copy, map1);
let mut map2_copy = map1.clone();
map2_copy.clone_from(&map2);
assert_eq!(map2_copy, map2);
map2.insert(100 * j + 1, 2 * j + 1);
}
map1.insert(i, 10 * i);
}
}

#[test]
#[allow(dead_code)]
fn test_variance() {
Expand Down

0 comments on commit 12c9562

Please sign in to comment.