Branch on needs_drop in Vec::drop?

[SimonSapin]

When a arena::TypedArena is dropped, it uses std::intrinsics::needs_drop to skip entirely the loop that reads items and runs their destructors.

Vec however does no such thing, and runs a similar loop unconditionally. Should it check needs_drop as well, or can we rely on the loop being optimized away entirely when it does nothing?

cc @pnkfelix, @gankro

[Gankra]

First I've seen of needs_drop, but it seems to me that this is something best handled by general optimizations. especially for a case as simple as Vec::drop. Is it not being optimized away today?

[SimonSapin]

Is it not being optimized away today?

I admit I haven’t bothered writing a benchmark to find out :]

[SimonSapin]

@steveklabnik, does "needstest" include comparative benchmarks as well?

[SimonSapin]

Or is it possible to test directly the output of the optimizer to see that some bit of code that ends up doing nothing is actually optimized away?

[steveklabnik]

I don't know off the top of my head, to be honest.

[pnkfelix]

(i don't think this qualifies as "needstest"; that is usually applied to issues, usually bugs, where the only remaining task post bug fix was to put in a regression test. The use of the word "test" in the title for this issue is referring to a conditional in the low-level code, not a regression test; not sure if that was part of confusion here or not. Anyway, removing the E-needstest label.)

[pnkfelix]

(i would not object, however, if we used a hypothetical B-datagathering label, as in "blocked on data gathering".)

[SimonSapin]

Ah, yes, "test" in the title was meant as "use an if statement".

[steveklabnik]

@SimonSapin ohhhhhhhhhhh yeah, I just had a parsing error 😉

[abonander]

Because it caught my attention, I spent most of the morning tinkering on this. I even built the exact revision of LLVM that Rust master is using so I could use the llvm-dis tool for the correct version of LLVM.

I believe I've come to the conclusion that the drop-loop is optimized out for Vec<T> where T: !Drop.

This is the code I used to test this:

fn main() {
    (0..256).collect::<Vec<_>>(); 
}

I then compiled it with rustc -O --emit=llvm-bc and disassembled the subsequent bitcode file with llvm-dis, which yielded this optimized IR.

Now, I can't really read LLVM IR, but I only see one loop-body section, and that looks to be part of the .collect() call. That's the only looping structure that I can see, scanning through this code. It actually looks like the Vec is unceremoniously deallocated immediately after the loop exits, at label then-block-1112.

But I could be wrong. This is half-gibberish to me.

[sfackler]

Branching will make things easier in the unoptimized case for things like BufStream that have 2 64k Vec<u8>s.

[abonander]

@sfackler I hadn't considered the unoptimized case. 👍 to branching from me.

[lilyball]

@cybergeek94 Is llvm-dis actually doing anything useful there that you can't get with --emit=llvm-ir instead?

[abonander]

@kballard Correct me if I'm wrong, but doesn't --emit=llvm-ir emit the IR before it's run through the optimizer? If not, I did a lot of work for nothing based on a faulty assumption. 😆

[lilyball]

@cybergeek94 Ok: "you're wrong" 😉 If you pass the -O flag it emits optimized IR.

[abonander]

Well, I learned how to build LLVM and work with IR, so maybe not a total waste.

[whitequark]

I have some image processing code that has to display video. It meets timings with -O2, but that takes 20s to compile. With -O0, it takes 4s, which gives a more reasonable feedback loop, but then it calls drop_in_place for every u8 in a 12MB Vec.

I have a workaround, which is really terrible:

        unsafe {
            let mut data = <whatever we drop>;
            let raw_vec = alloc::raw_vec::RawVec::from_raw_parts(
                                data.as_mut_ptr(), data.capacity());
            mem::forget(data);
            mem::drop(raw_vec);
        }

[bluss]

By the way, vec.set_len(0) is a much better workaround if the elements don't need drop.

[whitequark]

@bluss Thanks!

[SimonSapin]

So even if a loop of no-op drops is optimized away, it sounds like skipping it entirely based on needs_drop would help non-optimized builds. It should be an easy patch.

@gankro does it sound reasonable to do across all collections?

[Gankra]

@SimonSapin it's a bit of a bumber to have to do, but it seems reasonable, yeah.

[nagisa]

Fixed by #28531?

There’s still no set_len(0) else branch, though. Not sure if it matters much though.

[SimonSapin]

Yeah, I’d say this is fixed.

I don’t think set_len(0) would do anything here.

[bluss]

set_len was just a better alternative to @whitequark's workaround code (comment above)