GDExtension slower than GDscript

[mbrlabs]

System: Manjaro Linux
Godot Version 4.0.0 stable
godot-cpp version: 9d1c396

I experimented a bit with GDExtension in Godot 4 and found that it is quite a bit slower than GDscript. Note that for this to work this needs to be merged first because of a bug.

Consider the following code in GDscript which modifies every pixel of an image. On my system this runs in about 20ms for a 512x512 image.

func test(img: Image) -> void:
    for x in img.get_width():
        for y in img.get_height():
	    var color := img.get_pixel(x, y)
	    color.r *= 1.5
	    img.set_pixel(x, y, color)

This GDExtension version takes 30ms, so 10ms longer (compiled with scons target=template_release):

void GDExampleTest::test(Ref<Image> img) {
    for (int x = 0; x < img->get_width(); x++) {
        for (int y = 0; y < img->get_height(); y++) {
            Color color = img->get_pixel(x, y);
            color.r *= 1.5;
            img->set_pixel(x, y, color);
        }
    }
}

I expected GDExtension to have a bit of overhead compared to normal modules when calling engine methods in a tight loop like that, but i did not think that it would be slower than even GDscript.

Not sure what to think here. Does this look normal to you? Basically the only reason i wanted to use GDExtension is to boost performance.

[laingawbl]

i have no guesses on normality, but I ran into this same problem and got better performance by creating a PackedByteArray, manually packing the data, and then calling Image::create_from_data.

My first impulse was to guess that the slowdown is caused by the repeated interface conversions when calling set_pixel, but creating a PackedByteArray and manually packing the values calls the engine function encode_half (or encode_u8 or whatever) a greater number of times. I don't think it's the big switch statement (on encoding) in set_pixel, either, because then both the GDExtension and GDScript versions would be slow.

So either

• not all GDExtension->engine calls are created equal, and the PackedByteArray call setup really is faster, or
• I'm just seeing a speedup from building the PackedByteArray over calling set_pixel, but not from fewer GDExtension->engine calls or
• there's something else at work.

But FWIW you might be able to get some speedup without necessarily solving the question.

[mbrlabs]

Thank you for the input! Yeah i also thought it would be something like that.

I am now using compute shaders for image processing and it's approx. 80-100 times faster than the C++ version. So i'm not really worried about performance anymore but it's still pretty disappointing that the overhead is so big.
Definitely something to keep in mind when calling engine methods in GDExtension often. I wonder if other classes have the same problem.

[parkovski]

I think your C++ code is calling img->get_width() / img->get_height() at each iteration and the GDScript version probably only calls them once at the start of the loop. C++ has the potential to be fast but it's often not without some real scrutiny and/or profiling.

[redsett]

System: Windows
Godot Version 4.0.2 stable
godot-cpp version: feaba55

Just to add to this thread. The code below runs about 30-40% faster in GDScript than it does with GDExtension.

GDScript

func _draw() -> void:
	var alpha:float = 0
	var endPos2:Vector2 = Vector2(_startPos.x, _endPos.y * -0.1)
	var newStartPos:Vector2 = _startPos
	var newEndPos:Vector2 = _startPos
	
	while alpha <= 1:
		newStartPos = newEndPos
		
		var alphaReversed:float = abs(alpha - 1)
		newEndPos = (alphaReversed * _startPos) + (alpha * ((alphaReversed * endPos2) + (alpha * _endPos)))
		
		draw_line(newStartPos, newEndPos, LineColor, LineWidth)
		alpha += 0.05

Cpp - (compiled with scons target=template_release)

void MyNode::_draw()
{
	float alpha = 0.0f;
	const Vector2 endPos_2 = Vector2(starting_pos.x, ending_pos.y * -0.1f);
	Vector2 new_start_pos = starting_pos;
	Vector2 new_end_pos = starting_pos;

	while (alpha <= 1.0f) {
		new_start_pos = new_end_pos;
		
		const float alphaReversed = abs(alpha - 1);
		new_end_pos = (alphaReversed * starting_pos) + (alpha * ((alphaReversed * endPos_2) + (alpha * ending_pos)));
		
		draw_line(new_start_pos, new_end_pos, spline_color, spline_width);
		alpha += 0.05;
	}
}

[hilloftheking]

I believe the culprit is that the generated method bindings construct a StringName from a C string literal on each invocation. Calling a method over and over in a loop (such as Image::get_pixel and Image::set_pixel) results in a huge number of StringNames being constructed. I managed to create a quick solution myself but I'm not sure if it is acceptable for all of the bindings.

In my project, I call Noise::get_noise_3dv multiple times inside of a big loop. I found out I could hack together a very temporary solution (the method hash in _gde_method_bind is subject to change) that provides a huge speed up. I'm not exactly sure what this would do to the data segment of binaries if this were used for each generated binding.

#include <godot_cpp/godot.hpp>
#include <godot_cpp/core/engine_ptrcall.hpp>
#include <godot_cpp/variant/string_name.hpp>

// This example is for Noise::get_noise_3dv
#include <godot_cpp/classes/noise.hpp>

// You can make your own version of any method like this
double fast_get_noise_3dv(const Ref<Noise> &noise, const Vector3 &v) {
  static StringName method_name = "get_noise_3dv";
  static GDExtensionMethodBindPtr _gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(Noise::get_class_static()._native_ptr(), method_name._native_ptr(), 1109078154);
  CHECK_METHOD_BIND_RET(_gde_method_bind, 0.0);
  return internal::_call_native_mb_ret<double>(_gde_method_bind, noise->_owner,
                                               &v);
}

