Skip to content

Swap self and other in vec::append when it avoids a reallocation #77538

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Swap self and other in vec::append when it avoids a reallocation #77538

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84637e6
vec::append: swap vecs instead of moving elements when the LHS is empty
the8472 Oct 3, 2020
4cdbb8c
reword guarantee section to leave wiggle room for non-(de)allocating …
the8472 Oct 4, 2020
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
12 changes: 10 additions & 2 deletions library/alloc/src/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,8 @@ use crate::raw_vec::RawVec;
/// * It would penalize the general case, incurring an additional branch
/// on every access.
///
/// `Vec` will never automatically shrink itself, even if completely empty. This
/// ensures no unnecessary allocations or deallocations occur. Emptying a `Vec`
/// `Vec` will not automatically shrink itself, even if completely empty, when doing so
/// would cause unnecessary allocations or deallocations to occur. Emptying a `Vec`
Comment on lines +246 to +247
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe playing with words would confuse most users, this does not feel much different to me. Maybe

Suggested change
/// `Vec` will not automatically shrink itself, even if completely empty, when doing so
/// would cause unnecessary allocations or deallocations to occur. Emptying a `Vec`
/// `Vec` will not automatically shrink itself, even if completely empty. **But** it
/// will try to reduce unnecessary allocations or deallocations by reusing existing allocation. Emptying a `Vec`

Maybe a big but helps? Probably needs rewrapping.

/// and then filling it back up to the same [`len`] should incur no calls to
/// the allocator. If you wish to free up unused memory, use
/// [`shrink_to_fit`].
Expand Down Expand Up @@ -1257,6 +1257,14 @@ impl<T> Vec<T> {
#[inline]
#[stable(feature = "append", since = "1.4.0")]
pub fn append(&mut self, other: &mut Self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could just put a big notice that possible allocation stealing in the docs of append to give users a note.

// Reuses other if doing so allows us to turn a certain reallocation within self
// into a potential, future reallocation in other.
// The capacity limit ensures that we are not stealing a large preallocation from `other`
// that is not commensurate with the avoided reallocation in self.
if self.len == 0 && self.capacity() < other.len && other.capacity() / 2 <= other.len {
mem::swap(self, other);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this helps the general case but it may regress on some cases where users want to reuse the capacity of the other Vec.

Copy link
Member Author

Choose a reason for hiding this comment

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

without this change: it must allocate now to grow self
with this change: it may have to allocate later to grow other

return;
}
unsafe {
self.append_elements(other.as_slice() as _);
other.set_len(0);
Expand Down