Tested versions
tested on 4.7 dev
Saw same behaviour on release.
System information
Godot v4.7.dev (583596c) - Windows 11 (build 22631) - Multi-window, 2 monitors - Vulkan (Forward+) - dedicated Intel(R) Arc(TM) A770 Graphics (Intel Corporation; 32.0.101.8737) - 13th Gen Intel(R) Core(TM) i5-13600K (20 threads) - 31.77 GiB memory - WASAPI (48000 Hz, Stereo/mono)
Issue description
When Godot makes a GDExtensionClassMethodPtrCall to a method bound from GDE, if the return type of the method is an array Godot will initialize an empty array before passing the opaque pointer to a GDE implementation.
Compared with the Variant call GDExtensionClassMethodCall, which passes a nil variant.
This initialization happens at this line of code
github.com/godotengine/godot/blob/aaaf76403e88a3316073f8506672d7e88f167df8/core/extension/gdextension.cpp#L134
Video of the issue
https://youtu.be/2HqOd0hBjBg
Steps to reproduce
define a class struct with an array property
create function in the form of a ClassMethodPtrCall which is the getter of the array
bind the function to Godot
bind the property this is a getter for
add the class to the scene
call the getter function from GDScript
run game (editor goes through the variant call which passes a nil variant)
During the call process, because there is a return, this initializes an array in the variant
the opaque pointer to the array is sent to the GDE code as the r_return value in the ClassMethodPtrCall
Minimal reproduction project (MRP)
n/a
Concerns
confusion
A person working on a new GDE implementation may question what the correct action would be, as I had. Additional confusion can be had as the variant callback passes a nil variant rather than an initialized type.
mem leaks
Myself, working on an implementation of GDE using Odin and Splizard the maintainer of graphics.gd were unaware of this additional allocation. I know for myself that this caused a memory leak of 500KB per second (frame rate dependent, was at 1k fps). Leaving this with allocated heap memory can catch a developer off guard as they may not be able to easily identify the source of such a leak.
what is best practice
As this is passing an allocated piece of memory the question arose of what should be done with it. This was answered by dsnopek.
If one follows what godot-cpp did the array received in GDE should be destroyed in favor of a fresh ref counted reference to the return. This means 2 complete destructions of a class type.
implementation impact
The two array and dictionary returns need to be handled as a special case in order to clear memory before assigning into it.
The previously available array_ref method would take the necessary actions on the initialized array in a single method call. It would also reuse the existing array class rather than allocate a new class.
the deprecation mentions a copy constructor but I'm not sure what/where that is. If it is constructor1 this does not perform the same task as array_ref which calls the operator= method. (please advise)
initialization cost
TBD. Performance testing would be needed to determine how detrimental the creation and destruction of an empty array and dictionary is. It would be worth testing packed arrays as well despite the end result being an empty pointer.
Excluding the following from the init process would not affect existing bindings: Packed arrays; callable; signal.
Cannot exclude Packed*Arrays from init process. Callable Signal maybe.
Other leak areas
Are there other areas where an initialized array or dictionary is passed to GDE which would need to be cleaned by the GDE library?
Proposal
Reinstate the array_ref method in order to ensure we have the = operator override available to GDE.
Create an equivalent for Dictionary.
PR completed by @dsnopek
#119461
Once performance impact has been assessed determine if it would be worthwhile to create a v2 of this call. A branch dependent on how a method was registered may negatively impact performance too much.
- Based on further testing if this is meant to store a value into a variant it would be unsafe for most types to pass uninitialized. The next best action would be to provide the entire variant as a nil variant rather than a typePtr. It would be up to the GDE implementer to determine how to fill the data field.
** I am not sure where else the ptr call is used.
Once performance has been assessed determine if we should skip the init of certain types such as packed arrays.
Packed arrays cannot be skipped because they need the PackedArrayRef wrapper while inside a variant. Otherwise ref count of the wrapper will start at 0 and it will get weird. Still not entirely sure why this is fine when refcounting the Packed*Array via constructor1.
Tested versions
tested on 4.7 dev
Saw same behaviour on release.
System information
Godot v4.7.dev (583596c) - Windows 11 (build 22631) - Multi-window, 2 monitors - Vulkan (Forward+) - dedicated Intel(R) Arc(TM) A770 Graphics (Intel Corporation; 32.0.101.8737) - 13th Gen Intel(R) Core(TM) i5-13600K (20 threads) - 31.77 GiB memory - WASAPI (48000 Hz, Stereo/mono)
Issue description
When Godot makes a GDExtensionClassMethodPtrCall to a method bound from GDE, if the return type of the method is an array Godot will initialize an empty array before passing the opaque pointer to a GDE implementation.
Compared with the Variant call GDExtensionClassMethodCall, which passes a nil variant.
This initialization happens at this line of code
github.com/godotengine/godot/blob/aaaf76403e88a3316073f8506672d7e88f167df8/core/extension/gdextension.cpp#L134
Video of the issue
https://youtu.be/2HqOd0hBjBg
Steps to reproduce
define a class struct with an array property
create function in the form of a ClassMethodPtrCall which is the getter of the array
bind the function to Godot
bind the property this is a getter for
add the class to the scene
call the getter function from GDScript
run game (editor goes through the variant call which passes a nil variant)
During the call process, because there is a return, this initializes an array in the variant
the opaque pointer to the array is sent to the GDE code as the r_return value in the ClassMethodPtrCall
Minimal reproduction project (MRP)
n/a
Concerns
confusion
A person working on a new GDE implementation may question what the correct action would be, as I had. Additional confusion can be had as the variant callback passes a nil variant rather than an initialized type.
mem leaks
Myself, working on an implementation of GDE using Odin and Splizard the maintainer of graphics.gd were unaware of this additional allocation. I know for myself that this caused a memory leak of 500KB per second (frame rate dependent, was at 1k fps). Leaving this with allocated heap memory can catch a developer off guard as they may not be able to easily identify the source of such a leak.
what is best practice
As this is passing an allocated piece of memory the question arose of what should be done with it. This was answered by dsnopek.
If one follows what godot-cpp did the array received in GDE should be destroyed in favor of a fresh ref counted reference to the return. This means 2 complete destructions of a class type.
implementation impact
The two array and dictionary returns need to be handled as a special case in order to clear memory before assigning into it.
The previously available array_ref method would take the necessary actions on the initialized array in a single method call. It would also reuse the existing array class rather than allocate a new class.
the deprecation mentions a copy constructor but I'm not sure what/where that is. If it is constructor1 this does not perform the same task as array_ref which calls the operator= method. (please advise)
initialization cost
TBD. Performance testing would be needed to determine how detrimental the creation and destruction of an empty array and dictionary is. It would be worth testing packed arrays as well despite the end result being an empty pointer.
Excluding the following from the init process would not affect existing bindings: Packed arrays; callable; signal.Cannot exclude Packed*Arrays from init process. Callable Signal maybe.
Other leak areas
Are there other areas where an initialized array or dictionary is passed to GDE which would need to be cleaned by the GDE library?
Proposal
Reinstate the array_ref method in order to ensure we have the = operator override available to GDE.
Create an equivalent for Dictionary.
PR completed by @dsnopek
#119461
Once performance impact has been assessed determine if it would be worthwhile to create a v2 of this call. A branch dependent on how a method was registered may negatively impact performance too much.
** I am not sure where else the ptr call is used.
Once performance has been assessed determine if we should skip the init of certain types such as packed arrays.Packed arrays cannot be skipped because they need the PackedArrayRef wrapper while inside a variant. Otherwise ref count of the wrapper will start at 0 and it will get weird. Still not entirely sure why this is fine when refcounting the Packed*Array via constructor1.