Enum layout optimizations

[luqmana]

This extends the nullable enum opt to traverse beyond just the first level to find possible fields to use as the discriminant. So now, it'll work through structs, tuples, and fixed sized arrays. This also introduces a new lang item, NonZero, that you can use to wrap raw pointers or integral types to indicate to rustc that the underlying value is known to never be 0/NULL. We then use this in Vec, Rc and Arc to have them also benefit from the nullable enum opt.

As per rust-lang/rfcs#499 NonZero is not exposed via the libstd facade.

x86_64 Linux:
                        T       Option<T> (Before)      Option<T> (After)
----------------------------------------------------------------------------------
Vec<int>                24          32                      24
String                  24          32                      24
Rc<int>                 8           16                      8
Arc<int>                8           16                      8
[Box<int>, ..2]         16          24                      16
(String, uint)          32          40                      32

Fixes #19419.
Fixes #13194.
Fixes #9378.
Fixes #7576.

[jdm]

👯

[cgaebel]

\o/

[pythonesque]

Woo!

[flaper87]

awesome 🍰

[nikomatsakis]

f? @jld

[eddyb]

I don't think this is safe at all, even if right now I can't come up with a memory safety violation in safe code using it.
Though someone more motivated could potentially use it to transmute arbitrary values.

[sfackler]

The existence of NonZero::null does seem like a bit of an oxymoron in any case.

[nikomatsakis]

why special case these things? why not just call type_is_sized all the time? seems like it just makes the code less DRY

[nikomatsakis]

So, this looks basically good to me, the big thing that seems to be missing are tests that do matches and so forth on values that contain NonZero instances. There are some tests that check the size, but none that actually use the values in various mysterious ways that I can see. (Admittedly, using this in Rc and Vec etc provides a certain amount of testing, but I'd prefer to see targeted unit tests, probably just as #[test] fns in the NonZero module.)

[luqmana]

@nikomatsakis Updated.

[nikomatsakis]

@luqmana I'm assuming there were only minor changes here? (r+ under that assumption)

[nikomatsakis]

Actually, revoking r+. The main thing I wanted added was unit tests that test matching against a Option<Foo<T>> for each type Foo that uses NonZero (Vec, Rc, etc). I see some tests checking the size of an Option<Rc<T>>, but no tests that actually check that we correctly distinguish None from Some (and in particular, testing empty vectors Some(Vec::new()) vs None). If I'm wrong and the tests are there, sorry about that.

[jld]

Nit: could this return Option<DiscrField>? (And likewise for find_ptr.) That seems to be what it's doing anyway, and it would be a little clearer to read.

[luqmana]

@jld It did originally but I changed it to this way to get rid of all the extraneous allocation.

[jld]

I think I expessed that badly: I meant returning an optional vector in reverse order, like it's doing now; the recursive cases would move the Vec, push onto it, and then return it. It's not a big deal in any case.

[luqmana]

Yep, that makes sense. I've updated the code.

[brson]

Nice wins.

[vadimcn]

@luqmana: Is there any reason this couldn't have been pub struct NonZero<T: Zeroable>(pub T)?
The way it's currently defined, NonZero cannot be used to initialize statics and consts. :-(

[eddyb]

Because we don't have unsafe fields, this has the same issue as UnsafeCell (public safely modifiable fields can lead to unsafety).

[vadimcn]

Is this issue explained anywhere? I must have missed that discussion.

[flaper87]

@vadimcn I don't think there's an extended documentation/discussion written anywhere about this but this falls into the safety guarantees. As @eddyb mentioned, there's no support for unsafe fields, therefore we don't have a way to tell the user that accessing a certain field is considered an unsafe operation.

In the case of UnsafeCell, the field is public but, as the docstring in the file says, it shouldn't be.

Now, I wonder if it'd be fair to allow calls to constructors on static items by requiring them to be in an unsafe block.

[vadimcn]

Why accessing the inner value of NonZero is unsafe?

[huonw]

I think creating, not accessing, the value is unsafe in this case: NonZero(0) is bad!

[flaper87]

In addition to what @huonw said: NonZero guarantees the wrapped raw pointer will never be NULL or 0. If public access to the wrapped pointer is allowed, it would be possible to zero the value out.

[vadimcn]

@huonw, @flaper87: I can still do these things, since NonZero::new() does not perform any input validation. So what's gained? That I have to wrap it in an unsafe {} block?