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

[NativeAOT] don't generate __GetFieldHelper if struct is never boxed? #111544

Closed
NinoFloris opened this issue Jan 17, 2025 · 5 comments
Closed

[NativeAOT] don't generate __GetFieldHelper if struct is never boxed? #111544

NinoFloris opened this issue Jan 17, 2025 · 5 comments

Comments

@NinoFloris
Copy link
Contributor

NinoFloris commented Jan 17, 2025

I'm unsure how feasible this is but it would be helpful if the helper isn't emitted if the struct is never boxed (and never gets a constrained call on Equals/GetHashCode either).

I understand the difficulty of tracking whether a struct is 'boxed', as generics (and (T)(object)value casts etc) significantly complicate the analysis. Though with struct types forcing specialization it should not be impossible to get some certainty?

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 17, 2025
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky
Copy link
Member

Do you have an example where the struct is not boxed but we still generate the helper? We do not generate the helper unless the compiler thinks it could be boxed.

For example:

using System.Runtime.CompilerServices;

Consume(default, new Bar[1]);

[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume(Foo f, Bar[] b) { }

struct Foo { double d; }

struct Bar { double d; }

Sizoscope reports:

Image

Notice Foo is gone.

Bar is there even though not boxed, but there are easy ways to get it as boxed. For example: call Array.GetValue. Or cast to non-generic IEnumerable and enumerate. Etc. The compiler doesn't track what exact object instance the interface methods get called on and these foundational interfaces are pretty much guaranteed to get called somewhere.

@NinoFloris
Copy link
Contributor Author

This is the repro, not great.

var action = (Struct _) => { };
struct Struct;

@MichalStrehovsky
Copy link
Member

This is the repro, not great.

This is from two things:

  1. The Invoke method on all delegates is implicitly considered reflected on. We had to do this because there's lots of code in this repo and other repos that needs to know the delegate Invoke method signature. Because of LINQ expressions we need to consider it not just target of reflection introspection, but also invocation: Generate method bodies for delegate Invoke methods #70883: that PR is the reason why this is so expensive.
  2. Types in signatures of reflected-on methods trigger logic that considers the types implicitly boxed. I did some work around that in Try rooting less stuff for reflectable method signatures #92994. Looks like I already looked if we could get rid of this for valuetypes and it is known it would be a saving if we could ("I'm already a bit uncomfortable with this change that limits it to reference types only, but it does save 0.1% on ASP.NET scenarios. We'd get triple that if we could also do this for value types, but it doesn't look likely we can."). I don't remember the details anymore, just that I looked at it and it didn't look straightforward.

Making it so that 1 or 2 doesn't apply would fix this. I don't know if it's really possible. We had reasons for both behaviors.

@MichalStrehovsky MichalStrehovsky added this to the Future milestone Jan 20, 2025
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Jan 22, 2025
MichalStrehovsky added a commit that referenced this issue Mar 6, 2025
When we make a method reflection-callable, we have to make sure the reflection stack can get type handles of types within the method signature. The type handles are used as part of reflection activation to do castability checks (is it valid to call the method with this parameter?) and to do boxing of return values.

In the past, what we did:

* If the type has a canonical form, generate type loader template for it.
* Otherwise, generate a constructed MethodTable.

This was too much, so we restricted it in #92994 into:

* If the type is a GC pointer, don't generate anything. Update reflection stack to be able to deal with a null type handle if and only if this is a reference type and nobody actually allocated such reference type.
* Otherwise do what we did above.

This is still a bit too much, as demonstrated in #111544 (comment).

In this PR, I'm restricting this further to:

* If the type is in the canonical form (not just "has canonical form" - "_is_ canonical form"), generate type loader template.
* If the type is an out valuetype parameter, generate constructed method table (since reflection stack is going to end up boxing)
* Generate necessary (unconstructed) method table otherwise (since we're only going to do castability checks and necessary method tables are exactly for that)

I'm deleting the "GC pointer could have null methodtable" complication since it doesn't help much in practice once we downgraded to necessary method tables (that revert is in dc34018 and rt-sz measurements show it really doesn't affect much).

Cc @dotnet/ilc-contrib
@MichalStrehovsky
Copy link
Member

Making it so that 1 or 2 doesn't apply would fix this. I don't know if it's really possible. We had reasons for both behaviors.

Broke the pattern by addressing 2 in #111610.

The TodosApi is an app that use Npgsql.

Size statistics

Pull request #111610

Project Size before Size after Difference
TodosApi-windows 23398400 23340032 -58368
avalonia.app-windows 19270144 19204096 -66048
hello-minimal-windows 855040 855040 0
hello-windows 1100288 1100288 0
kestrel-minimal-windows 4902400 4861952 -40448
reflection-windows 1745920 1744896 -1024
webapiaot-windows 9244160 9201152 -43008
winrt-component-full-windows 5725696 5726208 512
winrt-component-minimal-windows 1757696 1756672 -1024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants