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

Allow in-place allocation of FuriString #2461

Closed
dcoles opened this issue Mar 5, 2023 · 2 comments
Closed

Allow in-place allocation of FuriString #2461

dcoles opened this issue Mar 5, 2023 · 2 comments
Assignees
Labels
Core+Services HAL, furi & core system services Feature Request New feature or user-story you wanna add to flipper

Comments

@dcoles
Copy link
Contributor

dcoles commented Mar 5, 2023

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()) })
    }
}
@hedger hedger added the Feature Request New feature or user-story you wanna add to flipper label Mar 6, 2023
@skotopes skotopes added the Core+Services HAL, furi & core system services label Mar 13, 2023
@skotopes
Copy link
Member

In theory possible, but not going to save much.

@DrZlo13
Copy link
Member

DrZlo13 commented Jun 22, 2023

  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.

@DrZlo13 DrZlo13 closed this as completed Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core+Services HAL, furi & core system services Feature Request New feature or user-story you wanna add to flipper
Projects
None yet
Development

No branches or pull requests

4 participants