-
Notifications
You must be signed in to change notification settings - Fork 54
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
Comments
Another option for Consequently, it would be helpful if exception marshaling could be inhibited in a similar manner. |
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.
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.
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.
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 Calling through the delegates is not a "zero-cost" operation, so the idea behind However, with the "new" P/Invoke-based system in 926e4bc (not really "new" anymore), I thus don't see much need for
|
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
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
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
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
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
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)
:Calling
JniEnviornment.BeginGetValueBehaviors()
would alter the behavior ofJavaVM.GetValue()
: ifGetValueBehaviors.CreateValues
is specified, thenJavaVM.GetValue()
will instead behave likeJavaVM.CreateValue()
. This allows the end user to maintain some degree of control:The above allows disposing of the temporary with impunity, as
BeginGetValueScope()
will ensure thatTypeface.Create()
returns unique wrappers instead of possibly shared wrappers.The text was updated successfully, but these errors were encountered: