# Idea: Make \`std::mem::needs\_drop\` smarter

**URL:** <https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862>\
**Category:** Uncategorized\
**Created:** [August 9, 2020, 3:23pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862 "2020-08-09T15:23:34Z")\
**Posts on this page:** 18\
**Page:** 1

<div class="post-metadata">

**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\
**Post date:** [August 9, 2020, 3:23pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/1 "2020-08-09T15:23:35Z")

</div>

Currently it is possible and useful to use `ManuallyDrop`, `MaybeUninit` or `std::ptr::drop_in_place` for writing generic datatypes `Foo<T>` that implement `Drop` in a way that actually does _nothing_ if `std::mem::needs_drop<T>() == false`. I can imagine things like owned pointers to the stack or an arena, or using `ManuallyDrop` just to specify drop order. And there’s probably lots of scenarios I’m not thinking of, basically everything where the destructor must be written manually but does not handle any synchronization logic or deallocation. I guess, this can also include `Box<T>` for zero-sized drop-glue-free `T`.

Right now, for such a type `Foo<T>` we have `std::mem::needs_drop::<Foo<T>>` evaluating to `true`. This in turn means that something like `Vec<Foo<T>>` will loop through the whole vector with a no-op destructor (unless the optimizer catches this, which seems likely; but there ought to be some better examples, otherwise `needs_drop()` wouldn’t exist, right?)

As far as I’m aware, `needs_drop` _is_ allowed to change its implementation without this being considered a breaking change, which means that the current behavior can be changed, and also my proposed new behavior below is not a semver hazard (as it will introspect the actual `Drop` implementation _source code_ which breaks encapsulation a bit).

* * *

So my idea is that we could have the Rust compiler realize certain kinds of no-op `Drop` implementations are doing nothing and use this to improve the information provided by `std::mem::needs_drop`.

Possible cases for improvement:

- Definitely possible and quite minimal approach: If a type where none of the fields have drop glue implements `Drop` in a way that the whole body of `fn drop` is wrapped in a `if COND { ... }` where `COND` is an expression that can be `const`-evaluated (or how do you call that? I mean something that could be assigned to `const`s or `static`s), then `std::mem::needs_drop()` should return `false` whenever this `COND` evaluates to `false`; and in this case the type itself would also be considered not to have drop glue. This would give people that _want_ their types to not have drop glue a quite powerful way to archive it.
- Maximal approach: Whenever a destructor (including its drop glue) is compiled to a no-op _after optimization_, then the corresponding `needs_drop()` check returns `false` for that type. Possibly difficult / impossible since this would make a `const fn` depend on the result of the optimizer, which probably includes LLVM (however I have no idea how the rust compiler actually works, so who knows). This approach would however give the greatest performance benefit to unmaintained or non-handoptimized code.
- Middle ground: Allow a few more things next to the `if COND` guarded stuff, like `MaybeUninit::assume_init` or `std::ptr::drop_in_place` or `ManuallyDrop::drop`, etc, to make some very simple `Drop` implementations work without need for change. It is however highly nontrivial to decide what to include in such an analysis.

* * *

I’m interested in whether you think this is reasonable idea and a valuable improvement, and if yes, which version/kind of analysis you think is possible and useful. Also links to previous discussion about anything related would be appreciated. With positive feedback, we could try to make an RFC out of this.

---

<div class="post-metadata">

**Author:** ![CAD97](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cad97/32/3460_2.png) [@CAD97](https://internals.rust-lang.org/u/CAD97)\
**Post date:** [August 9, 2020, 3:57pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/2 "2020-08-09T15:57:43Z")

</div>

As a more heavy-handed alternative, offer an default-provided `const Drop::NEEDS_DROP: bool` to provide an explicit hint for types that are trivially droppable in some configurations.

(Reminder: it's never unsafe to fail to drop something (that isn't pinned).)

---

<div class="post-metadata">

**Author:** ![mcy](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/mcy/32/6512_2.png) [@mcy](https://internals.rust-lang.org/u/mcy)\
**Post date:** [August 10, 2020, 3:05pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/3 "2020-08-10T15:05:15Z")

</div>

Presumably setting such a constant to `false` should _only_ make `needs_drop` return `false` when none of the fields need to be destroyed, either...? Feels like it would make it somewhat safer to use.

Also, your suggestion is a technically breaking change, because it makes `Drop` no longer object-safe. (Because `dyn Drop` is a type that we all desperately need...?)

I would maybe accept a middle ground where we stick an attribute on the `Drop` implementation along the lines of `#[maybe_nop]`, and, if and only if that is applied, then we do the `if CONST` optimization.

---

<div class="post-metadata">

**Author:** ![mjbshaw](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/mjbshaw/32/5103_2.png) [@mjbshaw](https://internals.rust-lang.org/u/mjbshaw)\
**Post date:** [August 10, 2020, 4:18pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/4 "2020-08-10T16:18:29Z")

</div>

Alternatively you could introduce a new trait (and even make it `derive`-able based on the type's fields) that has a `const NEEDS_DROP: bool`. That would avoid the breaking change.

Another alternative would be const eval in where clauses. Something like:

```rust
impl<T> Drop for MyType<T> where core::mem::needs_drop::<T>() {
  fn drop(&mut self) {
    // ...
  }
}

```

People have brought up const eval in where clauses before, and there are a number of proposed syntaxes for it. I'm not interested in bikeshedding the syntax (that should be in its own thread). But you don't really even need full const eval. Just const generics and specialization is enough, because you can use something like:

```rust
trait True {}
struct If<const B: bool>;
impl True for If<true> {
}

impl<T> Drop for MyType<T> where If<core::mem::needs_drop::<T>()>: True {
  fn drop(&mut self) {
    // ...
  }
}

```

But this currently generates an error because "`Drop` impl requires `If<...>: True` but the struct it is implemented for does not". I'm not sure why Rust has that restriction or if it can/should be relaxed.

---

<div class="post-metadata">

**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\
**Post date:** [August 10, 2020, 4:40pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/5 "2020-08-10T16:40:36Z")

</div>

> [@CAD97](#):
>
> As a more heavy-handed alternative, offer an default-provided `const Drop::NEEDS_DROP: bool` to provide an explicit hint for types that are trivially droppable in some configurations.

I think this works quite well:

```rust
pub trait Drop {
    const NEEDS_DROP: bool = true;
    fn drop(&mut self);
}

struct S1;

// still works as before
impl Drop for S1 {
    fn drop(&mut self) {}
}

// type with manual drop order
use std::mem::ManuallyDrop;
struct S2<T1, T2> {
    x: ManuallyDrop<T1>,
    y: ManuallyDrop<T2>,
}
impl<T1, T2> Drop for S2<T1, T2> {
    const NEEDS_DROP: bool = needs_drop::<T1>() || needs_drop::<T2>();
    fn drop(&mut self) {
        unsafe {
            ManuallyDrop::drop(&mut self.y);
            ManuallyDrop::drop(&mut self.x);
        }
    }
}

// Box, but without support for unsized T
use std::marker::PhantomData;
use std::ptr::NonNull;
struct Box<T>(NonNull<T>, PhantomData<T>);

use std::mem::size_of;
impl<T> Drop for Box<T> {
    const NEEDS_DROP: bool = needs_drop::<T>() || size_of::<T>() > 0;
    fn drop(&mut self) {
        if Self::NEEDS_DROP {
            // do deallocation and drop_in_place
        }
    }
}

```

([extended example in playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=474a15f3a1871f2375f65dc7506df09d))

---

<div class="post-metadata">

**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\
**Post date:** [August 10, 2020, 4:51pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/6 "2020-08-10T16:51:57Z")

</div>

> [@mcy](#):
>
> Presumably setting such a constant to `false` should _only_ make `needs_drop` return `false` when none of the fields need to be destroyed, either...? Feels like it would make it somewhat safer to use.

I think this is good idea. It would mean that defining `NEEDS_DROP` and then guarding the drop implementation with `if Self::NEEDS_DROP` is a straightforward way to keep the `needs_drop` information consistent with the actual `drop` implementation plus its drop glue. (Since an incorrect `NEEDS_DROP` value is otherwise a pretty hard to catch bug.)

---

<div class="post-metadata">

**Author:** ![CAD97](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cad97/32/3460_2.png) [@CAD97](https://internals.rust-lang.org/u/CAD97)\
**Post date:** [August 10, 2020, 8:14pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/7 "2020-08-10T20:14:14Z")

</div>

Your `Box`, it's incorrect to have it ever not need a drop, as it should always deallocate.

It's already maximally correct for a `Box` to `drop_in_place` and deallocate (as `drop_in_place` has an internal `needs_drop` check).

---

<div class="post-metadata">

**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\
**Post date:** [August 10, 2020, 8:23pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/8 "2020-08-10T20:23:49Z")

</div>

I thought, for a zero-sized type inside of the Box, one would not need to allocate anything, and hence also not deallocate on drop either. Any correctly aligned non-zero pointer should be sufficient, no need to talk to an allocator.

---

<div class="post-metadata">

**Author:** ![CAD97](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cad97/32/3460_2.png) [@CAD97](https://internals.rust-lang.org/u/CAD97)\
**Post date:** [August 10, 2020, 10:23pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/9 "2020-08-10T22:23:49Z")

</div>

`needs_drop` returns `false` for any type without drop glue, not just zero sized types.

Oh wait your box also is requiring `size_of`, my bad. (I'm going to blame horizontal scroll on mobile because I'm in the middle of a move)

---

<div class="post-metadata">

**Author:** ![H2CO3](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/h2co3/32/2849_2.png) [@H2CO3](https://internals.rust-lang.org/u/H2CO3)\
**Post date:** [August 11, 2020, 7:23am UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/10 "2020-08-11T07:23:26Z")

</div>

The suggestions that people came up with in this thread are so heavy-handed that I have to point out that all of the performance gain we are talking about is _pure speculation_ until actually and comprehensively measured.

Just how big of a problem is missed drop-optimization today? Does it really warrant adding new lang-item traits, or breaking one of the most fundamental traits, or heavily impeding decoupling in the architecture of the compiler by requiring feedback from the optimizer at semantic analysis time?

---

<div class="post-metadata">

**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\
**Post date:** [August 11, 2020, 10:31am UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/11 "2020-08-11T10:31:31Z")

</div>

Indeed, having at least one use case with performance benefits would be great. That’s why I remarked

> [@steffahn](#):
>
> there ought to be some better examples, otherwise `needs_drop()` wouldn’t exist, right?

I’m still waiting for anyone to mention a type in the standard library or a popular crate that _uses_ and _benefits_ from it’s use of `needs_drop`. Also I’m not yet sure which existing types from the standard library or common crates apart from `Box` (and similarly `Vec`) in the uncommon case of storing zero-sized drop-glue-free contents, could actually change their `NEEDS_DROP` value.

However, regarding

> [@H2CO3](#):
>
> breaking one of the most fundamental traits

I don’t see (yet?) how adding an associated `const` _with a default value_ breaks anything. The only thing that would change for the user is the documentation of the `Drop` trait.

I’m agreeing that

> [@H2CO3](#):
>
> heavily impeding decoupling in the architecture of the compiler by requiring feedback from the optimizer at semantic analysis time

is not a reasonable approach for this.

---

<div class="post-metadata">

**Author:** ![CAD97](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cad97/32/3460_2.png) [@CAD97](https://internals.rust-lang.org/u/CAD97)\
**Post date:** [August 11, 2020, 12:46pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/12 "2020-08-11T12:46:11Z")

</div>

> [@steffahn](#):
>
> I’m still waiting for anyone to mention a type in the standard library or a popular crate that _uses_ and _benefits_ from it’s use of `needs_drop` .

The benefit is second-order, like in [hashbrown](https://github.com/rust-lang/hashbrown/pull/182), where it can avoid iterating the map to drop all the members if they're all `!needs_drop` (or if the map is empty, which is what the linked PR is adjusting).

So for a significant advantage of a more accurate `needs_drop`, you'd need a collection of whatever trivial case of an otherwise interesting type to drop... which is probably not all that common.

---

<div class="post-metadata">

**Author:** ![mbrubeck](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/mbrubeck/32/174_2.png) [@mbrubeck](https://internals.rust-lang.org/u/mbrubeck)\
**Post date:** [August 11, 2020, 5:18pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/13 "2020-08-11T17:18:12Z")

</div>

Something like `HashMap<u32, ArrayVec<[u8; N]>>` would be an example. (ArrayVec has a no-op destructor when its element type has no destructors.) It would be interesting to see if this impacts benchmarks that use maps/sets whose elements contain ArrayVecs.

---

<div class="post-metadata">

**Author:** ![mbrubeck](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/mbrubeck/32/174_2.png) [@mbrubeck](https://internals.rust-lang.org/u/mbrubeck)\
**Post date:** [August 12, 2020, 7:37pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/14 "2020-08-12T19:37:25Z")

</div>

In my experiments with this, it appears that the iteration in the `HashMap` destructor gets optimized away completely, despite the "false positive" from `needs_drop`. For example, both of the following benchmarks return 0 ns/iter, no matter how many elements or sets they create:

```rust
#![feature(test)]
extern crate test;
use arrayvec::ArrayVec;
use std::{collections::HashSet, hash::Hash, iter::repeat};
use test::{Bencher, black_box};

fn bench_drop_sets<T>(b: &mut Bencher, n: usize, len: usize)
where T: Default + Clone + Hash + Eq
{
    let set: HashSet<T> = repeat(T::default()).take(len).collect();
    let sets: Vec<HashSet<T>> = repeat(set).take(n).collect();
    let mut sets = black_box(sets);
    b.iter(|| sets.clear());
}

#[bench]
fn arrayvec_drop(b: &mut Bencher) {
    bench_drop_sets::<ArrayVec<[usize; 31]>>(b, 100, 1000);
}
#[bench]
fn array_drop(b: &mut Bencher) {
    bench_drop_sets::<[usize; 32]>(b, 100, 1000);
}

```

---

<div class="post-metadata">

**Author:** ![cuviper](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cuviper/32/1897_2.png) [@cuviper](https://internals.rust-lang.org/u/cuviper)\
**Post date:** [August 12, 2020, 8:10pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/15 "2020-08-12T20:10:56Z")

</div>

I'm not sure that proves anything -- `Vec::clear` has nothing to drop after the first time.

---

<div class="post-metadata">

**Author:** ![cuviper](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cuviper/32/1897_2.png) [@cuviper](https://internals.rust-lang.org/u/cuviper)\
**Post date:** [August 12, 2020, 8:31pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/16 "2020-08-12T20:31:17Z")

</div>

I changed that to use hashbrown directly, and clearing the sets individually:

```rust
    b.iter(|| sets.iter_mut().for_each(|set| set.clear()));

```

With hashbrown 0.8.1 that only checked `needs_drop::<T>()`:

```nohighlight
test array_drop ... bench: 2,235 ns/iter (+/- 11)
test arrayvec_drop ... bench: 6,937 ns/iter (+/- 120)

```

With hashbrown 0.8.2 that checks `needs_drop::<T>() && len() != 0`:

```nohighlight
test array_drop ... bench: 2,175 ns/iter (+/- 35)
test arrayvec_drop ... bench: 2,125 ns/iter (+/- 40)

```

So before, the bogus `needs_drop` did indeed cause extra work.

---

<div class="post-metadata">

**Author:** ![mbrubeck](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/mbrubeck/32/174_2.png) [@mbrubeck](https://internals.rust-lang.org/u/mbrubeck)\
**Post date:** [August 12, 2020, 8:35pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/17 "2020-08-12T20:35:48Z")

</div>

Ah, thanks for fixing my broken benchmark.

---

<div class="post-metadata">

**Author:** ![system](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/system/32/14092_2.png) [@system](https://internals.rust-lang.org/u/system)\
**Post date:** [November 10, 2020, 8:35pm UTC](https://internals.rust-lang.org/t/idea-make-std-needs-drop-smarter/12862/18 "2020-11-10T20:35:59Z")

</div>

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.