[Java.Interop] Add RegisterNativeMembers attribute

[radekdoulik]

Implementation of improvement noted in
#205 comments.

Instead of "hard-wiring" the name of a method to register native
members in a given type, introduce new attribute to mark appropriate
method in that type.

It is now possible to have multiple registration methods. The
TestType.cs is updated to test this new behavior.

JniNativeMethodRegistrationArguments::RegisterNativeMethods is now
internal to avoid calling it from 3rd party registration methods as it
would be overriden later and the registrations would be lost.

Updated all related code to use the new attribute.

It also fixes fixes issues reported by Gendarme, R: Gendarme.Rules.Performance.AvoidUnusedParametersRule and gets us to
116 defects reported.

[jonpryor]

Fundamental semantic method here: Do we -- should we -- limit to only a single method? That's what this change does: it means a type can only have a single [RegisterNativeMembers] method.

Is that useful? Is it desirable?

It may be useful and desirable, but it's also not supportable. The intention of the [RegisterNativeMembers] method is to call JNIEnv::RegisterNativeMethods() function. JNIEnv::RegisterNativeMethods() can only be called once. (Not strictly true -- you can of course call more than once, but the invocations are not "additive"; they're replacements. The last invocation "wins.")

As such, allowing multiple [RegisterNativeMembers] methods doesn't make sense. It suggests behavior which can't be provided, not unless we change the semantics entirely.

---

Random musings for other purposes, which allowed me to answer the above question.

Additionally, there's a performance question. The need for [RegisterNativeMembers] is to help improve xamarin-android process startup time. The eventual process would look like:

Packaging time:

1. Run jnimarshalmethod-gen.exe on all assemblies in the app, producing *-JniMarshalMethod.dll assemblies. For example, if the app has a Scratch.MyApp.dll assembly, packaging-time would produce a Scratch.MyApp-JniMarshalMethod.dll assembly. These new assemblies will be included with the app (fast deployment or inclusion in the .apk).
2. The .apk would contain "normal" Java Callable Wrappers, as currently produced.

Runtime:

1. "Eventually," the static constructor for a Java Callable Wrapper will execute, calling mono.android.Runtime.register().
2. Runtime.register() is JNIEnv.RegisterJniNatives()
3. JNIEnv.RegisterJniNatives() will "call" JniRuntime.CurrentRuntime.TypeManager.RegisterNativeMembers() -- probably by by having AndroidTypeManager override AndroidTypeManager.RegisterNativeMembers().
4. AndroidTypeManager.RegisterNativeMembers() will need to:
   1. Try to load the *-JniMarshalMethod.dll assembly. if it exists, invoke the [RegisterNativeMethods] type located there.
   2. If (4.1) fails, then "fallback" to the current string.Split()+ Reflection heavy JNIEnv.RegisterJniMethods() implementation.

The hope is that using jnimarshalmethod-gen.exe and *-JniMarshalMethod.dll assemblies will be faster than what we currently do, by avoiding the need for System.Reflection.Emit.

The question: is it?

Unfortunately we can't answer that question without trying.

Meanwhile, it would also be useful to try to imagine other ways that JNI method registration could be performed. Is going through Type.GetType() + TypeInfo.GetRuntimeMethods() + ... going to be efficient? Or do we need some other mechanism for mapping Java types to managed types?

Maybe what we instead/also need to do is use an "int" mapping:

// Java Callable Wrapper
class MyActivity {
    static final int JavaInteropIndex = 0;
    static {
        Runtime.register (JavaInterpIndex, MyActivity.class);
    }
}

// helper glue in `Mono.Android.dll`?
struct RegistrationInfo {
    Type RuntimeType;
    Action<JniType, string> RegistrationMethod;
    public RegistrationInfo (Type type, Action<JniType, string> method)
    {
        RuntimeType = type;
        RegistrationMethod = method;
    }
}

// ...generated during some packaging step
static class Registrations {
    public static RegistrationInfo[] Info = new[]{
        /* index 0 */ new RegistrationInfo (typeof (MyActivity), MyActivity.RegisterNativeMethods), // method from jnimarshalmethod-gen
        // ...
    };
}

This would allow some variation on JNIEnv.RegisterJniNatives() to directly turn the int index value from Runtime.register() into an index into Registrations.Info.

This sounds cool.

What's unknown is what the startup runtime/overhead penalty will be for the Registrations static constructor, which will have thousands of typeof() expressions + method lookups (off).

Turning this back to the original question -- should we support more than one [RegisterNativeMethod] method? -- supporting more than one method would in turn require that RegistrationInfo have a Action<JniType, string>[], not a single. Is that meaningful? ¯_(ツ)_/¯

However, supporting more than one method does mean that one could plausibly (but sanely?!) use "custom" method registration alongside jnimarshalmethod-gen-based registration.

...which is exactly the problem, and why this cannot work.

[jonpryor]

As such, allowing multiple [RegisterNativeMembers] methods doesn't make sense. It suggests behavior which can't be provided, not unless we change the semantics entirely.

