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

Size optimization for Option #48

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Size optimization for Option #48

wants to merge 7 commits into from

Conversation

dragazo
Copy link

@dragazo dragazo commented Aug 25, 2023

Closes #47.

As far as I can tell, there was no real need to use MaybeUninit<InlineString> instead of just InlineString directly, due to it being a POD type. All that was required was changing the types of pointer casts we do to directly use &self.data as *const _ rather than self.data.as_ptr() in various places.

Due to no longer using the MaybeUninit union type, we are able to get Option size optimization out of the InlineString. To do this, I changed Marker(u8) to Marker(NonZeroU8) and changed it to use the low 2 bits for storing both the discriminant (as it already did), and also a non-zero bit to guarantee the requirement of NonZeroU8. I've also changed the allocator alignment requirement to 4 instead of 2 to guarantee these bits are free to use; this should not cause issues on 32 or 64 bit systems, which are our targets.

To ensure that a BoxedString write over the memory of the InlineString does not violate the requirements of NonZeroU8 (and/or produce a None value), the ptr field in BoxedString was changed to a new TaggedPtr type that automatically handles setting the low 2 bits appropriately (and removing them when queried).

I feel that additional testing should be done, but it currently passes all existing test cases and a few that I added specifically about size requirements and Some vs None checks for small and large strings.

@dragazo
Copy link
Author

dragazo commented Aug 26, 2023

I also just took the liberty of solving #33 by using alloc::borrow::Cow, which is available in no_std mode.

@dragazo
Copy link
Author

dragazo commented Aug 26, 2023

I'll also note that this PR now uses 2 bits for the expanded discriminant, but only 3 states (None, InlineString, and BoxedString). The fourth pattern could be used to implement a feature like storing a &'static str like some other sso libs can do. That's outside the scope of this PR, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Size optimization for Option
1 participant