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

[Performance Issue] Array methods duplicate Variant param or Callable take ownership when it could not. #790

Closed
Ughuuu opened this issue Jul 5, 2024 · 3 comments · Fixed by #906
Labels
performance Performance problems and optimizations

Comments

@Ughuuu
Copy link
Contributor

Ughuuu commented Jul 5, 2024

From Performance Investigation

  1. Extra clones
    Calling set and many other methods of Array with Variant arguments clones the Variant.
    Example:
let arr = Array::new();
arr.set(0, Variant::nil());

The set will call internally:

    pub fn set(&mut self, index: usize, value: T) {
         let ptr_mut = self.ptr_mut(index);

         // SAFETY: `ptr_mut` just checked that the index is not out of bounds.
         unsafe {
             value.to_variant().move_into_var_ptr(ptr_mut);
         }
     }

which will do to_variant(), even though the argument is already variant. There are multiple such cases throughout the Array class.

  1. Ownership

Calling callv:

    pub fn callv(&self, arguments: VariantArray) -> Variant {
        self.as_inner().callv(arguments)
    }

Takes ownership of VariantArray. It might seem small thing, but for my use cases, i make about 10000 or more calls using callv. Allocating the array every time takes about 100-150ms of a 4s timeslice. I would like to be able to reuse the array. Eg. send it to the callv method, and after the method is called be able to reuse my array:

    pub fn callv(&self, arguments: &VariantArray) -> Variant {
        self.as_inner().callv(arguments)
    }
@Bromeon Bromeon added the performance Performance problems and optimizations label Jul 8, 2024
@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

which will do to_variant(), even though the argument is already variant. There are multiple such cases throughout the Array class.

The argument is not Variant, it is T. Just your specific monomorphization uses T = Variant. Unfortunately we don't have specialization in Rust, so optimizing for individual T's is hard (not impossible, but very annoying).

However, there are possible alternatives:

  1. value: &T parameter -- since to_variant(&self) also takes a reference, we don't need a value here. However, in your example of passing a newly constructed variant, this would still involve a clone.
  2. a new ToGodot::into_variant() method which takes values and is a no-op for Self = Variant.

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

3. Ownership

Calling callv [...] Takes ownership of VariantArray. It might seem small thing, but for my use cases, i make about 10000 or more calls using callv. Allocating the array every time takes about 100-150ms of a 4s timeslice. I would like to be able to reuse the array. Eg. send it to the callv method, and after the method is called be able to reuse my array:

Yes, this is definitely planned. There are thousands of generated APIs that currently take value where they don't have to, also for Gd<T>.

@lilizoey
Copy link
Member

lilizoey commented Jul 8, 2024

2. a new `ToGodot::into_variant()` method which takes values and is a no-op for `Self = Variant`.

I've started working on this btw, this does require changing things in several places to make sure it's used everywhere it's appropriate.

To make this work for Array in particular, we do need to optimize the constructors that take owned values. For instance FromIterator. Since currently From<&[T]> is our fastest implementation since it can just use pointers to write all the values to the array, but it can't use into_variant since it's borrowed. Maybe just a From<Vec<T>> could work for now though, optimizing FromIterator is harder since we'd need to rely on size_hint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance problems and optimizations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants