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

Reduce enum stubs codegen with NativeAOT #1441

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

Sergio0694
Copy link
Member

This PR includes some optimizations to reduce the codegen a bit more for Marshaler<T> with enum types.
It also is a little performance optimizations in that all shared stubs are now properly cached with beforefieldinit too.

@Sergio0694 Sergio0694 added performance Related to performance work authoring Related to authoring feature work AOT labels Jan 13, 2024
@Sergio0694 Sergio0694 marked this pull request as draft January 13, 2024 21:47
@Sergio0694
Copy link
Member Author

Changing this back to draft as it doesn't actually seem to be improving the size.
There's one thing though that I don't understand: both before and after, we're instantiating all lambdas for all types:

image

But in both cases, all of this code is guarded under some typeof(T).IsEnum check:

if (typeof(T).IsEnum)
{
// For marshaling non-blittable enum arrays via MarshalNonBlittable
if (typeof(T).GetEnumUnderlyingType() == typeof(int))
{
CopyAbi = Marshaler.CopyIntEnumFunc;
CopyManaged = (T value, IntPtr dest) => Marshaler.CopyIntEnumFunc(value, dest);
}
else
{
CopyAbi = Marshaler.CopyUIntEnumFunc;
CopyManaged = (T value, IntPtr dest) => Marshaler.CopyUIntEnumFunc(value, dest);
}
}

@MichalStrehovsky would you know why is the linker apparently not respecting this? We're wasting a lot of size just compiling generic versions of all of those helpers that we know could not possibly be used. Why is this happening despite the check? 🤔

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky would you know why is the linker apparently not respecting this? We're wasting a lot of size just compiling generic versions of all of those helpers that we know could not possibly be used. Why is this happening despite the check?

This is constructing delegates, so:

  • Are you on .NET 9?
  • Do you have Delegate.Method somewhere in the program?

Without those, the whole program analysis will figure out the targets of reflection before RyuJIT starts eliminating unreachable codepaths and that's a done deal unfortunately.

@Sergio0694
Copy link
Member Author

Ooh I see, thank you! I guess this is just also blocked by #1444 then and fixing that will also improve this. Thank you! 🙂

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/reduce-enum-stubs-codegen branch 2 times, most recently from 47e4332 to bba9b52 Compare January 18, 2024 21:58
@Sergio0694
Copy link
Member Author

@MichalStrehovsky I don't get how this is still regressing on .NET 9, and we're not using .Method anymore. I see this:

image

With roots like this one:

image

I don't understand where are all of those generic instantiations coming from and on what closure. This PR literally only removes an existing lambda expression and replaces it with a cached, reused delegate. The weird thing is that I also don't see any generic instantiation over WithTypedT1<T>, so it's not like that path is being compiled multiple times. But then where are all of those new generic instantiations for that <>c type coming from? I don't get why is this PR regressing size 😅

@Sergio0694 Sergio0694 marked this pull request as ready for review January 19, 2024 11:54
@Sergio0694
Copy link
Member Author

Chatted with Michal, publishing this as it's not actually regressing, just not improving much.

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/reduce-enum-stubs-codegen branch from bba9b52 to 47050b1 Compare January 23, 2024 11:29
@Sergio0694 Sergio0694 merged commit ee362c0 into staging/AOT Jan 24, 2024
9 checks passed
@Sergio0694 Sergio0694 deleted the user/sergiopedri/reduce-enum-stubs-codegen branch January 24, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AOT authoring Related to authoring feature work performance Related to performance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants