[DRAFT] Avoid creating objects that Godot is going to use placement new to initialize (mark 2)

[dsnopek]

This a new idea for a way to fix issue https://github.com/godotengine/godot-cpp/issues/1240

It's a less hacky alternative to PR https://github.com/godotengine/godot-cpp/pull/1378

Rather than messing around with blobs of memory, and trying to cleverly avoid calling object constructors, this just adds a new private constructor that does what we want. :-)

The code is generating for all the variant types, but I've only updated Array and Dictionary to use the new constructors.

[allenwp]

I think the array is being corrupted with the current state of this PR... Here's my test code:

void GDExample::test_memory_behaviour() {
	UtilityFunctions::print("GDExtension::test_memory_behaviour()...");
	{
		Array original_array;
		original_array.push_back(123);
		{
			Variant my_variant = original_array;
			{
				Array my_array = my_variant;
				int value = my_array[0]; // This line causes an exception.
				UtilityFunctions::print("value: ", value);
			}
		}
	}
	UtilityFunctions::print("GDExtension::test_memory_behaviour() COMPLETE");
}

void GDExample::test_memory_behaviour_multiple() {
	for (int i = 0; i < 1000; i++) {
		test_memory_behaviour();
	}
}

With that, I get this exception when it hits the int value = my_array[0]; line:

Exception thrown: read access violation.
this was nullptr.

 	godot.windows.editor.dev.x86_64.exe!Variant::operator __int64() Line 1457	C++
>	godot.windows.editor.dev.x86_64.exe!VariantTypeConstructor<__int64>::type_from_variant(void * r_value, void * p_variant) Line 1544	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::Variant::operator __int64() Line 264	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::Variant::operator int() Line 269	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::GDExample::test_memory_behaviour() Line 27	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::GDExample::test_memory_behaviour_multiple() Line 38	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::call_with_variant_args_helper<godot::GDExample>(godot::GDExample * p_instance, void(godot::GDExample::*)() p_method, const godot::Variant * * p_args, GDExtensionCallError & r_error, IndexSequence<> __formal) Line 242	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::call_with_variant_args_dv<godot::GDExample>(godot::GDExample * p_instance, void(godot::GDExample::*)() p_method, const void * const * p_args, int p_argcount, GDExtensionCallError & r_error, const std::vector<godot::Variant,std::allocator<godot::Variant>> & default_values) Line 366	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::MethodBindT<godot::GDExample>::call(void * p_instance, const void * const * p_args, __int64 p_argument_count, GDExtensionCallError & r_error) Line 325	C++
 	libgdexample.windows.template_debug.dev.x86_64.dll!godot::MethodBind::bind_call(void * p_method_userdata, void * p_instance, const void * const * p_args, __int64 p_argument_count, void * r_return, GDExtensionCallError * r_error) Line 99	C++
 	godot.windows.editor.dev.x86_64.exe!GDExtensionMethodBind::call(Object * p_object, const Variant * * p_args, int p_arg_count, Callable::CallError & r_error) Line 213	C++
 	godot.windows.editor.dev.x86_64.exe!Object::callp(const StringName & p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 775	C++
 	godot.windows.editor.dev.x86_64.exe!Callable::callp(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 68	C++
 	godot.windows.editor.dev.x86_64.exe!Object::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 1128	C++
 	godot.windows.editor.dev.x86_64.exe!Node::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 3673	C++
 	godot.windows.editor.dev.x86_64.exe!Object::emit_signal<>(const StringName & p_name) Line 922	C++
 	godot.windows.editor.dev.x86_64.exe!BaseButton::_pressed() Line 139	C++
 	godot.windows.editor.dev.x86_64.exe!BaseButton::on_action_event(Ref<InputEvent> p_event) Line 177	C++
 	godot.windows.editor.dev.x86_64.exe!BaseButton::gui_input(const Ref<InputEvent> & p_event) Line 70	C++
 	godot.windows.editor.dev.x86_64.exe!Control::_call_gui_input(const Ref<InputEvent> & p_event) Line 1798	C++
 	godot.windows.editor.dev.x86_64.exe!Viewport::_gui_call_input(Control * p_control, const Ref<InputEvent> & p_input) Line 1603	C++
 	godot.windows.editor.dev.x86_64.exe!Viewport::_gui_input_event(Ref<InputEvent> p_event) Line 1869	C++
 	godot.windows.editor.dev.x86_64.exe!Viewport::push_input(const Ref<InputEvent> & p_event, bool p_local_coords) Line 3369	C++
 	godot.windows.editor.dev.x86_64.exe!Window::_window_input(const Ref<InputEvent> & p_ev) Line 1616	C++
 	godot.windows.editor.dev.x86_64.exe!call_with_variant_args_helper<Window,Ref<InputEvent> const &,0>(Window * p_instance, void(Window::*)(const Ref<InputEvent> &) p_method, const Variant * * p_args, Callable::CallError & r_error, IndexSequence<0> __formal) Line 304	C++
 	godot.windows.editor.dev.x86_64.exe!call_with_variant_args<Window,Ref<InputEvent> const &>(Window * p_instance, void(Window::*)(const Ref<InputEvent> &) p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 419	C++
 	godot.windows.editor.dev.x86_64.exe!CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 99	C++
 	godot.windows.editor.dev.x86_64.exe!Callable::callp(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 57	C++
 	godot.windows.editor.dev.x86_64.exe!Callable::call<Ref<InputEvent>>(Ref<InputEvent> <p_args_0>) Line 864	C++
 	godot.windows.editor.dev.x86_64.exe!DisplayServerWindows::_dispatch_input_event(const Ref<InputEvent> & p_event) Line 2961	C++
 	godot.windows.editor.dev.x86_64.exe!DisplayServerWindows::_dispatch_input_events(const Ref<InputEvent> & p_event) Line 2931	C++
 	godot.windows.editor.dev.x86_64.exe!Input::_parse_input_event_impl(const Ref<InputEvent> & p_event, bool p_is_emulated) Line 760	C++
 	godot.windows.editor.dev.x86_64.exe!Input::flush_buffered_events() Line 1026	C++
 	godot.windows.editor.dev.x86_64.exe!DisplayServerWindows::process_events() Line 2647	C++
 	godot.windows.editor.dev.x86_64.exe!OS_Windows::run() Line 1476	C++
 	godot.windows.editor.dev.x86_64.exe!widechar_main(int argc, wchar_t * * argv) Line 182	C++
 	godot.windows.editor.dev.x86_64.exe!_main() Line 204	C++
 	godot.windows.editor.dev.x86_64.exe!main(int argc, char * * argv) Line 218	C++
 	godot.windows.editor.dev.x86_64.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 232	C++

[dsnopek]

Ack, I made a really dumb mistake (fixed in my last push)! Sorry for wasting your effort on testing that. I'll do some testing of my own with your test code.

[dsnopek]

@allenwp I just tested with the code you shared above, and in my latest version I'm not seeing the corruption or the memory leak! When I have a chance, I'm going to expand this PR to address all Variant types and take it out of draft.

[allenwp]

Oh, yeah, at first glance, this looks 1000x better than the other PR. Nicely done! Thanks!

I've tried testing this with my boiled down minimum reproduction code as well as my personal project where I first noticed the leak: Both of these were fixed by this PR draft!

[cyberpuffin-digital]

I applied these changes to my project, and the memory leak disappeared. My benchmark, which usually leaks ~4 GB / min, shows a steady 183MiB static memory throughout.

[dsnopek]

@cyberpuffin-digital Thanks for the review and testing!

I've updated this PR to cover most variant types: definitely all the ones that use reference counting and could leak, plus a couple others which were easy to do. So, I've taken this PR out of draft!

There's a few types where we could avoid initializing them to their defaults, such as Vector2/3/i and Transform2D/3D, but those classes are hand-written and don't call into Godot for anything else, so I've left them as-is for now. We can come back and make that slight optimization later.

[allenwp]

There's a few types where we could avoid initializing them to their defaults, such as Vector2/3/i and Transform2D/3D, but those classes are hand-written and don't call into Godot for anything else, so I've left them as-is for now. We can come back and make that slight optimization later.

Maybe add a comment describing this? If any future issues come up, it would be good to easily see that there is no strong intention for the difference between types that use the new generated constructor vs. the "old" way of creating a temporary variable and calling to_type_constructor.

[dsnopek]

Maybe add a comment describing this?

Makes sense! I've added a @todo comment in the conversion methods where we could consider avoiding the extra initialization in the future.

[allenwp]

Makes sense! I've added a @todo comment in the conversion methods where we could consider avoiding the extra initialization in the future.

Thanks! That leaves only the following untouched, which all have slight differences compared to the other operators which have now been converted or had the @todo added:

bool
int64_t
double
Object *
ObjectID

[dsnopek]

Yep, those ones should stay the way they are, because they are already not initializing their values.

[allenwp]

I've been using this PR in my fork of godot-cpp for a week or so and haven't discovered any instabilities with it.

[dsnopek]

Thanks!

[PiCode9560]

Is this somehow also fixed for other branches/tags, or is it just master?

[dsnopek]

This is marked to be cherry-picked to 4.2 and 4.1, but it hasn't been done yet

[dsnopek]

Cherry-picked for 4.1 in PR https://github.com/godotengine/godot-cpp/pull/1411

[dsnopek]

Cherry-picked for 4.2 in PR https://github.com/godotengine/godot-cpp/pull/1410