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

Add a JniEnvironment.BeginGetValueScope() method. #4

Open
jonpryor opened this issue Apr 7, 2015 · 3 comments
Open

Add a JniEnvironment.BeginGetValueScope() method. #4

jonpryor opened this issue Apr 7, 2015 · 3 comments
Labels
enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java

Comments

@jonpryor
Copy link
Member

jonpryor commented Apr 7, 2015

Building upon Issue #3, we expect JavaVM.GetValue() calls to be "buried" within binding code, not directly invoked by developers (most of the time). Thus, how does a developer tell a binding method that a new value should be created instead of retrieving a possibly shared value?

Introduce JniEnvironment.BeginGetValueScope(GetValueScope):

[Flags]
public enum GetValueBehaviors {
    Default                = 0,
    CreateValues           = 1,
    DoNotMarshalExceptions = 2,
}

partial class JniEnvironment {
    public static IDisposable BeginGetValueBehaviors (GetValueBehaviors scope);
}

Calling JniEnviornment.BeginGetValueBehaviors() would alter the behavior of JavaVM.GetValue(): if GetValueBehaviors.CreateValues is specified, then JavaVM.GetValue() will instead behave like JavaVM.CreateValue(). This allows the end user to maintain some degree of control:

using (var scope = JniEnvironment.BeginGetValueBehaviors (GetValueBehaviors.CreateValues))
using (var typeface = Typeface.Create (...)) {
    // use typeface
}

The above allows disposing of the temporary with impunity, as BeginGetValueScope() will ensure that Typeface.Create() returns unique wrappers instead of possibly shared wrappers.

@jonpryor
Copy link
Member Author

Another option for GetValueScope is to inhibit exception marshaling: Xamarin.Forms has a request to just have the app exit if an exception is thrown, because it Can't Happen, and if it does there's nothing reasonable to do.

Consequently, it would be helpful if exception marshaling could be inhibited in a similar manner.

jonpryor added a commit that referenced this issue Sep 26, 2015
Remember commit 775b34f?

> Con: OMFG IS THIS SLOW.
>
> Like, wow, incredibly slow, like ~11s to lookup the names and
> parameter types of all methods on java.util.Arrays.

So... WTF is going on there?

Object identity is what's going on there.

For "sane"/"correct"/"normal" semantics, JavaObjectArray<T> follows
object identity semantics, e.g. JavaVM.GetObject() will always return
the same (registered) wrapper for Java handle that refers to the
"same" Java instance, as per JavaVM.RegisteredInstances.

Supporting Object identity is...not fast. See the new
JavaArrayTiming.ObjectArrayEnumerationTiming() and
MiscRuntimeTiming.ObjectCreationTiming() tests.

ObjectArrayEnumerationTiming() uses Java reflection to grab all of the
methods of java.util.Arrays. The stunning output?

	# methodHandles(JavaObjectArray<JavaObject>) creation timing: 00:00:00.6106453 Count=114
	# methodHandles(JavaVM.GetObject) creation timing: 00:00:00.5959359 Count=114
	# methodHandles(JavaObject[]) creation timing: 00:00:00.0890776 Count=114
	# methodHandles(JniGlobalReference) creation timing: 00:00:00.1536897 Count=114

Given a JNI handle to the java.lang.reflect.Method[] returned by
Class.getMethods(), the above shows four different ways of reading
that Method[] and turning it into a List<Something>.

The JavaObjectArray<JavaObject> implementation takes 0.61 *seconds*
just to grab each element from the JavaObjectArray<JavaObject> and
copy it into a List<JavaObject>. For 115 items.

Which is *deplorable*.

Why is it so bad? Next is an implementation which grabs the JNI handle
for each element in the array and passes it through
JavaVM.GetObject<T>() before adding to a List<JavaObject>.
Note that it's VERY CLOSE in time to the JavaObjectArray<T> version.

The methodHandles(JavaObject[]) version grabs each element from the
array, then immediately constructs a new JavaObject instance around
it. NO OBJECT IDENTITY is involved; aliases can (will) be created, and
that's OKAY (here). Note that we drop from ~0.6s to ~0.089s --
nearly 7x faster, simply by skipping object identity.

The final methodHandles(JniGlobalReference) is the same-but-different
as methodHandles(JavaObject[]): instead of a new JavaObject, it
greates a new JNI Global Reference. (Why a GREF? Futureproof!) It's
slower than JavaObject (must be Local references vs. Global refs), but
still much faster than our object identity infrastructure.

TL;DR: Object identity is problematic.

There is an issue to attempt to address it:

	#4

...but we're not fixing that here.

The ObjectCreationTiming() tests are a different take on the above
results, comparing the timing of JNIEnv::AllocObject(),
JNIEnv::NewObject(), `new JavaObject()`, and
JavaVM.GetObject<JavaObject>():

	## ObjectCreationTiming Timing:
	Total=00:00:35.1368016
	AllocObject=00:00:02.8751501
	NewObject=00:00:02.8724492
	`new JavaObject()`=00:00:06.0586476
	JavaVM.GetObject()=00:00:14.5206758

This somewhat agrees with the previous discussion:
JavaVM.GetObject<T>() (object identity) is slower than direct object
creation.

With that in mind, we can turn our attention back to
Java.Interop.Dynamic and its terrible performance by replacing it's
JavaObjectArray<JavaObject> use with low-level array access.

This in turn necessitates making the JniEnvironment.Arrays type public
(previously `internal`) so that Java.Interop.Dynamic can skip/bypass
the object identity semantics that JavaObjectArray<T> enforces.
This improves things from taking ~11s to lookup the names to taking
~4s to run the entire Java.Interop.Dynamic-Tests assembly.

	Total time           : 3.9523533 seconds

This is much more reasonable. (Perhaps not perfect, but reasonable.)

While investigating, fix a variety of other minor issues:

  * Fix JavaObjectArray<T> so that it doesn't needlessly re-access the
    Length property all the damn time, which SHOULD reduce JNI calls
    and thus improve performance.

  * Fix JavaVM.CreateWrapperType() to not create so many JniType
    instances. Investigation was suggesting that the JniType creation
    and disposal was adding a bit of overhead, so avoiding the JniType
    abstraction provides *some* (small) speedup.

  * Cleanup JniType.Name (commit 9f380eb), moving its logic into
    JniEnvironment.Types.GetJniTypeNameFromClass(). This further
    increases consistency.
jonpryor added a commit to jonpryor/java.interop that referenced this issue Aug 17, 2016
When `JniRuntime.CreationOptions.DestroyRuntimeOnDispose` is true,
`JavaVM::DestroyJavaVM()` will be invoked when the `JniRuntime`
instance is disposed *or* finalized.

`JreRuntime.CreateJreVM()` would *always* set
`DestroyRuntimeOnDispose` to true, because it called
`JNI_CreateJavaVM()`, so *of course* you'd want to destroy the
Java VM, right?

Which brings us to unit tests. I don't know of any "before all test
fixtures run" and "after all test fixtures run" extension points,
which means:

1. The JVM needs to be created implicitly, "on demand."
2. There's no good way to destroy the JVM created in (1) after all
    tests have finished executing.

Which *really* means that the `JreRuntime` instance is *finalized*,
which sets us up for the unholy trifecta of AppDomain unloads,
finalizers, and JVM shutdown:

For unknown reasons, ~randomly, when running the unit tests (e.g.
`make run-tests`), the test runner will *hang*, indefinitely.

Attaching `lldb` and triggering a backtrace shows the unholy trifecta:

Finalization:

	thread dotnet#4: tid = 0x403831, 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10, name = 'tid_1403'
	  ...
	  frame dotnet#10: 0x00000001001ccb4a mono64`mono_gc_run_finalize(obj=<unavailable>, data=<unavailable>) + 938 at gc.c:256 [opt]
	  frame dotnet#11: 0x00000001001cdd4a mono64`finalizer_thread [inlined] finalize_domain_objects + 51 at gc.c:681 [opt]
	  frame dotnet#12: 0x00000001001cdd17 mono64`finalizer_thread(unused=<unavailable>) + 295 at gc.c:730 [opt]

JVM destruction:

	thread dotnet#4: tid = 0x403831, 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10, name = 'tid_1403'
	  frame #0: 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10
	  frame dotnet#1: 0x00007fffa04d4728 libsystem_pthread.dylib`_pthread_cond_wait + 767
	  frame dotnet#2: 0x000000010ba5bc76 libjvm.dylib`os::PlatformEvent::park() + 192
	  frame dotnet#3: 0x000000010ba38e32 libjvm.dylib`ParkCommon(ParkEvent*, long) + 42
	  frame dotnet#4: 0x000000010ba39708 libjvm.dylib`Monitor::IWait(Thread*, long) + 168
	  frame dotnet#5: 0x000000010ba398f0 libjvm.dylib`Monitor::wait(bool, long, bool) + 246
	  frame dotnet#6: 0x000000010bb3dca2 libjvm.dylib`Threads::destroy_vm() + 80
	  frame dotnet#7: 0x000000010b8fd665 libjvm.dylib`jni_DestroyJavaVM + 254

AppDomain unload:

	thread dotnet#37: tid = 0x4038fb, 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10
	  frame #0: 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10
	  frame dotnet#1: 0x00007fffa04d4728 libsystem_pthread.dylib`_pthread_cond_wait + 767
	  frame dotnet#2: 0x0000000100234a7f mono64`mono_os_cond_timedwait [inlined] mono_os_cond_wait(cond=0x0000000102016e50, mutex=0x0000000102016e10) + 11 at mono-os-mutex.h:105 [opt]
	  frame dotnet#3: 0x0000000100234a74 mono64`mono_os_cond_timedwait(cond=0x0000000102016e50, mutex=0x0000000102016e10, timeout_ms=<unavailable>) + 164 at mono-os-mutex.h:120 [opt]
	  frame dotnet#4: 0x0000000100234828 mono64`_wapi_handle_timedwait_signal_handle(handle=0x0000000000000440, timeout=4294967295, alertable=1, poll=<unavailable>, alerted=0x0000700000a286f4) + 536 at handles.c:1554 [opt]
	  frame dotnet#5: 0x0000000100246370 mono64`wapi_WaitForSingleObjectEx(handle=<unavailable>, timeout=<unavailable>, alertable=<unavailable>) + 592 at wait.c:189 [opt]
	  frame dotnet#6: 0x00000001001c832e mono64`mono_domain_try_unload [inlined] guarded_wait(timeout=4294967295, alertable=1) + 30 at appdomain.c:2390 [opt]
	  frame dotnet#7: 0x00000001001c8310 mono64`mono_domain_try_unload(domain=0x000000010127ccb0, exc=0x0000700000a287a0) + 416 at appdomain.c:2482 [opt]
	  frame dotnet#8: 0x00000001001c7db2 mono64`ves_icall_System_AppDomain_InternalUnload [inlined] mono_domain_unload(domain=<unavailable>) + 20 at appdomain.c:2379 [opt]
	  frame dotnet#9: 0x00000001001c7d9e mono64`ves_icall_System_AppDomain_InternalUnload(domain_id=<unavailable>) + 46 at appdomain.c:2039 [opt]

This randomly results in deadlock, and hung Jenkins bots.

Fix this behavior by altering `JreRuntime.CreateJreVM()` to *not*
override the value of
`JniRuntime.CreationOptions.DestroyRuntimeOnDispose`. This allows the
constructor of the `JreRuntime` instance to decide whether or not the
JVM is destroyed.

In the case of TestJVM, we *don't* want to destroy the JVM.

This prevents the JVM from being destroyed, which in turn prevents the
hang during process shutdown.
jonpryor added a commit that referenced this issue Aug 17, 2016
When `JniRuntime.CreationOptions.DestroyRuntimeOnDispose` is true,
`JavaVM::DestroyJavaVM()` will be invoked when the `JniRuntime`
instance is disposed *or* finalized.

`JreRuntime.CreateJreVM()` would *always* set
`DestroyRuntimeOnDispose` to true, because it called
`JNI_CreateJavaVM()`, so *of course* you'd want to destroy the
Java VM, right?

Which brings us to unit tests. I don't know of any "before all test
fixtures run" and "after all test fixtures run" extension points,
which means:

1. The JVM needs to be created implicitly, "on demand."
2. There's no good way to destroy the JVM created in (1) after all
    tests have finished executing.

Which *really* means that the `JreRuntime` instance is *finalized*,
which sets us up for the unholy trifecta of AppDomain unloads,
finalizers, and JVM shutdown:

For unknown reasons, ~randomly, when running the unit tests (e.g.
`make run-tests`), the test runner will *hang*, indefinitely.

Attaching `lldb` and triggering a backtrace shows the unholy trifecta:

Finalization:

	thread #4: tid = 0x403831, 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10, name = 'tid_1403'
	  ...
	  frame #10: 0x00000001001ccb4a mono64`mono_gc_run_finalize(obj=<unavailable>, data=<unavailable>) + 938 at gc.c:256 [opt]
	  frame #11: 0x00000001001cdd4a mono64`finalizer_thread [inlined] finalize_domain_objects + 51 at gc.c:681 [opt]
	  frame #12: 0x00000001001cdd17 mono64`finalizer_thread(unused=<unavailable>) + 295 at gc.c:730 [opt]

JVM destruction:

	thread #4: tid = 0x403831, 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10, name = 'tid_1403'
	  frame #0: 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10
	  frame #1: 0x00007fffa04d4728 libsystem_pthread.dylib`_pthread_cond_wait + 767
	  frame #2: 0x000000010ba5bc76 libjvm.dylib`os::PlatformEvent::park() + 192
	  frame #3: 0x000000010ba38e32 libjvm.dylib`ParkCommon(ParkEvent*, long) + 42
	  frame #4: 0x000000010ba39708 libjvm.dylib`Monitor::IWait(Thread*, long) + 168
	  frame #5: 0x000000010ba398f0 libjvm.dylib`Monitor::wait(bool, long, bool) + 246
	  frame #6: 0x000000010bb3dca2 libjvm.dylib`Threads::destroy_vm() + 80
	  frame #7: 0x000000010b8fd665 libjvm.dylib`jni_DestroyJavaVM + 254

AppDomain unload:

	thread #37: tid = 0x4038fb, 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10
	  frame #0: 0x00007fff9656bdb6 libsystem_kernel.dylib`__psynch_cvwait + 10
	  frame #1: 0x00007fffa04d4728 libsystem_pthread.dylib`_pthread_cond_wait + 767
	  frame #2: 0x0000000100234a7f mono64`mono_os_cond_timedwait [inlined] mono_os_cond_wait(cond=0x0000000102016e50, mutex=0x0000000102016e10) + 11 at mono-os-mutex.h:105 [opt]
	  frame #3: 0x0000000100234a74 mono64`mono_os_cond_timedwait(cond=0x0000000102016e50, mutex=0x0000000102016e10, timeout_ms=<unavailable>) + 164 at mono-os-mutex.h:120 [opt]
	  frame #4: 0x0000000100234828 mono64`_wapi_handle_timedwait_signal_handle(handle=0x0000000000000440, timeout=4294967295, alertable=1, poll=<unavailable>, alerted=0x0000700000a286f4) + 536 at handles.c:1554 [opt]
	  frame #5: 0x0000000100246370 mono64`wapi_WaitForSingleObjectEx(handle=<unavailable>, timeout=<unavailable>, alertable=<unavailable>) + 592 at wait.c:189 [opt]
	  frame #6: 0x00000001001c832e mono64`mono_domain_try_unload [inlined] guarded_wait(timeout=4294967295, alertable=1) + 30 at appdomain.c:2390 [opt]
	  frame #7: 0x00000001001c8310 mono64`mono_domain_try_unload(domain=0x000000010127ccb0, exc=0x0000700000a287a0) + 416 at appdomain.c:2482 [opt]
	  frame #8: 0x00000001001c7db2 mono64`ves_icall_System_AppDomain_InternalUnload [inlined] mono_domain_unload(domain=<unavailable>) + 20 at appdomain.c:2379 [opt]
	  frame #9: 0x00000001001c7d9e mono64`ves_icall_System_AppDomain_InternalUnload(domain_id=<unavailable>) + 46 at appdomain.c:2039 [opt]

This randomly results in deadlock, and hung Jenkins bots.

Fix this behavior by altering `JreRuntime.CreateJreVM()` to *not*
override the value of
`JniRuntime.CreationOptions.DestroyRuntimeOnDispose`. This allows the
constructor of the `JreRuntime` instance to decide whether or not the
JVM is destroyed.

In the case of TestJVM, we *don't* want to destroy the JVM.

This prevents the JVM from being destroyed, which in turn prevents the
hang during process shutdown.
@jpobst jpobst added enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java labels Apr 16, 2020
@jonpryor
Copy link
Member Author

WRT inhibiting exception marshaling, the whole concern was around needing to have two JNI invocations "everywhere", e.g. https://github.com/xamarin/java.interop/blob/main/tests/invocation-overhead/jni.cs#L6095-L6099

(in which JniEnvironment.GetExceptionForLastThrowable() called JNIEnv::ExceptionOccurred() occurred.)

Calling through the delegates is not a "zero-cost" operation, so the idea behind GetValueBehaviors.DoNotMarshalExceptions was as an assertion that "this method will not throw", and thus we could avoid the JNIEnv::ExceptionOccurred() invocation.

However, with the "new" P/Invoke-based system in 926e4bc (not really "new" anymore), JNIEnv::ExceptionOccurred() is done as part of the C layer, not C#+delegates, and is faster. (Still not zero-cost, but faster.)

I thus don't see much need for GetValueBehaviors.DoNotMarshalExceptions.

GetValueBehaviors.CreateValues still seems like a good idea, but needs to be "paired with" a noun to describe existing behavior, e.g. .IdentityValues?

jonpryor added a commit to jonpryor/java.interop that referenced this issue May 5, 2021
Fixes: dotnet#4

Context: dotnet#426

Alternate names?

  * JavaPeerableCleanupPool
  * JavaPeerCleanup
  * JavaReferenceCleanup

Issue dotnet#426 is an idea to implement a *non*-GC-Bridged
`JniRuntime.JniValueManager` type, primarily for use with .NET Core.
This was begun in a666a6f.

What's missing is the answer to a question: what do do about
`JniRuntime.JniValueManager.CollectPeers()`?  With a Mono-style
GC bridge, `CollectPeers()` is `GC.Collect()`.  In a666a6f with
.NET Core, `CollectPeers()` calls `IJavaPeerable.Dispose()` on all
registered instances, which is "extreme".

@jonpryor thought that if there were a *scope-based* way to
selectively control which instances were disposed, that might be
"better" and more understandable.  Plus, this is issue dotnet#4!

Add `JniPeerRegistrationScope`, which introduces a scope-based
mechanism to control when `IJavaPeerable` instances are cleaned up:

	public enum JniPeerRegistrationScopeCleanup {
	    RegisterWithManager,
	    Dispose,
	    Release,
	}

	public ref struct JniPeerRegistrationScope {
	    public JniPeerRegistrationScope(JniPeerRegistrationScopeCleanup cleanup);
	    public void Dispose();
	}

`JniPeerRegistrationScope` is a [`ref struct`][0], which means it can
only be allocated on the runtime stack, ensuring that cleanup
semantics are *scope* semantics.

TODO: is that actually a good idea?

If a `JniPeerRegistrationScope` is created using
`JniPeerRegistrationScopeCleanup.RegisterWithManager`, existing
behavior is followed.  This is useful for nested scopes, should
instances need to be registered with `JniRuntime.ValueManager`.

If a `JniPeerRegistrationScope` is created using `.Dispose` or
`.Release`, then:

 1. Object references created within the scope are "thread-local";
    they can be *used* by other threads, but
    `JniRuntime.JniValueManager.PeekPeer()` won't find existing values.

 2. At the end of a `using` block / when
   `JniPeerRegistrationScope.Dispose()` is called, all collected
    instances will be `Dispose()`d (with `.Dispose`) or released
    (with `.Release`), and left to the GC to eventually finalize.

For example:

	using (new JniPeerRegistrationScope (JniPeerRegistrationScopeCleanup.Dispose)) {
	    var singleton = JavaSingleton.Singleton;
	    // use singleton
	}
	// `singleton.Dispose()` is called at the end of the `using` block

TODO: docs?

TODO: *nested* scopes, and "bound" vs. "unbound" instance construction
around `JniValueManager.GetValue<T>()` or `.CreateValue<T>()`,
and *why* they should be treated differently.

TODO: Should `CreateValue<T>()` be *removed*?  name implies it always
"creates" a new value, but implementation will return existing instances,
so `GetValue<T>()` alone may be better.  One related difference is that`
`CreateValue<T>()` uses `PeekBoxedObject()`, while `GetValue<T>()` doesn't.
*Should* it?

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
jonpryor added a commit to jonpryor/java.interop that referenced this issue Jun 17, 2021
Fixes: dotnet#4

Context: dotnet#426

Alternate names?

  * JavaPeerableCleanupPool
  * JavaPeerCleanup
  * JavaReferenceCleanup
  * JniPeerRegistrationScope

Issue dotnet#426 is an idea to implement a *non*-GC-Bridged
`JniRuntime.JniValueManager` type, primarily for use with .NET Core.
This was begun in a666a6f.

What's missing is the answer to a question: what do do about
`JniRuntime.JniValueManager.CollectPeers()`?  With a Mono-style
GC bridge, `CollectPeers()` is `GC.Collect()`.  In a666a6f with
.NET Core, `CollectPeers()` calls `IJavaPeerable.Dispose()` on all
registered instances, which is "extreme".

@jonpryor thought that if there were a *scope-based* way to
selectively control which instances were disposed, that might be
"better" and more understandable.  Plus, this is issue dotnet#4!

Add `JavaScope`, which introduces a scope-based mechanism to control
when `IJavaPeerable` instances are cleaned up:

	public enum JavaScopeCleanup {
	    RegisterWithManager,
	    Dispose,
	    Release,
	}

	public ref struct JavaScope {
	    public JavaScope(JavaScopeCleanup cleanup);
	    public void Dispose();
	}

`JavaScope` is a [`ref struct`][0], which means it can only be
allocated on the runtime stack, ensuring that cleanup semantics are
*scope* semantics.

TODO: is that actually a good idea?

If a `JavaScope` is created using
`JavaScopeCleanup.RegisterWithManager`, existing behavior is followed.
This is useful for nested scopes, should instances need to be
registered with `JniRuntime.ValueManager`.

If a `JavaScope` is created using `JavaScopeCleanup.Dispose` or
`JavaScopeCleanup.Release`, then:

 1. Object references created within the scope are "thread-local";
    they can be *used* by other threads, but
    `JniRuntime.JniValueManager.PeekPeer()` won't find existing values.

 2. At the end of a `using` block / when
   `JavaScope.Dispose()` is called, all collected instances will be
   `Dispose()`d (with `.Dispose`) or released (with `.Release`), and
   left to the GC to eventually finalize.

For example:

	using (new JavaScope (JavaScopeCleanup.Dispose)) {
	    var singleton = JavaSingleton.Singleton;
	    // use singleton
	}
	// `singleton.Dispose()` is called at the end of the `using` block

TODO: docs?

TODO: *nested* scopes, and "bound" vs. "unbound" instance construction
around `JniValueManager.GetValue<T>()` or `.CreateValue<T>()`,
and *why* they should be treated differently.

TODO: Should `CreateValue<T>()` be *removed*?  name implies it always
"creates" a new value, but implementation will return existing instances,
so `GetValue<T>()` alone may be better.  One related difference is that`
`CreateValue<T>()` uses `PeekBoxedObject()`, while `GetValue<T>()` doesn't.
*Should* it?

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
@jonpryor
Copy link
Member Author

jonpryor added a commit that referenced this issue Aug 2, 2024
Fixes: #4

Context: #426
Context: a666a6f

Alternate names?

  * JavaScope
  * JavaPeerableCleanupPool
  * JavaPeerCleanup
  * JavaReferenceCleanup
  * JniPeerRegistrationScope

Issue #426 is an idea to implement a *non*-GC-Bridged
`JniRuntime.JniValueManager` type, primarily for use with .NET Core.
This was begun in a666a6f.

What's missing is the answer to a question: what to do about
`JniRuntime.JniValueManager.CollectPeers()`?  With a Mono-style
GC bridge, `CollectPeers()` is `GC.Collect()`.  In a666a6f with
.NET Core, `CollectPeers()` calls `IJavaPeerable.Dispose()` on all
registered instances, which is "extreme".

@jonpryor thought that if there were a *scope-based* way to
selectively control which instances were disposed, that might be
"better" and more understandable.  Plus, this is issue #4!

Which brings us to the background for Issue #4, which touches upon
[bugzilla 25443][0] and [Google issue 37034307][1]: Java.Interop
attempts to provide referential identity for Java objects: if two
separate Java methods invocations return the same Java instance, then
the bindings of those methods should also return the same instance.

This is "fine" on the surface, but has a few related issues:

 1. JNI Global References are a limited resource: Android only allows
    ~52000 JNI Global References to exist, which sounds like a lot,
    but might not be.

 2. Because of (1), it is not uncommon to want to use `using` blocks
    to invoke `IJavaPeerable.Dispose()` to release
    JNI Global References.

 3. However, because of object identity, you could unexpectedly
    introduce *cross-thread sharing* of `IJavaPeerable` instances.
    This might not be at all obvious; for example, in the Android 5
    timeframe, [`Typeface.create()`][2] wouldn't necessarily return
    new Java instances, but may instead return cached instances.

Meaning that this:

	var t1 = new Thread(() => {
	    using var typeface = Typeface.Create("Family", …);
	    // use typeface…
	});
	var t2 = new Thread(() => {
	    using var typeface = Typeface.Create("Family", …);
	    // use typeface…
	});
	t1.Start();
	t2.Start();
	t1.Join();
	t2.Join();

could plausibly result in `ObjectDisposedException`s (or worse), as
the threads could be unexpectedly sharing the same bound instance.

Which *really* means that you can't reliably call `Dispose()`, unless
you *know* you created that instance:

	using var safe      = new Java.Lang.Double(42.0);       // I created this, therefore I control all access and can Dispose() it
	using var unsafe    = Java.Lang.Double.ValueOf(42.0);   // I have no idea who else may be using this instance!

Attempt to address both of these scenarios -- a modicum of .NET Core
support, and additional sanity around JNI Global Reference lifetimes --
by introducing a new `JavaPeerableRegistrationScope` type, which
introduces a scope-based mechanism to control when `IJavaPeerable`
instances are cleaned up:

        public enum JavaPeerableRegistrationScopeCleanup {
            RegisterWithManager,
            Dispose,
            Release,
        }

        public ref struct JavaPeerableRegistrationScope {
            public JavaPeerableRegistrationScope(JavaPeerableRegistrationScopeCleanup cleanup);
            public void Dispose();
        }

`JavaPeerableRegistrationScope` is a [`ref struct`][3], which means
it can only be allocated on the runtime stack, ensuring that cleanup
semantics are *scope* semantics.

TODO: is that actually a good idea?

If a `JavaPeerableRegistrationScope` is created using
`JavaPeerableRegistrationScopeCleanup.RegisterWithManager`, existing
behavior is followed.  This is useful for nested scopes, should
instances need to be registered with `JniRuntime.ValueManager`.

If a `JavaPeerableRegistrationScope` is created using
`JavaPeerableRegistrationScopeCleanup.Dispose` or
`JavaPeerableRegistrationScopeCleanup.Release`, then:

 1. `IJavaPeerable` instances created within the scope are
    "thread-local": they can be *used* by other threads, but
    `JniRuntime.JniValueManager.PeekPeer()` will only find the
    value on the creating thread.

 2. At the end of a `using` block / when
   `JavaScope.Dispose()` is called, all collected instances will be
   `Dispose()`d (with `.Dispose`) or released (with `.Release`), and
   left to the GC to eventually finalize.

For example:

	using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
	    var singleton = JavaSingleton.Singleton;
	    // use singleton

	    // If other threads attempt to access `JavaSingleton.Singleton`,
	    // they'll create their own peer wrapper
	}
	// `singleton.Dispose()` is called at the end of the `using` block

However, if e.g. the singleton instance is already accessed, then it
won't be added to the registration scope and won't be disposed:

	var singleton = JavaSingleton.Singleton;
	// singleton is registered with the ValueManager

	// later on the same thread or some other threa…
	using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
	    var anotherSingleton = JavaSingleton.Singleton;
	    // use anotherSingleton…
	}

then `anotherSingleton` will *not* disposed, as it already existed.
*Only newly created instances* are added to the current scope.

By default, only `JavaPeerableRegistrationScopeCleanup.RegisterWithManager`
is supported.  To support the other cleanup modes,
`JniRuntime.JniValueManager.SupportsPeerableRegistrationScopes` must
return `true`, which in turn requires that:

  * `JniRuntime.JniValueManager.AddPeer()` calls
    `TryAddPeerToRegistrationScope()`,
  * `JniRuntime.JniValueManager.RemovePeer()` calls
    `TryRemovePeerFromRegistrationScopes()`
  * `JniRuntime.JniValueManager.PeekPeer()` calls
    `TryPeekPeerFromRegistrationScopes()`.

See `ManagedValueManager` for an example implementation.

Finally, add the following methods to `JniRuntime.JniValueManager`
to help further assist peer management:

	partial class JniRuntime.JniValueManager {
	    public virtual bool CanCollectPeers { get; }
	    public virtual bool CanReleasePeers { get; }
	    public virtual void ReleasePeers ();
	}

TODO: docs?

TODO: *nested* scopes, and "bound" vs. "unbound" instance construction
around `JniValueManager.GetValue<T>()` or `.CreateValue<T>()`,
and *why* they should be treated differently.

TODO: Should `CreateValue<T>()` be *removed*?  name implies it always
"creates" a new value, but implementation will return existing instances,
so `GetValue<T>()` alone may be better.  One related difference is that`
`CreateValue<T>()` uses `PeekBoxedObject()`, while `GetValue<T>()` doesn't.
*Should* it?

[0]: https://web.archive.org/web/20211106214514/https://bugzilla.xamarin.com/25/25443/bug.html#c63
[1]: https://issuetracker.google.com/issues/37034307
[2]: https://developer.android.com/reference/android/graphics/Typeface#create(java.lang.String,%20int)
[3]: https://docs.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
jonpryor added a commit that referenced this issue Aug 6, 2024
Fixes: #4

Context: #426
Context: a666a6f

Alternate names?

  * JavaScope
  * JavaPeerableCleanupPool
  * JavaPeerCleanup
  * JavaReferenceCleanup
  * JniPeerRegistrationScope

Issue #426 is an idea to implement a *non*-GC-Bridged
`JniRuntime.JniValueManager` type, primarily for use with .NET Core.
This was begun in a666a6f.

What's missing is the answer to a question: what to do about
`JniRuntime.JniValueManager.CollectPeers()`?  With a Mono-style
GC bridge, `CollectPeers()` is `GC.Collect()`.  In a666a6f with
.NET Core, `CollectPeers()` calls `IJavaPeerable.Dispose()` on all
registered instances, which is "extreme".

@jonpryor thought that if there were a *scope-based* way to
selectively control which instances were disposed, that might be
"better" and more understandable.  Plus, this is issue #4!

Which brings us to the background for Issue #4, which touches upon
[bugzilla 25443][0] and [Google issue 37034307][1]: Java.Interop
attempts to provide referential identity for Java objects: if two
separate Java methods invocations return the same Java instance, then
the bindings of those methods should also return the same instance.

This is "fine" on the surface, but has a few related issues:

 1. JNI Global References are a limited resource: Android only allows
    ~52000 JNI Global References to exist, which sounds like a lot,
    but might not be.

 2. Because of (1), it is not uncommon to want to use `using` blocks
    to invoke `IJavaPeerable.Dispose()` to release
    JNI Global References.

 3. However, because of object identity, you could unexpectedly
    introduce *cross-thread sharing* of `IJavaPeerable` instances.
    This might not be at all obvious; for example, in the Android 5
    timeframe, [`Typeface.create()`][2] wouldn't necessarily return
    new Java instances, but may instead return cached instances.

Meaning that this:

	var t1 = new Thread(() => {
	    using var typeface = Typeface.Create("Family", …);
	    // use typeface…
	});
	var t2 = new Thread(() => {
	    using var typeface = Typeface.Create("Family", …);
	    // use typeface…
	});
	t1.Start();
	t2.Start();
	t1.Join();
	t2.Join();

could plausibly result in `ObjectDisposedException`s (or worse), as
the threads could be unexpectedly sharing the same bound instance.

Which *really* means that you can't reliably call `Dispose()`, unless
you *know* you created that instance:

	using var safe      = new Java.Lang.Double(42.0);       // I created this, therefore I control all access and can Dispose() it
	using var unsafe    = Java.Lang.Double.ValueOf(42.0);   // I have no idea who else may be using this instance!

Attempt to address both of these scenarios -- a modicum of .NET Core
support, and additional sanity around JNI Global Reference lifetimes --
by introducing a new `JavaPeerableRegistrationScope` type, which
introduces a scope-based mechanism to control when `IJavaPeerable`
instances are cleaned up:

        public enum JavaPeerableRegistrationScopeCleanup {
            RegisterWithManager,
            Dispose,
            Release,
        }

        public ref struct JavaPeerableRegistrationScope {
            public JavaPeerableRegistrationScope(JavaPeerableRegistrationScopeCleanup cleanup);
            public void Dispose();
        }

`JavaPeerableRegistrationScope` is a [`ref struct`][3], which means
it can only be allocated on the runtime stack, ensuring that cleanup
semantics are *scope* semantics.

TODO: is that actually a good idea?

If a `JavaPeerableRegistrationScope` is created using
`JavaPeerableRegistrationScopeCleanup.RegisterWithManager`, existing
behavior is followed.  This is useful for nested scopes, should
instances need to be registered with `JniRuntime.ValueManager`.

If a `JavaPeerableRegistrationScope` is created using
`JavaPeerableRegistrationScopeCleanup.Dispose` or
`JavaPeerableRegistrationScopeCleanup.Release`, then:

 1. `IJavaPeerable` instances created within the scope are
    "thread-local": they can be *used* by other threads, but
    `JniRuntime.JniValueManager.PeekPeer()` will only find the
    value on the creating thread.

 2. At the end of a `using` block / when
   `JavaPeerableRegistrationScope.Dispose()` is called, all collected
   instances will be `Dispose()`d (with `.Dispose`) or released (with
   `.Release`), and left to the GC to eventually finalize.

For example:

	using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
	    var singleton = JavaSingleton.Singleton;
	    // use singleton

	    // If other threads attempt to access `JavaSingleton.Singleton`,
	    // they'll create their own peer wrapper
	}
	// `singleton.Dispose()` is called at the end of the `using` block

However, if e.g. the singleton instance is already accessed, then it
won't be added to the registration scope and won't be disposed:

	var singleton = JavaSingleton.Singleton;
	// singleton is registered with the ValueManager

	// later on the same thread or some other threa…
	using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
	    var anotherSingleton = JavaSingleton.Singleton;
	    // use anotherSingleton…
	}

then `anotherSingleton` will *not* disposed, as it already existed.
*Only newly created instances* are added to the current scope.

By default, only `JavaPeerableRegistrationScopeCleanup.RegisterWithManager`
is supported.  To support the other cleanup modes,
`JniRuntime.JniValueManager.SupportsPeerableRegistrationScopes` must
return `true`, which in turn requires that:

  * `JniRuntime.JniValueManager.AddPeer()` calls
    `TryAddPeerToRegistrationScope()`,
  * `JniRuntime.JniValueManager.RemovePeer()` calls
    `TryRemovePeerFromRegistrationScopes()`
  * `JniRuntime.JniValueManager.PeekPeer()` calls
    `TryPeekPeerFromRegistrationScopes()`.

See `ManagedValueManager` for an example implementation.

Additionally, add the following methods to
`JniRuntime.JniValueManager` to help further assist peer management:

	partial class JniRuntime.JniValueManager {
	    public virtual bool CanCollectPeers { get; }
	    public virtual bool CanReleasePeers { get; }
	    public virtual void ReleasePeers ();
	}

Finally, many of the new members are marked with the
[`[Experimental]` attribute][4], which indicates that an API is
experimental and could change.  In order to use the new APIs,
then the `JI9999` warning must be disabled, e.g. via
[`#pragma warning disable JI9999`][5].  This will allow us to break
API and semantics, should that be necessary.

TODO: docs?

TODO: *nested* scopes, and "bound" vs. "unbound" instance construction
around `JniValueManager.GetValue<T>()` or `.CreateValue<T>()`,
and *why* they should be treated differently.

TODO: Should `CreateValue<T>()` be *removed*?  name implies it always
"creates" a new value, but implementation will return existing instances,
so `GetValue<T>()` alone may be better.  One related difference is that`
`CreateValue<T>()` uses `PeekBoxedObject()`, while `GetValue<T>()` doesn't.
*Should* it?

[0]: https://web.archive.org/web/20211106214514/https://bugzilla.xamarin.com/25/25443/bug.html#c63
[1]: https://issuetracker.google.com/issues/37034307
[2]: https://developer.android.com/reference/android/graphics/Typeface#create(java.lang.String,%20int)
[3]: https://docs.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
[4]: https://learn.microsoft.com/dotnet/api/system.diagnostics.codeanalysis.experimentalattribute?view=net-8.0
[5]: https://learn.microsoft.com/dotnet/csharp/language-reference/preprocessor-directives#pragma-warning
jonpryor added a commit that referenced this issue Aug 7, 2024
Fixes: #4

Context: #426
Context: a666a6f

Alternate names?

  * JavaScope
  * JavaPeerableCleanupPool
  * JavaPeerCleanup
  * JavaReferenceCleanup
  * JniPeerRegistrationScope

Issue #426 is an idea to implement a *non*-GC-Bridged
`JniRuntime.JniValueManager` type, primarily for use with .NET Core.
This was begun in a666a6f.

What's missing is the answer to a question: what to do about
`JniRuntime.JniValueManager.CollectPeers()`?  With a Mono-style
GC bridge, `CollectPeers()` is `GC.Collect()`.  In a666a6f with
.NET Core, `CollectPeers()` calls `IJavaPeerable.Dispose()` on all
registered instances, which is "extreme".

@jonpryor thought that if there were a *scope-based* way to
selectively control which instances were disposed, that might be
"better" and more understandable.  Plus, this is issue #4!

Which brings us to the background for Issue #4, which touches upon
[bugzilla 25443][0] and [Google issue 37034307][1]: Java.Interop
attempts to provide referential identity for Java objects: if two
separate Java methods invocations return the same Java instance, then
the bindings of those methods should also return the same instance.

This is "fine" on the surface, but has a few related issues:

 1. JNI Global References are a limited resource: Android only allows
    ~52000 JNI Global References to exist, which sounds like a lot,
    but might not be.

 2. Because of (1), it is not uncommon to want to use `using` blocks
    to invoke `IJavaPeerable.Dispose()` to release
    JNI Global References.

 3. However, because of object identity, you could unexpectedly
    introduce *cross-thread sharing* of `IJavaPeerable` instances.
    This might not be at all obvious; for example, in the Android 5
    timeframe, [`Typeface.create()`][2] wouldn't necessarily return
    new Java instances, but may instead return cached instances.

Meaning that this:

	var t1 = new Thread(() => {
	    using var typeface = Typeface.Create("Family", …);
	    // use typeface…
	});
	var t2 = new Thread(() => {
	    using var typeface = Typeface.Create("Family", …);
	    // use typeface…
	});
	t1.Start();
	t2.Start();
	t1.Join();
	t2.Join();

could plausibly result in `ObjectDisposedException`s (or worse), as
the threads could be unexpectedly sharing the same bound instance.

Which *really* means that you can't reliably call `Dispose()`, unless
you *know* you created that instance:

	using var safe      = new Java.Lang.Double(42.0);       // I created this, therefore I control all access and can Dispose() it
	using var unsafe    = Java.Lang.Double.ValueOf(42.0);   // I have no idea who else may be using this instance!

Attempt to address both of these scenarios -- a modicum of .NET Core
support, and additional sanity around JNI Global Reference lifetimes --
by introducing a new `JavaPeerableRegistrationScope` type, which
introduces a scope-based mechanism to control when `IJavaPeerable`
instances are cleaned up:

        public enum JavaPeerableRegistrationScopeCleanup {
            RegisterWithManager,
            Dispose,
            Release,
        }

        public ref struct JavaPeerableRegistrationScope {
            public JavaPeerableRegistrationScope(JavaPeerableRegistrationScopeCleanup cleanup);
            public void Dispose();
        }

`JavaPeerableRegistrationScope` is a [`ref struct`][3], which means
it can only be allocated on the runtime stack, ensuring that cleanup
semantics are *scope* semantics.

TODO: is that actually a good idea?

If a `JavaPeerableRegistrationScope` is created using
`JavaPeerableRegistrationScopeCleanup.RegisterWithManager`, existing
behavior is followed.  This is useful for nested scopes, should
instances need to be registered with `JniRuntime.ValueManager`.

If a `JavaPeerableRegistrationScope` is created using
`JavaPeerableRegistrationScopeCleanup.Dispose` or
`JavaPeerableRegistrationScopeCleanup.Release`, then:

 1. `IJavaPeerable` instances created within the scope are
    "thread-local": they can be *used* by other threads, but
    `JniRuntime.JniValueManager.PeekPeer()` will only find the
    value on the creating thread.

 2. At the end of a `using` block / when
   `JavaPeerableRegistrationScope.Dispose()` is called, all collected
   instances will be `Dispose()`d (with `.Dispose`) or released (with
   `.Release`), and left to the GC to eventually finalize.

For example:

	using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
	    var singleton = JavaSingleton.Singleton;
	    // use singleton

	    // If other threads attempt to access `JavaSingleton.Singleton`,
	    // they'll create their own peer wrapper
	}
	// `singleton.Dispose()` is called at the end of the `using` block

However, if e.g. the singleton instance is already accessed, then it
won't be added to the registration scope and won't be disposed:

	var singleton = JavaSingleton.Singleton;
	// singleton is registered with the ValueManager

	// later on the same thread or some other threa…
	using (new JavaPeerableRegistrationScope (JavaPeerableRegistrationScopeCleanup.Dispose)) {
	    var anotherSingleton = JavaSingleton.Singleton;
	    // use anotherSingleton…
	}

then `anotherSingleton` will *not* disposed, as it already existed.
*Only newly created instances* are added to the current scope.

By default, only `JavaPeerableRegistrationScopeCleanup.RegisterWithManager`
is supported.  To support the other cleanup modes,
`JniRuntime.JniValueManager.SupportsPeerableRegistrationScopes` must
return `true`, which in turn requires that:

  * `JniRuntime.JniValueManager.AddPeer()` calls
    `TryAddPeerToRegistrationScope()`,
  * `JniRuntime.JniValueManager.RemovePeer()` calls
    `TryRemovePeerFromRegistrationScopes()`
  * `JniRuntime.JniValueManager.PeekPeer()` calls
    `TryPeekPeerFromRegistrationScopes()`.

See `ManagedValueManager` for an example implementation.

Additionally, add the following methods to
`JniRuntime.JniValueManager` to help further assist peer management:

	partial class JniRuntime.JniValueManager {
	    public virtual bool CanCollectPeers { get; }
	    public virtual bool CanReleasePeers { get; }
	    public virtual void ReleasePeers ();
	}

Finally, many of the new members are marked with the
[`[Experimental]` attribute][4], which indicates that an API is
experimental and could change.  In order to use the new APIs,
then the `JI9999` warning must be disabled, e.g. via
[`#pragma warning disable JI9999`][5].  This will allow us to break
API and semantics, should that be necessary.

TODO: docs?

TODO: *nested* scopes, and "bound" vs. "unbound" instance construction
around `JniValueManager.GetValue<T>()` or `.CreateValue<T>()`,
and *why* they should be treated differently.

TODO: Should `CreateValue<T>()` be *removed*?  name implies it always
"creates" a new value, but implementation will return existing instances,
so `GetValue<T>()` alone may be better.  One related difference is that`
`CreateValue<T>()` uses `PeekBoxedObject()`, while `GetValue<T>()` doesn't.
*Should* it?

[0]: https://web.archive.org/web/20211106214514/https://bugzilla.xamarin.com/25/25443/bug.html#c63
[1]: https://issuetracker.google.com/issues/37034307
[2]: https://developer.android.com/reference/android/graphics/Typeface#create(java.lang.String,%20int)
[3]: https://docs.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct#ref-struct
[4]: https://learn.microsoft.com/dotnet/api/system.diagnostics.codeanalysis.experimentalattribute?view=net-8.0
[5]: https://learn.microsoft.com/dotnet/csharp/language-reference/preprocessor-directives#pragma-warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants