Uses StringName in GDExtension perf critical instance creation & method/properties setter/getter

[touilleMan]

Godot-CPP twin PR: godotengine/godot-cpp#896

Function such as gdnative_classdb_construct_object would obsviously benefit from using StringName instead of char * (it's the whole point of StringName after all !)

Functions such as gdnative_variant_get_ptr_builtin_method also benefit in case it is called as part of the actual function call (e.g. in dynamic language or if the output of gdnative_variant_get_ptr_builtin_method is not cached)

On the other hand, I've kept use of char * in classdb_register_extension_*, GDNativeExtensionClassGetVirtual functions and GDNativePropertyInfo/GDNativeMethodInfo/GDNativeExtensionClassMethodInfo structures.

The reason is those are not performance critical:

• classdb_register_extension_* are called once when the extension is initialized
• GDNativeExtensionClassMethodInfo is only used by classdb_register_extension_*
• GDNativeExtensionClassGetVirtual is cached by classDB
• GDNativePropertyInfo/GDNativeMethodInfo are only used by ClassDB::get_method_list/ClassDB::get_property_list which are called by stuff like editor a gdscript analyzer, so I guess this is not used at release. However we may want to convert it anyway to speed up gdscript analyzer (though I don't know this code at all so it's very possible this would bring no real performance).

I can update the PR if you think it would be better for consistency to use StringName everywhere.

[neikeq]

I've said this in the past. I think all these methods should receive StringName. IIRC, the char * conversion (StringName(const char *)) copy ctor) doesn't support Unicode. Of course, the conversion could be fixed, but it's easier to just receive the StringName.

[touilleMan]

@neikeq good point, I'm convertion everything to StringName then ;-)

[touilleMan]

@neikeq I've updated the PR, now what we have:

GDExtension C API uses StringName instead of const char * (some places uses String depending on what is used by ClassDB, e.g. hint_string)

GDNativeExtensionClassMethodInfo has been simplified by removing the get_argument_type_func/get_argument_info_func/get_argument_metadata_func.
The main reason for that is simplifies the memory management of the String/StringName objects: GDNativeMethodInfo/GDNativePropertyInfo/GDNativeExtensionClassMethodInfo are now only used as argument to to the lassdb_register_extension_* method or for the get_property_list_func/free_property_list_func.

• for lassdb_register_extension_* the GDExtension user know he can free the resources once the function has return
• for get_property_list_func/free_property_list_func, the free_property_list_func is the obvious place to do the resource cleanup

On top of that, this simplifies the implemention for GDExtension user (it's much simpler to pass an array of GDNativePropertyInfo that to pass a function pointer that should construct a GDNativePropertyInfo !).

[touilleMan]

@bruvzg can I merge it then ?

[Bromeon]

It looks like this affects #61968, right?

[touilleMan]

@Bromeon exactly ! I didn't even know about this issue 😄

[Zireael07]

Maybe also will help with #68375

[reduz]

I feel this PR was kind of pointless. None of these functions are performance critical and since StringName construction still needs to hash the cstring, it would have allowed you to optimize this on the side of the language bind. Now this is probably a lot more complex to implement and a lot more inefficient unless you keep StringNames cached everywhere for everything you do.

[reduz]

Even worse, if you want to keep a hashtable on the extension side to optimize, how are you going to use the StringName for this?

[reduz]

Maybe one possibility could be to have a C function to get some uint64_t from the stringname you can use for hashing.

[neikeq]

... and since StringName construction still needs to hash the cstring, it would have allowed you to optimize this on the side of the language bind.

Even worse, if you want to keep a hashtable on the extension side to optimize, how are you going to use the StringName for this?

I don't understand, can you explain these optimizations in more details? Some pseudo-code may help.

[reduz]

@neikeq StringName contains a pointer to a string table, which is what makes it fast. If you use it from GDExtension directly its probably going to be slower because you have to call the function pointers to compare it.

I wonder maybe each implementation can implement comparison operators just checking the memory (pointer). If this is the case it should be fine.

Either case, if there is consensus on using StringName for these things from those implementing extensions, I won't oppose. I just feel it may not have been necessary.

[neikeq]

I wonder maybe each implementation can implement comparison operators just checking the memory (pointer). If this is the case it should be fine.

This is what I do from C#. StringName has a very simple layout, just a pointer.

[reduz]

@neikeq I have no idea how easy is to expose this to different languages, but if extension implementers are fine doing this, then I don't see the problem with this PR.