Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No niche optimization for enum {One(Enum), Two(Enum, Enum)} #93739

Closed
lyphyser opened this issue Feb 7, 2022 · 3 comments
Closed

No niche optimization for enum {One(Enum), Two(Enum, Enum)} #93739

lyphyser opened this issue Feb 7, 2022 · 3 comments
Labels
C-bug Category: This is a bug.

Comments

@lyphyser
Copy link

lyphyser commented Feb 7, 2022

Rust seems to fail to take advantage of niches for very simple cases where it seems like it would be trivial to do so.

#[repr(u8)]
pub enum Bar {A = 0, B = 1}

enum Foo {
    Two(Bar, Bar),
    One(Bar)
}

fn main() {
    println!("{}", std::mem::size_of::<Foo>());
}

This code gives a size of 3 for Foo while obviously it can be stored in 2 bytes, representing the Two variant as two consecutive byte-sized Bar values, and the One variant as a byte with any value other than 0 or 1 followed by a byte-size Bar value.

Using an array of two values for the Two variant, specifying non-zero discriminants for Bar and and using NonZeroU8 all seem to have no effect.

@lyphyser lyphyser added the C-bug Category: This is a bug. label Feb 7, 2022
@scottmcm
Copy link
Member

scottmcm commented Feb 7, 2022

Here's a simpler case that still doesn't optimize either: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=170f393d830719b60798a5a9dd1e963c

#[repr(u8)]
pub enum Bar {A = 0, B = 1}

#[repr(u8)]
enum AlwaysTwo { Two = 2 }

enum Foo {
    Two(Bar, Bar),
    One(AlwaysTwo, Bar),
}

fn main() {
    println!("{}", std::mem::size_of::<Foo>());
}

(With just One(Bar) there's a question of whether the rest is undef, which would keep it from being usable for discriminant storage.)

@erikdesjardins
Copy link
Contributor

erikdesjardins commented Feb 12, 2022

Duplicate of #46213.

The latest entry in the saga of trying to fix this is #75866, which was closed due to perf regressions. (Niche extraction is much more complex than loading a discriminant--and to a degree, unnecessarily so.) nox was working on improvements to match codegen which may unblock it.

(With just One(Bar) there's a question of whether the rest is undef, which would keep it from being usable for discriminant storage.)

I don't think that issue applies in this case. What we can't do is use padding in another type to store the discriminant, because it's not guaranteed to be preserved when manipulating that type on its own.

It is, however, okay to use padding within enum variants themselves to store the discriminant. Which is less "using the padding" and more "replacing would-be padding in the layout with the field holding the discriminant". (edit: not true, 2nd/3rd variants below are still treated as zero size. This is actually a stronger argument for it being allowed though. :P) That is what we currently do for enums like the following; nothing significantly changes when the other variants have non-zero size:

enum NoPayload {
    Two(Bar, Bar),
    Zero1,
    Zero2,
}

@dtolnay
Copy link
Member

dtolnay commented Jul 21, 2022

Closing as a duplicate of #46213.

@dtolnay dtolnay closed this as completed Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

4 participants