Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Java.Interop] Expunge
SafeHandles from the public API.
(The mother of all commits!) Context: https://trello.com/c/1HLPYTWt/61-optimize-method-invocation Context: https://bugzilla.xamarin.com/show_bug.cgi?id=32126 Xamarin.Android needs a form of JniPeerMembers to cache jmethodIDs for JNIEnv::CallNonvirtual*Method() invocations and constructors. There are two ways to resolve this: 1. Copy/"fork" JniPeerMembers into Xamarin.Android. 2. Update Xamarin.Android to use Java.Interop. (1) is ugly, and makes a possible future migration more difficult -- either we'd need to use different type names, or we'd need to use type forwards. It's a possible mess, and I'd rather avoid it. Which leaves (2): Get Java.Interop to a point that Xamarin.Android can bundle it and use it. Java.Interop is nowhere near that; it's too incomplete. We could, however, create a JniPeerMembers-using *subset* of Java.Interop suitable for use by Xamarin.Android. The problem? JniPeerInstanceMethods.CallObjectMethod() (and many related methods!) need to return a JNI Local Reference, and since the beginning (commit e646783) Java.Interop has used SafeHandles for this. Which doesn't sound like a problem, except for two matters: 1. @jassmith has been profiling Xamarin.Android, and GC allocations would be "a problem", as increased allocations will increase GC-related overheads, slowing down the app. We want a "GC steady state" wherein once things are running, and things are cached, future JNI invocations such as JNIEnv::CallObjectMethod() won't allocate additional garbage (unless *Java* is returning new "garbage" instances...) 2. SafeHandles are thread-safe: http://blogs.msdn.com/b/bclteam/archive/2005/03/16/396900.aspx (2) is a major performance killer. BEST CASE, it *adds* ~40% to runtime execution over an "equivalent" struct-based API. Normal case, JNIEnv::ExceptionOccurred() needs to be invoked, and SafeHandles add *360%+* to the exeuction (wall clock) time. (See README.md and tests/invocation-overhead.) Whatever form of Java.Interop is used by Xamarin.Android, SafeHandles CANNOT be a part of it. The performance impact is unnacceptable. What's the fix, then? Follow Xamarn.Android and use IntPtrs everywhere? While this is possible, the very idea fills me with dread. The Xamarin.Android JNIEnv API is FUGLY, and all the IntPtrs are a significant part of that. (The lack of "grouping" of of related methods such as JniEnvironment.Types is another.) The fix, then? The problem is SafeHandles in particular, and GC-allocated values in general, so avoid them by using a struct: JniObjectReference, which represents a `jobject` -- a JNI local reference, global reference, or weak global reference value. Publicly, it's an immutable value type, much like an IntPtr, but it also "knows" its type, removing the need for the ugly Xamarin.Android JniHandleOwnership.TransferLocalRef and JniHandleOwnership.TransferGlobalRef enum values: public strict JniObjectReference { public IntPtr Handle {get;} public JniObjectReferenceType Type {get;} public JniObjectReference (IntPtr reference, JniObjectReferenceType type); } Where JniObjectReferenceType is what was formerly known as JniReferenceType: public enum JniObjectReferenceType { Invalid, Local, Global, WeakGlobal, } The one "problem" with this is that there's no type distinction between Local, Global, and Weak Global references: instead, they're all JniObjectReference, so improperly intermixing them isn't a compiler type error, it's a runtime error. This is not desirable, but Xamarin.Android has worked this way for *years*, so it's liveable. Another problem is that an IntPtr-based JniObjectReference implementation can't support garbage collecting errant object references, which is also sucky, but (again) Xamarin.Android has been living with that same restriction for years, so this is acceptable. Not ideal, certainly -- ideal would be SafeHandles performing as fast as structs and a GC fast enough that overhead isn't a concern -- but merely acceptable. (For now, internally, JniObjectReference IS using SafeHandles, when FEATURE_HANDLES_ARE_SAFE_HANDLES is #defined. This is done to support long-term performance comparison of handle implementations, and to simplify migrating the unit tests, which DO expect a GC!) With a new core "abstraction" in JniObjectReference, we get a corresponding change in naming: `SafeHandle` is no longer a useful name, because it's not a SafeHandle. I thought of PeerHandle, but that results in ugly `value.PeerHandle.Handle` expressions. Thus, JNI Object References (former JniReferenceSafeHandle and subclasses) are "references", *not* "handles", resulting in e.g. the IJavaObject.PeerReference property (formerly IJavaObject.SafeHandle). With that typing and naming change explained, update tools/jnienv-gen to emit JniEnvironment types that follow this New World Order, and additionally generate an IntPtr-based instead of SafeHandle-based delegate implementation. (This was used in tools/invocation-overhead, but will likely need additional work to actually be usable.) The next "style" change -- which may be a mistake! -- has to do with reference *disposal*. I'd like to prevent "use after free"-style bugs. SafeHandles were ideal for this: since they were reference types, Dispose()ing any variable to a SafeHandle disposed "all" of them, as they all referred to the same instance. That isn't possible with a value type, as assigning between variables copies the struct members. Thus, we could get this: var r = JniEnvironment.Members.CallObjectMethod (...); JniEnvironment.Handles.Dispose (r); Assert.IsTrue (r.IsValid); // *not* expected; `r` has the SAME reference // value, yet is still valid! ARGH! The reason is that because structs are copied by value, passing `r` as a parameter results in a copy, and it's the copy that has its contents updated. It's because of this that I'm wary of having JniObjectReference implement IDisposable: var r = JniEnvironment.Members.CallObjectMethod (...); using (r) { } // `r.Dispose()` called? Assert.IsTrue (r.IsValid); // wat?! A `using` block *copies* the source variable. For reference types, this behaves as you expect, but for struct types it can be "weird": as we see above, `r` IS NOT MODIFIED (but it was in a `using` block!). Though it doesn't *look* it, this is really the same as the previous JniEnvironment.Handles.Dispose() example. This is also why JniObjectReference shouldn't implement IDisposable. In short, an immutable struct makes it *really* easy for "invalid" handles to stick around, so I thought...is there a way to fix this? (Again: this may be a mistake!) The current solution answers the above question with "yes", by using `ref` variables: var r = JniEnvironment.Members.CallObjectMethod (...); JniEnvironment.Handles.Dispose (ref r); Assert.IsFalse (r.IsValid); // Yay! Passing the JNI reference by reference (ugh...?) allows it to be cleared, with the "cleared" state visible to the caller. Explicit copies are still a problem: var r = JniEnvironment.Members.CallObjectMethod (...); var c = r; JniEnvironment.Handles.Dispose (ref r); Assert.IsFalse (r.IsValid); // Yay! Assert.IsTrue (c.IsValid); // Darn! ...but hopefully those won't be common. (Hopefully?) This idea has far-reaching changes: *everything* that previously took a (JniReferenceSafeHandle, JniHandleOwnership) argument pair -- JavaVM.GetObject(), the JavaObject and JavaException constructors -- is now a (ref JniObjectReference, JniHandleOwnership) argument pair. This also complicates method invocations, as new explicit temporaries are required: // Old-and-busted (but nice API!) var name = JniEnvironment.Strings.ToString (Field_getName.CallVirtualObjectMethod (field), JniHandleOwnership.Transfer); // New hawtness? (requires temporary) var n_name = Field_getName.CallVirtualObjectMethod (field); var name = JniEnvironment.Strings.ToString (ref n_name, JniHandleOwnership.Transfer); 99.99% of the time, this is actually a *code generator* problem, not a human one, but on those rare occasions human intervention is warranted...requiring `ref JniObjectReference` is fugly. This idea requires thought. (Fortunately it should be outside of the Xamairn.Android integration API. I hope.) This idea also requires one of the fugliest things I've ever done: dereferencing a null pointer for fun and profit! unsafe partial class JavaObject { protected static readonly JniObjectReference* InvalidJniObjectReference = null; public JavaObject (ref JniObjectReference reference, JniHandleOwnership transfer); } class Subclass : JavaObject { public unsafe Subclass () : base (ref *InvalidJniObjectReference, JniHandleOwnership.Invalid) { } } Look at that! `ref *InvalidJniObjectReference`! A sight to behold! (OMFG am I actually considering this?!) It does work, so long as you don't actually cause the null pointer to be dereferenced...hence the new JniHandleOwnership.Invalid value.
- Loading branch information
