[Java.Runtime.Environment] Partial support for .NET Core #804

jonpryor · 2021-02-18T22:47:38Z

Enable C#8 Nullable Reference Types for
Java.Runtime.Environment.dll.

Add partial support for a "non-bridged backend", so that a
JniRuntime.JniValueManager exists for .NET Core. This new
"managed" backed is used if the Mono runtime is not used.

To work, ManagedValueManager holds strong references to
IJavaPeerable instances. As such, tests which required the use of
GC integration are now "optional", conditional on the
!NO_GC_BRIDGE_SUPPORT define.

The ManagedValueManager.CollectPeers() method calls
IJavaPeerable.Dispose() on all currently referenced peers, then
stops referencing the managed peers.
This causes all GREFs to be dropped, allowing Java peers to be
collected, and then allows the .NET GC to collect the IJavaPeerable
values.

Any and all exceptions thrown by IJavaPeerable.Dispose() are
caught and re-thrown by an AggregateException.

Update Java.Interop-Tests.csproj to define NO_GC_BRIDGE_SUPPORT
and NO_MARSHAL_MEMBER_BUILDER_SUPPORT when building for .NET Core.
This excludes all currently "troublesome"/non-passing tests.

These changes allow all remaining Java.Interop-Tests unit tests to
execute under .NET Core:

    % dotnet test -v diag '--logger:trx;verbosity=detailed' bin/TestDebug-netcoreapp3.1/Java.Interop-Tests.dll
    Passed!  - Failed:     0, Passed:   617, Skipped:     1, Total:   618, Duration: 1 s

Other changes:

The attempt to retain useful Java-side exceptions in 89a5a22
proved to be incomplete. Add a comment to invoke
JNIEnv::ExceptionDescribe(). We don't always want this
to be present, but when we do want it…
While NO_MARSHAL_MEMBER_BUILDER_SUPPORT is set -- which means
that Java.Interop.Export-related tests aren't run -- there are
some fixes for Java.Interop.Export & related unit tests for
.NET Core, to avoid the use of generic delegate types and to
avoid a Type.GetType() which is no longer needed.

jonpryor · 2021-02-18T22:49:29Z

@radekdoulik , @jpobst : thoughts on the new API additions? Are current ReleasePeers() semantics a good idea, in which the peers are simply "released" (GREFs freed, a managed root removed) w/ no notification to the peers?

Or should we treat it "as if" they were disposed or finalized?

jonpryor · 2021-02-19T00:51:10Z

Related discussion & elaboration: #426 (comment)

jonpryor · 2021-02-19T01:02:55Z

@jpobst: if you know how to filter a YAML -task so that it only runs on macOS hosts, we can enable the Java.Interop-Tests.dll run for dotnet test. Otherwise we'll need to wait until we have a Windows build of src/java-interop…

(…which shouldn't be that hard. We'll just want to write a CMakeLists.txt for it and require cmake to build…)

jonpryor · 2021-02-19T16:18:48Z

Design thoughts…

Instead of a bool PeersRequireRelease {get;} property, perhaps it should be an enum of some form?

ReleasePeers() doesn't need to be the abstraction; it could be implemented in terms of GetSurfacedPeers(). It could be an extension method. (Not that it should, but it could…)

Additionally, ReleasePeers() is too much.

A scenario behind #426 was e.g. to do a "C# plugin for a machine learning pipeline": have Tensorflow+Java installed on a server, write a plugin for it within C#, and then process lots of images in a pipeline with the C# plugin.

In such an environment, the normal GC bridge isn't necessarily required: between each image you can "release everything" and do some GC's so that you don't leak.

My suspicion, though, is that you wouldn't want to release everything. You'd just want to release "everything created since some well-defined point". This would allow one to initialize things once, get a good "starting state", and then process the pipeline, without needn't to re-initialize between each item.

ReleasePeers() does not work with such a construct. It nukes everything.

I think we need a better abstraction than what is proposed here. Probably something "stack-like".

Context: #804 Two changes within commit 89a5a22 allow `dotnet test` to be used to run the net472 mono-required NUnit tests: 1. The `java-interop` native library is reliably copied to the `$(OutputPath)` of referencing projects. This ensures that e.g. `bin/TestDebug/libjava-interop.dylib` exists without extra `make` commands or an `msbuild /t:RunTests` invocation. 2. More importantly, `tests/TestJVM` was updated to use `Xamarin.Android.Tools.AndroidSdk.dll` along with `JdkInfo.GetKnownSystemJdkInfos()`. (2) means that the `JI_JVM_PATH` environment variable doesn't *need* to be set before running unit tests, and is largely responsible for allowing `dotnet test` to work: % make prepare all % dotnet test bin/TestDebug/Java.Interop-Tests.dll … Passed! - Failed: 0, Passed: 631, Skipped: 1, Total: 632, Duration: 686 ms Note that these tests are *not* executed under .NET Core/CoreCLR! It's simply using `dotnet test` as an equivalent to/replacement for `mono nunit3-console.exe`. Java.Interop-Tests to be run under .NET Core are built into e.g. `bin/TestDebug-netcoreapp3.1/Java.Interop-Tests.dll`, and trying to run *that* test suite crashes and burns: % dotnet test bin/TestDebug-netcoreapp3.1/Java.Interop-Tests.dll … Failed! - Failed: 16, Passed: 325, Skipped: 1, Total: 342, Duration: 311 ms See e.g. PR #804 for an attempt to make .NET Core support work.

build-tools/automation/templates/core-tests.yaml

jonpryor · 2021-02-20T22:54:53Z

In the interest of expediency, I've removed API additions which attempt to fix Issue #426. What remains is "potentially weird" -- the JniRuntime.JniValueManager.CollectPeers() override in ManagedValueManager releases all values -- the result of which is that three tests are disabled if NO_GC_BRIDGE_SUPPORT is defined, which is the case when building for netcoreapp3.1. (This is in addition to the "original" inclusion of NO_MARSHAL_MEMBER_BUILDER_SUPPORT, so I could worry about fixing Java.Interop.Export "later".)

There is thus no API breakage, and no new semantics to review here. Issue #426 will contain the primary discussion of how to "properly" support a non-bridging GC environment.

jpobst

It looks fine to me, though not really my area of expertise. Hopefully Radek can review better than me!

jpobst · 2021-02-22T19:06:38Z

src/Java.Interop/Java.Interop/JniEnvironment.Types.cs

@@ -47,6 +48,8 @@ public static unsafe JniObjectReference FindClass (string classname)
 					return r;
 				}

+				// NativeMethods.java_interop_jnienv_exception_describe (info.EnvironmentPointer);


Yes, but not entirely; from the commit message:

The attempt to retain useful Java-side exceptions in 89a5a22
proved to be incomplete. Add a comment to invoke
JNIEnv::ExceptionDescribe(). We don't always want this
to be present, but when we do want it…

radekdoulik · 2021-02-23T15:29:11Z

src/Java.Interop.Export/Java.Interop/MarshalMemberBuilder.cs

@@ -370,7 +370,12 @@ static Expression GetRuntime ()
 			return Expression.Property (null, typeof (JniEnvironment), "Runtime");
 		}

-		static  MethodInfo  FormatterServices_GetUninitializedObject    = Type.GetType ("System.Runtime.Serialization.FormatterServices", throwOnError: true)
+		static  MethodInfo  FormatterServices_GetUninitializedObject    =
+#if NETFRAMEWORK || NET_2_0


#if NETCOREAPP might be simpler here (plus switch the branches). Nothing important though.

When we start targeting .NET 5/6, won't we also need NET? Meaning we'd likewise want #if NETCOREAPP || NET? Which doesn't actually seem simpler…

net5, net6, ... also define NETCOREAPP

src/Java.Interop/Java.Interop/JniEnvironment.Types.cs

radekdoulik · 2021-02-23T15:55:08Z

src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs

+				builder.ValueManager            = builder.ValueManager              ?? new ManagedValueManager ();
+				builder.ObjectReferenceManager  = builder.ObjectReferenceManager    ?? new ManagedObjectReferenceManager (builder.JniGlobalReferenceLogWriter, builder.JniLocalReferenceLogWriter);
+			}
+


Nice. We don't have to create the managers before creating the VM. I like that change.

radekdoulik · 2021-02-23T16:06:30Z

src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs

+			var peers = new List<IJavaPeerable> ();
+
+			lock (RegisteredInstances) {
+				foreach (var ps in RegisteredInstances.Values) {


Here it might be more efficient to do:

List<IDisposable> values; lock (RegisteredInstances) { values = new List<IDisposable> (RegisteredInstances.Values); RegisteredInstances.Clear (); }

and do the rest of cleaning after that? It will also lower time spent in lock.

RegisteredInstances.Values is of type ICollection<List<IJavaPeerable>>, not ICollection<IJavaPeerable>. Thus, new List<IDisposable> (RegisteredInstances.Values) won't work.

I was updating the code from elswhere, so it would need to be new List<List<IJavaPeerable>> (RegisteredInstances.Values), the idea is the same though. To leave the lock block quickly.

src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs

Enable C#8 [Nullable Reference Types][0] for `Java.Runtime.Environment.dll`. Add partial support for a "non-bridged backend", so that a `JniRuntime.JniValueManager` exists for .NET Core. This new "managed" backed is used if the Mono runtime is *not* used. To work, `ManagedValueManager` holds *strong* references to `IJavaPeerable` instances. As such, tests which required the use of GC integration are now "optional", conditional on the `!NO_GC_BRIDGE_SUPPORT` define. The `ManagedValueManager.CollectPeers()` method calls `IJavaPeerable.Dispose()` on all currently referenced peers, then stops referencing the managed peers. This causes all GREFs to be dropped, allowing Java peers to be collected, and then allows the .NET GC to collect the `IJavaPeerable` values. Any and all exceptions thrown by `IJavaPeerable.Dispose()` are caught and re-thrown by an `AggregateException`. Update `Java.Interop-Tests.csproj` to define `NO_GC_BRIDGE_SUPPORT` and `NO_MARSHAL_MEMBER_BUILDER_SUPPORT` when building for .NET Core. This excludes all currently "troublesome"/non-passing tests. These changes allow all remaining `Java.Interop-Tests` unit tests to execute under .NET Core: % dotnet test -v diag '--logger:trx;verbosity=detailed' bin/TestDebug-netcoreapp3.1/Java.Interop-Tests.dll Passed! - Failed: 0, Passed: 617, Skipped: 1, Total: 618, Duration: 1 s Other changes: * The attempt to retain useful Java-side exceptions in 89a5a22 proved to be incomplete. Add a comment to invoke [`JNIEnv::ExceptionDescribe()`][1]. We don't always want this to be present, but when we do want it… * While `NO_MARSHAL_MEMBER_BUILDER_SUPPORT` is set -- which means that `Java.Interop.Export`-related tests aren't run -- there are some fixes for `Java.Interop.Export` & related unit tests for .NET Core, to avoid the use of generic delegate types and to avoid a `Type.GetType()` which is no longer needed. [0]: https://docs.microsoft.com/dotnet/csharp/nullable-references [1]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#ExceptionDescribe

jonpryor requested review from radekdoulik and jpobst and removed request for radekdoulik February 18, 2021 22:47

jonpryor mentioned this pull request Feb 19, 2021

Reference Counting, non-bridged backend #426

Open

jonpryor force-pushed the jonp-jre-dotnet-managers branch from 98e0288 to 3bd2bb1 Compare February 19, 2021 00:57

jonpryor added the do-not-merge label Feb 19, 2021

jonpryor force-pushed the jonp-jre-dotnet-managers branch from 3bd2bb1 to f77edab Compare February 19, 2021 23:05

jpobst reviewed Feb 20, 2021

View reviewed changes

build-tools/automation/templates/core-tests.yaml Show resolved Hide resolved

jonpryor force-pushed the jonp-jre-dotnet-managers branch from f77edab to 4d281fb Compare February 20, 2021 22:50

jonpryor changed the title ~~[Java.Runtime.Environment] Support .NET Core~~ [Java.Runtime.Environment] Partial support for .NET Core Feb 20, 2021

jonpryor removed the do-not-merge label Feb 20, 2021

jonpryor requested a review from jpobst February 20, 2021 22:55

jonpryor force-pushed the jonp-jre-dotnet-managers branch from 4d281fb to 3d6166b Compare February 20, 2021 22:57

jpobst approved these changes Feb 22, 2021

View reviewed changes

radekdoulik reviewed Feb 23, 2021

View reviewed changes

src/Java.Interop/Java.Interop/JniEnvironment.Types.cs Show resolved Hide resolved

jonpryor force-pushed the jonp-jre-dotnet-managers branch from 3d6166b to 203b18a Compare February 23, 2021 15:47

radekdoulik reviewed Feb 23, 2021

View reviewed changes

src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs Show resolved Hide resolved

jonpryor force-pushed the jonp-jre-dotnet-managers branch from 203b18a to 33da2df Compare February 23, 2021 16:50

jonpryor force-pushed the jonp-jre-dotnet-managers branch from 33da2df to 763963a Compare February 23, 2021 17:40

jonpryor merged commit a666a6f into dotnet:main Feb 23, 2021

jpobst added this to the 11.3 (16.10 / 8.10) milestone Mar 1, 2021

github-actions bot locked and limited conversation to collaborators Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java.Runtime.Environment] Partial support for .NET Core #804

[Java.Runtime.Environment] Partial support for .NET Core #804

jonpryor commented Feb 18, 2021 •

edited

Loading

jonpryor commented Feb 18, 2021

jonpryor commented Feb 19, 2021

jonpryor commented Feb 19, 2021

jonpryor commented Feb 19, 2021

jonpryor commented Feb 20, 2021

jpobst left a comment

jpobst Feb 22, 2021

jonpryor Feb 22, 2021

radekdoulik Feb 23, 2021

jonpryor Feb 23, 2021

radekdoulik Feb 23, 2021

radekdoulik Feb 23, 2021

radekdoulik Feb 23, 2021

jonpryor Feb 23, 2021

radekdoulik Feb 23, 2021

[Java.Runtime.Environment] Partial support for .NET Core #804

[Java.Runtime.Environment] Partial support for .NET Core #804

Conversation

jonpryor commented Feb 18, 2021 • edited Loading

jonpryor commented Feb 18, 2021

jonpryor commented Feb 19, 2021

jonpryor commented Feb 19, 2021

jonpryor commented Feb 19, 2021

jonpryor commented Feb 20, 2021

jpobst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpryor commented Feb 18, 2021 •

edited

Loading