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

Fast path for vec.clear/truncate #64375

Merged
merged 1 commit into from Sep 14, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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;

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 12, 2019

Contributor

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);

?

This comment has been minimized.

Copy link
@gnzlbg

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Sep 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 12, 2019

Contributor

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 ?

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 12, 2019

Contributor

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?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Sep 12, 2019

Contributor

If T::drop panics inside of Vec::truncate, unwinding will (usually) eventually call Vec::drop so those other items will have another chance to be correctly dropped at that point.

If T::drop panics inside Vec::drop, that’s it. There is no later opportunity to drop other elements.

(Unless we decide the loop should catch panics and keep trying to drop other elements, but neither cases do this today.)

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Sep 12, 2019

Contributor

while if truncate panics, then maybe observing a partially truncated Vec is easier ?

struct Bomb;
impl Drop for Bomb {
    fn drop(&mut self) { panic!(); }
}
let mut v = vec![Bomb, Bomb];
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
    v.truncate(0);
}));
assert_eq!(v.len(), 1);
std::mem::forget(v);

works.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 12, 2019

Contributor

@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).

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Sep 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@kornelski

kornelski Sep 12, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 13, 2019

Contributor

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

}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.