...that said, maybe that's the right thing to do? Instead of a [RegisterNativeMembers] method being a method that itself needs to call JniType.RegisterNativeMethods(), maybe what we need is an indirection. Instead of:

[RegisterNativeMethods]
static void RegisterMethods (JniType type, string members)
{
    // ...
    type.RegisterNativeMethods (...);
}

we could instead do:

[JniNativeMethodRegistration]
static IEnumerable<JniNativeMethodRegistration> GetNativeMethods (JniType type, string members)
{
    return ...;
}

The actual JniType.RegisterNativeMethods() call would be moved into core code that we control. The "called" code would merely be responsible for returning the methods to register, not perform the registration.

For example, in ExportTest:

partial class ExportTest {
	[JniNativeMethodRegistration]
	static IEnumerable<JniNativeMethodRegistration> RegisterNativeMembers (JniType type, string members)
	{
		var methods = JniEnvironment.Runtime.MarshalMemberBuilder
			.GetExportedMemberRegistrations (typeof (ExportTest));
		return methods;
	}
}

This could plausibly support intermixing registration of methods that jnimarshalmethod-gen.exe produces alongside manually constructed methods. JniTypeManager.TryRegisterNativeMembers() would become:

static bool TryRegisterNativeMembers (JniType nativeClass, Type marshalType, string methods)
{
	var registrations = new List<JniNativeMethodRegistration>();
	foreach (var m in marshalType.GetRuntimeMethods ()) {
		if (m.GetCustomAttribute (typeof (RegisterNativeMembersAttribute)) == null)
			continue;
		var reg = (Func<JniType, string, IEnumerable<JniNativeMethodRegistration >>)registerMethod.CreateDelegate (typeof (Func<JniType, string, IEnumerable<JniNativeMethodRegistration >>));
		registrations.AddRange (reg (nativeClass, methods));
	}
	nativeClass.RegisterNativeMethods (registrations.ToArray ());
	return true;
}

[jonpryor]

struct JniNativeMethodRegistrationArguments {
	public ICollection<JniNativeMethodRegistration> Registrations {get;}
	public string Methods {get;}
}

class Example {
	[JniAddNativeMethodRegistration]
	static void Register (JniNativeMethodRegistrationArguments e)
	{
		e.Registrations.Add (new JniNativeMethodRegistration (...));
	}
}

[radekdoulik]

build

[jonpryor]

Your editor is weird, and is inserting spaces. This should be two tab stops before the .GetExportedMemberRegistrations().

[radekdoulik]

It is VS/Mac with Mono coding style. This case is not defined by http://www.mono-project.com/community/contributing/coding-guidelines/, so I think it tries to align it with the beginning of the parameter.

I will put it just on single line as there are even longer lines in that file.

[jonpryor]

Please format this member conventionally:

public ICollection<JniNativeMethodRegistration> Registrations {
    get { return _registrations; }
}

[jonpryor]

Please null-check the arguments.

[jonpryor]

Please rename as AddRegistrations().

[jonpryor]

This type is a struct. As such, it has an implicit default constructor, from which _registrations will be null.

We should choose one of three things:

1. Ignore it, and realize that NullReferenceException can be thrown from here
2. null-check _registrations, and throw InvalidOperationException when null.
3. Use _registrations?.AddRange(), and "ignore" it.

I'm inclined toward (3).

[jonpryor]

Additionally, an alternate idea comes to mind:

If the constructor instead took ICollection<JniNativeMethodRegistration>, we could support that by just using .Add() here:

foreach (var v in registrations) {
    Registrations.Add (v);
}

This would allow removing the Gendarme ignore on the constructor, and would allow non-List<JniNativeMethodRegistration> values to be provided. It just means we have more .Add() calls.

[radekdoulik]

That is how I implemented it first. Later I used List::AddRange to improve performance, which I think is our goal here.

Optionally we can use ICollection<JniNativeMethodRegistration> and try to cast it to List<JniNativeMethodRegistration> and use AddRange in that case?

[jonpryor]

I think it would help "intrinsic understanding" ("who reads documentation?") if the constructor parameter type matched the property type.

As such, both the constructor parameter and the property should either be ICollection<T> or List<T>. (Not sure how this would impact Gendarme, but for consistency and understanding they should be the same type.)

Given that we're mostly after performance, I think using List<T> everywhere would be better.

We still need to handle/answer the question of "what happens when Registrations is null?"

[radekdoulik]

OK, I changed them both to ICollection<T> and in AddRegistrations I check whether it can be cast to List<T> to speed it up.

When Registrations is null, I made it throw an InvalidOperationException as it is invalid state we would like to avoid. We should not get into such state, so throwing the exception might potentially helps us find a bug.

[jonpryor]

It's probably still a good idea to leave this as __RegisterNativeMembers, or use some other "unique" name, because the type generated will also contain names under user control, as the marshal method names mirror the method names to register.

Otherwise we need to rely on the parameter signature being unique, which is also probably fine, but doesn't make me feel entirely good.