godot-rust / gdext

Rust bindings for Godot 4
https://mastodon.gamedev.place/@GodotRust
Mozilla Public License 2.0
2.92k stars 180 forks source link

[Performance Issue] Unneeded calls to class_name::to_string_name #792

Closed Ughuuu closed 3 weeks ago

Ughuuu commented 1 month ago

Not sure why it happens, @lilizoey has more insight, but it seems there are some casses that could be optimized out where we call class_name::to_string_name. In my performance investigation, things that should be no-op appear on the times instead. Specifically this happens inside the Array::clone:

impl<T: ArrayElement> Clone for Array<T> {
    fn clone(&self) -> Self {
        // SAFETY: `self` is a valid array, since we have a reference that keeps it alive.
        let array = unsafe {
            Self::new_with_uninit(|self_ptr| {
                let ctor = sys::builtin_fn!(array_construct_copy);
                let args = [self.sys()];
                ctor(self_ptr, args.as_ptr());
            })
        };

        array
            .with_checked_type()
            .expect("copied array should have same type as original array")
    }
}

But I remember seeing it in other functions too. @lilizoey said on discord: in here, that final expect can be turned into an unwrap_unchecked() which should be safe and also avoids constructing any string names

lilizoey commented 1 month ago

But I remember seeing it in other functions too. @lilizoey said on discord: in here, that final expect can be turned into an unwrap_unchecked() which should be safe and also avoids constructing any string names

i did try to do this but it actually didnt seem to improve performance much (in fact it seemed to make it slightly worse with a simple benchmark at least?). i suspect that rust might not be able to inline as well if we do that for some reason.