Follow multiple transparent links in Option layout #699
Conversation
Option can take advantage of layout optimizations when its inner type is nullable or zeroable. Before, we were only looking at the Option's direct generic parameter when deciding whether we could use this optimization. But the layout optimization "propagates through" typedefs and transparent structs. We now repeatedly follow these type aliases until we reach a non-aliased type, and only then check whether the type is nullable/zeroable.
dcreager
force-pushed
the
option-transparent
branch
from
e3f3036
to
b073b3b
Compare
| file: Handle<File>, | ||
| maybe_file: Option<Handle<File>>, |
There was a problem hiding this comment.
One thing I don't like about this patch is that you end up with different types for these two fields.
| Handle_File file; | ||
| uint32_t maybe_file; |
There was a problem hiding this comment.
like here. Ideally both fields would be Handle_File.
There was a problem hiding this comment.
Yeah, agreed... In practice it shouldn't be a huge deal for C. For C++ it can be a bit more annoying (I can imagine someone defining their own Handle<> for example.
There was a problem hiding this comment.
The latest patch fixes this, at the expense of the infinite_recursion_typedef_monomorph test going into an infinite loop. I'd really love to be able to use the Monomorphs data here, but that's not generated until later in the process, and it looks like it depends on all of the types already having been simplified.
| /// This happens via `#[repr(transparent)]` structs and via typedefs. | ||
| #[derive(Default)] | ||
| pub struct TransparentTypes { | ||
| transparent: HashMap<Path, (Type, GenericParams)>, |
There was a problem hiding this comment.
So it is quite a bit unfortunate to have to go through all items again creating this data structure when we already have a <Path, Struct> etc hashmap in ItemMap, it just feels like a lot of upfront work for something that could be much more efficient.
There was a problem hiding this comment.
Yeah, I definitely agree. At first I tried passing &ItemMap<Struct> and &ItemMap<Typedef> references in to the simplify_standard_types methods, but that runs afoul of the borrow checker, since we're calling them from within a for_all_items_mut loop that is updating the contents of those ItemMaps. Do you have any suggestions for how to break that double-borrow?
| Handle_File file; | ||
| uint32_t maybe_file; |
There was a problem hiding this comment.
Yeah, agreed... In practice it shouldn't be a huge deal for C. For C++ it can be a bit more annoying (I can imagine someone defining their own Handle<> for example.
As part of simplifying, we follow any typedefs or transparent structs to determine if a path type is aliased to a type that is nullable or zeroable. If so, that path type can participate in the Option layout optimization just like primitive nullable/zeroable types.
|
Just in case you are looking for a complex example. I tried to run a build from this PR on libsignal-client and ran into a stackoverflow (with 16 GB of RAM). I guess this might be result of (endless) recursion. |
Option can take advantage of layout optimizations when its inner type is nullable or zeroable. Before, we were only looking at the Option's direct generic parameter when deciding whether we could use this optimization. But the layout optimization "propagates through" typedefs and transparent structs. We now repeatedly follow these type aliases until we reach a non-aliased type, and only then check whether the type is nullable/zeroable.
Fixes #690
Fixes #326