Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Interface Invokers #910

Closed
jonpryor opened this issue Nov 9, 2021 · 0 comments · Fixed by #1145 or dotnet/android#8339
Closed

Optimize Interface Invokers #910

jonpryor opened this issue Nov 9, 2021 · 0 comments · Fixed by #1145 or dotnet/android#8339
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

jonpryor commented Nov 9, 2021

Context: http://github.com/xamarin/monodroid/commit/c0ad930298013425e4797f89b0c9243c22827152
Context: #858

From xamarin/monodroid@c0ad930298013425e4797f89b0c9243c22827152 (2011-Nov-28):

Remember xamarin/monodroid@62a4766, and how the issue was an intermixing of static and
instance data on the interface Invokers?

One of the related questions was why the Invoker.class_ref field was
an instance field in the first place. We use static class_ref fields
everywhere else, so being an instance field was certainly different,
but my efforts to figure out why via the commit messages and
git blame didn't pan out...

The reason Invoker.class_ref was an instance field is as follows:
consider the following set of Java interfaces:

	package android.view;

	public interface ViewManager {
		void addView(View view, ViewGroup.LayoutParams params);
		void updateViewLayout(View view, ViewGroup.LayoutParams params);
		void removeView(View view);
	}

	public interface WindowManager extends ViewManager {
		Display getDefaultDisplay();
		void removeViewImmediate(View view);
	}

The generated C# IWindowManagerInvoker type is (more or less):

	[global::Android.Runtime.Register ("android/view/WindowManager", DoNotGenerateAcw=true)]
	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
		static IntPtr class_ref = JNIEnv.FindClass ("android/view/WindowManager");
		// ...
		static IntPtr id_addView;
		public void AddView (Android.Views.View view, Android.Views.ViewGroup.LayoutParams @params)
		{
			if (id_addView == IntPtr.Zero)
				id_addView = JNIEnv.GetMethodID (class_ref, "addView",
						"(Landroid/view/View;Landroid/view/ViewGroup$LayoutParams;)V");
			JNIEnv.CallVoidMethod (Handle, id_addView,
					new JValue (JNIEnv.ToJniHandle (view)), new JValue (JNIEnv.ToJniHandle (@params)));
		}
	}

Nice. Straightforward. So let's invoke IViewManager.AddView() via an
IWindowManager instance (see Hello.cs):

	var die = new TextView (this) {
		Text = "Added!",
	};
	WindowManager.AddView (die, new WindowManagerLayoutParams ());

What could possibly go wrong™?

D/dalvikvm( 6645): GetMethodID: method not found: Landroid/view/WindowManager;.addView:(Landroid/view/View;Landroid/view/ViewGroup$LayoutParams;)V
I/MonoDroid( 6645): UNHANDLED EXCEPTION: Java.Lang.NoSuchMethodError: Exception of type 'Java.Lang.NoSuchMethodError' was thrown.
I/MonoDroid( 6645): at Android.Runtime.JNIEnv.GetMethodID (intptr,string,string) <0x0007c>
I/MonoDroid( 6645): at Android.Views.IWindowManagerInvoker.AddView (Android.Views.View,Android.Views.ViewGroup/LayoutParams) <0x00067>
I/MonoDroid( 6645): at Mono.Samples.Hello.HelloActivity.OnCreate (Android.OS.Bundle) <0x005ab>
I/MonoDroid( 6645): at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (intptr,intptr,intptr) <0x00057>
I/MonoDroid( 6645): at (wrapper dynamic-method) object.ecadbe0b-9124-445e-a498-f351075f6c89 (intptr,intptr,intptr) <0x00033>

Ouch.

What goes wrong is that the addView() method is declared in the
android.view.ViewManager interface, not the android.view.WindowManager
interface, but we're doing a GetMethodID() on WindowManager. This
sounds like it should work -- WindowManager does implement the
ViewManager interface, after all -- but GetMethodID() doesn't like it
and bombs accordingly.

This is why Invoker.class_ref was an instance field: because we need
to call GetMethodID() against the actual runtime type of the target
object, not against the (compile-time) interface type.

Consequently, in order to fix the bug fixed in xamarin/monodroid@62a4766 in addition to
the bug outlined above, we need to not only make Invoker.class_ref an
instance field (a'la pre- xamarin/monodroid@62a47668), but we also need to make all the
GetMethodID values instance members as well (to fix the issue that
xamarin/monodroid@62a4766 fixed).

The structure of that fix are still with us today; consider: https://github.com/xamarin/java.interop/blob/dcdcce13220d98a848ae3d3598b9b87b22a62c84/tests/generator-Tests/expected/java.lang.Enum/Java.Lang.IComparable.cs

  • IComparableInvoker.class_ref is an instance field.
  • IComparableInvoker.id_compareTo_Ljava_lang_Object_ is an instance field.
  • IComparableInvoker.CompareTo() looks up the jmethodID, storing into id_compareTo_Ljava_lang_Object_. As this is per-instance data, only subsequent IComparable.CompareTo() invocations on this instance will be cached; other instances will be separate.

Consequently, no interface *Invoker data is shared between instances; each instance must re-lookup jmethodID values.

Additionally, in the context of #858, none of the infrastructure currently used by interface *Invoker types will exist.

Thus, the proposal: optimize interface *Invoker types by using JniPeerMembers, one per implemented interface. For IWindowManager, this would be:

[global::Android.Runtime.Register ("android/view/WindowManager", DoNotGenerateAcw=true)]
internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {

	// One JniPeerMembers value per implemented interface; as IWindowManager implements IViewManager, we need two values.
	static JniPeerMembers _members_ViewManager      = new XAPeerMembers ("android/view/ViewManager", typeof (IWindowManagerInvoker), isInterface: true);
	static JniPeerMembers _members_WindowManager    = new XAPeerMembers ("android/view/WindowManager", typeof (IWindowManagerInvoker), isInterface: true);

	public override JniPeerMembers JniPeerMembers {
		get => _members_WindowManager;
	}

	public override Type ThresholdType {
		get => _members.ManagedPeerType;
	}

	new IntPtr class_ref;

	public override IntPtr ThresholdClass {
		get => class_ref;
	}

	public IWindowManagerInvoker (IntPtr handle, JniHandleOwnership transfer) : base (Validate (handle), transfer)
	{
		IntPtr local_ref = JNIEnv.GetObjectClass (((global::Java.Lang.Object) this).Handle);
		this.class_ref = JNIEnv.NewGlobalRef (local_ref);
		JNIEnv.DeleteLocalRef (local_ref);
	}

	public unsafe void AddView (Android.Views.View? view, Android.Views.ViewGroup.LayoutParams? @params)
	{
		const string __id = "addView.(Landroid/view/View;Landroid/view/ViewGroup$LayoutParams;)V";
		JniArgumentValue* __args = stackalloc JniArgumentValue [2];
		__args [0] = new JniArgumentValue ((view == null) ? IntPtr.Zero : ((global::Java.Lang.Object) view).Handle);
		__args [1] = new JniArgumentValue ((@params == null) ? IntPtr.Zero : ((global::Java.Lang.Object) @params).Handle);
		// As AddView() is from `IViewManager`, use `_members_ViewManager` to invoke it.
		_members_ViewManager.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
	}

	// …

	// Note: SKIP java_class_ref, GetObject(), Validate(); those aren't needed anymore.
}

In particular, all methods which implement the IViewManager interface would use _members_ViewManager, while all methods which implement the IWindowManager interface would use _members_WindowManager.

This should allow jmethodID values to be cached across instances, while also allowing appropriate JNI method lookup.

@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Nov 11, 2021
jonpryor added a commit to jonpryor/GenericBindingPrototype that referenced this issue Mar 14, 2022
Context: https://devblogs.microsoft.com/oldnewthing/20210830-00/?p=105617
Context: dotnet/java-interop#795
Context: dotnet/java-interop#910

It occurs to @jonpryor that what we're dealing with is *kinda* like
the world of COM and COM-related technologies; see [oldnewthing][0].

> C++/WinRT and C++/CX set up parallel universes like this:
>
> | C++/WinRT               | ABI                       | C++/CX            |
> | ----------------------- | ------------------------- | ----------------- |
> | winrt::Contoso::Widget  | ABI::Contoso::Widget\*    | Contoso::Widget^  |

We have an "ABI": what JNI expects/requires.

Then we have a "projection" ("binding") from the ABI, for consumption
by end users.

*Historically*, we "hand-waved" over the ABI; it was *implicit* to
how things worked, but *not* something that customers interacted with
directly.

@jonpryor would like to change that, and this seems like a reasonable
"two birds, one stone" opportunity.

The support for generic types, as described in `readm.md`, is
contingent upon a "layer of indirection":

> - Java calls the static invoker
> - The static invoker calls the instance invoker
> - The instance invoker calls the desired method

The "instance invoker" is the "layer of indirection".

The original prototype "named" this "layer of indirection"
`I[Java type name]Invoker`; e.g. given Java:

	// Java
	package example;

	public interface ArrayList<E> {
	    void add(E obj);
	}

we'd have "binding + infrastructure" of:

	// C#
	namespace Example {
	    public partial interface IArrayList<E> {
	        void Add(E obj);
	    }
	    public partial interface IArrayListInvoker {
	        void InvokeAdd(Java.Lang.Object obj);
	    }
	}

My proposal is to take some ideas from dotnet/java-interop#795
and WinRT projections, "formalizing" the ABI:

	// Convention: `jniabi.` + *Java* package name
	namespace jniabi.example {

	    // Convention: `Jniabi` prefix + Java type name
	    [EditorBrowsable (EditorBrowsableState.Never)]
	    public static partial class JniabiArrayList {
	        // Convention: Java method name + "suffix" for overloads; uses "lowest level" types.
	        public static void add(IJavaPeerable self, JniObjectReference obj);
	    }

	    // Convention: `_Jniabi` prefix + Java name suffix;
	    [EditorBrowsable (EditorBrowsableState.Never)]
	    [Register ("example/ArrayList", DoNotGenerateAcw=true)]
	    public partial interface _JniabiArrayList : IJavaObject, IJavaPeerable {
	        // Convention: Java method name + "suffix" for overloads; uses "lowest level" types.
	        [Register ("add", "(Ljava/lang/Object;)V", …);
	        void add(JniObjectReference obj);

	        // jni marshal method glue…
	        private static bool n_Add_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native_p0)
	        {
	            var __this = Java.Lang.Object.GetObject<_ArrayList>(native__this);
	            __this!.add (new JniObjectReference (native_p0));
	        }
	    }
	}

[`[EditorBrowsable(EditorBrowsableState.Never)]`][1] is used to hide
these types from normal IDE code completion scenarios.  They're
intended for internal use.

The `jniabi.example.JniabiArrayList` static class is for "manual
binding" scenarios; see also xamarin-android/src/Mono.Android, which
has a fair bit of "hand-written" code which was produced by taking
`generator` output and copying it.  `jniabi.example.JniabiArrayList`
is meant to remove the need for hand-copying code, but *only* provides
low-level bindings.  Actual type marshaling/etc. is "elsewhere".
It is also useful as part of dotnet/java-interop#910, to allow
"interface Invokers" to begin caching `jmethodID` values.

The `jniabi.example._JniabiArrayList` interface is the layer of
indirection, so that generic types *can* work.  It's not "meant" for
implementation by "normal" people, but *can* be implemented if they
want/need to take control of marshaling.  (For example, Photo data
marshaling is like a 12MB `byte[]` that needs to be marshaled EVERY
TIME, contributing to perf issues.  Being able to interject and do
"something else" could be *really* useful for some scenarios.)

Note: wrt dotnet/java-interop#795, `_JniabiArrayList` was instead
written as `jnimarshalmethod.example.ArrayList`.  I tried this
initially, but wasn't "thrilled" with how it looked.  Please discuss.

We then separate out the "projection" ("binding"), in terms of the ABI:

	namespace Example {
	    // Note: *NO* [Register]; always implements ABI
	    public partial interface IArrayList<E> : IJavaObject, IJavaPeerable, global::jniabi.example._ArrayList
	    {
	        void Add (E obj); // projection

	        // "glue" from ABI to projection; explicitly implemented
	        void jniabi.example._JniabiArrayList.add(JniObjectReference native_obj)
	        {
	            var obj = Java.Lang.Object.GetObject<T>(native_obj.Handle);
	            Add (obj);
	        }
	    }
	}

"Of course", we also want/need the raw types in the projection, if
only for backward compatibility (we've historically *only* bound raw
types), and this "slots in":

	namespace Example {
	    // Note: *NO* [Register]; always implements ABI
	    public partial interface IArrayList : IJavaObject, IJavaPeerable, global::jniabi.example._ArrayList
	    {
	        void Add (Java.Lang.Object? obj); // projection

	        // "glue" from ABI to projection; explicitly implemented
	        void jniabi.example._JniabiArrayList.add(JniObjectReference native_obj)
	        {
	            var obj = Java.Lang.Object.GetObject<Java.Lang.Object>(native_obj.Handle);
	            Add (obj);
	        }
	    }
	}

The downside to having the raw type implement the ABI
`_JniabiArrayList` type is that it *also* incurs the indirection
penalty.

It *is* really flexible though!

Pros to this approach vs. the "original prototype":

  * Reduces "namespace pollution" by moving the "indirection" types
    into a separate set of namespaces.  I rather doubt that anyone
    will "accidentally" find the `jniabi.…` namespace.

  * Makes the ABI explicit, which in turn inserts flexibility.
    What if you don't *want* `java.util.Collection` to be marshaled
    as an `IEnumerable<T>`?  With this approach, you can control it.

  * It's also really awesome looking to be within a generic class and
    use `Object.GetObject<T>()`, using the type parameter directly.

  * Use of Java-originated identifier names (hopefully?) reduces
    complexity.  (I can hope, right?)

Cons:

  * Use of Java-originated identifier names may complicate things.
    Sure, they're still accessible via the `generator` object model,
    but that doesn't mean everything is "fine".
    (If wishes were horses…)

  * "ABI" names *will* need to involve "hashes" for overloads, at
    some point, because all reference types "devolve" to
    `JniObjectReference`, so unless the number of parameters differs,
    method overloads that use "separate yet reference types" will
    collide, e.g. `java.lang.String.getBytes(String)` vs.
    `java.lang.String.getBytes(java.nio.charset.Charset)`.
    The `jniabi.java.lang.String` type will need to expose e.g.
    `getBytes_hash1()` and `getBytes_hash2()` methods, where the
    hash is e.g. the CRC for the JNI method signature.

[0]: https://devblogs.microsoft.com/oldnewthing/20210830-00/?p=105617
[1]: https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.editorbrowsableattribute?view=net-6.0
jonpryor added a commit that referenced this issue Sep 14, 2023
Fixes: #910

Context: bc5bcf4
Context: #858

Consider the Java `java.lang.Runnable` interface:

	package java.lang;
	public interface Runnable {
	    void run ();
	}

This is bound as:

	package Java.Lang;
	public interface IRunnable : IJavaPeerable {
	    void Run ();
	}

with some slight differences depending on whether we're dealing with
.NET Android (`generator --codegen-target=xajavainterop1`) or
`src/Java.Base` (`generator --codegen-target=javainterop1`).

Now, assume a Java API + corresponding binding which returns a
`Runnable` instance:

	package example;
	public class Whatever {
	    public static Runnable createRunnable();
	}

You can invoke `IRunnable.Run()` on the return value:

	IRunnable r = Whatever.CreateRunnable();
	r.Run();

but how does that work?

This works via an "interface Invoker", which is a class emitted by
`generator` which implements the interface and invokes the interface
methods through JNI:

	internal partial class IRunnableInvoker : Java.Lang.Object, IRunnable {
	    public void Run() => …
	}

Once Upon A Time™, the interface invoker implementation mirrored that
of classes: a static `IntPtr` field held the `jmethodID` value, which
would be looked up on first-use and cached for subsequent invocations:

	partial class IRunnableInvoker {
	    static IntPtr id_run;
	    public unsafe void Run() {
	        if (id_run == IntPtr.Zero)
	            id_run = JNIEnv.GetMethodID (class_ref, "run", "()V");
	        JNIEnv.CallVoidMethod (Handle, id_run, …);
	    }
	}

This approach works until you have interface inheritance and methods
which come from different interfaces:

	package android.view;
	public /* partial */ interface ViewManager {
	    void addView(View view, ViewGroup.LayoutParams params);
	}
	public /* partial */ interface WindowManager extends ViewManager {
	    void removeViewImmediate(View view);
	}

This would be bound as:

	namespace Android.Views;
	public partial interface IViewManager : IJavaPeerable {
	    void AddView (View view, ViewGroup.LayoutParams @params);
	}
	public partial IWindowManager : IViewManager {
	    void RemoveViewImmediate (View view);
	}
	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
	    static IntPtr id_addView;
	    public void AddView(View view, ViewGroup.LayoutParams @params)
	    {
	        if (id_addView == IntPtr.Zero)
	            id_run = JNIEnv.GetMethodID (class_ref, "addView", "…");
	        JNIEnv.CallVoidMethod (Handle, id_addView, …);
	    }
	}

Unfortunately, *invoking* `IViewManager.AddView()` through an
`IWindowManagerInvoker` would crash!

	D/dalvikvm( 6645): GetMethodID: method not found: Landroid/view/WindowManager;.addView:(Landroid/view/View;Landroid/view/ViewGroup$LayoutParams;)V
	I/MonoDroid( 6645): UNHANDLED EXCEPTION: Java.Lang.NoSuchMethodError: Exception of type 'Java.Lang.NoSuchMethodError' was thrown.
	I/MonoDroid( 6645): at Android.Runtime.JNIEnv.GetMethodID (intptr,string,string)
	I/MonoDroid( 6645): at Android.Views.IWindowManagerInvoker.AddView (Android.Views.View,Android.Views.ViewGroup/LayoutParams)
	I/MonoDroid( 6645): at Mono.Samples.Hello.HelloActivity.OnCreate (Android.OS.Bundle)
	I/MonoDroid( 6645): at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (intptr,intptr,intptr)
	I/MonoDroid( 6645): at (wrapper dynamic-method) object.ecadbe0b-9124-445e-a498-f351075f6c89 (intptr,intptr,intptr)

Interfaces are not classes, and this is one of the places that this
is most apparent.  Because of this crash, we had to use *instance*
`jmethodID` caches:

	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
	    IntPtr id_addView;
	    public void AddView(View view, ViewGroup.LayoutParams @params)
	    {
	        if (id_addView == IntPtr.Zero)
	            id_run = JNIEnv.GetMethodID (class_ref, "addView", "…");
	        JNIEnv.CallVoidMethod (Handle, id_addView, …);
	    }
	}

