Add arena parameter to various APIs

[dcharkes]

I wonder if it would make sense for all APIs that return JReferences to add an arena optional named argument:

map.put('Hello'.toJString()..deletedIn(arena), helloExample);

would become

map.put('Hello'.toJString(arena: arena), helloExample);

Pros:

• You get a hint in the API that the returned value is something you might want to do resource-management for.
• The implementation can use the arena internally, if the implementation somehow throws, then we have no worries about releasing resources.
• It teaches library writers wrapping native resources to make their API work with arenas, instead of having users wrap stuff with arenas as afterthought.

Cons:

• We need to consistently add it everywhere in the API. Not an issue for the generated part, but painful for the package:jni handwritten parts.
• Resource management is no longer orthogonal to returning JReferences (or other things which need to be released).
• Library writers will need to write arena parameters everywhere. (But maybe that's a good thing, see last pro.)

@HosseinYousefi This is if we continue the current approach for using arena's. As discussed last week we should also consider:

1. Just letting finalizers do everything (what is the performance penalty?)
2. Relying on some context. With zones in Dart/Flutter, or with widgets in Flutter.

[mahesh-hegde]

We discussed about zones before, and concluded in favor of a simpler approach. But can this be accomplished more elegantly using zone-local variables and scoped callback like using? The implementation can be hidden from user I guess.

[mahesh-hegde]

Can we close this in favor of dart-lang/native#653 ? I think a scoped API will be better than relying on programmer discipline.

(The final design might be still taking an optional named parameter inside the scope-enclosed methods)

[mahesh-hegde]

I think this can be closed now. cc: @HosseinYousefi.

[HosseinYousefi]

@dcharkes Coming back to this. Can't we use withZoneArena here? We can make every single object register itself via automatically to zoneArena (if it exists) via releasedBy.

Both withZoneArena and zoneArena can be modified a bit and live in package:jni as we don't really need to access the arena itself.

withZoneArena -> jAutoRelease(() {}):

R jAutoRelease<R>(R Function() computation) {
  final arena = Arena();
  var arenaHolder = [arena];
  bool isAsync = false;
  try {
    return runZoned(() {
      final result = computation();
      if (result is Future) {
        isAsync = true;
        return result.whenComplete(() {
          arena.releaseAll();
        }) as R;
      }
      return result;
    }, zoneValues: {#_jArena: arenaHolder});
  } finally {
    if (!isAsync) {
      arena.releaseAll();
      arenaHolder.clear();
    }
  }
}

And users don't really need to access zoneArena, so it can live in internal_helpers_for_jnigen.dart (I really need to change this name :P)

Arena? get jZoneArena {
  final arenaHolder = Zone.current[#_jArena] as List<Arena>?;
  if (arenaHolder == null) {
    return null;
  }
  if (arenaHolder.isNotEmpty) {
    return arenaHolder.single;
  }
  throw StateError('Arena has already been cleared with releaseAll.');
}

This makes it relatively easy for users to make certain blocks auto-release instead of spamming ..releasedBy every where.

jAutoRelease(() {
  final foo = Foo();
  print(foo);
});
// foo is released!

All .fromRef constructors register themselves to the arena:

Foo.fromRef(super.reference) {
  final zone = jZoneArena;
  if (zone != null) releasedBy(zone);
}

A better name for jAutoRelease is appreciated!

cc/ @mkustermann

[brianquinlan]

Do you have an example where using arenas is more clear or more performant than releaseOriginal?

[HosseinYousefi]

Do you have an example where using arenas is more clear or more performant than releaseOriginal?

It's not only for releaseOriginal. It's definitely not more performant or more clear.

It's just syntactic sugar. Adding it makes every jni call slightly less performant since it has to lookup a key in the current zone. Also assigning an object created within the zone to some field outside is not possible as the object will be released.

It is useful for releasing a bunch of java objects at once without repeating release or releaseOriginal.

Of course not releasing is not leaking as the native finalizer takes care of releasing them but sometimes you want to eagerly release, maybe because the method can be called many times in a loop.

For example:

final jstr = "hello".toJString();
final str = jstr.toDartString(releaseOriginal: true);
final foo = Foo();
final bar = foo.bar();
foo.release();
return bar;

Turns to:

jAutoRelease(() {
  final jstr = "hello".toJString();
  final str = jstr.toDartString();
  final foo = Foo();
  return foo.bar();
});

Using arena:

using((arena) {
  final jstr = "hello".toJString()..releasedBy(arena);
  final str = jstr.toDartString();
  final foo = Foo()..releasedBy(arena);
  return foo.bar();
});

Because of the cons mentioned, I only want to introduce this feature if our users actually gain some meaningful benefit from it.

[mkustermann]

Because of the cons mentioned, I only want to introduce this feature if our users actually gain some meaningful benefit from it.

:+1: Without noticeable user demand, we may not want to introduce it.

But what I think we should do invest in debugging tools to allow users to identify situations when lots of java handles get "leaked" in the sense that code doesn't need them anymore and we rely on GC to close them. I'm worried that users aren't aware of the issue, are not manually closing them, but instead rely on GC, but GC isn't guaranteed to run frequently and there's a 64k limit on the handles. In testing the issue may not occur so apps may get random errors after deployed to end users.

[HosseinYousefi]

But what I think we should do invest in debugging tools to allow users to identify situations when lots of java handles get "leaked" in the sense that code doesn't need them anymore and we rely on GC to close them.

We could use a devtools extension for this: https://github.com/dart-lang/native/issues/1034 I presume that I would use vm_services here.

[HosseinYousefi]

Moving this to backlog for now.