// With this being the original godot-cpp binding (in gen/src/classes)
double Noise::get_noise_3dv(const Vector3 &v) const {
  const StringName _gde_class_name = Noise::get_class_static();
  const StringName _gde_method_name = "get_noise_3dv";
  static GDExtensionMethodBindPtr _gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(_gde_class_name._native_ptr(), _gde_method_name._native_ptr(), 1109078154);
  CHECK_METHOD_BIND_RET(_gde_method_bind, 0.0);
  return internal::_call_native_mb_ret<double>(_gde_method_bind, _owner, &v);
}

[BastiaanOlij]

Wow, looks like we lost the entire caching, it shouldn't be grabbing the method bind on every call, it should do that once, and then reuse the pointer...

So I'm guessing that:

int32_t Image::get_width() const {
	const StringName _gde_class_name = Image::get_class_static();
	const StringName _gde_method_name = "get_width";
	static GDExtensionMethodBindPtr _gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(_gde_class_name._native_ptr(), _gde_method_name._native_ptr(), 3905245786);
	CHECK_METHOD_BIND_RET(_gde_method_bind, 0);
	return internal::_call_native_mb_ret<int64_t>(_gde_method_bind, _owner);
}

Should be something like:

int32_t Image::get_width() const {
	static GDExtensionMethodBindPtr _gde_method_bind = nullptr;
	if (_gde_method_bind == nullptr) {
		const StringName _gde_class_name = Image::get_class_static();
		const StringName _gde_method_name = "get_width";
		_gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(_gde_class_name._native_ptr(), _gde_method_name._native_ptr(), 3905245786);
	}
	CHECK_METHOD_BIND_RET(_gde_method_bind, 0);
	return internal::_call_native_mb_ret<int64_t>(_gde_method_bind, _owner);
}

[BastiaanOlij]

cc @vnen @dsnopek @Faless

[dsnopek]

Heh, sorry for an earlier (now deleted) comment, where I misinterpreted which code snippet represented what is currently in godot-cpp, and which snippet is what @hilloftheking is suggesting :-)

Wow, looks like we lost the entire caching, it shouldn't be grabbing the method bind on every call, it should do that once, and then reuse the pointer...

Since the variable holding the method bind is a local static variable, that appears to be what's already happening. It should only be assigned the first time the function is run, and then all subsequent times it'll reuse the cached value.

See this excerpt from cppreference: https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables

Variables declared at block scope with the specifier static or thread_local (since C++11) have static or thread (since C++11) storage duration but are initialized the first time control passes through their declaration [... snip], which can be performed before the block is first entered). On all further calls, the declaration is skipped.

(I snipped out a parenthetical in the middle for clarity)

Now, looking at @BastiaanOlij's example of a current function in godot-cpp:

int32_t Image::get_width() const {
	const StringName _gde_class_name = Image::get_class_static();
	const StringName _gde_method_name = "get_width";
	static GDExtensionMethodBindPtr _gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(_gde_class_name._native_ptr(), _gde_method_name._native_ptr(), 3905245786);
	CHECK_METHOD_BIND_RET(_gde_method_bind, 0);
	return internal::_call_native_mb_ret<int64_t>(_gde_method_bind, _owner);
}

I'm not sure if we need to worry about the declaration of _gde_class_name, because I think the compiler might be smart enough to inline that? This is something we should check.

However, I think @hilloftheking is right that adding static to the declaration of _gde_method_name would avoid re-initializing that StringName on every invocation. But I don't know how much time that would actually save - we need to do some benchmarking!

[hilloftheking]

adding static to the declaration of _gde_method_name would avoid re-initializing that StringName on every invocation

I don't really like my solution because it would increase the data segment of binaries. I prefer what @BastiaanOlij suggests because it is clean and the data segment stays the same. A less clean but more concise way would be to put both StringNames onto the same line as _gde_method_bind like so:

int32_t Image::get_width() const {
  static GDExtensionMethodBindPtr _gde_method_bind =
      internal::gdextension_interface_classdb_get_method_bind(
          Image::get_class_static()._native_ptr(),
          StringName("get_width")._native_ptr(), 3905245786);
  CHECK_METHOD_BIND_RET(_gde_method_bind, 0);
  return internal::_call_native_mb_ret<int64_t>(_gde_method_bind, _owner);
}

But I don't know how much time that would actually save - we need to do some benchmarking!

I found out that my project was not as performant as I would have liked it to be with the Visual Studio profiler.

Original generated function; compiled with /O2 and debug symbols

Constructing the method name is more expensive than actually calling the method. _gde_class_name isn't quite as bad, but it is only used for getting the method bind pointer so it should still be avoided.

[dsnopek]

I just posted PR #1176 to generate @BastiaanOlij's suggested code. The automated test suite ran fine, so I think it works, but I haven't done any kind of benchmarking on it

[vnen]

int32_t Image::get_width() const {
  static GDExtensionMethodBindPtr _gde_method_bind =
      internal::gdextension_interface_classdb_get_method_bind(
          Image::get_class_static()._native_ptr(),
          StringName("get_width")._native_ptr(), 3905245786);
  CHECK_METHOD_BIND_RET(_gde_method_bind, 0);
  return internal::_call_native_mb_ret<int64_t>(_gde_method_bind, _owner);
}

I think I do prefer this approach because it only tries to get the method one time. Otherwise, if the method does not exist it will keep trying to load every call (it is a degenerate case anyway, but I still think best to avoid).

If we want to go with @BastiaanOlij's solution, the CHECK_METHOD_BIND becomes redundant, or should be put inside the if block, since it's not relevant once we know the method bind isn't nullptr.

[dsnopek]

I think I do prefer this approach because it only tries to get the method one time. If the method does not exist it will keep trying to load every call (it is a degenerate case anyway, but I still think best to avoid).

Ah, that's a good point!

And given that this is generated code, I don't think there's really any downside to cramming it all onto one line anyway (ie readability doesn't matter).