Pro: no more crash!

Con: *every different instance* of `IWindowManagerInvoker` needs to
separately lookup whatever methods are required.  There is *some*
caching, so repeated calls to `AddView()` on the same instance will
hit the cache, but if you obtain a different `IWindowManager`
instance, `jmethodID` values will need to be looked up again.

This was "fine", until #858 enters the picture:
interface invokers were full of Android-isms --
`Android.Runtime.JNIEnv.GetMethodID()`! -- and thus ***not*** APIs
that @jonpryor wished to expose within desktop Java.Base bindings.

Enter `generator --lang-features=emit-legacy-interface-invokers`:
when *not* specified, interface invokers will now use
`JniPeerMembers` for method lookup and invocation, allowing
`jmethodID` values to be cached *across* instances.  In order to
prevent the runtime crash, an interface may have *multiple*
`JniPeerMembers` values, one per inherited interface, which is used
to invoke methods from that interface.

`IWindowManagerInvoker` now becomes:

	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
	    static readonly JniPeerMembers _members_ViewManager = …;
	    static readonly JniPeerMembers _members_WindowManager = …;

	    public void AddView(View view, ViewGroup.LayoutParams @params)
	    {
	        const string __id = "addView.…";
	        _members_ViewManager.InstanceMethods.InvokeAbstractVoidMethod (__id, this, …);
	    }

	    public void RemoveViewImmediate(View view)
	    {
	        const string __id = "removeViewImmediate.…";
	        _members_WindowManager.InstanceMethods.InvokeAbstractVoidMethod (__id, this, …);
	    }
	}

This has two advantages:

 1. More caching!
 2. Desktop `Java.Base` binding can now have interface invokers.

TODO: Performance?  How much better is this?
Update tests/invocation-overhead.

Update `tests/generator-Tests` expected output.
Note: to keep this patch small, only JavaInterop1 output uses the
new pattern.  XAJavaInterop1 output is unchanged, *even though*
it will use the new output *by default*.

Added [CS0114][0] to `$(NoWarn)` in `Java.Base.csproj` to ignore
warnings such as:

	…/src/Java.Base/obj/Debug-net7.0/mcw/Java.Lang.ICharSequence.cs(195,25): warning CS0114:
	'ICharSequenceInvoker.ToString()' hides inherited member 'Object.ToString()'. To make the current member override that implementation, add the override keyword. Otherwise add the new keyword.

Ignoring CS0114 is also done in `Mono.Android.dll` as well, so this
is not a new or unique requirement.

Update `Java.Interop.dll` so that
`JniRuntime.JniValueManager.GetActivationConstructor()` now knows
about and looks for `*Invoker` types, then uses the activation
constructor from the `*Invoker` type when the source type is an
abstract `class` or `interface`.

Update `tests/Java.Base-Tests` to test for implicit `*Invoker` lookup
and invocation support.

[0]: https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0114
jonpryor added a commit that referenced this issue Oct 26, 2023
#1145)

Fixes: #910

Context: bc5bcf4
Context: #858

Consider the Java `java.lang.Runnable` interface:

	package java.lang;
	public interface Runnable {
	    void run ();
	}

This is bound as:

	package Java.Lang;
	public interface IRunnable : IJavaPeerable {
	    void Run ();
	}

with some slight differences depending on whether we're dealing with
.NET Android (`generator --codegen-target=xajavainterop1`) or
`src/Java.Base` (`generator --codegen-target=javainterop1`).

Now, assume a Java API + corresponding binding which returns a
`Runnable` instance:

	package example;
	public class Whatever {
	    public static Runnable createRunnable();
	}

You can invoke `IRunnable.Run()` on the return value:

	IRunnable r = Whatever.CreateRunnable();
	r.Run();

but how does that work?

This works via an "interface Invoker", which is a class emitted by
`generator` which implements the interface and invokes the interface
methods through JNI:

	internal partial class IRunnableInvoker : Java.Lang.Object, IRunnable {
	    public void Run() => …
	}

Once Upon A Time™, the interface invoker implementation mirrored that
of classes: a static `IntPtr` field held the `jmethodID` value, which
would be looked up on first-use and cached for subsequent invocations:

	partial class IRunnableInvoker {
	    static IntPtr id_run;
	    public unsafe void Run() {
	        if (id_run == IntPtr.Zero)
	            id_run = JNIEnv.GetMethodID (class_ref, "run", "()V");
	        JNIEnv.CallVoidMethod (Handle, id_run, …);
	    }
	}

This approach works until you have interface inheritance and methods
which come from inherited interfaces:

	package android.view;
	public /* partial */ interface ViewManager {
	    void addView(View view, ViewGroup.LayoutParams params);
	}
	public /* partial */ interface WindowManager extends ViewManager {
	    void removeViewImmediate(View view);
	}

This would be bound as:

	namespace Android.Views;
	public partial interface IViewManager : IJavaPeerable {
	    void AddView (View view, ViewGroup.LayoutParams @params);
	}
	public partial IWindowManager : IViewManager {
	    void RemoveViewImmediate (View view);
	}
	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
	    static IntPtr id_addView;
	    public void AddView(View view, ViewGroup.LayoutParams @params)
	    {
	        if (id_addView == IntPtr.Zero)
	            id_run = JNIEnv.GetMethodID (class_ref, "addView", "…");
	        JNIEnv.CallVoidMethod (Handle, id_addView, …);
	    }
	}

Unfortunately, *invoking* `IViewManager.AddView()` through an
`IWindowManagerInvoker` would crash!

	D/dalvikvm( 6645): GetMethodID: method not found: Landroid/view/WindowManager;.addView:(Landroid/view/View;Landroid/view/ViewGroup$LayoutParams;)V
	I/MonoDroid( 6645): UNHANDLED EXCEPTION: Java.Lang.NoSuchMethodError: Exception of type 'Java.Lang.NoSuchMethodError' was thrown.
	I/MonoDroid( 6645): at Android.Runtime.JNIEnv.GetMethodID (intptr,string,string)
	I/MonoDroid( 6645): at Android.Views.IWindowManagerInvoker.AddView (Android.Views.View,Android.Views.ViewGroup/LayoutParams)
	I/MonoDroid( 6645): at Mono.Samples.Hello.HelloActivity.OnCreate (Android.OS.Bundle)
	I/MonoDroid( 6645): at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (intptr,intptr,intptr)
	I/MonoDroid( 6645): at (wrapper dynamic-method) object.ecadbe0b-9124-445e-a498-f351075f6c89 (intptr,intptr,intptr)

Interfaces are not classes, and this is one of the places that this
is most apparent.  Because of this crash, we had to use *instance*
`jmethodID` caches:

	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
	    IntPtr id_addView;
	    public void AddView(View view, ViewGroup.LayoutParams @params)
	    {
	        if (id_addView == IntPtr.Zero)
	            id_run = JNIEnv.GetMethodID (class_ref, "addView", "…");
	        JNIEnv.CallVoidMethod (Handle, id_addView, …);
	    }
	}

Pro: no more crash!

Con: *every different instance* of `IWindowManagerInvoker` needs to
separately lookup whatever methods are invoked.  There is *some*
caching, so repeated calls to `AddView()` on the same instance will
hit the cache, but if you obtain a different `IWindowManager`
instance, `jmethodID` values will need to be looked up again.

This was "fine", until #858 enters the picture:
interface invokers were full of Android-isms --
`Android.Runtime.JNIEnv.GetMethodID()`! `JNIEnv.CallVoidMethod()`! --
and thus ***not*** APIs that @jonpryor wished to expose within
desktop Java.Base bindings.

Enter `generator --lang-features=emit-legacy-interface-invokers`:
when *not* specified, interface invokers will now use
`JniPeerMembers` for method lookup and invocation, allowing
`jmethodID` values to be cached *across* instances.  In order to
prevent the runtime crash, an interface may have *multiple*
`JniPeerMembers` values, one per implemented interface, which is used
to invoke methods from that interface.

`IWindowManagerInvoker` now becomes:

	internal partial class IWindowManagerInvoker : Java.Lang.Object, IWindowManager {
	    static readonly JniPeerMembers _members_android_view_ViewManager    = …;
	    static readonly JniPeerMembers _members_android_view_WindowManager  = …;

	    public void AddView(View view, ViewGroup.LayoutParams @params)
	    {
	        const string __id = "addView.…";
	        _members_android_view_ViewManager.InstanceMethods.InvokeAbstractVoidMethod (__id, this, …);
	    }

	    public void RemoveViewImmediate(View view)
	    {
	        const string __id = "removeViewImmediate.…";
	        _members_android_view_WindowManager.InstanceMethods.InvokeAbstractVoidMethod (__id, this, …);
	    }
	}

This has two advantages:

 1. More caching!
 2. Desktop `Java.Base` binding can now have interface invokers.

Update `tests/generator-Tests` expected output.
Note: to keep this patch smaller, JavaInterop1 output uses the
new pattern, and only *some* XAJavaInterop1 tests use the new
pattern.

Added [CS0114][0] to `$(NoWarn)` in `Java.Base.csproj` to ignore
warnings such as:

	…/src/Java.Base/obj/Debug-net7.0/mcw/Java.Lang.ICharSequence.cs(195,25): warning CS0114:
	'ICharSequenceInvoker.ToString()' hides inherited member 'Object.ToString()'. To make the current member override that implementation, add the override keyword. Otherwise add the new keyword.

[Ignoring CS0114 is also done in `Mono.Android.dll` as well][1], so
this is not a new or unique requirement.

Update `Java.Interop.dll` so that
`JniRuntime.JniValueManager.GetActivationConstructor()` now knows
about and looks for `*Invoker` types, then uses the activation
constructor from the `*Invoker` type when the source type is an
abstract `class` or `interface`.

Update `tests/Java.Base-Tests` to test for implicit `*Invoker` lookup
and invocation support.


~~ Property Setters ~~

While testing on dotnet/android#8339, we hit this error
(among others, to be addressed later):

	src/Mono.Android/obj/Debug/net8.0/android-34/mcw/Android.Views.IWindowInsetsController.cs(304,41): error CS0103: The name 'behavior' does not exist in the current context

