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

draft: Optimize Option<CompactStr> to be the same size as CompactStr #22

Closed
wants to merge 8 commits into from

Conversation

ParkMyCar
Copy link
Owner

This is a proof of concept to show that it's possible in stable Rust to get std::mem::size_of<CompactStr> == std::mem::size_of::<Option<CompactStr>>().

Unfortunately as-is the performance of CompactStr is degraded. We would need to fix this before merging.

Fixes #19

@zlstringham
Copy link

How attached are you to the Packed representation? I think you can accomplish everything you want without the ReprWithNiche or the std::mem::transmute, etc., if you can drop Packed.

Then I think you can extract the discriminant out of the union, and use a NonMaxU8 for it; e.g.

pub struct DiscriminantMask {
    val: NonMaxU8,
}

pub struct Repr {
    mask: DiscriminantMask,
    inner: ReprInner,
}

pub union ReprInner {
    heap: ManuallyDrop<HeapString>,
    inline: InlineString,
}

I think there will be some amount of refactoring (e.g. working with the size value of Inline outside of the union) but I don't think that's major.

@ParkMyCar
Copy link
Owner Author

I'm pretty attached to the Packed representation. Specifically because that allows you to inline 12 ASCII character strings on 32-bit machines, and 12 characters is the most common length for filenames because of the 8.3 notation from the early days of FAT, e.g. DSC_1234.jpg.

That being said, maybe we can use the layout you proposed and only transmute for the Packed repr. In the common case this removes the transmute and thus the extra copy, at the cost of code complexity 🤔

@CAD97 CAD97 mentioned this pull request Mar 31, 2022
nottirb pushed a commit to nottirb/compact_str that referenced this pull request May 15, 2022
@ParkMyCar
Copy link
Owner Author

Completed in #105

@ParkMyCar ParkMyCar closed this Jun 17, 2022
@ParkMyCar ParkMyCar deleted the feature/option-niche branch June 17, 2022 23:51
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.

Is it possible to make Option<CompactStr> same size as Option<String>?
2 participants