[jnimarshalmethod-gen] Avoid creating duplicate marshal methods

[radekdoulik]

Fixes #384

Sort the methods before generating marshal methods. Sort them so that
methods with name containing the interface name are at the end.

During processing check whether we already have a marshal method with
the same name. (note that the name contains parameter types)

When sorting methods so that interface implementation methods are
last, use MethodDefinition:HasOverrides to identify interface
implementations.

Introduce new cache to map reflection methods to cecil method
definitions. Example cache hit ratio:

    ~ 92% (1210/1318)

when generating marshal methods for
Mono.Android.dll:Java.Nio.CharBuffer* types.

[radekdoulik]

I am also cooking a test for that. It would take a bit of effort though as I will need to use cecil to check the generated marshaling class. I would like to add the test in a separate PR to not block the XA's dotnet/android#2153 progress.

[jonpryor]

This is not an adequate way to determine if a MethodInfo is for an explicit interface implementation, as mentioned elsewhere.

For example -- and you made me dig up F# just to do this?! ;-) -- consider this F# declaration which implements the System.IServiceProvider interface:

open System;

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

Compile, then disassemble, and the method we see is:

    // 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

(Look mah, no .s! It instead uses dashes!)

Which means simply checking for . won't work with F# code.

Instead, we need to use Type.GetInterfaceMap(Type).

csharp example of use:

var t = typeof(List<int>);
var c = t.GetProperty("Count");

// `cm` implements ICollection<int>.Count.  How do we actually tell?
var cm = c.GetGetMethod ();

System.Reflection.InterfaceMapping map = cm.DeclaringType.GetInterfaceMap(typeof(ICollection<int>));
map.TargetMethods.Contains(cm); // true!

Alas, there doesn't appear to be anything as easy as Cecil's MethodDefinition.Overrides, which you used last time.

Also, calling Type.GetInterfaceMap() separately for every method will be terrible, perf-wise, so some form of machine would be needed.

That aside, simply checking for . is insufficient. It will break.

[jonpryor]

As an alternative to using Type.GetInterfaceMapping(), I wonder if it would be equally effective to just check the return type, and prefer non-interface return types over interface return types?

Suggested change

	bool aContainsDot = a.Name.IndexOf ('.') != -1;
	int e = string.Compare (a.Name, b.Name);
	if (e == 0 && a.ReturnType.IsInterface != b.ReturnType.IsInterface) {
	if (!a.ReturnType.IsInterface)
	return -1;
	if (!b.ReturnType.IsInterface)
	return 1;
	throw new InvalidOperationException ("What condition did I miss?");
	}
	return e;

[radekdoulik]

This is not an adequate way to determine if a MethodInfo is for an explicit interface implementation, as mentioned elsewhere.

I didn't like the string comparison either, but because I thought the method was not overriden in that class, I didn't find better solution and stopped thinking about it at that point. My bad. The class has the methods with override as I found later (it was partial class split into 2 source files).

Anyway. Thinking more about it, I guess the whole solution is actually wrong and we should have both marshal methods. Because we will be missing the marshal method, when someone calls the iface method from Java side.

I think we should just name the marshal methods differently and include the return type in the method name. So in this case we would have:

__<$>_jni_marshal_methods.n_append_Ljava_lang_CharSequence__Ljava_lang_Appendable
__<$>_jni_marshal_methods.n_append_Ljava_lang_CharSequence__Ljava_lang_CharBuffer

Note double '_' before the return type name. It would actually be enough to have it for the iface method I guess:

__<$>_jni_marshal_methods.n_append_Ljava_lang_CharSequence__Ljava_lang_Appendable
__<$>_jni_marshal_methods.n_append_Ljava_lang_CharSequence

Then I hope it should be reachable from Java side as the registration signature contains the return type. Does it look OK to you? If so then I should probably try it to confirm it works.

[radekdoulik]

Thinking even more about it, we might not need to change it here at all as the registration class doesn't go thru C# and on IL level it should be OK. We should probably just check signatures from the register attribute in the linker, when updating the registration class.

[radekdoulik]

I have tried to update linker to update the registration methods with duplicates. There were issues with cecil usage, so resolved back to not generate the "duplicate" methods in the generator.

[dellis1972]

My guess is this can now return null? If so do we know where this method is called? Do those calling points need to be updated to handle a null?

[radekdoulik]

The updated path with null goes only thru CompareTypes where it is used only for comparison.