Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upOptimize enums by niche-filling even when the smaller variants also have data. #46213
Comments
kennytm
added
the
C-enhancement
label
Nov 23, 2017
This comment was marked as resolved.
This comment was marked as resolved.
|
cc me |
This comment has been minimized.
This comment has been minimized.
|
what is "niche" in this case? |
This comment has been minimized.
This comment has been minimized.
|
A niche is a location in the type where some bit patterns aren't valid. For instance |
This comment has been minimized.
This comment has been minimized.
|
@Gankro thanks, that makes sense. |
This comment has been minimized.
This comment has been minimized.
|
We probably need better terminology, "invalid" values and "invalid reuse" optimization? |
This comment has been minimized.
This comment has been minimized.
|
Here is a test case we currently fail to optimize, but this issue would fix:
The compiler sees:
which should lower to:
because the Err payload fits after the niche ( |
This comment has been minimized.
This comment has been minimized.
leonardo-m
commented
Nov 23, 2017
|
Before merging this PR I think we should see benchmarks of both memory saved and run-time saved for some real program, like the rust compiler and Servo. |
This comment has been minimized.
This comment has been minimized.
leonardo-m
commented
Nov 23, 2017
|
I have yet to see similar benchmaks for #45225 . |
This comment has been minimized.
This comment has been minimized.
|
Some slightly more complex cases from webrender that would also benefit from this: // Use the Opacity.0.tag niche
pub enum FilterOp {
Blur(f32),
Brightness(f32),
Contrast(f32),
Grayscale(f32),
HueRotate(f32),
Invert(f32),
Opacity(PropertyBinding<f32>, f32),
Saturate(f32),
Sepia(f32),
}
pub enum PropertyBinding<T> {
Value(T),
Binding(PropertyBindingKey<T>),
}
pub struct PropertyBindingKey<T> {
pub id: PropertyBindingId,
_phantom: PhantomData<T>,
}
pub struct PropertyBindingId {
namespace: IdNamespace,
uid: u32,
}
pub struct IdNamespace(pub u32);// use the RoundedRect.1.mode.tag niche
pub enum LocalClip {
Rect(LayoutRect),
RoundedRect(LayoutRect, ComplexClipRegion),
}
#[repr(C)]
pub struct ComplexClipRegion {
pub rect: LayoutRect,
pub radii: BorderRadius,
pub mode: ClipMode,
}
#[repr(C)]
pub struct BorderRadius {
pub top_left: LayoutSize,
pub top_right: LayoutSize,
pub bottom_left: LayoutSize,
pub bottom_right: LayoutSize,
}
#[repr(C)]
pub enum ClipMode {
Clip,
ClipOut,
}
#[repr(C)]
struct LayoutRect(f32, f32, f32, f32);
#[repr(C)]
struct LayoutSize(f32, f32); |
This comment has been minimized.
This comment has been minimized.
|
A simpler case that I think fits this optimization (or a specialization of it as a starting point): // my use-case, a single value is most common
enum OccupiedSmallVec {
Single(Foo),
Multiple(Vec<Foo>),
}If #![feature(untagged_unions)]
union OccupiedSmallVec {
// where `single.0 == 0`
single: (usize, Foo),
multiple: Vec<Foo>,
}However, it currently requires a separate tag and padding to align the pointers, wasting basically another |
This comment has been minimized.
This comment has been minimized.
|
@abonander Not really, that's the entire optimization, assuming |
This comment has been minimized.
This comment has been minimized.
|
From IRC (cc @nox): we could extend this to always compute the size of the tagged layout (which would likely be larger than Then |
This comment has been minimized.
This comment has been minimized.
nox
referenced this issue
Mar 20, 2018
Closed
Layout optimization for Result<&T, E>-like types #48741
gnzlbg
referenced this issue
May 28, 2018
Closed
Tracking issue for the GlobalAlloc trait and related APIs #49668
This comment has been minimized.
This comment has been minimized.
|
IIUC, this optimization could also work to make |
This comment has been minimized.
This comment has been minimized.
|
Yes. |
kennytm
referenced this issue
Jul 12, 2018
Closed
Odd enum size_of computation with named struct variant #52283
leoschwarz
referenced this issue
Aug 24, 2018
Closed
Optimize layout of nested field-less enums #53669
This comment has been minimized.
This comment has been minimized.
|
Another example which can be optimized is:
Currently |
This comment has been minimized.
This comment has been minimized.
|
@newpavlov That’s way harder to optimise because it becomes more complicated to compute the discriminant of a Foo value.
|
This comment has been minimized.
This comment has been minimized.
This cannot happen, unless we make |
This comment has been minimized.
This comment has been minimized.
|
@nagisa It doesn't fit the "niche" scheme, though. |
This comment has been minimized.
This comment has been minimized.
|
@nox I think it can be handled as a special case, i.e. if all enum elements are field-less enums, check if tags do not overlap and fit into a minimal repr. Yes, making it more generic will be significantly more involved, but even with field-less constraint this optimization will help a lot for FFI and parsing cases. (currently you often have to keep huge amount of @nagisa Ah, I should've explicitly mentioned, I want it foremost for field-less enums. (see linked issue before my first message) More generic handling would be nice, but less important. |
This comment has been minimized.
This comment has been minimized.
|
It makes |
This comment has been minimized.
This comment has been minimized.
|
Optimizing for discriminant extraction seems, to me, far less valuable on average than optimizing for space. Optimizing for the ease of writing an intrisinc for it seems even less valuable. In the provided example, |
This comment has been minimized.
This comment has been minimized.
|
I just now realized that In fact, I believe that the only way you can observe the discriminant of a variant of a |
This comment has been minimized.
This comment has been minimized.
|
You need to distinguish between the tag and the discriminant. The tag is an arbitrary bit pattern that stores a value that can be used to extract the discriminant. The discriminant exists even for types like tag extraction really is just a field access. Getting the correct discriminant from that can technically be arbitrarily complex. In practice it is desirable to keep the complexity low though. |
This comment has been minimized.
This comment has been minimized.
|
Oh, I put that message in the wrong thread, oops. |
alercah
referenced this issue
Sep 11, 2018
Open
Allow arbitrary enums to have explicit discriminants #2363
This comment has been minimized.
This comment has been minimized.
Should struct field reordering be tweaked to try to push a niche to the start or end of a struct layout (if possible without increasing the struct size) in order to make this optimization more effective when that struct is used in an enum? |
This comment has been minimized.
This comment has been minimized.
Note that complicated discriminant extraction can have second-order effects on LLVM otpimizations and can slow down code more than the speedups you get from slightly smaller type sizes. |
This comment has been minimized.
This comment has been minimized.
rhendric
commented
Dec 21, 2018
|
I think frunk's Coproducts would also benefit from this optimization; currently, a |
eddyb commentedNov 23, 2017
The current implementation only handles enums of roughly the form:
This can be seen as a special-case, where B and C occupy 0 bytes, of:
As long as
BandCcan fit before or afterA'sNiche, we can still apply the optimization.Also see rust-lang/rfcs#1230 (comment) for the initial description.