This was caused because of code such as:

	public partial interface IWindowInsetsController {
	    public unsafe int SystemBarsBehavior {
	        get {
	            const string __id = "getSystemBarsBehavior.()I";
	            try {
	                var __rm = _members_IWindowInsetsController.InstanceMethods.InvokeAbstractInt32Method (__id, this, null);
	                return __rm;
	            } finally {
	            }
	        }
	        set {
	            const string __id = "setSystemBarsBehavior.(I)V";
	            try {
	                JniArgumentValue* __args = stackalloc JniArgumentValue [1];
	                __args [0] = new JniArgumentValue (behavior);
	                _members_IWindowInsetsController.InstanceMethods.InvokeAbstractVoidMethod (__id, this, __args);
	            } finally {
	            }
	        }
	    }
	}

This happened because when emitting the property setter, we need
to update the `set*` method's parameter name to be `value` so that
the normal property setter body is emitted properly.

Update `InterfaceInvokerProperty.cs` so that the parameter name
is set to `value`.


~~ Performance ~~

What does this do for performance?

Add a new `InterfaceInvokerTiming` test fixture to
`Java.Interop-PerformanceTests.dll`, which:

 1. "Reimplements" the "legacy" and "JniPeerMembers" Invoker
    strategies
 2. For each Invoker strategy:
     a. Invokes a Java method which returns a `java.lang.Runnable`
        instance
     b. Invokes `Runnable.run()` on the instance returned by (2.a)
        …100 times.
     c. Repeat (2.a) and (2.b) 100 times.

The result is that using `JniPeerMembers` is *much* faster:

	% dotnet build tests/Java.Interop-PerformanceTests/*.csproj && \
		dotnet test --logger "console;verbosity=detailed"  bin/TestDebug-net7.0/Java.Interop-PerformanceTests.dll --filter "Name~InterfaceInvokerTiming"
	…
	 Passed InterfaceInvokers [1 s]
	 Standard Output Messages:
	## InterfaceInvokers Timing: instanceIds: 00:00:01.1095502
	## InterfaceInvokers Timing: peerMembers: 00:00:00.1400427

Using `JniPeerMembers` takes ~1/8th the time as using `jmethodID`s.

TODO: something is *probably* wrong with my test -- reviews welcome!
-- as when I increase the (2.b) iteration count, the `peerMembers`
time is largely unchanged (~0.14s), while the `instanceIds` time
increases linearly.

*Something* is wrong there.  I'm not sure what.  (Or *nothing* is
wrong, and instance `jmethodID` are just *that* bad.)


[0]: https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0114
[1]: https://github.com/xamarin/xamarin-android/blob/d5c4ec09f7658428a10bbe49c8a7a3eb2f71cb86/src/Mono.Android/Mono.Android.csproj#L12C7-L12C7
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Oct 26, 2023
Fixes: dotnet/java-interop#910
Fixes: dotnet/java-interop#1139

Changes: dotnet/java-interop@3c83179...1adb796

  * dotnet/java-interop@1adb7964: [generator] `generator --lang-features=emit-legacy-interface-invokers` (dotnet/java-interop#1145)
  * dotnet/java-interop@6bd7ae48: [Xamarin.Android.Tools.Bytecode] Kotlin internal prop visibility (dotnet/java-interop#1151)

Updates `class-parse` to better support parsing the visibility of
Kotlin properties.

Updates `generator` to emit optimized interface Invokers.

Add support for a new `$(AndroidEmitLegacyInterfaceInvokers)` MSBuild
property; when `true`, causes `generator` to emit legacy interface
Invokers.  The default value is `false`.
jonpryor added a commit to dotnet/android that referenced this issue Nov 7, 2023
Fixes: dotnet/java-interop#910
Fixes: dotnet/java-interop#1139

Changes: dotnet/java-interop@3c83179...38c8a82

  * dotnet/java-interop@38c8a827: [Xamarin.Android.Tools.Bytecode] Kotlin unsigned internal props (dotnet/java-interop#1156)
  * dotnet/java-interop@1adb7964: [generator] `generator --lang-features=emit-legacy-interface-invokers` (dotnet/java-interop#1145)
  * dotnet/java-interop@6bd7ae48: [Xamarin.Android.Tools.Bytecode] Kotlin internal prop visibility (dotnet/java-interop#1151)

Updates `class-parse` to better support parsing the visibility of
Kotlin properties.

Updates `generator` to emit optimized interface Invokers.
The previous interface invoker codegen strategy can be enabled by
setting `$(_AndroidEmitLegacyInterfaceInvokers)`=True.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
2 participants