Implement VecDeque::truncate_to_range

[cammeresi]

Tracking issue: #156215

[rustbot]

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 7 candidates
• Random selection from Mark-Simulacrum, jhpratt, nia-e

[qaijuang]

without dropper, won't this leak on panic ? same for line 1537

View changes since the review

[nia-e]

Where would it panic? _g_{a,b} will only have their destructors run after we already attempt to drop c

[qaijuang]

Right, i see

[qaijuang]

Would it make sense to add a fast path for types that don’t need dropping? Something like:

around line 1493

if !mem::needs_drop::<T>() {
    self.head = self.to_physical_idx(start);
    self.len = end - start;
    return;
}

what do you think ?

View changes since the review

[nia-e]

See @qaijuang's point on checking for drop; otherwise, r=me

[nia-e]

Could you also add a test for the drop impl? e.g.

// in a test fn

static DROPPED: AtomicUsize = AtomicUsize::new(0);

struct Foo(/* whatever */);

impl Drop for Foo {
    fn drop(&mut self) {
        DROPPED.fetch_add(1, Ordering::Relaxed);
    }
}

let mut dq = /* get a vecdeque of Foo, say 12 elems long */
dq.truncate_to_range(1..5);
assert_eq!(8, DROPPED.load(Ordering::Relaxed));

[cammeresi]

I will make the requested updates, but I suspect LLVM could drop all the drop code since there would be no side effect. I also note that needs_drop is very lightly used in the library and essentially never to optimize collections like this. E.g. Vec::truncate does not do this, nor does VecDeque::truncate (which is what I was cribbing from).

[nia-e]

True, but that sounds to me like it should be used more ^^ and this all is unlikely to be optimised away since the drop glue will be defined in the crate where the element type is defined, and thus may not get properly inlined

[cammeresi]

This patch is now blocked behind the rename of truncate_front, so in the interim I examined what LLVM is actually capable of in terms of ellision.

I am able to report that the pessimism here is half right and half wrong.

In a release build, when calling regular truncate, the whole thing gets inlined and eliminated other than the length check and set. But debug mode doesn't do any of that and produces a whole bunch of instructions.

The location of the Point type seemed to make no difference. Moving it to an external crate did not change the assembly.

So do you actually want this call? Do you want more of them through all the collection types?

Sorry if I have, through any ignorance, failed to construct or analyze this properly.

use std::collections::VecDeque;

#[derive(Debug, Default)]
pub struct Point {
    pub x: i32,
    pub y: i32,
}

#[unsafe(no_mangle)]
pub fn truncate_point(d: &mut VecDeque<Point>, len: usize) {
    d.truncate(len);
}

fn main() {
    let mut d = VecDeque::new();
    d.push_back(Point::default());
    truncate_point(&mut d, 0);
    println!("{d:?}");
}

truncate_point:
    .cfi_startproc
    cmp rsi, qword ptr [rdi + 24]
    jae .LBB5_2
    mov qword ptr [rdi + 24], rsi
.LBB5_2:
    ret

[qaijuang]

In a release build, when calling regular truncate, the whole thing gets inlined and eliminated other than the length check and set. But debug mode doesn't do any of that and produces a whole bunch of instructions.

Interesting, the elimination is real on release builds, i was able to reproduce(external crate included).

Safe to say, since the Dropper has no side effect, the !mem::needs_drop::<T>() is a no-op IMO