[Mono.Android] fix trimming warnings, part 2

[jonathanpeppers]

This fixes the next set of trimmer warnings found via:

<IsTrimmable>true</IsTrimmable>
<EnableAotAnalyzer>true</EnableAotAnalyzer>

JavaObjectExtensions

JavaCast<T>() now requires PublicConstructors and NonPublicConstructors because of Activator.CreateInstance<T>(). This change bubbles up to various other types that have a Find*ById<T>() method:

• Activity
• Dialog
• FragmentManager
• View
• Window

GetInvokerType() also has suppressions around Assembly.GetType() and Type.MakeGenericType(). We track this for the future at:

#8724

AndroidRuntime

Update [DynamicallyAccessedMembers] based on changes to RegisterNativeMembers in:

dotnet/java-interop@b8f6f88

JNINativeWrapper

$(EnableAotAnalyzer) found usage of DynamicMethod. Suppress for now, as we track this for the future at:

#8724

ResourceIdManager

Usage of Type.GetMethod ("UpdateIdValues") leads to decoration of [ResourceDesignerAttribute] with:

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
public string FullName { get; set; }

I also had to suppress warnings around Assembly.GetType(). This should be OK, as Resource.designer.cs is always in the "root assembly" of Android application projects.

JavaProxyThrowable

Suppress warning around StackFrame.GetMethod(), we already handle null return values and exceptions. The existing code appears to be "best effort" to provide additional stack trace information.

TypeManager

Suppress warning around a call to Type.GetType() with a string passed in from Java. Not really much we can do yet, except rely on MarkJavaObjects trimmer step.

Likely also a problem for the future:

#8724

[jonathanpeppers]

/cc @simonrozsival @vitek-karas

[simonrozsival]

Does this really work? I can't see how the trimmer would be able to determine what the returned type is and so it won't be able to preserve the public methods on it. I think at the very least the name would need to be attributed as well, but I'm not sure if that's enough.

[jonathanpeppers]

No, I don't think the trimmer preserves anything here -- this is just ignoring the warning.

This code is looking for a Resource.designer.cs file that is generated in every app, which is the root assembly. There will likely be some problem here for NativeAOT one day.

[jonathanpeppers]

I did preserve the attribute that is kind of the "entry point":

[AttributeUsage (AttributeTargets.Assembly)]
public class ResourceDesignerAttribute : Attribute
{
	public ResourceDesignerAttribute (string fullName)
	public ResourceDesignerAttribute (
			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
			string fullName)
	{
		FullName = fullName;
	}

	[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
	public string FullName { get; set; }

[simonrozsival]

Could we add a Type property to the attribute?

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] public Type Type { get; set; }

That would be perfectly trimmable and AOT friendly.

[jonathanpeppers]

We can add a new API in .NET 9, but currently Xamarin.Android class libraries built against Mono.Android.dll from ~2012 still work. This is different from iOS, as they have breaking changes.

So there would be some number of libraries out there that won't use the new API.

[jonpryor]

This PR is for .NET 9,so we can certainly add the suggested Type property, and also have appropriate fallback code for when Type is null. (This in turn means that the new property would need to allow null, e.g. Type? Type {get;set;}.) We'd also want a "semantically meaningful" name; what is ResourceDesignerAttribute.Type supposed to mean? A better name is warranted. ResourceDesignerType, perhaps?

[assembly: Android.Runtime.ResourceDesignerAttribute(
    "Example.Android.Resource",
    ResourceDesignerType=typeof(Example.Android.Resource),
    IsApplication=true
)]

That said… the "new" Resource design of dc3ccf2 was thought/hoped to (somewhat?) address trimming support; by using properties/methods for everything the trimmer should be able to "see" what methods are used and preserve them, while allowing everything else to be trimmed away.

Setting [DynamicallyAccessedMembers-(DynamicallyAccessedMemberTypes.PublicMethods)] feels contrary to that intention? Why would all methods on the Resource type need to be preserved?

Rando aside: why'd we use a string to name the Resource type instead of typeof() in the first place? A constructor overload feels reasonable here!

partial class ResourceDesignerAttribute {
    public ResourceDesignerAttribute (Type resourceDesignerType);
}

[jonathanpeppers]

Setting [DynamicallyAccessedMembers-(DynamicallyAccessedMemberTypes.PublicMethods)] feels contrary to that intention? Why would all methods on the Resource type need to be preserved?

We used System.Reflection to find an UpdateIdValues() method and call it, so I think the current trimmer design is to add DynamicallyAccessedMemberTypes.PublicMethods?

partial class ResourceDesignerAttribute {
public ResourceDesignerAttribute (Type resourceDesignerType);
}

For this part, let me see what it would take to:

1. Generate usage of this overload, as we create Resource.designer.cs at build time
2. Maybe the old overload could emit a trimmer warning.

[jonpryor]

@jonathanpeppers wrote:

We used System.Reflection to find an UpdateIdValues() method and call it,

Yes, we used to. Commit dc3ccf2 should have removed that call for most (all?) cases. Yes, UpdateIdValues() still exists, but "nothing" should call it (unless some customer decided to manually call it For Reasons™?). We certainly shouldn't call it, and we should be actively removing it, e.g. in ClearDesignerClass():

https://github.com/xamarin/xamarin-android/blob/e873731229274e5382af7672fa4a419f1f1d9bb1/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/LinkDesignerBase.cs#L137

@dellis1972: do we even need [assembly: ResourceDesigner(…)] anymore?

[jonpryor]

@dellis1972 replies on Discord:

Its not called if they are using the new Designer Assembly. If they turn it off then it will be used.

I overlooked/forgot that the new stuff can be disabled. It's controlled via $(AndroidUseDesignerAssembly).

When can we drop support for this? It was added in .NET 8; removing support for $(AndroidUseDesignerAssembly)=false from .NET 9 feels too early, maybe? Maybe .NET 10?

[dellis1972]

I'm on with .NET 10 , people should have found all the bugs in the new stuff by then :P

[jonathanpeppers]

There is a small app size change here.

It looks like some are:

• Past .NET bumps were under the threshold
• libmonodroid.so changed, which must be other recent native code changes
• We are actually preserving more stuff now. Perhaps we had some subtle trimmer bugs?

[jonpryor]

@jonathanpeppers wrote:

We are actually preserving more stuff now. Perhaps we had some subtle trimmer bugs?

Are we preserving more stuff? What stuff and why? I think this might warrant more investigation. Consider my earlier comment: we should not be preserving all public methods on the Resource type! That also is apparently the case: we're not preserving all public methods, as indicated by the fact that UnnamedProject.dll is smaller, as is _Microsoft.Android.Resource.Designer.dll (by two bytes!).

Java.Interop.dll increased in size by ~3KB, while Mono.Android.dll shrank by ~2KB.

Of interest is that classes*.dex also increased by ~7KB:

    "classes.dex": {
-     "Size": 9458972
+     "Size": 9418292
    },
    "classes2.dex": {
-     "Size": 103648
+     "Size": 150904
    },

This implies that we're emitting Java Callable Wrappers for more types. What types are we now emitting JCWs for, and why? That could be the "subtle trimmer bug" you elude to, and I would like more details here.

[vitek-karas]

Will this reason hold for actual usage?
For example - in OTel, we had a similar issue, and the outcome of the discussion was that it's very important to get something for each frame. So there we rely on .ToString() on the StackFrame which works even in NativeAOT/trimming (unless one explicitly disable stack frame info in NativeAOT as a size optimization).

I'm not saying this is wrong, just asking where is this used, and if we would like to get at least some info all the time in those scenarios.

[jonathanpeppers]

Let me investigate if we could just use StackFrame.ToString() for this. It's possible we would get the same text, and the tests would pass: 1aa0ea7

[jonathanpeppers]

I tested in a console app, and it appears that StackFrame.GetMethod() returns null under NativeAOT. But calling StackFrame.ToString() returns useful information that contains the method name, example:

MainActivity.OnCreate() + 0x37 at offset 55 in file:line:column <filename unknown>:0:0

To support that one day, we could parse the string, but I think delaying that work for now is best.

I left a code comment for now, linking to:

• Fix known issues with NativeAOT #8724

[jonathanpeppers]

@jonpryor on the app size changes, this is main vs this PR:

    Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
      +      47,432 classes2.dex
      +         361 assemblies/Mono.Android.dll
      +          81 assemblies/Xamarin.Google.Android.Material.dll
      +          64 assemblies/Xamarin.AndroidX.CoordinatorLayout.dll
      +          54 assemblies/Xamarin.AndroidX.AppCompat.dll
      -           1 assemblies/UnnamedProject.dll
      -      40,604 classes.dex
    Summary:
      +           0 Other entries 0.00% (of 1,566,414)
      +         559 Assemblies 0.01% (of 4,140,795)
      +       6,828 Dalvik executables 0.07% (of 9,562,368)
      +           0 Shared libraries 0.00% (of 4,731,120)
      +           0 Package size difference 0.00% (of 10,897,699)

So we crossed the multi-dex threshold, and that's why classes2.dex appeared.

I have a diff of the two apps, and it looks like constructors like these were preserved now, and so we have JCWs for JLO subclasses like:

So I bet decorating JavaObjectExtensions.JavaCast<T> with:

[DynamicallyAccessedMembers (Constructors)]

Preserved these constructors.