Skip to content
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

Reoptimize VecDeque::append #53564

Merged
merged 5 commits into from
Aug 29, 2018
Merged
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
52 changes: 47 additions & 5 deletions src/liballoc/collections/vec_deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

use core::cmp::Ordering;
use core::fmt;
use core::isize;
use core::iter::{repeat, FromIterator, FusedIterator};
use core::mem;
use core::ops::Bound::{Excluded, Included, Unbounded};
Expand Down Expand Up @@ -202,6 +203,33 @@ impl<T> VecDeque<T> {
len);
}

/// Copies all values from `src` to the back of `self`, wrapping around if needed.
///
/// # Safety
///
/// The capacity must be sufficient to hold self.len() + src.len() elements.
/// If so, this function never panics.
#[inline]
unsafe fn copy_slice(&mut self, src: &[T]) {
/// This is guaranteed by `RawVec`.
debug_assert!(self.capacity() <= isize::MAX as usize);

let expected_new_len = self.len() + src.len();
debug_assert!(self.capacity() >= expected_new_len);

let dst_high_ptr = self.ptr().add(self.head);
let dst_high_len = self.cap() - self.head;

let split = cmp::min(src.len(), dst_high_len);
let (src_high, src_low) = src.split_at(split);

ptr::copy_nonoverlapping(src_high.as_ptr(), dst_high_ptr, src_high.len());
ptr::copy_nonoverlapping(src_low.as_ptr(), self.ptr(), src_low.len());

self.head = self.wrap_add(self.head, src.len());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the head wrap over the tail ? Maybe it might be worth it to comment what the implementation is doing and why it works. Also maybe it might be worth it to add a debug_assert! that checks that the head does not wrap over the tail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the capacity is sufficient to hold src.len() new elements, this can't wrap over the tail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh, head points to the first element where data can be written, and tail to the first element that can be read. So this makes sense now, thanks.

debug_assert!(self.len() == expected_new_len);
}

/// Copies a potentially wrapping block of memory len long from src to dest.
/// (abs(dst - src) + len) must be no larger than cap() (There must be at
/// most one continuous overlapping region between src and dest).
Expand Down Expand Up @@ -1024,7 +1052,7 @@ impl<T> VecDeque<T> {
iter: Iter {
tail: drain_tail,
head: drain_head,
ring: unsafe { self.buffer_as_mut_slice() },
ring: unsafe { self.buffer_as_slice() },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to triple check - this change has nothing to do with the other changes right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is just a consistency change.

},
}
}
Expand Down Expand Up @@ -1834,8 +1862,22 @@ impl<T> VecDeque<T> {
#[inline]
#[stable(feature = "append", since = "1.4.0")]
pub fn append(&mut self, other: &mut Self) {
// naive impl
self.extend(other.drain(..));
unsafe {
// Guarantees there is space in `self` for `other`.
self.reserve(other.len());

{
let (src_high, src_low) = other.as_slices();

// This is only safe because copy_slice never panics when capacity is sufficient.
self.copy_slice(src_low);
self.copy_slice(src_high);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these two lines in the wrong order? src_high should be appended before src_low.

}

// Some values now exist in both `other` and `self` but are made inaccessible
// in`other`.
other.tail = other.head;
}
}

/// Retains only the elements specified by the predicate.
Expand Down Expand Up @@ -2593,8 +2635,8 @@ impl<T> From<VecDeque<T>> for Vec<T> {
let mut right_offset = 0;
for i in left_edge..right_edge {
right_offset = (i - left_edge) % (cap - right_edge);
let src: isize = (right_edge + right_offset) as isize;
ptr::swap(buf.add(i), buf.offset(src));
let src = right_edge + right_offset;
ptr::swap(buf.add(i), buf.add(src));
}
let n_ops = right_edge - left_edge;
left_edge += n_ops;
Expand Down