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

impl From<String> for SharedString shouldn't unnecessarily clone (Rust) #6597

Open
Enyium opened this issue Oct 20, 2024 · 1 comment
Open
Labels
a:tool classes & property system runtime core classes (SharedVector,SharedString) and property system (mO,bS) enhancement New feature or request

Comments

@Enyium
Copy link
Contributor

Enyium commented Oct 20, 2024

The code in v1.8.0 is:

impl From<String> for SharedString {
    fn from(s: String) -> Self {
        s.as_str().into()
    }
}

impl From<&str> for SharedString {
    fn from(value: &str) -> Self {
        SharedString {
            inner: SharedVector::from_iter(
                value.as_bytes().iter().cloned().chain(core::iter::once(0)),
            ),
        }
    }
}

You could:

  • Call String::push('\0').
  • Because String::into_raw_parts() is Nightly-only, use String's capacity() and leak() with as_mut_ptr() and len() on the returned slice instead.
    • Note that the implementations of String's into_raw_parts() and leak() (and the same of Vec that are used there) are very similar.
  • Either build the inner SharedVector<u8> in fn from(s: String) -> SharedString, or add something like from_raw_parts() to SharedVector (at least with pub(crate)) and call this.

EDIT: It seems the layout of your SharedVectorInner would make it necessary to shift the data by the size of the SharedVectorHeader struct to place it in front of the String data. This is unfortunate. Wouldn't avoiding the reallocation, if possible, still be desirable? If reallocation would be unavoidable, it should definitely only happen once. So, no String::push(), but a new SharedVector::from_raw_data_buffer_with_reserve(ptr: ..., len: usize, capacity: usize, additional_reserve: usize).

@FloVanGH FloVanGH added the need triaging Issue that the owner of the area still need to triage label Oct 21, 2024
@ogoffart ogoffart added a:tool classes & property system runtime core classes (SharedVector,SharedString) and property system (mO,bS) enhancement New feature or request and removed need triaging Issue that the owner of the area still need to triage labels Oct 21, 2024
@ogoffart
Copy link
Member

What I was thinkink is to allow SharedString to represent static data or small strings without allocations.
But converting from Rust's String to SharedString without alocation may be difficult.
One reason we have our own string is such that we can share it with C++.
We also do store the reference count in the allocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:tool classes & property system runtime core classes (SharedVector,SharedString) and property system (mO,bS) enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants