Orphan StringName when using `static LazyLock<StringName>` and useless `p_is_static=true`

[beicause]

If I use:

static STATIC_STRING_NAME: LazyLock<StringName> =
    LazyLock::new(|| StringName::from(c"static_string_name")); // Or from("str");

Then the static StringName will be orphan (static count 1, refcount 2) because inc_ref increments refcount and it is never freed.

gdext/godot-core/src/builtin/string/string_name.rs

Lines 402 to 412 in 07c34e7

	fn from(c_str: &'static std::ffi::CStr) -> Self {
	// SAFETY: c_str is nul-terminated and remains valid for entire program duration.
	let result = unsafe {
	Self::new_with_string_uninit(|ptr| {
	sys::interface_fn!(string_name_new_with_latin1_chars)(
	ptr,
	c_str.as_ptr(),
	sys::conv::SYS_TRUE, // p_is_static
	)
	})
	};

In my opinion, creating a p_is_static=true StringName from CStr without assigning it to a static variable makes no sense, since otherwise you're still constructing and destroying the StringName. I did a quick benchmark that shows no performance advantage comparing with p_is_static=false. I believe it's better to make From<&CStr> non-static because we do not have static cache.

Not sure if we can add a method like Godot's SNAME to create a static StringName, or use a global HashMap cache
(Actually in C#, global HashMap cache can significantly improve performance, but in C++ or rust it may be not worth).

[Bromeon]

Discussion in #1307. If you want to open an issue and PR at the same time, you can also do the PR directly -- this reduces admin overhead and two parallel discussions. Many of the PRs I open have no associated issue, I wouldn't get anything done if I had to document each problem twice 😉

[beicause]

I opened this issue because I haven't resolved the usage of static StringName in #1307 yet. Not sure if we can wrap it as a dedicated method or macro. Actually, godot-cpp only exposes this parameter for users to implement themselves. We might be able to do this too if there are no other options.

[Bromeon]

Orphan StringName when using static LazyLock and useless p_is_static=true

This sounds like a bug report, but it's a combination of Rust statics never deallocating and godot-rust's current &CStr constructor never deallocating. As such it's mostly badly-documented, and could benefit from a performance improvement.

I opened this issue because I haven't resolved the usage of static StringName in #1307 yet. Not sure if we can wrap it as a dedicated method or macro.

That on the other hand is a new feature request. We should first discuss if this is needed at all, or if it can be trivially added on user side.

So which is it 🙂

[beicause]

This sounds like a bug report, but it's a combination of Rust statics never deallocating and godot-rust's current &CStr constructor never deallocating.

This is not because rust doesn't deallocating. Godot will clean up all StringNames on exit.

p_static determines whether StringName should increments static_count, and if static_count is equal to refcount on exit, the StringName is considered not orphan. godot-rust wrongly uses p_static=True and clone it to deal it like a regular StringName.

That on the other hand is a new feature request. We should first discuss if this is needed at all, or if it can be trivially added on user side.

Since p_static is not exposed in godot-rust, users can't implement it (Of course, them can ignore the orphan StringName warnings and use LazyLock/OnceLock). godot-cpp exposes p_static so users can implement SNAME.

That is to say, current StringName::from(&CStr) doesn't really create a static StringName, it just unnecessarily increments static_count and increments a leaked refcount. The orphan StringName report is as expected.

We can add a macro for static StringName(I prefer). Or exposing p_static and let users to create static StringName themselves like this:

static SNAME: OnceLock<StringName> = OnceLock::new();
    SNAME
        .get_or_init(|| unsafe {
            StringName::new_with_string_uninit(|ptr| {
                sys::interface_fn!(string_name_new_with_latin1_chars)(
                    ptr,
                    c_str.as_ptr(),
                    sys::conv::SYS_TRUE, // p_is_static
                )
            })
        })
        .clone()

I think this is needed if you want to call a method that accepts StringName thousands of times per frame.

[Bromeon]

What is then the proper usage of p_static=true?

Do we need it at all -- especially if we cache the StringName on Rust side? You mentioned in the PR that there's barely a performance difference when not using it.

[beicause]

What is then the proper usage of p_static=true?

Using p_static= true for static variables to suppress the orphan StringName report.

You mentioned in the PR that there's barely a performance difference when not using it.

I mean whether p_static is true or not does not affect performance when creating, but cache does. Cloning StringName from a static cache is almost 3x faster than recreating it (0.06us ->0.02us).

[beicause]

I'm going to close this because I found it's possible to create static StringName and cleanup it from user side.
This is an implemention similar to #1316 (comment) but registries all static StringName to a global vec for convenient cleanup.

pub static STATIC_SNAMES: Mutex<Vec<StringNamePtr>> = Mutex::new(Vec::new());

pub unsafe fn string_name_cleanup() {
    STATIC_SNAMES
        .lock()
        .unwrap()
        .iter()
        .for_each(|s| unsafe { s.drop_static() });
}

#[derive(Clone, Copy)]
pub struct StringNamePtr {
    pub sn: *mut StringName,
}

unsafe impl Sync for StringNamePtr {}
unsafe impl Send for StringNamePtr {}

impl StringNamePtr {
    pub unsafe fn drop_static(&self) {
        unsafe {
            drop(Box::from_raw(self.sn));
        }
    }
}

impl Deref for StringNamePtr {
    type Target = StringName;

    fn deref(&self) -> &Self::Target {
        unsafe { self.sn.as_ref_unchecked() }
    }
}

#[macro_export]
macro_rules! declare_static_sname {
    ($vis:vis,$name:ident,$value:expr) => {
        $vis static $name: std::sync::LazyLock<$crate::util::string::StringNamePtr> =
            std::sync::LazyLock::new(|| {
                use godot::prelude::StringName;

                let sn: *mut StringName = Box::into_raw(Box::new(StringName::from($value)));
                let p = $crate::util::string::StringNamePtr { sn };
                $crate::util::string::STATIC_SNAMES
                    .lock()
                    .unwrap()
                    .push(p);
                p
            });
    };
}

    fn on_level_deinit(level: InitLevel) {
        if level == InitLevel::Core {
            unsafe {
                string_name_cleanup();
            }
        }
    }