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.

Sign up for GitHub

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

Jump to bottom

[3.x] Don't box params on Native->C# calls with Variant params #53942

Merged
merged 2 commits into from
Nov 8, 2021
Merged

[3.x] Don't box params on Native->C# calls with Variant params #53942

merged 2 commits into from
Nov 8, 2021

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Oct 18, 2021 •
edited
Loading

Backport of #44106 and #53975 to 3.x.
Also includes some changes from #37050 since they were intertwined.

@raulsntos raulsntos requested a review from a team as a code owner October 18, 2021 01:22
@raulsntos raulsntos mentioned this pull request Oct 18, 2021
Merged
neikeq
neikeq reviewed Oct 18, 2021
View reviewed changes
modules/mono/mono_gd/gd_mono_method.h Outdated
bool attrs_fetched;
MonoCustomAttrInfo *attributes;
bool attrs_fetched = false;
MonoCustomAttrInfo *attributes = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

What C++ version are we using in 3.x? Is this allowed?

Copy link
Member Author

@raulsntos raulsntos Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think you are right, this C++ feature is not used in 3.x so I undid these changes.

modules/mono/mono_gd/gd_mono_property.cpp Outdated Show resolved Hide resolved
@neikeq
Copy link
Contributor

neikeq commented Oct 18, 2021

Since 3.4 is late in beta, I'll let @akien-mga decide whether he prefers to leave this for 3.4.1/3.5. It's not a breaking change but it risks introducing bugs.

@akien-mga akien-mga added this to the 3.5 milestone Oct 18, 2021
@akien-mga
Copy link
Member

Let's play it safe and wait for 3.5 then.

@neikeq neikeq mentioned this pull request Oct 18, 2021
Merged
neikeq added 2 commits November 3, 2021 13:02
@neikeq @raulsntos
Don't box params on Native->C# calls with Variant params
b775573 
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 @raulsntos
C#: Fix property set call boxing value when unboxed was expected
e282ee0
@akien-mga akien-mga merged commit d0b1e3d into godotengine:3.x Nov 8, 2021
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the no-boxing branch November 8, 2021 13:36
@Calinou Calinou mentioned this pull request Nov 8, 2021
Closed
@raulsntos raulsntos mentioned this pull request Dec 14, 2021
Closed
@zaevi zaevi mentioned this pull request Dec 31, 2021
Closed
@raulsntos raulsntos mentioned this pull request Jan 1, 2022
Merged
@raulsntos raulsntos mentioned this pull request Aug 12, 2022
Open
@raulsntos raulsntos mentioned this pull request Sep 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neikeq neikeq neikeq approved these changes

Assignees
No one assigned
Labels
enhancement topic:dotnet
Projects
None yet
Milestone
3.5
Development

Successfully merging this pull request may close these issues.
4 participants
@raulsntos @neikeq @akien-mga @Calinou