Add GDExtension variant_get_ptr_internal_getter, facilitating VariantGetInternalPtr-like behavior in GDExtension

[Ivorforce]

The function returns a pointer to the value, which can be helpful to avoid copies of the content being made from normal conversions.

The godotengine equivalents of this function are VariantInternalAccessor<> as well as VariantGetInternalPtr<>. With this PR, these definitions could be re-created in godot-cpp and other GDExtension projects - with the notable difference that the Object* is validated before being returned. The various use-cases of these two structs can be seen throughout the godot codebase itself.

One example of a use-case for this function is mostly explained in GOP 10830: It allows for in-place modification of (wrapped ref-counted) COW variant arguments, i.e. arrays.

Another use-case example, this allows reading from the arrays without a copy being made from the conversion.

Tagging @dsnopek because we had talked about this kind of functionality before in #98392.

Supersedes #98392.
Related to #98373 (and GOP 10830) by addressing a similar, but not equivalent, concern.

Note that this is my first PR to propose an addition to the GDExtension interface, let me know if I missed anything :)

[dsnopek]

Thanks!

At a high-level, I think this makes sense. However, I'm not crazy about the name - there really isn't anything "validated" about it. With Variant::get_validated_object(), we're looking up the object in ObjectDB to validate it, however, here we're just returning a pointer. Maybe the GDExtension function could be named variant_get_internal_ptr? That would also mirror VariantGetInternalPtr<T>.

I'm also not sure this should be part of the Variant class's public API? In godot-cpp, are we going to expose this exact function, or instead provide VariantGetInternalPtr<T> like we currently have in Godot? And if we're doing the latter, we don't need to add a new public method to Variant, we could put the big switch statement inside of gdextension_variant_get_validated_value() (or whatever we rename that to) which could just use VariantGetInternalPtr<T>.

I think I'd probably lean towards an approach that was only in the GDExtension code, and didn't add anything new to Variant.

[Ivorforce]

I think I'd probably lean towards an approach that was only in the GDExtension code, and didn't add anything new to Variant.

Thank you for the review! I did ponder if that wouldn't be a better approach, but now that I've heard it again in your words I agree the implementation would be better that way. I've changed up the PR, let me know if it looks better now.

I'm also not sure this should be part of the Variant class's public API? In godot-cpp, are we going to expose this exact function, or instead provide VariantGetInternalPtr like we currently have in Godot?

Yeah, I think that would be best, given godot-cpp's ambitions to mimic godot's API.

[dsnopek]

Thanks!

Overall, this looks good to me - just a couple notes below.

[Ivorforce]

Just noting here that the behavior differs to VariantGetInternalPtr on mismatched types - in Godot, it may return an invalid pointer or even crash due to bad memory access, while this function will return a valid pointer to a mismatched type pointee. Given that this is an unsupported failcase anyway I don't think it's important enough to warrant a different function for every single variant type.
Edit: This is no longer the case.

[dsnopek]

Thanks! This looks good to me :-)

[Ivorforce]

Sorry about the late change @dsnopek, for some reason I second guessed my decision to unify the function, to avoid branch misprediction in calls. Should be the same as before, just with function references to accelerate the actual calls by a few nanoseconds. I've probably been reading too much from Agner Fog, lol.

Let me know if it still works for you.

[dsnopek]

Sorry about the late change @dsnopek, for some reason I second guessed my decision to unify the function, to avoid branch misprediction in calls. Should be the same as before, just with function references to accelerate the actual calls by a few nanoseconds.

This is makes sense, and it matches some other Variant-related APIs.

However, I have a bunch of notes on the implementation :-)

[Bromeon]

Just to double-check, the reason for the reversal (returning function pointers instead of the data pointers directly) is branch prediction? Was anything measured in that regard?

This approach is only beneficial if:

• someone knows a-priori what type is contained in the variant (otherwise, a get_type call is necessary)
• function pointers are cached on user side

Is this the intended use case? Because in the "I want to see what type the variant has, and get the internal pointer" scenario, the branching is still necessary, and this API adds indirection for no reason.

If yes, could you document these assumptions in the GDExtension header? To make sure that people aren't using this for premature optimization when there's no benefit 🙂

[Ivorforce]

Just to double-check, the reason for the reversal (returning function pointers instead of the data pointers directly) is branch prediction?

Yep, i wanted to eliminate one source of potential indirection misprediction.
The reason I changed my mind is that at the point of call, the type has already been needed to be established anyway, as the returned pointer holds no type information. So I wanted to eliminate the unnecessary branching after this. You could also look at it from the perspective of 'properly' copying godot upstream behavior, because godot upstream also calls these function directly without a switch on the type.

Here's a breakdown of my call stack analysis:

Old implementation

// GDEXTENSION SIDE
switch (variant.get_type()){}  // likely mispredicted static indirection to case PACKED_BYTE_ARRAY:
VariantInternal::get_byte_array(variant);  // (likely) inlined call
// GODOT-CPP SIDE
get_internal_value_ptr(variant);  // Predicable static indirection
// GODOT SIDE
switch (variant.get_type()){}  // likely mispredicted static indirection
get_bool(variant);  // (likely inlined call)
return &variant.bool;

New implementation

// GDEXTENSION SIDE
switch (variant.get_type())  // likely mispredicted indirection to case PACKED_BYTE_ARRAY:
VariantInternal::get_byte_array(variant);  // (likely) inlined call
// GODOT-CPP SIDE
get_internal_value_functions[GDEXTENSION_BOOL_TYPE](variant);  // Predicable virtual indirection
// GODOT SIDE
return &variant.bool ;

The biggest cost factors in terms of performance are likely to be branch mispredictions, which may cost 12-25 clock cycles depending on architecture. The former implementation had 1, the new zero (leading to 2 total vs 1 total in most use-cases). The cost of a predictable virtual indirection is "a few clock cycles". Again, miniscule difference for most use cases. It may make a noticeable difference when acting on large quantities of variants though, which is something I actually may encounter - because I actually may call this function tens of thousands of times for object dtype tensor math.
I guess the optimal implementation would add one separate GDExtension interface method for each variant, to make the virtual call a static one. But this doesn't seem to be employed anywhere else and would be kind of verbose.

Was anything measured in that regard?

Nope! Let me know if you want to see performance tests.

someone knows a-priori what type is contained in the variant (otherwise, a get_type call is necessary)
"I want to see what type the variant has, and get the internal pointer"

Type information has to be gleamed separately from variant.get_type() anyway, because the returned pointer holds no type information. See above call stack.

function pointers are cached on user side

I agree this would be smart to do by GDExtension implementations. godot-cpp already does this for similar functions.

If yes, could you document these assumptions in the GDExtension header? To make sure that people aren't using this for premature optimization when there's no benefit 🙂

Will do! I was going to document it on godot-cpp side but it's good to do it here already too.

[Bromeon]

Thanks a lot for this detailed elaboration! ❤️

Some questions though, about the new implementation:

// GDEXTENSION SIDE
switch (variant.get_type())  // likely mispredicted indirection to case PACKED_BYTE_ARRAY:
VariantInternal::get_byte_array(variant);  // (likely) inlined call
// GODOT-CPP SIDE
get_internal_value_functions[GDEXTENSION_BOOL_TYPE](variant);  // Predicable virtual indirection
// GODOT SIDE
return &variant.bool ;

The "GDEXTENSION SIDE" part only needs to happen once for each type, and the function pointers are then cached in the get_internal_value_functions array, correct? Which is a one-time effort, and from then on, dispatching on a concrete variant would be the (predictable) array[BOOL](variant) call.

However, in typical use cases, the "GODOT-CPP SIDE" part needs an additional get_type() too. Even if you can use array indices to avoid branching a switch on the returned type, the result of the call will still only give you a void*, and then you still need to dispatch on that to interpret it correctly. Because ultimately you want to run user code depending on the variant type.

You do benefit if you have something like Array<int> which stores Variant(INT) for each element, and you can directly dispatch to the right function and interpret the result in a hardcoded way. But I don't think for general variants you can avoid the dispatch(es), which is why I think it's good to document this.

---

Btw, I don't think array-of-function-pointers is generally faster than switch. The latter should be optimized to a jump table by any decent compiler, leading to equivalent code. On the other hand it may be harder to reason about function tables if they appear "opaque" to the compiler (it cannot infer the concrete functions being called at compile time), in which case an actual indirect function call needs to happen.

[Ivorforce]

The "GDEXTENSION SIDE" part only needs to happen once for each type, and the function pointers are then cached in the get_internal_value_functions array, correct? Which is a one-time effort, and from then on, dispatching on a concrete variant would be the (predictable) array[BOOL](variant) call.

Sorry, i was being unclear. The GDEXTENSION side refers to some function a gdextension creates that operates on a Variant of unknown type (like this switch statement). It doesn't refer to creating the function table. You are correct that the function table is only created once, and that can be ignored in the performance analysis.

However, in typical use cases, the "GODOT-CPP SIDE" part needs an additional get_type() too. Even if you can use array indices to avoid branching a switch on the returned type, the result of the call will still only give you a void*, and then you still need to dispatch on that to interpret it correctly. Because ultimately you want to run user code depending on the variant type.

Yep. You either need a get_type() before or after getting the internal ptr. I included this in the "GDEXTENSION SIDE" part of my analysis! I hope it's clear now :)
In either case, this get_type() based indirection will be needed for both the old implementation and the new one, because the returned type is always void*, holding no type information.

You do benefit if you have something like Array<int> which stores Variant(INT) for each element, and you can directly dispatch to the right function and interpret the result in a hardcoded way. But I don't think for general variants you can avoid the dispatch(es), which is why I think it's good to document this.

Oh! That's right, i totally forgot about this use-case. Yep, this one should also (slightly) benefit from the new implementation, as only one switch will be needed (instead of one for each variant type).

Btw, I don't think array-of-function-pointers is generally faster than switch. The latter should be optimized to a jump table by any decent compiler, leading to equivalent code.

Yep, I agree.

On the other hand it may be harder to reason about function tables if they appear "opaque" to the compiler (it cannot infer the concrete functions being called at compile time), in which case an actual indirect function call needs to happen.

That's true. I don't know in which ways a switch may be optimized, so I can't guarantee that the new solution is faster (especially, faster with all compilers and arches). I just thought it more likely to be.

[dsnopek]

Thanks! The reworked version (which is returning function pointers) is looking good to me now :-)

[Bromeon]

(Bikeshedding) the existing variant_get_ptr_* names indicate that a function is returned, with names such as "evaluator", "method" and "constructor":

variant_get_ptr_operator_evaluator
variant_get_ptr_builtin_method
variant_get_ptr_constructor

The name "internal" doesn't seem to fit that scheme. Maybe variant_get_ptr_internal_accessor or something?

Just something to indicate it doesn't return the internal pointer, but a function that retrieves it.

[dsnopek]

I'd be OK with variant_get_ptr_internal_accessor or variant_get_ptr_internal_getter or something like that.

(I'd also be OK with sticking with what is already in this PR, since all the other variant_get_ptr_* functions return function pointers)

[Ivorforce]

I've renamed the function to variant_get_ptr_internal_getter. The use of 'getter' seems to already be somewhat established in the interface.

[Bromeon]

I think this is a great paragraph, but as mentioned in my comments, I would maybe add something along:

This function is particularly useful in situations where you know the variant type upfront, and can thus directly fetch the pointer of the correct type. It can make sense to cache the returned function pointers.

I don't have a great wording, but it would convey why this function exists in spite of other similar APIs. People won't have access to the information discussed in this PR anymore.

[Ivorforce]

Alright. I've added some documentation to hopefully address your concerns, let me know if that works for you!

[Bromeon]

Thanks a lot for the updates!

I guess the new @note annotation shouldn't lead to any problems in parsers or so?

[Ivorforce]

I guess the new @note annotation shouldn't lead to any problems in parsers or so?

I hope not. It seems to be supported by CLion and doxygen, for what it's worth. I used it because it seemed like the most appropriate option from the ctrl-space context menu.

[Repiteo]

One style nitpick; otherwise looks good!

[Repiteo]

Thanks!