flipperdevices / flipperzero-firmware

Flipper Zero firmware source code
https://flipperzero.one
GNU General Public License v3.0
11.83k stars 2.61k forks source link

Allow in-place allocation of `FuriString` #2461

Closed dcoles closed 1 year ago

dcoles commented 1 year ago

Describe the enhancement you're suggesting.

Currently the only way to create a FuriString is via a function like furi_string_alloc that returns an opaque FuriString*. However the underlying m-string does support in-place allocation of string_t:

{
    string_t s1;
    string_init (s1); // initialize `string_t` structure
    string_set_str (s1, "Hello, world!");
    string_clear(s1); // frees any memory dynamically allocated by `s1`
}

In-place allocation of string_t allows taking advantage of the short-string optimisation that allocates small strings directly in string_t, rather than always allocating space on the heap.

I believe it would be possible to add a furi_string_init and furi_string_clear function to the SDK that can initialize a stack-allocated FuriString. However this means that a stack-allocated FuriString can't be passed into any function that assumes it can take ownership of a FuriString* (and thus call furi_string_free). It would also require exposing the implementation of FuriString so that the compiler can allocate the correct amount of space.

Anything else?

The reason I'm interested in doing this is that it allows creation of a stack-allocated Rust FuriString type in flipperzero-rs (see discussion here). The type would look approximately like the following (note: may have some syntax errors):

/// Dynamically allocated string.
/// This is a wrapper around the SDK's C `FuriString` type).
#[repr(transparent)]
pub struct FuriString(sys::FuriString);

impl FuriString {
    /// Create a new [`FuriString`]
    pub fn new() -> FuriString {
        // Uninitialized structure
        let mut furi_string = MaybeUninit<sys::FuriString> = MaybeUninit::uninit();
        unsafe {
            // Initialize `sys::FuriString` memory
            sys::furi_string_init(furi_string.as_mut_ptr());
        }

        // Wrap the initialized `sys::FuriString` in a wrapper type
        FuriString(unsafe { furi_string.assume_init() })
    }

    /// Get a pointer to this string that can be used with Flipper Zero SDK APIs.
    pub fn to_raw(&self) -> *const sys::FuriString {
        &self.0 as &sys::FuriString as *const sys::FuriString
    }

    // ...
}

impl Drop for FuriString {
    fn drop(&mut self) {
        unsafe {
            // Ensure any dynamic memory allocations are freed
            sys::furi_string_clear(self.to_mut_raw());
        }
    }
}

/// We can trivially get a `&CStr` from a `FuriString`
impl AsRef<CStr> for FuriString {
    #[inline]
    fn as_ref(&self) -> &CStr {
        CStr::from_ptr(unsafe { sys::furi_string_get_cstr(self.to_raw()) })
    }
}
skotopes commented 1 year ago

In theory possible, but not going to save much.

DrZlo13 commented 1 year ago
  1. SSO (small string optimization) requires disclosed FuriString structure, but we want as much encapsulation as possible.
  2. If we do not reveal the inners of the FuriString, but use the pointer as is - SSO will hold a maximum of 3-character strings.
  3. When we migrated from M*Lib to our wrapper, I was profiling and the performance impact was very negligible.

Given all of the above - I see no point in this optimization.