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.

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

Fast path for vec.clear/truncate #64375

Merged
merged 1 commit into from
Sep 14, 2019
Merged

Fast path for vec.clear/truncate #64375

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 17 additions & 13 deletions src/liballoc/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,21 +685,25 @@ impl<T> Vec<T> {
/// [`drain`]: #method.drain
#[stable(feature = "rust1", since = "1.0.0")]
pub fn truncate(&mut self, len: usize) {
let current_len = self.len;
unsafe {
let mut ptr = self.as_mut_ptr().add(self.len);
// Set the final length at the end, keeping in mind that
// dropping an element might panic. Works around a missed
// optimization, as seen in the following issue:
// https://github.com/rust-lang/rust/issues/51802
let mut local_len = SetLenOnDrop::new(&mut self.len);
if mem::needs_drop::<T>() {
let current_len = self.len;
unsafe {
let mut ptr = self.as_mut_ptr().add(self.len);
// Set the final length at the end, keeping in mind that
// dropping an element might panic. Works around a missed
// optimization, as seen in the following issue:
// https://github.com/rust-lang/rust/issues/51802
let mut local_len = SetLenOnDrop::new(&mut self.len);

// drop any extra elements
for _ in len..current_len {
local_len.decrement_len(1);
ptr = ptr.offset(-1);
ptr::drop_in_place(ptr);
// drop any extra elements
for _ in len..current_len {
local_len.decrement_len(1);
ptr = ptr.offset(-1);
ptr::drop_in_place(ptr);
}
}
} else if len <= self.len {
self.len = len;
Copy link
Contributor

@gnzlbg gnzlbg Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why isn't this function just implemented as:

if len >= self.len { return; }
let s = &mut self[len..] as *mut [_];
self.len = len;
ptr::drop_in_place(&mut *s);

?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @SimonSapin

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t know, maybe it’s older than drop_in_place?

One difference however, as hinted by the code comment, is that the explicit loop + SetLenOnDrop doesn’t leak other items when T::drop panics.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t know, maybe it’s older than drop_in_place?

Might be.

One difference however, as hinted by the code comment, is that the explicit loop + SetLenOnDrop doesn’t leak other items when T::drop panics.

Yes, I think this leaks as little as it possibly can - only some fields of the T for which T::drop panics might be leaked with the current implementation.

I don't see this guaranteed or documented anywhere, but it is a reasonable thing to do. We don't do this for, e.g., Vec<T>::drop, but observing a partially dropped Vec is hard, while if truncate panics, then maybe observing a partially truncated Vec is easier ?

Copy link
Contributor

Choose a reason for hiding this comment

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

then maybe observing a partially truncated Vec is easier ?

Well yes, it is really easy:

let mut vec: Vec<T>;
catch_unwind(AssertUnwindSafe(|| vec.truncate(N)));
// partially-truncated vec easily observable

Does ptr::drop_in_place([T]) stop dropping elements the first time a panic happens ? Or does it continue dropping elements and re-raises the panic at the end?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does ptr::drop_in_place([T]) stop dropping elements the first time a panic happens ?

No.

Or does it continue dropping elements and re-raises the panic at the end?

It continues dropping elements, and re-raises the panic at the end, just like for all other aggregates. If a second element panics, then we have a double panic. See here.

So the main difference between this implementation and one using ptr::drop_in_place, is that this one stops dropping elements the moment there is a panic, and with ptr::drop_in_place, all elements are always dropped, except at most one if it panics, and if more than one panic the program aborts.

Copy link
Contributor

@gnzlbg gnzlbg Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@bjorn3 so that example works with the current implementation, but it wouldn't work if we were to use ptr::drop_in_place (it would abort due to a double panic).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I didn’t realize drop_in_place did that. So maybe it’s preferable to use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific implementation has more instructions for clear, because it changes unconditional store 0 into a check. Changing it to if len > self.len { return; } generates the same code for u8. Drop cases are different of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! Yes, with if len > self.len { return; } the assembly for Vec::clear looks much nicer: https://rust.godbolt.org/z/6pSj7K

}
}

Expand Down