Skip to content

reuse RHS allocation for vec.extend(vec.into_iter()) when they do not fit into the LHS #77496

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
the8472 wants to merge 12 commits into from
Closed

reuse RHS allocation for vec.extend(vec.into_iter()) when they do not fit into the LHS #77496

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c5af975
reuse RHS allocation in vec.extend() when the LHS is empty
the8472 Oct 3, 2020
2409653
vec.extend() reuse RHS allocation if combined data does not fit into LHS
the8472 Oct 4, 2020
8a0a13a
add comment for IntoIter::move_to
the8472 Oct 4, 2020
0c443cf
rename function
the8472 Oct 5, 2020
153a916
add debug asserts
the8472 Oct 5, 2020
cb477d4
rename function
the8472 Oct 6, 2020
5e6abf1
add ascii illustration
the8472 Oct 6, 2020
567cd52
move self-replacement to separate line
the8472 Oct 6, 2020
1dbab48
memory-based ascii illustration
the8472 Oct 6, 2020
aa4a4ac
annotate used primitives in ascii illustration
the8472 Oct 7, 2020
1885f38
reserve() was not annotated with free
the8472 Oct 9, 2020
d460c85
fix debug_assert and improve associated comment
the8472 Oct 15, 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
34 changes: 29 additions & 5 deletions library/alloc/src/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2204,7 +2204,7 @@ impl<T> SpecFromIter<T, IntoIter<T>> for Vec<T> {
}

let mut vec = Vec::new();
iterator.move_to(&mut vec);
iterator.move_into(&mut vec);
vec
}
}
Expand Down Expand Up @@ -2386,22 +2386,46 @@ where
impl<T> SpecExtend<T, IntoIter<T>> for Vec<T> {
fn spec_extend(&mut self, iterator: IntoIter<T>) {
// Avoid reallocation if we can use iterator's storage instead. This requires 1 memcpy and 0-1 memmove
// while reallocation would require 1 alloc, 1-2 memcpy, 1-2 free
// while reallocation would require 1 alloc, 1-2 memcpy, 1-2 free.
//
// ## non-empty self, partially consumed iterator
//
// == step == == memory == == self == == iter / v ==
// 0123456789abcdef0123456789abcdef
// 0---------------1---------------
//
// [initial] AAAA_-----__BBB___-------------- Vec(0x00, 4, 5) IntoIter(0x0a, 0x0c, 0x0f, 8)
// into_vec AAAA_-----____BBB_-------------- Vec(0x00, 4, 5) Vec(0x0a, 7, 8)
// prepend _____-----AAAABBB_-------------- Vec(0x00, 0, 5) Vec(0x0a, 7, 8)
// *self = v ----------AAAABBB_-------------- Vec(0x0a, 7, 8)
//
// ## empty self, partially consumed iterator
//
// [initial] ____------__BBBB__-------------- Vec(0x00, 0, 4) IntoIter(0x0a, 0x0c, 0x10, 8)
// into_vec ____------BBBB____-------------- Vec(0x00, 0, 4) Vec(0x0a, 4, 8)
// *self = v ----------BBBB____-------------- Vec(0x0a, 4, 8)
//
// ## empty self, pristine iterator
//
// [initial] ----------BBBB____-------------- Vec(0x00, 0, 0) IntoIter(0x0a, 0x0a, 0x0e, 8)
// *self = v ----------BBBB____-------------- Vec(0x0a, 4, 8)
//
Copy link
Contributor

@pickfire pickfire Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
//

Copy link
Member Author

@the8472 the8472 Oct 8, 2020

Choose a reason for hiding this comment

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

That line was intentionally left blank.

Copy link
Contributor

@KodrAus KodrAus Oct 23, 2020

Choose a reason for hiding this comment

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

I think following the rest of the codebase convention here would have this blank line not have the comment //, and just be empty.

if mem::size_of::<T>() > 0
&& self.capacity() - self.len() < iterator.len()
&& iterator.cap - iterator.len() >= self.len()
Comment on lines +2428 to +2429
Copy link
Contributor

@pickfire pickfire Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can this be simplified as?

Suggested change
&& self.capacity() - self.len() < iterator.len()
&& iterator.cap - iterator.len() >= self.len()
&& iterator.len() - self.len() < iterator.cap - self.capacity()

I wonder if this will always grow the vec if the iterator is larger.

Copy link
Member Author

@the8472 the8472 Oct 5, 2020

Choose a reason for hiding this comment

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

The proposed change would underflow if self is larger than iterator

Copy link
Contributor

@pickfire pickfire Oct 6, 2020

Choose a reason for hiding this comment

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

Underflow?

Copy link
Contributor

@KodrAus KodrAus Oct 6, 2020

Choose a reason for hiding this comment

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

This is technically still an overflow, but that iterator.len() - self.len() would panic or wrap if, say, self.capacity() == 20_000 and self.len() == 19_950, and iterator.len() == 100.

Copy link
Member Author

@the8472 the8472 Oct 6, 2020

Choose a reason for hiding this comment

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

I meant the case where self.capacity() > iterator.cap . The subtraction would underflow the usize result and thus lead to the inequality unexpectedly evaluating to true which would then violate the safety constraints of into_vec_with_uninit_prefix

Copy link
Contributor

@pickfire pickfire Oct 8, 2020

Choose a reason for hiding this comment

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

Then how about?

Suggested change
&& self.capacity() - self.len() < iterator.len()
&& iterator.cap - iterator.len() >= self.len()
&& iterator.len().saturating_sub(self.len()) < iterator.cap.saturating_sub(self.capacity())

I

Copy link
Member Author

@the8472 the8472 Oct 15, 2020

Choose a reason for hiding this comment

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

For a self with len == 2 && cap == 2 and an iterator len == 2 && cap == 3 that would evaluate to true and attempt to store 4 elements into an allocation of 3. 💣💥

{
// Safety: we just checked that IntoIter has sufficient capacity to prepend our elements.
// Prepending will then fill the uninitialized prefix.
*self = unsafe {
let v = unsafe {
let mut v = iterator.into_vec_with_uninit_prefix(self.len() as isize);
ptr::copy_nonoverlapping(self.as_ptr(), v.as_mut_ptr(), self.len);
self.set_len(0);
v
};
*self = v;
return;
Copy link
Contributor

@KodrAus KodrAus Oct 6, 2020

Choose a reason for hiding this comment

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

Can we turn this into an else block instead of early returning?

Copy link
Member Author

@the8472 the8472 Oct 6, 2020

Choose a reason for hiding this comment

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

We can, but I prefer early returns since the later part indicates the default approach in contrast to the special case above.

Copy link
Contributor

@pickfire pickfire Oct 6, 2020

Choose a reason for hiding this comment

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

I mentioned the same thing above #77496 (comment)

Copy link
Contributor

@KodrAus KodrAus Oct 23, 2020

Choose a reason for hiding this comment

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

I find splitting the branches here a bit easier to follow so you can tell that there's two paths we can take and don't have any way to fall through this one.

}
iterator.move_to(self);
iterator.move_into(self);
Copy link
Contributor

@pickfire pickfire Oct 8, 2020

Choose a reason for hiding this comment

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

It would be cooler if the code are linked to the cool diagram.

Suggested change
iterator.move_into(self);
// Insufficient capacity
iterator.move_into(self);

Copy link
Member Author

@the8472 the8472 Oct 8, 2020

Choose a reason for hiding this comment

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

Well, yes and no.

With the optimization present that is indeed all it covers. But it is the general codepath that also works without the optimization. So I don't want to give the impression that it can only handle that case.

}
}

Expand Down Expand Up @@ -2959,7 +2983,7 @@ impl<T> IntoIter<T> {
}

/// Move remaining elements to the end of `dest`.
fn move_to(mut self, dest: &mut Vec<T>) {
fn move_into(mut self, dest: &mut Vec<T>) {
unsafe {
dest.append_elements(self.as_slice() as _);
}
Expand Down