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

Don't box params on Native->C# calls with Variant params #44106

Merged
merged 1 commit into from
Dec 6, 2020

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Dec 5, 2020

Godot uses Variant parameters for calls to script methods. Up until now we were boxing such parameters when marshalling them for invokation, even if they were value types.

Now Godot allocates the marshalled parameters on the stack, reducing the GC allocations resulted from boxing.

@neikeq neikeq added this to the 4.0 milestone Dec 5, 2020
@@ -38,15 +38,16 @@
class GDMonoMethod : public IMonoClassMember {
StringName name;

int params_count;
uint16_t params_count;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's always initialized in the constructor, in the call to update the method signature. Is there a performance penalty in adding a default value here?

@neikeq neikeq force-pushed the mono-invoke-no-params-boxing branch from 4283a82 to ed46171 Compare December 6, 2020 00:27
Godot uses Variant parameters for calls to script methods.
Up until now we were boxing such parameters when marshalling
them for invokation, even if they were value types.

Now Godot allocates the marshalled parameters on the stack,
reducing the GC allocations resulted from boxing.
@neikeq neikeq force-pushed the mono-invoke-no-params-boxing branch from ed46171 to a946f84 Compare December 6, 2020 00:36
@akien-mga akien-mga merged commit d834789 into godotengine:master Dec 6, 2020
@akien-mga
Copy link
Member

Thanks!

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.

2 participants