Remove static lifetime in StringName::from(&CStr)

[beicause]

See #1306 and https://github.com/godotengine/godot/blob/9b50ea8adec073c891763f7c8c2844bf207cf103/core/string/string_name.cpp#L204-L259

Godot copys the cstr to _data->name, so it doesn't need static lifetime. Also p_static just marks it's static count and should not affect performance, it's useless for godot-rust now.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1307

[Bromeon]

See also #531 (comment).

In #1306 you mentioned:

---

If I use:

static STATIC_STRING_NAME: LazyLock =
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.

statics are not dropped in Rust. I don't see why "never freed" is surprising?

Also, why would you use both LazyLock and the C-string initialization?
A regular StringName constructor does the job here.

---

I did a quick benchmark that shows no performance advantage comparing with p_is_static=false.

Please share that benchmark. Can you reproduce this with our existing #[bench] infrastructure?

[Bromeon]

Also, if we find that &'static CStr construction makes no sense, I'd rather remove the constructor entirely.

We already have StringName::try_from_cstr() for such cases, with explicit encoding.

[beicause]

statics are not dropped in Rust. I don't see why "never freed" is surprising?

Also, why would you use both LazyLock and the C-string initialization?
A regular StringName constructor does the job here.

In c++, using p_static=true for static StringName doesn't cause orphan StringName, but p_static=false will:

	static StringName a = StringName("a", false);
	static StringName b = StringName("b", true);

Orphan StringName: a (static: 0, total: 1)

So we should use p_static=true together with static variable and don't assign it to local variable. Current inc_ref approch is not right.

---

We already have StringName::try_from_cstr() for such cases, with explicit encoding.

It might make sense, but from(c"cstr") is more convenient for me to construct a StringName.

[beicause]

Benchmark in release mode:

#[bench(repeat = 1000)]
fn string_name_cstr() -> StringName {
    use godot::sys;
    let c_str = c"c string";
    unsafe {
        StringName::new_with_string_uninit(|ptr| {
            sys::interface_fn!(string_name_new_with_latin1_chars)(
                ptr,
                c_str.as_ptr(),
                sys::conv::SYS_FALSE, // p_is_static
            )
        })
    }
}

#[bench(repeat = 1000)]
fn string_name_cstr_static() -> StringName {
    use godot::sys;
    let c_str = c"c string";
    let sname = 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
            )
        })
    };
    std::mem::forget(sname.clone());
    sname
}

#[bench(repeat = 1000)]
fn string_name_cstr_static_correct() -> StringName {
    use godot::sys;
    let c_str = c"c string";
    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()
}

   -- string_name_cstr           ...      0.059μs      0.062μs
   -- string_name_cstr_static    ...      0.041μs      0.045μs
   -- string_name_cstr_static_co ...      0.018μs      0.020μs

My explanation is that the 2nd (current method) leaks one, so Godot constructing it would be faster than the 1st, but this usage is incorrect. The 3rd is the right way.

[Bromeon]

Thanks a lot! I'll try to have a detailed look at this on the weekend. 👍

---

In c++, using p_static=true for static StringName doesn't cause orphan StringName, but p_static=false will:

But C++ statics are very different from Rust static, as they call the destructor -- and not rarely cause initialization/destruction order fiasco by doing so. That said, I agree we should reconsider the p_static=true usefulness for Rust, especially if it doesn't deliver the necessary performance.

---

It might make sense, but from(c"cstr") is more convenient for me to construct a StringName.

The other string types don't offer such a constructor either, and C strings aren't a very common type in everyday Rust. You can easily create your own sname(...) function if that's a common use case, but for most godot-rust users I'd personally prefer explicitness about the encoding, to prevent potential mistakes. Especially since some users might have relied on the current &'static behavior, which would now change.

[beicause]

Found a way to create static StringName through macro (static_name!). I also benchmarked StringName::from(&str), it just slightly slower than from(&CStr), not as bad as imagined:

#[bench(repeat = 10000)]
fn string_name_utf8() -> StringName {
    StringName::from("a string")
}

#[bench(repeat = 10000)]
fn string_name_cstr() -> StringName {
    use godot::sys;
    let c_str = c"a string";
    unsafe {
        StringName::new_with_string_uninit(|ptr| {
            sys::interface_fn!(string_name_new_with_latin1_chars)(
                ptr,
                c_str.as_ptr(),
                sys::conv::SYS_FALSE, // p_is_static
            )
        })
    }
}

#[bench(repeat = 10000)]
fn string_name_cstr_static() -> StringName {
    use godot::sys;
    let c_str = c"a string";
    let sname = 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
            )
        })
    };
    std::mem::forget(sname.clone());
    sname
}

#[bench(repeat = 10000)]
fn string_name_cstr_static_correct() -> StringName {
    use godot::sys;
    let c_str = c"a string";
    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()
}

#[bench(repeat = 10000)]
fn string_name_cstr_static_name_macro() -> StringName {
    godot::builtin::static_name!(c"a string").clone()
}

   -- string_name_utf8           ...      0.068μs      0.071μs
   -- string_name_cstr           ...      0.057μs      0.059μs
   -- string_name_cstr_static    ...      0.040μs      0.041μs
   -- string_name_cstr_static_co ...      0.018μs      0.020μs
   -- string_name_cstr_static_na ...      0.018μs      0.020μs

[Bromeon]

Thanks a lot! I'll need to look in detail at a later point.

Regarding this PR, since it contains feature additions (which can happen without SemVer breakage), I'll postpone it after v0.4.0 release. The static creation macro will likely need some more discussion 🙂

---

From my earlier post:

Also, if we find that &'static CStr construction makes no sense, I'd rather remove the constructor entirely.

Created #1316 to free the way for future improvements.

[Bromeon]

Sorry, I accidentally merged this too soon. I will keep the code around, but unexpose static_name! from the public API until we have more information / measurements. Either way, thanks already 🙂

The From impl I will remove in #1316.