[Variant] Simplify creation of Variants from metadata and value #7663
Conversation
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| pub struct VariantObject<'m, 'v> { | ||
| pub metadata: &'m VariantMetadata<'m>, | ||
| pub metadata: VariantMetadata<'m>, |
There was a problem hiding this comment.
This is the core difference -- rather than having a reference to a VariantMetadata Objects and Arrays now have the objects inlined (and the objects themselves have references to the underlying &[u8])
Note that VariantMetadata is a pointer and a few usize so I think it is better to inline them anyways rather than have an extra layer of indirection
There was a problem hiding this comment.
One fat pointer, two usize, and a VariantMetadataHeader that should easily pack to one usize. So 40B instead of 16B. That does seem within the range of "probably doesn't matter" when it comes to memory consumption. And this avoids a double pointer indirection when accessing the metadata.
We could also shrink the header size down (by storing the field sizes as i32 for example) 🤔 I'll make a follow on PR to explore what that would look like
alamb
changed the title
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| pub struct VariantObject<'m, 'v> { | ||
| pub metadata: &'m VariantMetadata<'m>, | ||
| pub metadata: VariantMetadata<'m>, |
There was a problem hiding this comment.
One fat pointer, two usize, and a VariantMetadataHeader that should easily pack to one usize. So 40B instead of 16B. That does seem within the range of "probably doesn't matter" when it comes to memory consumption. And this avoids a double pointer indirection when accessing the metadata.
We could also shrink the header size down (by storing the field sizes as i32 for example) 🤔 I'll make a follow on PR to explore what that would look like
|
In order to avoid conflicts, I am just going to merge this PR and I will be happy to handle any other suggestions / comments as a follow on PR(s). I would like to unblock additional documentation and testing |
2 participants
Which issue does this PR close?
Rationale for this change
While making documentation / examples for working with
Variantin #7661, I found it was somewhat awkward to makeVariantvalues directly from the metadata and value. Specifically you have toI would really like to be able to create
Variantdirectly frommetadataandvaluewithout having to make aVariantMetadatastructureWhat changes are included in this PR?
This PR proposes a small change to the API so creating a Variant now looks like:
Are there any user-facing changes?
Yes, the API for creating APIs is slightly different (and I think better)