java.interop icon indicating copy to clipboard operation
java.interop copied to clipboard

Reference Counting, non-bridged backend

Open jonpryor opened this issue 6 years ago • 12 comments
trafficstars

As a proof-of-concept, can we support "one-way" Java object use, in which construction and use of bound types is supported, but no Java-to-managed object references.

This would mean:

  • Subclassing is (probably) out
    • though Java-to-managed method invocations should still be possible (static methods?)?
  • Managed code could refer to Java instances, e.g. List<Java.Lang.String> works
  • Java code can't refer to managed code, e.g. if you could subclass, that subclass couldn't contain any fields which reference any reference types. (Which is why subclassing would be out.)

Ideally, such a limited construct could be done via a JniRuntime.JniValueManager subclass.

jonpryor avatar May 16 '19 21:05 jonpryor

After…checks notes…almost two years of thought, I think I was wrong on the need for all the restrictions.

Related: PR #804.

The fundamental question/scenario is around object lifetimes, especially since there are two "peers" that both need to be kept alive: the Java peer and the Managed peer.

In Mono/Xamarin.Android, Mono's SGen Bridge is used, which allows us to "work with" Mono's GC. Each Managed peer holds a JNI Global Reference to the Java peer, keeping the Java peer alive, and the GC bridge is used to ensure that the Managed peer is kept alive so long as managed code holds a reference or so long as something in Java-land holds a reference to the Java peer. This allows the lifetimes of the two peers to be closely correlated.

Without a GC bridge, we can instead have stricter semantics:

  1. The managed peer holds a JNI Global Reference to the Java peer, as always. This keeps the Java peer alive.
  2. The JniRuntime.JniValueManager holds a strong reference to the managed peer. (The JniValueManager is in turn referenced by the JniRuntime instance, which in turn is rooted by a static variable.)

This prevents objects from being prematurely collected.

It also prevents any objects from ever being collected by the GC.

For that "minor" problem, all of the "meanings" implied in the initial issue are no longer implied. (Insert hand-waving here.)

  • Java code can still refer to Java peers, and as the Java peer is referenced by a JNI Global Reference, it's a root, and thus will always be kept alive by Java.
  • If Java code invokes a Java-Callable-Wrapper constructor for a Managed subclass, that will cause the Managed peer to be constructed, and the JniValueManager will reference it, keeping the Managed peer alive.

The (large!) remaining problem is one of coordination: eventually you want the objects to be collected. The only way to do so is to "dispose" of the Managed peer, which will free the Java peer. However, the Java peer may still be referenced by other Java code, and if that Java code invokes any methods on the (now disconnected) Java peer, those methods will fail. Thus the only "safe" way to fully "dispose" of a Managed peer is to ensure no Java code is referencing it.

…which is hard to do without a GC bridge.

(Baby steps?)

A place where this may still be viable is if "higher level" code can ensure that once "something is done", all the Java & Managed objects are no longer relevant, and thus can be freed.

This remains a "non-ideal" scenario, but it is sufficient to get most of the Java.Interop unit tests executing…

jonpryor avatar Feb 19 '21 00:02 jonpryor

To help "support"/"address" the coordination problem, PR #804 adds two members to JniRuntime.JniValueManager:

partial class JniRuntime {
    partial class JniValueManager {
        public abstract bool PeersRequireRelease {get;}
        public abstract void ReleasePeers ();
    }
}

The PeersRequireRelease property is a way of asking the value manager "is there GC integration?" If PeerRequireRelease is true, there is no GC integration.

The ReleasePeers() method releases all JNI Global References held by Managed peers referenced by the value manager, and stops referencing the managed peers. This allows the respective Java & managed GC's to collect their objects, as there is no longer anything keeping the instances alive.

There is a broader question of semantics: should ReleasePeers() "notify" the peers that they're being released?

There are currently two "lifetime" methods:

IJavaPeerable.Disposed() is invoked e.g. when someone calls IDisposable.Dispose() on a JavaObject or JavaException subclass.

IJavaPeerable.Finalized() is invoked when the GC invokes the finalizer of a JavaObject or JavaException subclass.

See e.g. Java.Interop.JavaObject, in which IDisposable.Dispose() calls JniValueManager.DisposePeer() which calls IJavaPeerable.Disposed() which calls Dispose(disposing:true):

https://github.com/xamarin/java.interop/blob/89a5a2297a7a79f9a0d769a429f80c8b459823d9/src/Java.Interop/Java.Interop/JavaObject.cs#L99-L102

https://github.com/xamarin/java.interop/blob/89a5a2297a7a79f9a0d769a429f80c8b459823d9/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs#L140-L159

https://github.com/xamarin/java.interop/blob/89a5a2297a7a79f9a0d769a429f80c8b459823d9/src/Java.Interop/Java.Interop/JavaObject.cs#L139-L142

In a CoreCLR environment, IJavaPeerable.Finalized() will eventually be executed, after ReleasePeers() is invoked and the GC (eventually) subsequently collects the instance.

Question: Should ReleasePeers() invoke the Disposed() "event" on the peers? Should there instead be another IJavaPeerable "event" to indicate that ReleasePeers() has been invoked? (If so, it should presumably be called before the GREF is freed, so that it can communicate with the Java peer.)

jonpryor avatar Feb 19 '21 00:02 jonpryor

Possibly related? https://github.com/xamarin/java.interop/issues/4

The idea in Issue #4 is that sometimes methods return the "same" value:

// Java
public sealed class Example {
    private Example e = new Example();
    public static Example getSingleton() {
        return e;
    }
}

We'd bind this kinda/sorta like:

// C# binding
[Register (…)\
public sealed class Example {
    static readonly JniPeerMembers _members = new JniPeerMembers ("…/Example", typeof (Example));

    public static Example Singleton {
        [Register (…)]
        get {
            const string __id = "getSingleton.()L…/Example;";
            var __rm = _members.StaticMethods.InvokeObjectMethod (__id, null);
            return JniEnvironment.Runtime.ValueManager.GetValue<Example>(__rm, JniObjectReferenceOptions.CopyAndDispose);
        }
    }

Due to our "identity preserving semantics", that means that the same instance is returned by Example.Singleton:

var a = Example.Singleton;
var b = Example.Singleton;
if (object.ReferenceEquals (a, b)) … // always true

However, it's not possible to know, in all circumstances, whether a "new value" or a "previously registered value" will be returned. Consequently, if you want to keep GREF use low, this won't work:

// DO NOT DO
using (var a = Example.Singleton) {
    // …
}

The above won't work in multithreaded environments, because if another thread B also accesses Example.Singleton, it's possible that the single registered instance will be disposed by the above thread A with the using block.

The current "solution" is, in effect, Don't Do That™: don't use using blocks on JavaObject subclasses unless you know you're creating the instance, i.e. you're using a new expression.

The proposed solution was to alter the "single registered instance" behavior to permit possible "aliasing", but this time in a good way!

using (var scope = JniEnvironment.BeginGetValueBehaviors (GetValueBehaviors.CreateValues)
using (var a = Example.Singleton) {
    // …
}

This could be thread-safe, as a is an alias to a to the "single registered instance", not the single registered instance itself.

jonpryor avatar Feb 19 '21 19:02 jonpryor

Proposal: Instead of JniEnvironment.BeginGetValueBehaviors(), we tie into JniValueManager, in a "stack-like" manner:

public enum JniValueManagerCollectsRegistrations {
    Never,
    WithSystemGC,
}
partial class JniValueManager {
    public abstract JniValueManagerCollectsRegistrations JniValueManagerCollectsRegistrations {get;}
    public abstract IDisposable CreateRegistrationScope (…);
}

The CreateRegistrationScope() "solves" the JniEnvironment.BeginGetValueBehaviors() (#4) scenario, and the we don't want to release everything scenario, by scoping the Release. Calling IDisposable.Dispose() on the value returned will dispose all values created "in" that scope:

using (var scope = JniEnvironment.Runtime.ValueManager.CreateRegistrationScope (…)) {
    // create new `JavaObject` subclass instances
}
// all instances created within `scope` are Disposed

Such scopes would need to support being thread-local, in order to avoid cross-thread sharing.

There also needs to be support for a "global" scope, to assist with the "Tensorflow+Java"-esque scenario: if other threads are created, and you're at "steady state", you may need to clear everything.

This suggests:

[Flags]
public enum RegistrationScope {
    None,
    Global = 1 << 0,
    ThreadLocal = 1 << 1,
    Dispose = 1 << 4,
    Release = 1 << 5,
}

.Dispose would cause .CreateRegistrationScope(…).Dispose() to call .Dispose() on all registered instances, while .Release would not. ("Solving" the original design conundrum: let someone else deal with it.)

Only one of Global | ThreadLocal can be provided, ditto Dispose | Release. (Separate enum types + parameters? Names?)

Still not sure adding a new "global" scope necessarily makes sense; how's that work in a multi-threaded scenario? Since a "scope" can't reasonably "partially nest", the entire .Global idea may be a non-starter, and only .ThreadLocal makes sense.

May also want/need to update JniObjectReferenceOptions to allow explicitly registering globally:

public partial enum JniObjectReferenceOptions {
    CopyAndRegisterGlobal = Copy + (1 << 3),
}

Current JniObjectReferenceOptions.CopyAndDoNotRegister support also needs review and possible re-thinking.

jonpryor avatar Feb 19 '21 19:02 jonpryor

Design thoughts…

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

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

Additionally, ReleasePeers() is too much.

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

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

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

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

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

jonpryor avatar Feb 19 '21 19:02 jonpryor

Related related idea? Model on Objective-C AutoRelease pools? https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAutoreleasePools.html

The problem here is that we don't have reference counting, so it's a bit of an "all or nothing" thing.

jonpryor avatar Feb 19 '21 19:02 jonpryor

Having slept on it, wrt previous design thoughts

We don't want to support explicitly registering "global" instances, at least not at first. It may be useful, but without a concrete scenario in mind, it's extra complexity.

Additionally, if the intent is something "reasonably easily used" for scoping, a JniValueManager as the "main" entry point is not good: it's too many .s, it's too hard to find. Instead, we should consider a new type as the primary entry point:

[Flags]
public enum JniPeerRegistrationScopeOptions {
    None,
    Register = (1 << 1),
    DoNotRegister = (1 << 2),
}
public enum JniPeerRegistrationScopeCleanup {
    Dispose,
    Release,
}
public sealed class JniPeerRegistrationScope : IDisposable {
    public JniPeerRegistrationScope (JniPeerRegistrationScopeOptions options = JniPeerRegistrationScopeOptions.Register, JniPeerRegistrationScopeCleanup cleanup = JniPeerRegistrationScopeCleanup.Dispose);
    public void Dispose ();
}

This would allow "reasonable" (?) @autoreleasepool scope semantics:

using (var scope = new JniPeerRegistrationScope()) {
    var list = new Java.Util.ArrayList ();
}
// at scope.Dispose(), `list` and everything it contains is automatically .Dispose()d.

JniPeerRegistrationScope internals would be a thread-local construct. Values created on other threads would not use the same registration scope; they would have their own, or the "root global" scope.

This in turn begs the question, how do we ensure it's thread-local? The answer here may be to be instead use a ref struct, which would prevent storing the instance into the GC heap.

Pity ref struct can't use interfaces…

However, using doesn't require IDisposable, so this works with the previous example, except that all constructor parameters cannot be optional. (They "can" have default values, but for some reason on mono 6.12, the constructor body isn't executed unless the construction-site provides parameters. ¯_(ツ)_/¯)

public ref struct JniPeerRegistrationScope {
    public JniPeerRegistrationScope (JniPeerRegistrationScopeOptions options, JniPeerRegistrationScopeCleanup cleanup = JniPeerRegistrationScopeCleanup.Dispose);
    public void Dispose ();
}

On the enum design, I'm still not thrilled with multiple enums, but I'm likewise not thrilled with a single enum containing distinct "sections", a'la System.Reflection.BindingFlags. Perhaps a separate struct is warranted?

public struct JniPeerRegistrationScopeOptions {
    public bool DisposePeers {get; set;}
    public bool ReleasePeers {get; set;}
    public bool DoNotRegisterPeers {get; set;}
}
public ref struct JniPeerRegistrationScope {
    public JniPeerRegistrationScope (JniPeerRegistrationScopeOptions);
    public void Dispose ();
}

jonpryor avatar Feb 20 '21 17:02 jonpryor

…and more thinking, the current DoNotRegister idea isn't really viable, as currently constructed, because there may be an entire object graph implicitly created, but -- in the idea of the singleton, or Typeface.create() -- you only have access to the one reference, not the whole object graph.

Instead, to support Issue #4, we'd want a CreateNew… semantic, wherein "object identity" is not preserved, and all new objects are "registered somewhere", so that at scope dispose all of the created instances are reachable and can be Disposed.

I'm increasingly thinking that JniPeerRegistrationScope need not have "public" changes to JniValueManager API, but instead to semantics.

A thread-local "scope stack" should be part of JniEnvironmentInfo, so that we only have one TLS slot to worry about/in use:

internal partial class JniEnvironmentInfo {
    public Stack<IDictionary<int, List<IJavaPeerable>> RegistrationScopes {get;}
}

public partial class JniEnvironment {
    public IDictionary<int, List<IJavaPeerable>>? CurrentRegistrationScope => {
        if (CurrentInfo.RegistrationScopes.TryPeek (out var scope))
            return scope;
        return null;
    }
}

jonpryor avatar Feb 22 '21 23:02 jonpryor

"Related" random thought: there is a System.Threading.ThreadPool.EnableDispatchAutoreleasePool feature switch: https://github.com/dotnet/runtime/blob/d0616f837f4ad1ceb7e36a341934c929fd790089/docs/workflow/trimming/feature-switches.md

When set to true, creates an NSAutoreleasePool around each thread pool work item on applicable platforms.

If we had a JniPeerRegistrationScope/JniPeerRegistrationPool type, would it be useful to support this switch with similar "semantics"?

jonpryor avatar Feb 26 '21 19:02 jonpryor

Back on the JniValueManager side of things, instead of a new enum, go for an easier semantic. (Still a breaking change vs. current API, but at least it's explicable.)

partial class JniRuntime.JniValueManager {
    public virtual bool CanCollectPeers  => false;
    public virtual void CollectPeers() => throw new NotSupportedException();
    public abstract void ReleasePeers();
}

This preserves the existing semantic that CollectPeers() involves/requires GC integration. "Collecting" peers without GC integration means that we can't ("reasonably efficiently) "mirror" the managed object graph into Java land, preventing the Java-side GC from being necessarily useful. (We could inefficiently do so by using Reflection to traverse the graph of all referenced Peers, trying to find all IJavaPeerable values, and reproduce that graph, but…ouch, that would be terribly inefficient.)

ReleasePeers() is likewise efficient: stop referencing all IJavaPeerable values. Do not .Dispose().

Documentation-wise -- once we have docs -- we should also mention how JniValueManager ties in to identity semantics, especially since we want to make that more "flexible" vis-a-vis the "auto release pool" idea.

jonpryor avatar Feb 26 '21 20:02 jonpryor

A possible alternate/additional idea suggested by @wasabi#2184:

Ever thought about using DependentHandles for the GC stuff? Not going to be performant. But might work.

What's a "dependent handle"? ConditionalWeakTable<TKey, TValue> uses them internally: https://yizhang82.dev/dependent-handle

So long as the key is kept alive, the value is kept alive.

A problem with this is that it still appears to require that all keys be in the managed VM. How do we have JVM-hosted Java instances keep managed instances alive then? They're in separate VMs!

However, that might not be as problematic as it first appears? Conceptually, we could introduce a ManagedHandle, which is kept alive via static variables:

internal partial class ManagedHandle {
    public int Id {get;}

    private ManagedHandle() {}

    static List<ManagedHandle> handles = new List<ManagedHandle>();
    public static ManagedHandle Alloc()
    {
        var h = new ManagedHandle();
        handles.Add(h);
        return h;
    }

    public static void Free(ManagedHandle h)
    {
        handles.Remove (h);
    }
    public static void Free(int id)
    {
        handles.RemoveAll (v => v.Id == id);
    }
}

We can then have a ConditionalWeakTable<ManagedHandle, IJavaPeerable>.

ManagedHandle.Alloc() invocation is straightforward enough. But what calls ManagedHandle.Free()? For that, we use Java finalizers / Cleaner / etc.:

// Java
/* partial */ class ManagedHandle {
    public ManagedHandle();
    @Override protected void finalize() { Runtime.freeManagedHandle(id); }
}

Because of ManagedHandle.handles, all ManagedHandle instances will be kept alive by the managed GC. However, when the "Java peer" of ManagedHandle is finalized/cleaned up/whatever, then the ManagedHandle instance will be removed from ManagedHandle.handles.

The setup:

class MyObject : Java.Lang.Object {}

var o = new MyObject();

A problem here is that in "steady state" nothing can be collected: managed o has a GREF keeping the Java instance alive, which would keep the ManagedHandle alive. There's nothing to "break" the circular reference.

Here, we could have JniValueManager.CollectPeers() toggle all handles and trigger a Java-side GC, which would (eventually) allow the Java-side ManagedHandle to be collected, which would remove the ManagedHandle.handles entry, which would allow it to be collected. However, the timing here feels "ugly".

More pondering required.

jonpryor avatar Jul 28 '21 22:07 jonpryor

Quasi-related: https://github.com/jonpryor/java.interop/commits/jonp-registration-scope

jonpryor avatar Oct 27 '23 17:10 jonpryor