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

Minor unsafe code cleanup suggestion #7

Closed
epage opened this issue Mar 25, 2022 · 7 comments
Closed

Minor unsafe code cleanup suggestion #7

epage opened this issue Mar 25, 2022 · 7 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@epage
Copy link

epage commented Mar 25, 2022

I've been meaning to give unions a try on kstring and finally did. A couple of differences in my implementation

  • I extract out the padding to a dedicated type. This helps enforce the unsafe invariant at compile time because "nothing" can access the MaybeUninit (yeah technically you can since its in the same file but it'll be more obvious)
  • I created an unused TagVariant that exists just to extract the tag from the union. I feel this makes the intent of the code clearer than arbitrarily using a specific union variant to access the tag.

If not interested; thats fine. I just figured I'd share in case you felt it improved things.

@nu11ptr
Copy link
Owner

nu11ptr commented Mar 25, 2022

Good ideas. Curious, why not use an enum for the tag?

@nu11ptr
Copy link
Owner

nu11ptr commented Mar 25, 2022

As a side note, there is a reason I keep the padding exposed and that is so the user can pick an inline size. Previously, that was VERY dangerous as if they picked the wrong padding size....well UB. Bad. Now, if they get it wrong they will get a panic on first string create on 0.9.1. So I'm still debating whether to leave that in or not (makes docs look kinda nasty). Could be interesting if someone found a use for large inline strings? Still on the fence.

@epage
Copy link
Author

epage commented Mar 25, 2022

I was originally going to experiment with 23-byte inline strings but backed off to do that as a separate experiment from using a union and left the tag as-is for the later experiments.

@epage
Copy link
Author

epage commented Mar 25, 2022

Could be interesting if someone found a use for large inline strings? Still on the fence.

Unsure if its still the case but I've found performance differences with smaller inline strings, which is why kstringdefaults to 15 inline bytes.

I'm interested in giving users more direct control over the backing storage (heap and inline string). If nothing else, the user would be able to more easily benchmark the options without having to re-compile. Unfortunately its a breaking change because going from no default type parameters to having them breaks some type inference cases. I might look to see if people relying on kstring are needing me to always stay 1.0 (my hope is kstring can be used in public APIs) and might do a 2.0.

@nu11ptr
Copy link
Owner

nu11ptr commented Mar 25, 2022

Interesting, my benchmarks show 20 byte inline strings as 25% faster than 10 byte inline strings. I was seeing the same thing, however....before the union due to the non-alignment of the inline string. You can see 0.8.0 vs 0.9.0 here:

https://github.com/nu11ptr/flexstr/blob/master/benchmarks/README.md#create-and-destroy---computed

It will also break all associated functions (those without a self) as you need to do: <KString>::my_func() afaik instead of KString::my_func()

@nu11ptr nu11ptr added enhancement New feature or request wontfix This will not be worked on labels Mar 26, 2022
@nu11ptr
Copy link
Owner

nu11ptr commented Mar 26, 2022

The pad as a type makes sense. Adding that.

The tag variant doesn't work well for me because of all the padding and custom type options. I'd need to add a 3rd pad length which is something that isn't worth it to me for the extra noise and burden on custom string impls.

@nu11ptr
Copy link
Owner

nu11ptr commented Mar 28, 2022

Added tag in new branch. Skipping tag variant for reasons mentioned for now.

@nu11ptr nu11ptr closed this as completed Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants