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.

Sign up for GitHub

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

Jump to bottom

Bump to xamarin/java.interop/main@1adb7964 #8339

Merged
merged 4 commits into from
Nov 7, 2023
Merged

Bump to xamarin/java.interop/main@1adb7964 #8339

merged 4 commits into from
Nov 7, 2023

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Sep 15, 2023 •
edited by jpobst
Loading

Fixes: dotnet/java-interop#910
Fixes: dotnet/java-interop#1139

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

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 jonpryor mentioned this pull request Sep 15, 2023
Merged
jonpryor added a commit to dotnet/java-interop that referenced this pull request Oct 23, 2023
@jonpryor
Fix property setter generation
7f77a70 
Context: dotnet/android#8339

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`.

Update `tests/generator-Tests/Integration-Tests/Interfaces.cs`
so that we test interface generation for JavaInterop1.

Update `tests/generator-Tests/SupportFiles/*.cs` so that
`Interfaces.cs` test output can be compiled.

Update `tests/generator-Tests/expected.ji/TestInterface/TestInterface.xml`
so that some parameter names for `set*()` methods are *not* already
named `value`, to induce the original error.

Flush `tests/generator-Tests/expected.ji/TestInterface`.
@jonpryor jonpryor force-pushed the jonp-try-ji-1145 branch 4 times, most recently from a5fb605 to 4cc2be9 Compare October 25, 2023 20:06
jonpryor added a commit to dotnet/java-interop that referenced this pull request Oct 26, 2023
@jonpryor
[generator] generator --lang-features=emit-legacy-interface-invokers (
1adb796 
#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
Bump to xamarin/java.interop/main@1adb7964
f202e3b 
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 jonpryor changed the title Try xamarin/java.interop#1145 Bump to xamarin/java.interop/main@1adb7964 Oct 26, 2023
@jonpryor jonpryor marked this pull request as ready for review October 26, 2023 18:15
@jpobst
Copy link
Contributor

jpobst commented Oct 26, 2023

I think we should make $(AndroidEmitLegacyInterfaceInvokers) temporary somehow, so we don't end up maintaining it forever.

Perhaps making it $(_AndroidEmitLegacyInterfaceInvokers) and documenting that we expect it to be temporary and you should file an issue if you find yourself needing it. (Or not really documenting it at all, and only pointing people to it if they raise an issue.)

See #6436.

Since this one also hasn't been removed, this might bring up a larger point: we should probably maintain a wiki page of planned future deprecations and removals. We have many instances where we add a new mechanism with the intent to remove the old one once we are sure it works fine, and then we never go back and remove the old one.

@jonpryor
Copy link
Member Author

Made $(AndroidEmitLegacyInterfaceInvokers) a private $(_AndroidEmitLegacyInterfaceInvokers).

@jonpryor
Copy link
Member Author

@jpobst: I think dotnet/java-interop@6bd7ae48 broke the build; the macOS > Tests > APKs .NET > run Xamarin.Android.JcwGen_Tests job is failing to build:

…/Xamarin.Android.JcwGen-Tests/KotlinUnsignedTypesTests.cs(20,4): error CS0200: Property or indexer 'UnsignedInstanceMethods.UnsignedInstanceProperty' cannot be assigned to -- it is read only
…/Xamarin.Android.JcwGen-Tests/KotlinUnsignedTypesTests.cs(26,4): error CS0200: Property or indexer 'UnsignedInstanceMethods.UshortInstanceProperty' cannot be assigned to -- it is read only
…/Xamarin.Android.JcwGen-Tests/KotlinUnsignedTypesTests.cs(32,4): error CS0200: Property or indexer 'UnsignedInstanceMethods.UlongInstanceProperty' cannot be assigned to -- it is read only
…/Xamarin.Android.JcwGen-Tests/KotlinUnsignedTypesTests.cs(38,4): error CS0200: Property or indexer 'UnsignedInstanceMethods.UbyteInstanceProperty' cannot be assigned to -- it is read only
…/Xamarin.Android.JcwGen-Tests/KotlinUnsignedTypesTests.cs(68,4): error CS0200: Property or indexer 'UnsignedInterfaceImplementedMethods.UnsignedInterfaceProperty' cannot be assigned to -- it is read only
…/Xamarin.Android.JcwGen-Tests/KotlinUnsignedTypesTests.cs(82,4): error CS0200: Property or indexer 'UnsignedAbstractImplementedMethods.UnsignedAbstractClassProperty' cannot be assigned to -- it is read only

@jonpryor
Copy link
Member Author

jonpryor commented Nov 7, 2023 •
edited
Loading

The test failures are in UpdateLayoutIdIsIncludedInDesigner(True):

/Users/runner/work/1/a/TestRelease/11-06_20.17.43/temp/UpdateLayoutIdIsIncludedInDesignerTrue Some Space/Resource.designer.cs and /Users/runner/work/1/s/bin/TestRelease/Expected/GenerateDesignerFileExpected.cs do not match.

These failures are also present on main, e.g. fff1f4c: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8638656&view=ms.vss-test-web.build-test-results-tab&runId=88047634&resultId=100092&paneView=debug

Ignoring these failures.

@jonpryor jonpryor closed this Nov 7, 2023
@jonpryor jonpryor reopened this Nov 7, 2023
@jonpryor jonpryor merged commit 8928f11 into dotnet:main Nov 7, 2023
26 of 47 checks passed
@jonpryor jonpryor mentioned this pull request Nov 7, 2023
Closed
grendello added a commit that referenced this pull request Nov 7, 2023
@grendello
Merge branch 'main' into dev/grendel/blobs-in-lib
4f46c74 
* main:
  Bump to xamarin/java.interop/main@38c8a827 (#8339)
@jpobst jpobst mentioned this pull request Nov 16, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@jpobst jpobst jpobst approved these changes

@dellis1972 dellis1972 Awaiting requested review from dellis1972 dellis1972 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kotlin: public set of private var shouldn't be eliminated Optimize Interface Invokers
3 participants
@jonpryor @jpobst @jonathanpeppers