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#: Optimize Variant conversion callbacks #68310

Merged

Conversation

neikeq
Copy link
Member

@neikeq neikeq commented Nov 6, 2022

These callbacks are used for marshaling by callables and generic Godot collections.

C# generics don't support specialization the way C++ templates do. I knew NativeAOT could optimize away many type checks when the types are known at compile time, but I didn't trust the JIT would do as good a job, so I initially went with cached function pointers.

Well, it turns out the JIT is also very good at optimizing in this scenario, so I'm changing the methods to do the conversion directly, rather than returning a function pointer for the conversion.

The methods were moved to VariantUtils, and were renamed from GetFromVariantCallback/GetToVariantCallback to ConvertTo/CreateFrom.

The new implementation looks like it goes through many if checks at runtime to find the right branch for the type, but in practice it works pretty much like template specialization. The JIT only generates code for the relevant branch. Together with inlining, the result is very close or the same as doing the conversion manually:

godot_variant variant;

int foo = variant.Int;
int bar = VariantUtils.ConvertTo<int>(variant);

If the type is a generic Godot collection, the conversion still goes through a function pointer call.

The new code happens to be much shorter as well, with the file going from 1057 lines to 407.

Side note: Variant.cs was mistakenly created in the wrong folder, so I moved it to the Core folder.

@neikeq neikeq added this to the 4.0 milestone Nov 6, 2022
@neikeq neikeq requested a review from a team as a code owner November 6, 2022 01:15
@neikeq
Copy link
Member Author

neikeq commented Nov 6, 2022

I made an example with some stubs in SharpLab to help visualize these optimizations (change the results view to JIT Asm):

For Godot projects, I don't know how to visualize JIT generated code (I trusted the SharpLab output in this case). If you enable NativeAOT, you can use objdump with the compiled library.

@neikeq neikeq force-pushed the csharp-opt-variant-generic-conv branch from fbabe2d to 415b068 Compare November 25, 2022 02:11
These callbacks are used for marshaling by callables and generic Godot
collections.

C# generics don't support specialization the way C++ templates do.
I knew NativeAOT could optimize away many type checks when the types
are known at compile time, but I didn't trust the JIT would do as good
a job, so I initially went with cached function pointers.

Well, it turns out the JIT is also very good at optimizing in this
scenario, so I'm changing the methods to do the conversion directly,
rather than returning a function pointer for the conversion.

The methods were moved to `VariantUtils`, and were renamed from
`GetFromVariantCallback/GetToVariantCallback` to `ConvertTo/CreateFrom`.

The new implementation looks like it goes through many `if` checks
at runtime to find the right branch for the type, but in practice it
works pretty much like template specialization. The JIT only generates
code for the relevant branch. Together with inlining, the result is
very close or the same as doing the conversion manually:

```cs
godot_variant variant;

int foo = variant.Int;
int bar = VariantUtils.ConvertTo<int>(variant);
```

If the type is a generic Godot collection, the conversion still goes
through a function pointer call.

The new code happens to be much shorter as well, with the file going
from 1057 lines to 407.

Side note: `Variant.cs` was mistakenly created in the wrong folder,
so I moved it to the `Core` folder.
@neikeq neikeq force-pushed the csharp-opt-variant-generic-conv branch from 415b068 to 3f645f9 Compare November 25, 2022 02:15
@@ -54,7 +54,7 @@ static void Trampoline(object delegateObj, NativeVariantPtrArgs args, out godot_
ThrowIfArgCountMismatch(args, 1);

((Action<T0>)delegateObj)(
VariantConversionCallbacks.GetToManagedCallback<T0>()(args[0])
VariantUtils.ConvertTo<T0>(args[0])
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, does the analyzer not complain that T0 is not annotated with [MustBeVariant]?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, the analyzer is in Godot.SourceGenerators but in GodotSharp we are only using Godot.SourceGenerators.Internal. So the analyzer is not run for GodotSharp but it will be run in user projects.

That's a shame because we missed the [MustBeVariant] attribute here and we need it if we want to prevent this:

Callable.From((NonMarshableType a) => ...);

Copy link
Member Author

Choose a reason for hiding this comment

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

We could extract the analyzer to its own project/assembly to re-use it.

@akien-mga akien-mga merged commit fcdded2 into godotengine:master Nov 25, 2022
@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.

None yet

3 participants