-
Notifications
You must be signed in to change notification settings - Fork 40
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
perf: Refactor underlying buffers for branchless access #229
Conversation
b102f55
to
3dd5b2e
Compare
e52c187
to
68c5e74
Compare
compact_str/src/repr2/mod.rs
Outdated
// SAFTEY: Our needed_capacity is >= our length, which is <= than MAX_SIZE | ||
let inline = unsafe { InlineBuffer::new(self.as_str()) }; | ||
// SAFETY: `InlineBuffer` and `Repr` are the same size | ||
*self = unsafe { mem::transmute(inline) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about implementing impl From<InlineBuffer> for Repr
and impl From<HeapBuffer> for Repr
so that we don't need these unsafe
transmute blocks everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'm going to stick with this pattern because I believe it'll be more performant. Otherwise we'll need to call through fn from(...)
which would incur a performance penalty
compact_str/src/repr2/mod.rs
Outdated
} | ||
} else { | ||
let pointer = &self as *const _ as *const u8; | ||
let length = core::cmp::min((last_byte.wrapping_sub(LENGTH_MASK)) as usize, MAX_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this a couple of time, maybe it can be extracted as a separate function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bad idea, I'll do this in a follow up PR
compact_str/src/repr2/mod.rs
Outdated
#[repr(C)] | ||
pub struct Repr( | ||
// We have a pointer in the repesentation to properly carry provenance | ||
*const (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to use ptr::NonNull<u8>
here if we modify push_str
, extend
and etc to ignore null bytes inserted into the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought! For now I'll just keep it *const ()
but we could change this in the future
Thanks for the review @NobodyXu! I'll update the PR soon with the feedback :) |
This PR refactors the underlying
InlineString
andBoxString
structs in a major way to allow for branch-less string accesses. It also renames these structs toInlineBuffer
andHeapBuffer
, treating these types strictly as "buffers". We also move string operations intoRepr
(e.g. pushing and popping), which de-dupes code.