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

C#: Remove VariantSpanDisposer and use constants in stackalloc #69194

Merged
merged 1 commit into from
Nov 27, 2022

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Nov 26, 2022

@raulsntos raulsntos added this to the 4.0 milestone Nov 26, 2022
@raulsntos raulsntos requested a review from a team as a code owner November 26, 2022 04:05
Comment on lines +132 to +133
Span<IntPtr> argsSpan = argc <= VarArgsSpanThreshold ?
stackalloc IntPtr[VarArgsSpanThreshold] :
Copy link
Member Author

Choose a reason for hiding this comment

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

The VarArgsSpanThreshold constant has a value of 5 in Callable, but 10 in code generated by bindings_generator. Not sure why they are different and maybe we should consider declaring this constant in a single place and reuse it rather than redefine it in every file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird that I added VarArgsSpanThreshold here but used a hard-coded value instead. It's obviously a mistake, but I don't know which one I truly wanted. I think 10 should be fine for both.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it to 10 in this PR and we can move the constant to a single place later.

@geowarin
Copy link
Contributor

geowarin commented Nov 26, 2022

This also fixes #68590 where custom resources with an array of variants would prevent reloading of the c# assemblies.
Great work!

@raulsntos raulsntos force-pushed the dotnet/begone-variant-disposer branch from a10f536 to 1343934 Compare November 26, 2022 16:31
- Remove `VariantSpanDisposer`, no need to dispose of the Variant Spans
since we are now borrowing the Variants instead of copying them.
- Remove `VariantSpanExtensions.Cleared` that was only used so the
Span was initialized for `VariantSpanDisposer` to know what to dispose.
- Fix stackalloc Spans to use constant VarArgsSpanThreshold
and avoid bound checks.
@raulsntos raulsntos force-pushed the dotnet/begone-variant-disposer branch from 1343934 to 3ff1810 Compare November 27, 2022 02:04
@neikeq neikeq merged commit 690199b into godotengine:master Nov 27, 2022
@neikeq
Copy link
Contributor

neikeq commented Nov 27, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants