[NativeAOT] Metadata not trimmed for nested Interface types when compiling TerraFx.Interop.Windows #66716
Comments
|
I have tried to debug this using dgml log by adding |
|
@MichalStrehovsky Does this ring any bells? |
dotnet-issue-labeler
bot
added
the
untriaged
|
We don't trim interface lists. I played with it in the past (MichalStrehovsky/runtimelab@2a702bc), but it's a bit more involved (the above commit was enough to get a size estimate and I remember the size saving being interesting - more than 1% - but not interesting enough to spend time on it at that point). It will require changing most of the occurrences of I'll put this into Future since this is a nice to have size optimization. |
Fixes issue reported in dotnet#66716 (comment). We create a hashset for the purpose of avoiding this problem, check it, but never add to it.
|
I had a quick look - the only reason why we're generating the interfaces is because we're generating the struct that implements them. And the only reason why we generate the struct is because we called some instance methods on it and it became a potential target of reflection in the "IL Linker compat mode" we run by default (where whatever is called is considered reflection-accessible at runtime). In the strict mode I wish we just enabled the strict mode by default... |
I have been thinking about the recently. I think we should enable the strict mode by default. |
Fixes issue reported in #66716 (comment). We create a hashset for the purpose of avoiding this problem, check it, but never add to it.
|
The default experience today is that you always see these messages:
We should change the default experience so that you only see If there are no trimming or AOT warnings, it is all you get. Nice and clean experience. If there are trimming or AOT warnings, we print additional message with a link to a top-level doc that talks about fixing the warnings properly and that has list of workarounds (e.g. setting IlcTrimMetadata to false, etc.). Thoughts? |
|
The conservative defaults made sense before we had the analyzers and enabling AOT was a guesswork. Now that we have the analyzers, we should be able to be more aggressive by default. |
|
The I can definitely get behind enabling
But this one I would like to do consistently - we either do this with IL Linker too, or we don't. It doesn't feel good for the AOT business to be more breaking than PublishTrimmed when it comes to trimming. We had a discussion about what the default should be in .NET 6. We went with rooting all assemblies that are not manifested as trimmable. There is an argument for doing the more aggressive thing - pretty much all our trimming warnings are not relevant for most apps because the target of their reflection is not in a trimmable assembly - so we spam dozens of warnings and the app still works fine. It feels better to generate fewer warnings that are all relevant (and the app being more likely to be broken) and have the old behavior behind another opt in. Cc @dotnet/ilc-contrib for thoughts. |
Agree. Also, I would like to keep emphasizing the importance of fixing the trimming warnings. The current defaults dilute this message since the conservative default covers up a lot of the issues, but not all of them. |
The compiler can generate two kinds of `MethodTable` structures: constructed and unconstructed. The constructed one has a fully populated vtable and GCInfo and is required whenever the object type could be allocated on the heap. The unconstructed one is generated for all other scenarios as a size optimization. We were previously also skipping emission of the interface list in the unconstructed case. But interface list might be required for variant casting. We could introduce yet another `MethodTable` kind for this specific scenario, but it doesn't seem to warrant the complexity. Emitting interface list for all types is a less than 0.1% size regression for the Todos app. It is a 0.5% size regression for Hello World. That part is unfortunate. It's mostly due to the useless numeric interfaces. We can get this size back if we do dotnet#66716 and start trimming interface lists. Fixes dotnet#95574.
* Generate interface lists for necessary EETypes The compiler can generate two kinds of `MethodTable` structures: constructed and unconstructed. The constructed one has a fully populated vtable and GCInfo and is required whenever the object type could be allocated on the heap. The unconstructed one is generated for all other scenarios as a size optimization. We were previously also skipping emission of the interface list in the unconstructed case. But interface list might be required for variant casting. We could introduce yet another `MethodTable` kind for this specific scenario, but it doesn't seem to warrant the complexity. Emitting interface list for all types is a less than 0.1% size regression for the Todos app. It is a 0.5% size regression for Hello World. That part is unfortunate. It's mostly due to the useless numeric interfaces. We can get this size back if we do #66716 and start trimming interface lists. Fixes #95574. * Update NecessaryCanonicalEETypeNode.cs
So far, the compiler could only trim unused interface __methods__; the interface __types__ themselves were left alone. There is not a huge cost associated with having these extra `MethodTable`s, but it sill impacts size/working set/startup. Initial estimate showed this could actually be up to 2% for BasicMinimalApi so I decided to investigate a fix. This is an attempt to start trimming them. I chose a relatively conservative approach: * Stop unconditionally rooting the interface `MethodTable` in the implementing class `MethodTable` InterfaceList. Instead check whether someone else marked it first. * Track whether an interface type definition is used in any way. This avoids problem with variance, etc. If an interface definition is used, all instantiations that we were trying to optimize away get the `MethodTable` and won't be optimized. We should be able to do better than this, maybe, at some point. * Classes that implement generic interfaces with default methods need special treatment because we index into interface list at runtime and we might not know the correct index yet. So just forget the optimization in that case. Fixes dotnet#66716.
dotnet-policy-service
bot
added
the
in-pr
Same repro as #66593 (comment)
Actual result: Nested
Interfacetypes likeTerraFX.Interop.Windows.IEnumWbemClassObject+Interfacetypes are present in the final binaryExpected result:
TerraFX.Interop.Windows.IEnumWbemClassObject+Interfacetypes are not present in the final binary (IL linker is able to trim these)The text was updated successfully, but these errors were encountered: