-
Notifications
You must be signed in to change notification settings - Fork 564
[trimming] mark Java objects as "instantiated" #9411
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
Conversation
Fixes: #9399 The .NET trimmer will inline cast statements such as: var d = Application.Context.Resources.GetDrawable (resId); Assert.IsNotNull (d as NinePatchDrawable); If `NinePatchDrawable` is not "instantiated" anywhere via `new()`, inlined to: var d = Application.Context.Resources.GetDrawable (resId); Assert.IsNotNull (null); // BOOM! We can solve this by: 1. `MarkJavaObjects` now inspects `Mono.Android.dll` 2. If we encounter an `IJavaObject` call: Annotations.MarkInstantiated (type); Since Java can create these objects, the most reasonable thing we can do is say that they are "instantiated". Hopefully, this still allows types to be trimmed away if they are completely unused. TBD. Unfortunately, the `MarkInstantiated()` method is not public in ILLink's reference assembly, so I had to resort to calling it via System.Reflection. TBD how much this affects app size, will find out what CI reports. I also fixed the `$(RuntimeIdentifiers)` in `TestApks.targets` to check if someone passed `-r android-arm64`, for example.
| } | ||
| }, | ||
| "PackageSize": 10673477 | ||
| "PackageSize": 10718533 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected, there is a size increase, but this might be reasonable.
|
@sbomer @vitek-karas do you have any thoughts on this one? This is mainly trying to avoid this optimization, if a type can be created from Java: We currently don't have a way to know if Java will instantiate one of these types. |
| // * https://github.com/dotnet/runtime/blob/def5e3240bdee3ee37ba22c41c840bbf431c4b15/src/tools/illink/src/linker/Linker/Annotations.cs#L237 | ||
| // Annotations.MarkInstantiated (type); | ||
|
|
||
| markInstantiated ??= Annotations.GetType ().GetMethod ("MarkInstantiated", new [] { typeof (TypeDefinition) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of reflecting over MarkInstantiated, I'd preserve the relevant constructors (like existing PreserveConstructors logic does) - that'll result in it being considered instantiated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it looks like the existing logic was written to call PreserveJavaObjectImplementation on the relevant types - those that implement IJavaObject, and are a custom view or have IJniNameProviderAttribute. If this was missing some types, should the remaining logic from PreserveJavaObjectImplementation run for all IJavaObject-implementing types too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing what happens to app size if we just call PreserveJavaObjectImplementation() on every type, but only mark in the two special cases.
|
I'm probably missing some things, but in general:
A general question - for types which can be instantiated via Java - I would think we have to assume they're going to be used, as there's nothing on the managed side which would tell us one way or another, so we have to preserve the type and its .ctors, right? |
Yes, that is where I'm at now, it looks like I can preserve the special constructor here:
This ctor would always be called from Java. |
| // PreserveJavaObjectImplementation, but don't mark the type | ||
| PreserveJavaObjectImplementation (type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might have gone too far, it made "hello world" grow a lot:
-- "PackageSize": 2865685
++ "PackageSize": 3213845Going to try just the IntPtr constructor, which probably will end up where the original commit was.
e5b9b4a to
9ad1523
Compare
An alternative to: #9411
|
Closing in favor of #9435 |
Fixes: #9399 An alternative to: #9411 The .NET trimmer will inline cast statements such as: var d = Application.Context.Resources.GetDrawable (resId); Assert.IsNotNull (d as NinePatchDrawable); If `NinePatchDrawable` is not "instantiated" anywhere via `new()`, then the above statements are rewritten/inlined to: var d = Application.Context.Resources.GetDrawable (resId); Assert.IsNotNull (null); // BOOM! We can solve this by passing `--disable-opt unusedtypechecks`, with `$(_AndroidEnableUnusedTypeChecks)` as a private MSBuild property to opt out if needed. Since Java can create these objects, this feels like the reasonable fix for .NET 9 GA. I also fixed the `$(RuntimeIdentifiers)` in `TestApks.targets` to check if someone passed `-r android-arm64`, for example.
Fixes: #9399
The .NET trimmer will inline cast statements such as:
If
NinePatchDrawableis not "instantiated" anywhere vianew(), inlined to:We can solve this by:
MarkJavaObjectsnow inspectsMono.Android.dllIf we encounter an
IJavaObjectcall:Since Java can create these objects, the most reasonable thing we can do is say that they are "instantiated". Hopefully, this still allows types to be trimmed away if they are completely unused. TBD.
Unfortunately, the
MarkInstantiated()method is not public in ILLink's reference assembly, so I had to resort to calling it via System.Reflection.TBD how much this affects app size, will find out what CI reports.
I also fixed the
$(RuntimeIdentifiers)inTestApks.targetsto check if someone passed-r android-arm64, for example.