[linker] remove unneeded string.Format calls

[jonathanpeppers]

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.

[jonathanpeppers]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jonpryor]

The short circuit and reduced memory usage is nice and all but...are we still doing too much?

The point behind PR #861 and the above IsInOverrides() invocation was to see if tMethod was supposed to override the interface method iMethod.

If that fails, then we fallback to...a whole mess of string work.

Why do we need to do that whole mess of string work at all? Why do we need HaveSameMethodName()? Why do we need to compare parameter types & return types?

Why isn't IsInOverrides() by itself "enough"? What test case am I not thinking about?

(With an added point that method names being {iface.FullName}.{iMethod.Name} or {declaringType}.{iface.Name}.{iMethod.Name} is really only true for C# code, and IIRC isn't true for VB.NET code or F# code, so if we can avoid string comparisons entirely that would be Really Nice...)

@radekdoulik?

[jonpryor]

More on the "F# names don't match the C# convention", consider this F# code which implements the IServiceProvider.GetService(Type) method:

open System;

type MyServiceProvider() =
  interface IServiceProvider with
    member this.GetService(serviceType : Type): System.Object = null

When using "Microsoft (R) F# Compiler version 10.2.3 for F# 4.5", the monodic output shows:

  .class nested public auto ansi serializable MyServiceProvider
  	extends [mscorlib]System.Object
  	implements [mscorlib]System.IServiceProvider  {
    .custom instance void class [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::'.ctor'(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) =  (01 00 03 00 00 00 00 00 ) // ........

    // method line 2
    .method private virtual hidebysig newslot 
           instance default object 'System-IServiceProvider-GetService' (class [mscorlib]System.Type serviceType)  cil managed 
    {
        // Method begins at RVA 0x205c
	.override class [mscorlib]System.IServiceProvider::GetService
	// Code size 2 (0x2)
	.maxstack 8
	IL_0000:  ldnull 
	IL_0001:  ret 
    } // end of method MyServiceProvider::System-IServiceProvider-GetService

  } // end of class MyServiceProvider

That is, the method name which implements IServiceProvider.GetService is named System-IServiceProvider-GetService, which uses hyphens (dashes? minus signs?) within the name, which is something we are not checking for.

Nor should we! The IL explicitly states which interface method is implemented!

	.override class [mscorlib]System.IServiceProvider::GetService

[radekdoulik]

Why isn't IsInOverrides() by itself "enough"?

IIRC, the .override is not always present in IL.

@jonathanpeppers do you have your new test case IL around? I think that might show that as the test case is going through the string comparison branch, right?

[jonpryor]

IIRC, the .override is not always present in IL.

...and this is the point where I start burbling like a looney.

In fact, virtual methods overrides do not use .override!

class App {
    public override string ToString ()
    {
        return "Hello, App!";
    }
}

disassembles (for csc /optimize+ output) to:

    // method line 2
    .method public virtual hidebysig 
           instance default string ToString ()  cil managed 
    {
        // Method begins at RVA 0x2061
        // Code size 6 (0x6)
        .maxstack 8
        IL_0000:  ldstr "Hello, App!"
        IL_0005:  ret 
    } // end of method App::ToString

which in no way specifies what method it's overriding.

This is why we need to fallback to signature-based matching: for virtual methods that are overridden in derived types. Interface methods get the "easy" path of .override.

😭

[jonpryor]

Possibly "silly" question, but why are we using System.Reflection.Emit to create MyAssembly.dll instead of using some C# code?

(Yes, FixAbstractMethodsStep_NestedTypes() does that too, but now I wonder the same thing as well!)

[jonathanpeppers]

I tried using CodeDom... It seems like sacrificing too many chickens, it required me adding global alias for System.dll:

jonathanpeppers@b259e62

ILMerge puts CodeDom types in Xamarin.Android.Build.Tasks.dll:

https://github.com/xamarin/xamarin-android/blob/19cd98b0f0f7709c9383185c64c5500ab9cae00f/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.targets#L308

I didn't get to the point of how to "add" a new method to an interface of an existing assembly. I guess it would overwrite Mono.Android.dll with a new one?

Using only Mono.Cecil it's easy to fabricate the types to make FixAbstractMethods do its work.

[jonpryor]

This is why we need to fallback to signature-based matching...

Still, that doesn't quite explain why we need the previous string.Format()-based logic. Sure, virtual methods don't have .override, but virtual methods also have the same name.

The only instances -- that I know of -- where a method name would be $"{iface.FullName}.{iMethod.Name}" or $"{declaringType}.{iface.Name}.{iMethod.Name}" is when the method is part of an explicit interface implementation, in which case it most certainly should have .override!

Which means the "original" question is still unanswered:

Why do we need to do that whole mess of string work at all? Why do we need HaveSameMethodName()?

An actual test case would be useful here. 😕

[jonathanpeppers]

@jonpryor I think I did what we needed here:

1. Just check the method names and that's it.
2. Document the release notes.
3. Make the test case using an explicit interface implementation.

I also experimented with using C# source code for these tests, such as:

https://github.com/xamarin/java.interop/blob/master/tests/generator-Tests/Integration-Tests/Compiler.cs

I encountered a lot of issues trying to make it work (beyond the hacks in java.interop). It might be worth revisiting later I think.