api: Update From<String> and From<Box<str>> to eagerly inline #256
Conversation
…d from_string_buffer API
compact_str/src/repr/mod.rs
Outdated
| @@ -165,9 +166,14 @@ impl Repr { | |||
| // We only hit this case if the provided String is > 16MB and we're on a 32-bit arch. We | |||
| // expect it to be unlikely, thus we hint that to the compiler | |||
| capacity_on_heap(s) | |||
| } else if og_cap == 0 { | |||
| } else if s.is_empty() { | |||
There was a problem hiding this comment.
If should_inline is false and s contains a large pre-allocated buffer, then should we re-use the buffer to avoid alloc?
There was a problem hiding this comment.
Yeah good thought, I updated the PR to be based on capacity now instead of length
Reason being, when created a String will only allocate as much space as needed. When appending to a string though it grows at a rate of 2x to amortize the cost of re-allocations. This leaves us with 2 scenarios:
- capacity == length, in which case it doesn't matter which we check
- capacity > length, this indicates the user appended to the string, or specifically allocated more space, which it seems like we'd want to keep
There was a problem hiding this comment.
hmmm, actually it seems like Clone-ing today actually doesn't preserve capacity, example, so my assumption that users would want to preserve capacity might be wrong, let me think more
compact_str/src/repr/mod.rs
Outdated
| @@ -232,7 +238,7 @@ impl Repr { | |||
| } | |||
|
|
|||
| #[inline] | |||
| pub fn from_box_str(s: Box<str>) -> Self { | |||
| pub fn from_box_str(s: Box<str>, should_inline: bool) -> Self { | |||
There was a problem hiding this comment.
IMO this can be forwarded to Repr::from_string
There was a problem hiding this comment.
Good call! I made this change
- add doc comment to CompactString explaining the behavior of From<String> - change Repr::from_string to eagerly inline based on string length, not capacity, which results in us possibly truncating capacity - delete Repr::from_box_str, and have impl From<Box<str>> for CompactString use Repr::from_string - update tests
Currently when converting from a
Stringto aCompactStringusingFrom<String>(also forBox<str>) we wouldn't inline short strings, this was to avoid needing to de-allocate the originalString. After some feedback collected in #227 we realized while this was performant, it's not what most users expect to happen.This PR updates the implementation of
From<String>andFrom<Box<str>>to inline short strings, and it adds a new APICompactStr::from_string_buffer, which retains the current behavior of always re-using the underlying buffer. Note, in #254 we implemented inline-ing duringClone, so even if you usefrom_string_buffer, future short strings should get inlined.Fixes #227