In generated methods, only allocate the method StringName the first time

[dsnopek]

This updates the generated method code to match what @BastiaanOlij suggested in this comment.

Currently, even though the method bind pointer itself is being cached (by virtual of being a local static variable), the StringName with the method name (which is only created as an intermediate value in order to get the method bind pointer) is still constructed every time the function is called.

With the changes in this PR, it should only be constructed the first time.

While this does probably improve performance, it will need more benchmarking to see if it sufficiently solves issue #1063, or if only helps things a little bit.

Fixes #1063

[rburing]

Should this be applied to get_singleton as well?

godot-cpp/binding_generator.py

Lines 1454 to 1468 in d627942

	if is_singleton:
	result.append(f"{class_name} *{class_name}::get_singleton() {{")
	result.append(f"\tconst StringName _gde_class_name = {class_name}::get_class_static();")
	result.append(
	"\tstatic GDExtensionObjectPtr singleton_obj = internal::gdextension_interface_global_get_singleton(_gde_class_name._native_ptr());"
	)
	result.append("#ifdef DEBUG_ENABLED")
	result.append("\tERR_FAIL_COND_V(singleton_obj == nullptr, nullptr);")
	result.append("#endif // DEBUG_ENABLED")
	result.append(
	f"\tstatic {class_name} *singleton = reinterpret_cast<{class_name} *>(internal::gdextension_interface_object_get_instance_binding(singleton_obj, internal::token, &{class_name}::_gde_binding_callbacks));"
	)
	result.append("\treturn singleton;")
	result.append("}")
	result.append("")

Getting _gde_class_name every time can be avoided there. And is it necessary to keep both pointers static? Seems like just the last one would be enough (and that one can be initialized to nullptr and compared with nullptr).

[bruvzg]

The same probably should be done to the get_singleton (internal::gdextension_interface_global_get_singleton) and utility functions (internal::gdextension_interface_variant_get_ptr_utility_function).

[vnen]

As I commented on the issue, this should be moved inside the if block:

godot-cpp/binding_generator.py

Lines 1491 to 1496 in d627942

	if has_return:
	result.append(
	f'\tCHECK_METHOD_BIND_RET(_gde_method_bind, {get_default_value_for_type(method["return_value"]["type"])});'
	)
	else:
	result.append("\tCHECK_METHOD_BIND(_gde_method_bind);")

Because in the else case this check becomes redundant.

[dsnopek]

@vnen made a great point on the issue:

If the method does not exist it will keep trying to load every call

So, I've switched to generating code per his suggestion, which puts everything on the static GDExtensionMethodBindPtr _gde_method_bind = ...; line, rather than using intermediate variables.

This makes the code less readable, but as generated code, I don't think that really matters.

The same probably should be done to the get_singleton (internal::gdextension_interface_global_get_singleton) and utility functions (internal::gdextension_interface_variant_get_ptr_utility_function).

I've also applied a similar change in both these cases as well!

(I strongly suspect that the compiler might be smart enough to inline the CLASS::get_class_static() for singletons, but there's no harm leaving out the intermediate variable in any case.)

[dsnopek]

I finally got around to doing some benchmarking!

My test project has image modifying functions based on the ones from issue #1063

It runs the GDScript and GDExtension versions of each function 50 times, and then reports on the total, average and median times of each function call. It takes the readings in microseconds, but converts them to milliseconds for the output.

Here's what I got with current master (ie. without this PR):

GDSCRIPT VERSION:
 TOTAL: 700.135 - AVERAGE: 14.0027 - MEDIAN: 13.897
GDEXTENSION VERSION:
 TOTAL: 3231.165 - AVERAGE: 64.6233 - MEDIAN: 64.699

And here's with the PR:

GDSCRIPT VERSION:
 TOTAL: 726.888 - AVERAGE: 14.53776 - MEDIAN: 14.507
GDEXTENSION VERSION:
 TOTAL: 227.255 - AVERAGE: 4.5451 - MEDIAN: 4.632

That's a pretty huge improvement!

We go from ~4.5x slower than GDScript (on this benchmark) to ~3x faster - I'm kinda blown away :-)

Thanks to @hilloftheking for tracking this issue down!