Arena use with Java references

[dcharkes]

Problem

When using JNI, references to Java objects need to be deleted. Manual management leads to the following type of code.

  testWidgets("Long.intValue() using JniObject", (tester) async {
    final longClass = jni.findJniClass("java/lang/Long");
    final longCtor = longClass.getConstructorID("(J)V");
    final long = longClass.newObject(longCtor, [176]);

    final intValue = long.callIntMethodByName("intValue", "()I", []);
    expect(intValue, equals(176));

    long.delete();
    longClass.delete();
  });

This type of code smells like it should use the Arena from package:ffi.

https://github.com/dart-lang/ffi/blob/master/lib/src/arena.dart

The question is what API we should use.

API 1: Extension method on Arena

  testWidgets("Long.intValue() using JniObject", (tester) async {
    using((Arena arena) {
      final longClass = arena.manage(jni.findJniClass("java/lang/Long"));
      final longCtor = longClass.getConstructorID("(J)V");
      final long = arena.manage(longClass.newObject(longCtor, [176]));

      final intValue = long.callIntMethodByName("intValue", "()I", []);
      expect(intValue, equals(176));
    });
  });

Pro:

• Conceptually makes it very clear that the object returned from manage is managed by the arena so that the API user doesn't have to worry about managing it.

Con:

• It requires the API user to write arena.manage before the thing they are thinking about.

API 2: Method on objects with delete

  testWidgets("Long.intValue() using JniObject", (tester) async {
    using((Arena arena) {
      final longClass = jni.findJniClass("java/lang/Long")..deletedBy(arena);
      final longCtor = longClass.getConstructorID("(J)V");
      final long = longClass.newObject(longCtor, [176])..deletedBy(arena);

      final intValue = long.callIntMethodByName("intValue", "()I", []);
      expect(intValue, equals(176));
    });
  });

Pro:

• The methods delete and deletedBy are clearly related.

Con:

• .. feels like it is easily forgotten.
• deletedBy doesn't feel like a natural method name. Should it be deleter? setDeleter?

Side note: should it be delete? It is DeleteGlobalRef in JNI, but if we want to talk about the abstraction of native resource management, we've used release as verb. Not free or delete.

API 3: Optional arena parameter

  testWidgets("Long.intValue() using JniObject", (tester) async {
    using((Arena arena) {
      final longClass = jni.findJniClass("java/lang/Long", arena: arena);
      final longCtor = longClass.getConstructorID("(J)V");
      final long = longClass.newObject(longCtor, [176], arena: arena);

      final intValue = long.callIntMethodByName("intValue", "()I", []);
      expect(intValue, equals(176));
    });
  });

Pro:

• Most concise

Con:

• Generated for a huge API surface

This approach would be similar to the optional allocator parameter in toNativeUtf8.

https://github.com/dart-lang/ffi/blob/master/lib/src/utf8.dart#L81

API 4: NativeFinalizers

We could attach a NativeFinalizer to all objects and have them be cleaned up automatically.

  testWidgets("Long.intValue() using JniObject", (tester) async {
    final longClass = jni.findJniClass("java/lang/Long");
    final longCtor = longClass.getConstructorID("(J)V");
    final long = longClass.newObject(longCtor, [176]);

    final intValue = long.callIntMethodByName("intValue", "()I", []);
    expect(intValue, equals(176));
  });
  // Magic, at some point the GC does cleanup...

However, even in that case we could keep delete and Arena support. Those would cancel the native finalizer.

@mahesh-hegde and I are slightly leaning towards API 2.
@liamappelbe @lrhn @mkustermann WDYT?

[dcharkes]

Side note that might be a bit too far fetched: could an allocator optional argument also take care of deciding whether to have a global or local JNI reference?

[mahesh-hegde]

Side note that might be a bit too far fetched: could an allocator optional argument also take care of deciding whether to have a global or local JNI reference?

If the optional argument is implemented, it has to be generated as a param declaration in dart binding methods anyway.

So can as well pass an extra argument to C bindings.

The question is whether it's worth the extra code, since local references aren't theoretically consistent with dart threading anyway.

[lrhn]

The best code is the one you don't have to write.

Because of that, I like the native finalizer approach, if it works.

It might still be possible to use an Arena based approach with no visible syntax, if the jni object and the testWidget method cooperates.

What if testWidgets run the body in a zone with a new "current arena", which is then cleaned up when the body ends (asynchronously). The current arena can be stored in the zone, and accessed as Arena.current, and the jni object then looks up Arena.current and registers everything in it (if there is an arena).

If jni becomes arena-aware, I'd also allow providing a an explicit arena: ... parameter, to override using the Arena.current default.

If we can't get that to work, then some kind of explicit registration is needed.
A precedence for that would be the ACX Disposer class, or maybe there is something similar in Flutter.

[mahesh-hegde]

It might still be possible to use an Arena based approach with no visible syntax, if the jni object and the testWidget method cooperates.

As I understand you're proposing writing something like

testWidgetsManaged(Jni jni, void Function() callback) {
// register an arena, and set it as default arena in `jni`
callback()
// delete the arena, and remove it from jni
}

But testWidgets is an example picked from integration test. In reality jni.newInstance can be called in any user defined method. So abstracting that away is not a choice.

I'm leaning towards setDeleter method (2), since that will let jnigen generate less code (by volume). But native finalizers maybe a realistic option depending on cost and how many references an average use case will need.

[dcharkes]

Because of that, I like the native finalizer approach, if it works.

I think it should.

Also performance should be good, but we should verify that.

• Finalizer and NativeFinalizer benchmarks sdk#49904

I've filed a separate issue for implementing finalizers:

• https://github.com/dart-lang/jnigen/issues/50

If finalizers have significant overhead or it turns out that theres another issue with the finalizer approach, we can consider API 1-3 again.

It might still be possible to use an Arena based approach with no visible syntax

Zones are much harder for users to understand. So I'd want to avoid that. With memory allocation we've got a fairly clean design that just enables arena<Int8>() instead of malloc<Int8>(), which is both explicit and concise. and the toNativeUtf8 is explicit, but a little bit less concise.

[liamappelbe]

I also prefer the native finalizer approach. This is what we do for the objective C bindings. Maybe I'm missing something, but I don't see what the argument against native finalizers are.

[mraleph]

I think finalizers works for global references, but don't really fit local references that well. Local references are supposed to be used in the scoped fashion just like Dart VM handles. My personal inclination here would be to wrap PushLocalFrame and PopLocalFrame into a closure based API, i.e. some variation of

JObject withLocalFrame(JObject Function() body, {int capacity = 10}) {
  JNI_PushLocalFrame(env, capacity);
  return JNI_PopLocalFrame(env, body());
}

withLocalFrame(() {
  // Local references created here would be automatically freed at 
  // the end of the scope.
}, capacity: ...)

[dcharkes]

Interesting, if we have thread-pinning in the Dart VM we could use the local JNI references in such a way.

We currently do not have thread-pinning in the VM, so we're not guaranteed to that a next JNI call from Dart will be on the same thread. This means we cannot use the local references from JNI. We're currently refactoring the jni helper package and the code generator to only use global references. We should maybe revisit that decision once we have thread-pinning.

[mraleph]

Interesting, if we have thread-pinning in the Dart VM we could use the local JNI references in such a way.

If the code is synchronous then thread pinning is not a concern - synchronous code is naturally pinned to a single thread.

Note that withLocalFrame has a non-generic return value and consequently does not allow Future, so you naturally can't use withLocalFrame with asynchronous code.

[mahesh-hegde]

Hi @mraleph,

1. What's the extent of same-thread guarantee in dart? Apart from async calls, what are the places where thread can change between 2 JNI calls (and local reference be invalidated)?

2. Initially, package:jni in this project was to support both local & global references. But since it will complicate the API design (mainly structure of generated bindings) as well as being unclear about same-thread guarantees, we decided to go with global refs exclusively.

In fact the current PR dart-archive/jnigen#53 completes this change.

I wonder if there's a clean design such that generated binding API (which now mirrors java classes and methods exactly), can be told to return global or local reference at call site, without generating an additional parameter and corresponding code at each method.

(Although this is not strictly necessary since local reference can be converted to global in Dart code later. I am just wondering where is the local optimum of designing this API.)

3. Also, do you know about the performance impact of local vs global references? It's something I have read about but have no concrete data on it.

[mraleph]

What's the extent of same-thread guarantee in dart? Apart from async calls, what are the places where thread can change between 2 JNI calls (and local reference be invalidated)?

Well, this is not documented, but for foreseeable future if the stack is not empty then execution can't switch between threads. All synchronous code will execute to completion on the same thread.

This might change at some point if we decide to implement some variant of lightweight threads or fibers, but this is far out.

I think in ART both local and global references are backed by the same structure called indirect reference table, https://cs.android.com/android/platform/superproject/+/master:art/runtime/indirect_reference_table.h;drc=dd6f7c69627e6d24c2cc026654f5ca118224f6db;l=46, so their performance characteristics might be pretty close. Though it would help if we did some benchmarking.

At the very least if you "globalise" all references that means you are effectively allocating twice as many references to begin with. You also risk extending lifetimes of some objects (unless you have some sort of scoped API - like the one @dcharkes describes above with arenas).

I do agree that choosing the right API here is difficult. The main concern here is around async code and users storing references to Java objects in Dart objects. This transparently works with global references but will not work with local references.

[mahesh-hegde]

The main concern here is around async code and users storing references to Java objects in Dart objects.

Yeah. If local reference is accidentally used in another thread, I think it will segfault directly. I don't think there's a way to catch it reliably, on Android, CheckJNI will produce a stack trace but it's not as reliable as throwing an exception.

In the beginning of this GSoC project the API I designed did use local references by default and required the user to manually create global reference to use across async calls, but looking back, it was a bad choice. It's very easy to segfault accidentally.

[dcharkes]

their performance characteristics might be pretty close. Though it would help if we did some benchmarking.

I suggested this earlier as well.

The best code is the one you don't have to write.

With this principal we should go for Finalizables with global references. It works out of the box and is the least code to write for all use cases. Then we can add options (local vs global, no finalizer vs finalizer) to either the generated code (in form of optional args) or to the generator later when it proves necessary for certain use cases.

[mahesh-hegde]

@dcharkes can this be closed? Now we have NativeFinalizer and deletedIn.

[dcharkes]

Yes, its basically API 2 with a better name. :)