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

[Java.Base-Tests] Test Java-to-Managed invocations for Java.Base #975

Merged
merged 1 commit into from
May 5, 2022

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented May 2, 2022

Commit bc5bcf4 (Desktop Java.Base binding) had a TODO:

~~ TODO: Marshal Methods ~~

Marshal methods are currently skipped. Java-to-managed invocations
are not currently supported.

No marshal methods means no facility for Java-to-managed invocations.

Add a new tests/Java.Base-Tests unit test assembly, and test
Java-to-managed invocations.

This requires touching and updating ~everything. 😅

Update Java.Interop.dll to contain a new
Java.Interop.JniMethodSignatureAttribute custom attribute.

Update generator to begin emitting [JniMethodSignature] on bound
methods. This is necessary so that jcw-gen knows the JNI method
signature of Java methods to emit. As part of this, remove the
JniTypeSignatureAttr type added in bc5bcf4; trying to follow the
JniTypeSignatureAttr pattern for JniMethodSignatureAttr would
make for more intrusive code changes. Instead, "re-use" the existing
RegisterAttr type, adding a RegisterAttr.MemberType property so
that we can select between Android [Register] output vs. "Desktop"
[JniTypeSignature] and [JniMethodSignature] output. This in turn
requires updating many generator-Tests artifacts.

Update Java.Interop.Tools.JavaCallableWrappers to look for
Java.Interop.JniTypeSignatureAttribute and
Java.Interop.JniMethodSignatureAttribute. This allows
jcw-gen Java.Base-Tests.dll to generate Java Callable Wrappers for
Java.BaseTests.MyRunnable.

Update Java.Interop.Export.dll so that MarshalMemberBuilder
doesn't use Expression.GetActionType() or
Expression.GetFuncType(), as .NET doesn't support the use of
generic types in Marshal.GetFunctionPointerForDelegate().
Instead, we need to use System.Reflection.Emit to define our own
custom delegate types.

Comparison with Android

Android bindings (both Classic Xamarin.Android and
.NET SDK for Android) use generator-emitted "connector methods"
which are specified in the [Register] attribute (see also 99897b2):

namespace Java.Lang {
    [Register ("java/lang/Object" DoNotGenerateAcw=true)]
    partial class Object {
        [Register ("equals", "(Ljava/lang/Object;)Z", "GetEquals_Ljava_lang_Object_Handler")]
        public virtual unsafe bool Equals (Java.Lang.Object? obj)
            => …

        static Delegate GetEquals_Ljava_lang_Object_Handler()
            => JNINativeWrapper.CreateDelegate((_JniMarshal_PPL_Z) n_Equals_Ljava_lang_Object_);
        static bool n_Equals_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native_obj)
            => …
    }
}

jcw-gen emits the "connector method" into the resulting
Java Callable Wrappers for appropriate subclasses:

String __md_methods =
    "n_equals:(Ljava/lang/Object;)Z:GetEquals_Ljava_lang_Object_Handler\n";

At runtime, AndroidTypeManager.RegisterNativeMembers() will lookup
Object.GetEquals_Ljava_lang_Object_Handler() and invoke it.
The returned Delegate instance is provided to
JNIEnv::RegisterNatives().

The problem with this approach is that it's inflexible: there is no
way to participate in the "connector method" infrastructure to alter
marshaling behavior. If you need custom behavior, e.g.
Android.Graphics.Color customizations, you need to update
generator and rebuild the binding assemblies.

(This has been "fine" for the past 10+ years, so this hasn't been a
deal breaker.)

For Java.Base, @jonpryor wants to support the custom marshaling
infrastructure introduced in 77a6bf8. This would allow types to
participate in JNI marshal method ("connector method") generation
at runtime, allowing specialization based on the current set of
types and assemblies.

This means we don't need to specify a "connector method" in
[JniMethodSignatureAttribute], nor generate them.

namespace Java.Lang {
    [JniTypeSignatureAttribute("java/lang/Object", GenerateJavaPeer=false)]
    partial class Object {
        [JniMethodSignatureAttribute("equals", "(Ljava/lang/Object;)Z")]
        public virtual unsafe bool Equals (Java.Lang.Object? obj)
            => …

        // No GetEquals_Ljava_lang_Object_Handler()
        // No n_Equals_Ljava_lang_Object_()
    }
}

This should result in smaller binding assemblies.

The downside is that runtime costs increase, significantly.
Instead of looking up and invoking a "connector method", the method
must be generated via System.Linq.Expressions expression trees,
then compiled into IL, then JIT'd, all before it can be used.

We have no timing data to indicate how "bad" this overhead will be.

As always, the hope is that tools/jnimarshalmethod-gen (176240d)
can be used to get the "best of both worlds": the flexibility of
custom marshalers, without the runtime overhead. But…

At this point in time, jnimarshalmethod-gen cannot work under
.NET 6. This will need to be addressed in order for custom marshalers
to be a useful solution.

Java.Base will use "connector method"-less bindings to act as a
"forcing function" in getting jnimarshalmethod-gen working in .NET.

Comment on lines +40 to +42
{ "codegen-target=",
"STYLE of Java Callable Wrappers to generate",
(JavaPeerStyle? v) => style = v.HasValue ? v.Value : style },
Copy link
Member

@jonathanpeppers jonathanpeppers May 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the switch here have a name like --java-peer-style=?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent was to be consistent with generator, so you would use generator --codegen-target=JavaInterop1 … ; jcw-gen --codegen-target=JavaInterop1 ….

That might not actually be useful, especially considering that MSBuild tasks should (eventually) be provided, not command-line tools…

tests/Java.Base-Tests/Java.Base-Tests.csproj Outdated Show resolved Hide resolved
tests/Java.Base-Tests/Java.Base-Tests.targets Outdated Show resolved Hide resolved
tests/Java.Base-Tests/Java.Base/JavaVMFixture.cs Outdated Show resolved Hide resolved
tools/jcw-gen/App.cs Outdated Show resolved Hide resolved
@jonpryor jonpryor force-pushed the jonp-java.base-j2m branch 4 times, most recently from bab160e to fc0f953 Compare May 4, 2022 18:42
jonpryor added a commit to dotnet/android that referenced this pull request May 4, 2022
@jonpryor
Copy link
Member Author

jonpryor commented May 4, 2022

…and to verify that I didn't accidentally break Xamarin.Android: dotnet/android#6984

@jonpryor
Copy link
Member Author

jonpryor commented May 4, 2022

Of particular note is that this change updates XAJavaInterop1
output to have RegisterAttribute.DoNotGenerateAcw=true on bound
interfaces

…which promptly broke the xamarin-android build: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6110214&view=logs&jobId=a1070d6b-7539-53cb-46be-b3395553b4a7&j=96fd57f5-f69e-53c7-3d47-f67e6cf9b93e&t=65256bb7-a34c-5353-bc4d-c02ee25dc133

…
error : CannotChangeAttribute : Attribute 'Android.Runtime.RegisterAttribute' on 'Org.W3c.Dom.INamedNodeMap' changed from '[RegisterAttribute("org/w3c/dom/NamedNodeMap", "", "Org.W3c.Dom.INamedNodeMapInvoker")]' in the contract to '[RegisterAttribute("org/w3c/dom/NamedNodeMap", "", "Org.W3c.Dom.INamedNodeMapInvoker", DoNotGenerateAcw=true)]' in the implementation.
…
error : Total Issues: 969 

Looks like I can't make that change.

@jonpryor jonpryor force-pushed the jonp-java.base-j2m branch 2 times, most recently from 0399f91 to 538f429 Compare May 4, 2022 19:45
jonpryor added a commit to dotnet/android that referenced this pull request May 4, 2022
Commit bc5bcf4 (Desktop `Java.Base` binding) had a TODO:

> ~~ TODO: Marshal Methods ~~
>
> Marshal methods are currently skipped.  Java-to-managed invocations
> are not currently supported.

No marshal methods means no facility for Java-to-managed invocations.

Add a new `tests/Java.Base-Tests` unit test assembly, and test
Java-to-managed invocations.

This requires touching and updating ~everything. 😅

Update `Java.Interop.dll` to contain a new
`Java.Interop.JniMethodSignatureAttribute` custom attribute.

Update `generator` to begin emitting `[JniMethodSignature]` on bound
methods.  This is necessary so that `jcw-gen` knows the JNI method
signature of Java methods to emit.  As part of this, *remove* the
`JniTypeSignatureAttr` type added in bc5bcf4; trying to follow the
`JniTypeSignatureAttr` pattern for `JniMethodSignatureAttr` would
make for more intrusive code changes.  Instead, "re-use" the existing
`RegisterAttr` type, adding a `RegisterAttr.MemberType` property so
that we can select between Android `[Register]` output vs. "Desktop"
`[JniTypeSignature]` and `[JniMethodSignature]` output.  This in turn
requires updating many `generator-Tests` artifacts.

Update `Java.Interop.Tools.JavaCallableWrappers` to look for
`Java.Interop.JniTypeSignatureAttribute` and
`Java.Interop.JniMethodSignatureAttribute`.  This allows
`jcw-gen Java.Base-Tests.dll` to generate Java Callable Wrappers for
`Java.BaseTests.MyRunnable`.

Update `Java.Interop.Export.dll` so that `MarshalMemberBuilder`
doesn't use `Expression.GetActionType()` or
`Expression.GetFuncType()`, as .NET doesn't support the use of
generic types in [`Marshal.GetFunctionPointerForDelegate()`][0].
Instead, we need to use `System.Reflection.Emit` to define our own
custom delegate types.

~~ Comparison with Android ~~

Android bindings (both Classic Xamarin.Android and
.NET SDK for Android) use `generator`-emitted "connector methods"
which are specified in the `[Register]` attribute (see also 99897b2):

	namespace Java.Lang {
	    [Register ("java/lang/Object" DoNotGenerateAcw=true)]
	    partial class Object {
	        [Register ("equals", "(Ljava/lang/Object;)Z", "GetEquals_Ljava_lang_Object_Handler")]
	        public virtual unsafe bool Equals (Java.Lang.Object? obj)
	            => …

	        static Delegate GetEquals_Ljava_lang_Object_Handler()
	            => JNINativeWrapper.CreateDelegate((_JniMarshal_PPL_Z) n_Equals_Ljava_lang_Object_);
	        static bool n_Equals_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native_obj)
	            => …
	    }
	}

`jcw-gen` emits the "connector method" into the resulting
Java Callable Wrappers for appropriate subclasses:

	String __md_methods =
	    "n_equals:(Ljava/lang/Object;)Z:GetEquals_Ljava_lang_Object_Handler\n";

At runtime, `AndroidTypeManager.RegisterNativeMembers()` will lookup
`Object.GetEquals_Ljava_lang_Object_Handler()` and invoke it.
The returned `Delegate` instance is provided to
`JNIEnv::RegisterNatives()`.

The problem with this approach is that it's inflexible: there is no
way to participate in the "connector method" infrastructure to alter
marshaling behavior.  If you need custom behavior, e.g.
`Android.Graphics.Color` customizations, you need to update
`generator` and rebuild the binding assemblies.

(This has been "fine" for the past 10+ years, so this hasn't been a
deal breaker.)

For `Java.Base`, @jonpryor wants to support the custom marshaling
infrastructure introduced in 77a6bf8.  This would allow types to
participate in JNI marshal method ("connector method") generation
*at runtime*, allowing specialization based on the current set of
types and assemblies.

This means we *don't* need to specify a "connector method" in
`[JniMethodSignatureAttribute]`, nor generate them.

	namespace Java.Lang {
	    [JniTypeSignatureAttribute("java/lang/Object", GenerateJavaPeer=false)]
	    partial class Object {
	        [JniMethodSignatureAttribute("equals", "(Ljava/lang/Object;)Z")]
	        public virtual unsafe bool Equals (Java.Lang.Object? obj)
	            => …

	        // No GetEquals_Ljava_lang_Object_Handler()
	        // No n_Equals_Ljava_lang_Object_()
	    }
	}

This should result in smaller binding assemblies.

The downside is that runtime costs *increase*, significantly.
Instead of looking up and invoking a "connector method", the method
must be *generated* via System.Linq.Expressions expression trees,
then compiled into IL, then JIT'd, all before it can be used.

We have no timing data to indicate how "bad" this overhead will be.

As always, the hope is that `tools/jnimarshalmethod-gen` (176240d)
can be used to get the "best of both worlds": the flexibility of
custom marshalers, without the runtime overhead.  But…

  * dotnet#14
  * dotnet#616

At this point in time, `jnimarshalmethod-gen` *cannot* work under
.NET 6.  This will need to be addressed in order for custom marshalers
to be a useful solution.

`Java.Base` will use "connector method"-less bindings to act as a
"forcing function" in getting `jnimarshalmethod-gen` working in .NET.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.getfunctionpointerfordelegate?view=net-6.0
jonpryor added a commit to jonpryor/java.interop that referenced this pull request May 5, 2022
Fixes: dotnet#962

Context: 3851b1a
Context: dotnet#936
Context: dotnet#975

We have several PRs which impact public API, and we want to land as
part of .NET 7, and *not* as part of .NET 6, e.g. dotnet#936.

Update `GitInfo.txt` to `7.0`, and update `$(_NetCoreLibVersion)` to
be `$(GitBaseVersionMajor).$(GitBaseVersionMinor).0.0` -- reverting
commit 3851b1a -- as an indicator that from this point onward, all
work on `main` is targeting .NET 7.
@@ -113,13 +113,16 @@ public partial class MyClass {

public abstract int AbstractCount {
// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/class[@name='MyClass']/method[@name='get_AbstractCount' and count(parameter)=0]"
[global::Java.Interop.JniMethodSignature ("get_AbstractCount", "()I")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the Linker to remove these attributes? Otherwise the size of binding assemblies may go up, resulting in larger .apks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these attributes were to be used in Android, yes we'd want to remove them.

However, unless someone is using the JavaInterop1 codegen target -- which is highly unlikely, considering it doesn't really work right (as evidenced by the fact that the Java.Base binding is excluding almost everything) -- then there won't be any attributes to remove.

This is thus a more "eventually…" thing to do, rather than something to worry about now. (.NET 8 timeframe? Maybe? Perhaps?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Got confused with XAJavaInterop1.

jonpryor added a commit that referenced this pull request May 5, 2022
Fixes: #962

Context: 3851b1a
Context: #936
Context: #975

We have several PRs which impact public API, and we want to land as
part of .NET 7, and *not* as part of .NET 6, e.g. #936.

Update `GitInfo.txt` to `7.0`, and update `$(_NetCoreLibVersion)` to
be `$(GitBaseVersionMajor).$(GitBaseVersionMinor).0.0` -- reverting
commit 3851b1a -- as an indicator that from this point onward, all
work on `main` is targeting .NET 7.

Note that CI continues to use .NET 6.  This is thus a more
"conceptual" change rather than a "everything requires .NET 7 now"
change.
@jonpryor jonpryor merged commit 4787e01 into dotnet:main May 5, 2022
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants