From 83928ff65f552e73d4b4790f4c3615f5aaad7bf6 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 12 Feb 2020 11:44:55 -0600 Subject: [PATCH] [linker] remove unneeded string.Format calls I was using the Mono profiler for our build, and I noticed: Allocation summary Bytes Count Average Type name 180765632 2142803 84 System.String 53218472 bytes from: MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethods (Mono.Cecil.AssemblyDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethodsUnconditional (Mono.Cecil.AssemblyDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethods (Mono.Cecil.TypeDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:HaveSameSignature (Mono.Cecil.TypeReference,Mono.Cecil.MethodDefinition,Mono.Cecil.MethodDefinition) (wrapper managed-to-native) string:FastAllocateString (int) ~53MB of string from this one method is a lot! I found that one of these `string.Format` calls are running for almost every method on every interface in every assembly: (string.Format ("{0}.{1}", iface.FullName, iMethod.Name) != tMethod.Name) : (string.Format ("{0}.{1}.{2}", iMethod.DeclaringType.DeclaringType, iface.Name, iMethod.Name) != tMethod.Name)) Even the simplest non-matching cases would call `string.Format`: iMethod.Name == "Foo" tMethod.Name == "Bar" In all of these cases... * Class implements interface implicitly * Class implements interface explicitly * Class implements abstract class We found that the `IsInOverrides` check should work or merely compare `iMethod.Name` and `tMethod.Name`. We don't seem to need the `string.Format` calls at all? ~~ Results ~~ An initial build of the Xamarin.Forms integration project on Windows/.NET framework: Before: 796 ms LinkAssembliesNoShrink 1 calls After: 767 ms LinkAssembliesNoShrink 1 calls The same project on macOS/Mono (slightly slower machine, too): Before: 1341 ms LinkAssembliesNoShrink 1 calls After: 1025 ms LinkAssembliesNoShrink 1 calls String allocations are way better, too: Before: Allocation summary Bytes Count Average Type name 180765632 2142803 84 System.String After: Allocation summary Bytes Count Average Type name 127736832 1652070 77 System.String Shows 53,028,800 bytes saved. Before: 53218472 bytes from: MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethods (Mono.Cecil.AssemblyDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethodsUnconditional (Mono.Cecil.AssemblyDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethods (Mono.Cecil.TypeDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:HaveSameSignature (Mono.Cecil.TypeReference,Mono.Cecil.MethodDefinition,Mono.Cecil.MethodDefinition) After: 1933624 bytes from: MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethodsUnconditional (Mono.Cecil.AssemblyDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:FixAbstractMethods (Mono.Cecil.TypeDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:HaveSameSignature (Mono.Cecil.TypeReference,Mono.Cecil.MethodDefinition,Mono.Cecil.MethodDefinition) MonoDroid.Tuner.FixAbstractMethodsStep:HaveSameMethodName (Mono.Cecil.TypeReference,Mono.Cecil.MethodDefinition,Mono.Cecil.MethodDefinition) Shows 51,284,848 bytes saved. I don't know about the discrepancy between these two sets of numbers. But I think this is roughly ~50MB less string allocations. --- Documentation/release-notes/4260.md | 24 +++++++ .../MonoDroid.Tuner/FixAbstractMethodsStep.cs | 2 +- .../Tasks/LinkerTests.cs | 63 +++++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 Documentation/release-notes/4260.md diff --git a/Documentation/release-notes/4260.md b/Documentation/release-notes/4260.md new file mode 100644 index 00000000000..cf9562f4ec3 --- /dev/null +++ b/Documentation/release-notes/4260.md @@ -0,0 +1,24 @@ +### Linker Behavior Change + +Xamarin.Android has to consolidate changes to the Android APIs and how +they affect C# types. If you consider an example such as: + +* API 28 has an `IFoo` interface in `Mono.Android.dll` for `v9.0`. +* C# code implements `IFoo` in class `Bar`. +* API 29 adds another method to `IFoo` in `Mono.Android.dll` for + `v10.0`. +* Apps built targeting API 30 / `v10.0` will fix up `Bar` to implement + the new method, but throw `Java.Lang.AbstractMethodError` if it was + called. + +If the linker didn't make this change, you would instead hit a crash +when trying to create an instance of type `Bar` such as: + + System.TypeLoadException: VTable setup of type 'Bar' failed. + +Xamarin.Android has some performance improvements to the linker in +this area. Existing apps *should* not be affected. + +Submit a [bug report][bug] if you find an issue in this area. + +[bug]: https://github.com/xamarin/xamarin-android/wiki/Submitting-Bugs,-Feature-Requests,-and-Pull-Requests diff --git a/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs b/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs index 5ba93c9992b..7564acd114c 100644 --- a/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs +++ b/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs @@ -158,7 +158,7 @@ bool HaveSameSignature (TypeReference iface, MethodDefinition iMethod, MethodDef if (IsInOverrides (iMethod, tMethod)) return true; - if (iMethod.Name != tMethod.Name && (iMethod.DeclaringType == null || (iMethod.DeclaringType.DeclaringType == null ? (string.Format ("{0}.{1}", iface.FullName, iMethod.Name) != tMethod.Name) : (string.Format ("{0}.{1}.{2}", iMethod.DeclaringType.DeclaringType, iface.Name, iMethod.Name) != tMethod.Name)))) + if (iMethod.Name != tMethod.Name) return false; if (!CompareTypes (iMethod.MethodReturnType.ReturnType, tMethod.MethodReturnType.ReturnType)) 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 a0e28bb54fa..cad294d6d29 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 @@ -70,6 +70,69 @@ static void CreateAbstractIfaceImplementation (string assemblyPath, AssemblyDefi } } + [Test] + public void FixAbstractMethodsStep_Explicit () + { + var path = Path.Combine (Path.GetFullPath (XABuildPaths.TestOutputDirectory), "temp", TestName); + var step = new FixAbstractMethodsStep (); + var pipeline = new Pipeline (); + + Directory.CreateDirectory (path); + + using (var context = new LinkContext (pipeline)) { + + context.Resolver.AddSearchDirectory (path); + + var myAssemblyPath = Path.Combine (path, "MyAssembly.dll"); + + using (var android = CreateFauxMonoAndroidAssembly ()) { + android.Write (Path.Combine (path, "Mono.Android.dll")); + CreateExplicitInterface (myAssemblyPath, android); + } + + using (var assm = context.Resolve (myAssemblyPath)) { + step.Process (context); + + var impl = assm.MainModule.GetType ("MyNamespace.MyClass"); + Assert.AreEqual (2, impl.Methods.Count, "MyClass should contain 2 methods"); + var method = impl.Methods.FirstOrDefault (m => m.Name == "MyNamespace.IMyInterface.MyMethod"); + Assert.IsNotNull (method, "MyNamespace.IMyInterface.MyMethod should exist"); + method = impl.Methods.FirstOrDefault (m => m.Name == "MyMissingMethod"); + Assert.IsNotNull (method, "MyMissingMethod should exist"); + } + } + + Directory.Delete (path, true); + } + + static void CreateExplicitInterface (string assemblyPath, AssemblyDefinition android) + { + using (var assm = AssemblyDefinition.CreateAssembly (new AssemblyNameDefinition ("NestedIFaceTest", new Version ()), "NestedIFaceTest", ModuleKind.Dll)) { + var void_type = assm.MainModule.ImportReference (typeof (void)); + + // Create interface + var iface = new TypeDefinition ("MyNamespace", "IMyInterface", TypeAttributes.Interface); + + var iface_method = new MethodDefinition ("MyMethod", MethodAttributes.Abstract, void_type); + iface.Methods.Add (iface_method); + iface.Methods.Add (new MethodDefinition ("MyMissingMethod", MethodAttributes.Abstract, void_type)); + + assm.MainModule.Types.Add (iface); + + // Create implementing class + var jlo = assm.MainModule.Import (android.MainModule.GetType ("Java.Lang.Object")); + var impl = new TypeDefinition ("MyNamespace", "MyClass", TypeAttributes.Public, jlo); + impl.Interfaces.Add (new InterfaceImplementation (iface)); + + var explicit_method = new MethodDefinition ("MyNamespace.IMyInterface.MyMethod", MethodAttributes.Abstract, void_type); + explicit_method.Overrides.Add (new MethodReference (iface_method.Name, void_type, iface)); + impl.Methods.Add (explicit_method); + + assm.MainModule.Types.Add (impl); + assm.Write (assemblyPath); + } + } + static AssemblyDefinition CreateFauxMonoAndroidAssembly () { var assm = AssemblyDefinition.CreateAssembly (new AssemblyNameDefinition ("Mono.Android", new Version ()), "DimTest", ModuleKind.Dll);