Enum layout optimizations #19765

luqmana · 2014-12-12T03:34:41Z

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 · 2014-12-12T05:06:15Z

👯

cgaebel · 2014-12-12T07:46:36Z

\o/

pythonesque · 2014-12-12T20:23:32Z

Woo!

flaper87 · 2014-12-13T10:52:22Z

awesome 🍰

nikomatsakis · 2014-12-15T17:40:28Z

f? @jld

eddyb · 2014-12-15T17:41:31Z

src/libcore/nonzero.rs

+
+impl<T> RawPtr<T> for NonZero<*const T> {
+    #[inline]
+    fn null() -> NonZero<*const T> { NonZero(null()) }


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.

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

nikomatsakis · 2014-12-16T10:34:33Z

src/librustc_trans/trans/adt.rs

+        // &T/&mut T/Box<T> could either be a thin or fat pointer depending on T
+        ty::ty_rptr(_, ty::mt { ty, .. }) | ty::ty_uniq(ty) => match ty.sty {
+            // &[T] and &str are a pointer and length pair
+            ty::ty_vec(_, None) | ty::ty_str => Some(vec![FAT_PTR_ADDR]),


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 · 2014-12-16T11:19:01Z

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 · 2014-12-22T04:34:30Z

@nikomatsakis Updated.

nikomatsakis · 2014-12-23T11:10:56Z

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

nikomatsakis · 2014-12-23T11:14:01Z

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 · 2014-12-23T21:07:40Z

src/librustc_trans/trans/adt.rs

+
+fn find_discr_field_candidate<'tcx>(tcx: &ty::ctxt<'tcx>,
+                                    ty: Ty<'tcx>,
+                                    path: &mut DiscrField) -> bool {


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.

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

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.

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

…ointer enum opt.

…nant field.

…th NonZero.

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.

brson · 2015-01-05T01:57:29Z

Nice wins.

vadimcn · 2015-01-21T06:32:29Z

src/libcore/nonzero.rs

+#[lang="non_zero"]
+#[deriving(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Show)]
+#[experimental]
+pub struct NonZero<T: Zeroable>(T);


@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. :-(

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

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

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

Why accessing the inner value of NonZero is unsafe?

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

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.

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

luqmana force-pushed the nonzero-lang-item branch from 6bb5e2b to b8eb099 Compare December 13, 2014 11:00

eddyb reviewed Dec 15, 2014
View reviewed changes

nikomatsakis self-assigned this Dec 15, 2014

nikomatsakis reviewed Dec 16, 2014
View reviewed changes

luqmana force-pushed the nonzero-lang-item branch from 44ba89c to 56503a4 Compare December 22, 2014 04:29

jld reviewed Dec 23, 2014
View reviewed changes

luqmana force-pushed the nonzero-lang-item branch 4 times, most recently from 49773d8 to c0badcd Compare December 28, 2014 20:19

luqmana added 9 commits December 28, 2014 19:40

librustc: Traverse arbitrarily deep for nullable enum opt.

e954fc4

librustc: Try looking in tuple fields for nullable enum opt.

e6b6234

librustc: Try looking in fixed sized arrays for nullable enum opt.

5fb1e6b

Add tests.

6d91419

librustc: Add NonZero lang item and use it if possible for nullable p…

46e7376

…ointer enum opt.

libcore: Add NonZero lang item and implement some methods.

ef5da14

libcollections: Use NonZero in Vec.

bb44737

liballoc: Use NonZero in Rc.

0d48f76

liballoc: Use NonZero in Arc.

4af5054

luqmana added 8 commits December 28, 2014 19:40

libcore: Make it unsafe to create NonZero and impl Deref.

466135b

Add tests for NonZero.

e83272b

Don't expose NonZero through libstd.

b44d7cb

libcore: Don't impl RawPtr* traits for NonZero.

c15df8e

librustc_trans: Get rid of unnecessary allocation in finding discrimi…

27617a1

…nant field.

libcore: Use Zeroable trait to try to limit what types may be used wi…

94f1261

…th NonZero.

libcoretest: Add tests for NonZero.

0e039ad

libcollections: impl Send/Sync for Vec.

766a719

luqmana force-pushed the nonzero-lang-item branch from c0badcd to 766a719 Compare December 29, 2014 00:43

bors merged commit 766a719 into rust-lang:master Dec 29, 2014

Ms2ger mentioned this pull request Dec 29, 2014

Use NonZero in our smart pointers. servo/servo#4502

Closed

luqmana deleted the nonzero-lang-item branch December 29, 2014 20:37

michaelwoerister mentioned this pull request Jan 1, 2015

debuginfo: Debugger pretty printers cannot handle all enum memory layouts #20405

Closed

vadimcn reviewed Jan 21, 2015
View reviewed changes

pnkfelix mentioned this pull request Jan 22, 2016

Option<char> should be represented as just one 32 bit value #5977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum layout optimizations #19765

Enum layout optimizations #19765

luqmana commented Dec 12, 2014

jdm commented Dec 12, 2014

cgaebel commented Dec 12, 2014

pythonesque commented Dec 12, 2014

flaper87 commented Dec 13, 2014

nikomatsakis commented Dec 15, 2014

eddyb Dec 15, 2014

sfackler Dec 15, 2014

nikomatsakis Dec 16, 2014

nikomatsakis commented Dec 16, 2014

luqmana commented Dec 22, 2014

nikomatsakis commented Dec 23, 2014

nikomatsakis commented Dec 23, 2014

jld Dec 23, 2014

luqmana Dec 23, 2014

jld Dec 23, 2014

luqmana Dec 24, 2014

brson commented Jan 5, 2015

vadimcn Jan 21, 2015

eddyb Jan 21, 2015

vadimcn Jan 21, 2015

flaper87 Jan 21, 2015

vadimcn Jan 21, 2015

huonw Jan 21, 2015

flaper87 Jan 21, 2015

vadimcn Jan 21, 2015

Enum layout optimizations #19765

Enum layout optimizations #19765

Conversation

luqmana commented Dec 12, 2014

jdm commented Dec 12, 2014

cgaebel commented Dec 12, 2014

pythonesque commented Dec 12, 2014

flaper87 commented Dec 13, 2014

nikomatsakis commented Dec 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Dec 16, 2014

luqmana commented Dec 22, 2014

nikomatsakis commented Dec 23, 2014

nikomatsakis commented Dec 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment