Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GDScript: Perform validated calls with static methods #91192

Merged

Conversation

vnen
Copy link
Member

@vnen vnen commented Apr 26, 2024

When the types are validated at compile time, this type of call runs faster. It is already used for instance methods, this adds this optimization to native static methods as well.

PS: Viewing the diff without whitespace makes it easier to review the changes. The opcodes table increased by one character, so the alignment creates a bigger diff.

When the types are validated at compile time, this type of call runs
faster. It is already used for instance methods, this adds this
optimization to native static methods as well.
@vnen vnen requested a review from a team as a code owner April 26, 2024 00:20
@akien-mga akien-mga added this to the 4.x milestone Apr 26, 2024
@dsnopek
Copy link
Contributor

dsnopek commented Apr 26, 2024

Thanks!

I'm not really qualified to review the code changes, since I don't know GDScript's internals very well.

However, I did try using this PR with godot-cpp's automated tests, which includes some static method calls that GDScript should do as validated calls. To be sure that a validated call to GDExtension was happening, I added some debug print statements:

diff --git a/core/extension/gdextension.cpp b/core/extension/gdextension.cpp
index 22a5df9935..a1c4606c80 100644
--- a/core/extension/gdextension.cpp
+++ b/core/extension/gdextension.cpp
@@ -259,6 +259,10 @@ public:
                ERR_FAIL_COND_MSG(vararg, "Vararg methods don't have validated call support. This is most likely an engine bug.");
                GDExtensionClassInstancePtr extension_instance = is_static() ? nullptr : p_object->_get_extension_instance();
 
+               if (is_static()) {
+                       print_line("validated_call: ", name);
+               }
+
                if (validated_call_func) {
                        // This is added here, but it's unlikely to be provided by most extensions.
                        validated_call_func(method_userdata, extension_instance, reinterpret_cast<GDExtensionConstVariantPtr *>(p_args), (GDExtensionVariantPtr)r_ret);

And, it printed out the function names I was expecting, as well as passed the tests!

@Bromeon Bromeon linked an issue Apr 28, 2024 that may be closed by this pull request
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 29, 2024
Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this in Rust, and it works, great job!

Interesting discovery: before this PR, static methods do use ptrcall if they are called as instance methods (on an object, rather than a type).

@Bromeon
Copy link
Contributor

Bromeon commented Apr 30, 2024

Not directly related to this PR, but is it possible to call static methods via reflection? I wanted to simulate falling back to regular calls (if no type information is available), but found it's only possible via actual instances.

@akien-mga akien-mga merged commit 731ea17 into godotengine:master May 1, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@vnen vnen deleted the gdscript-validated-native-static-calls branch May 2, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GDExtension: static methods do not use ptrcall convention
4 participants