From 2c288018391202827a7f09472b56f5e41488db34 Mon Sep 17 00:00:00 2001 From: Jonathan Pobst Date: Mon, 24 Feb 2025 08:07:33 -1000 Subject: [PATCH] [XABT] Don't run `AddKeepAlivesStep` on .NET for Android assemblies. (#9823) Fixes: https://github.com/dotnet/android/issues/8421 Context: https://github.com/dotnet/java-interop/issues/719 Context: https://github.com/dotnet/android/pull/5278 In https://github.com/dotnet/java-interop/issues/719, we realized we needed to call `GC.KeepAlive ()` on objects that are passed to native code to prevent the GC from collecting them prematurely. However, this only solved the issue for newly compiled binding libraries and not the existing ecosystem of binding libraries. Thus we added https://github.com/dotnet/android/pull/5278, a "linker" step that would add `GC.KeepAlive ()` calls to existing binding libraries. The `AddKeepAlivesStep` runs on all assemblies the contain a reference to `Java.Lang.Object`, including ones that already have the keep-alives compiled in. Modify the step to skip assemblies that are built against .NET for Android, as we know they have the compiled keep-alives. Technically we could go back to Xamarin.Android 11.1, but using .NET for Android as the cutoff will easily filter out the vast majority of binding libraries. This results in decent savings for the following test case: ``` dotnet new android dotnet add package Xamarin.AndroidX.Activity --version 1.9.3.1 dotnet build -p:AndroidAddKeepAlives=true ``` | Scenario (`_LinkAssembliesNoShrink`) | This PR | main | | -------- | ------- | ------ | | Full | 4.19 s | 4.72 s | However, in cases like https://github.com/dotnet/android/issues/8421 where this step seems to be taking an abnormal amount of time there should be substantial improvements. --- .../MonoDroid.Tuner/AddKeepAlivesStep.cs | 20 ++++++++++--------- .../Tasks/LinkerTests.cs | 3 +++ .../Utilities/MonoAndroidHelper.Linker.cs | 17 ++++++++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs b/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs index 786bc4e75ce..c916867cf2b 100644 --- a/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs +++ b/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs @@ -1,15 +1,12 @@ +#nullable enable using System; -using System.Collections.Generic; using System.Linq; - -using Mono.Cecil; - using Java.Interop.Tools.Cecil; - +using Mono.Cecil; +using Mono.Cecil.Cil; using Mono.Linker; using Mono.Linker.Steps; - -using Mono.Cecil.Cil; +using Xamarin.Android.Tasks; namespace MonoDroid.Tuner { @@ -33,6 +30,11 @@ internal bool AddKeepAlives (AssemblyDefinition assembly) if (!assembly.MainModule.HasTypeReference ("Java.Lang.Object")) return false; + // Anything that was built against .NET for Android will have + // keep-alives already compiled in. + if (MonoAndroidHelper.IsDotNetAndroidAssembly (assembly)) + return false; + bool changed = false; foreach (TypeDefinition type in assembly.MainModule.Types) changed |= ProcessType (type); @@ -60,7 +62,7 @@ bool MightNeedFix (TypeDefinition type) return !type.IsAbstract && type.IsSubclassOf ("Java.Lang.Object", Context); } - MethodDefinition methodKeepAlive = null; + MethodDefinition? methodKeepAlive = null; bool AddKeepAlives (TypeDefinition type) { @@ -117,7 +119,7 @@ protected virtual AssemblyDefinition GetCorlibAssembly () return Context.GetAssembly ("System.Private.CoreLib"); } - MethodDefinition GetKeepAliveMethod () + MethodDefinition? GetKeepAliveMethod () { var corlibAssembly = GetCorlibAssembly (); if (corlibAssembly == null) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs index 79981abdb51..035fca0f5c5 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs @@ -464,6 +464,9 @@ public unsafe bool MyMethod (Android.OS.IBinder windowToken, [global::Android.Ru proj.SetProperty ("AllowUnsafeBlocks", "True"); + // We don't want `[TargetPlatform ("android35")]` to get set because we don't do AddKeepAlives on .NET for Android assemblies + proj.SetProperty ("GenerateAssemblyInfo", "False"); + if (setAndroidAddKeepAlivesTrue) proj.SetProperty ("AndroidAddKeepAlives", "True"); diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Linker.cs b/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Linker.cs index b3b863dbda4..ec987463124 100644 --- a/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Linker.cs +++ b/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Linker.cs @@ -1,3 +1,4 @@ +#nullable enable using System; using System.Collections.Generic; using System.Linq; @@ -20,6 +21,22 @@ public static bool IsFrameworkAssembly (AssemblyDefinition assembly) => public static bool IsFrameworkAssembly (string assembly) => KnownAssemblyNames.Contains (Path.GetFileNameWithoutExtension (assembly)); + // Is this assembly a .NET for Android assembly? + public static bool IsDotNetAndroidAssembly (AssemblyDefinition assembly) + { + foreach (var attribute in assembly.CustomAttributes.Where (a => a.AttributeType.FullName == "System.Runtime.Versioning.TargetPlatformAttribute")) { + foreach (var p in attribute.ConstructorArguments) { + // Of the form "android30" + var value = p.Value?.ToString (); + + if (value is not null && value.StartsWith ("android", StringComparison.OrdinalIgnoreCase)) + return true; + } + } + + return false; + } + static readonly char [] CustomViewMapSeparator = [';']; public static Dictionary> LoadCustomViewMapFile (string mapFile)