[vm/ffi] Enable sending NativeFinalizers over Isolate.exit?

[dcharkes]

After https://dart-review.googlesource.com/c/sdk/+/354902, we will allow sending deeply immutable Finalizables to other isolates in the same group. However, if a NativeFinalizer lives in an isolate that exits, but deeply immutable Finalizables are still alive in other isolates, the native resources are prematurely finalized. Therefore, we should enable sending NativeFinalizers to another isolate via Isolate.exit.

Currently, sending NativeFinalizerss is disallowed:

sdk/runtime/vm/object_graph_copy.cc

Line 873 in 83ba591

HANDLE_ILLEGAL_CASE(NativeFinalizer)

sdk/runtime/vm/isolate.cc

Line 2389 in 83ba591

// TODO(http://dartbug.com/47777): Send and exit support.

Sending a NativeFinalizer to another isolate should have the effect of changing the isolate pointer to the receiving isolate.

Some open questions:

• What is the state in between that the message is send and that it is received? Running finalizers has the side effect of cleaning up the entries in the Dart heap via a message to the isolate. If there is temporarily no isolate while the finalizer is in flight, where should the clean up message be sent? (Presumably we can run the native finalizer while it is in flight from one isolate to another, we do have access to the heap to access the address of the native callback and the token.)
• What is the state if the exit message is never handled?

If we do decide to support this, maybe we should add a NativeFinalizer.adopt(NativeFinalizer other) that will check if the native callback function is equal and then merge all the entries from the other finalizer. This will enable storing a finalizer in a static field and on isolate shutdown sending it to another isolate and merging it into the finalizer that's in the "same" static field already. (The alternative is that one might have a collection of adopted finalizers and can never clean up that collection.)

If we do not decide to support it, probably the best practise is that NativeFinalizers should live on, and Finalizables should be created on, an isolate that will stay alive until the end of the program (the main isolate or platform isolate). cc @liamappelbe @HosseinYousefi Do you have use cases where you want to create a Finalizable on a helper isolate that is subsequently going to exit after you've sent the Finalizable to the main isolate?

cc @mkustermann @mraleph

(Things would be much easier if we could just mark the NativeFinalizer static field as shared across all isolates in the group, a la dart-lang/language#3531, shared final NativeFinalizer = .... We would not need to worry about sending it to another isolate on exit.)

[lrhn]

If you have a finalizer attached to an object, and you send that object through Isolate.exit, will the finalizer stay alive too (being implemented by heap merging would suggest so, but I can see arguments in either direction).
If so, that effectively keeps its callbacks alive, and allows them to run in the remaining isolate.
If not, I also assume the finalizer won't run, because cleaning up a reachable object is bad.

(I should check.)

But this is about native finalizers, which only do callouts to native functions, right?

Here it also matters what happens on Isolate.exit, even if you don't send the finalizer. Will it trigger, stay alive, or quietly go away with its isolate?

Sending (merging) the native finalizer to another isolate with Isolate.exit shoulder change how it behaves much, over not sending it, but it does allow you to interact and update the object during sending.

(Why does a native finalizer need an isolate reference? Is the native code run in a way that makes it being to that isolate?)

Whether an exit message is handled shouldn't matter. It exists, and is delivered. Whether someone looks at it after that is up to them. If it languishes in the buffer of a paused SteamSubscription, that's what someone asked for.

The adopt functionality seems reasonable. Biggest question is whether later additions to the adoptee should be forwarded to the adopter too.

[dcharkes]

Thanks for all the good questions @lrhn! I think I have a slightly better idea flashed out now. See my answers to your questions below.

If you have a finalizer attached to an object, and you send that object through Isolate.exit, will the finalizer stay alive too (being implemented by heap merging would suggest so, but I can see arguments in either direction).

Heap merging yes, but if that finalizer is not saved in a global variable or something, it will be GCed itself. Finalizers that are GCed will never be run.

But this is about native finalizers, which only do callouts to native functions, right?

Conceptually only a native function is called.

Implementation detail is that the finalizer entries need to be removed from the Dart heap, otherwise we would leak memory by accumulating entry objecs. So currently, a message is sent to the isolate to do so. If we were able to remove entries from a Dart Set in C++ code in the VM, or we would change to a different data structure than a Set, we would not need to execute any Dart code after a NativeFinalizer has run.

sdk/sdk/lib/_internal/vm/lib/internal_patch.dart

Lines 268 to 289 in 3ef08e1

	/// All active attachments.
	///
	/// This keeps the [FinalizerEntry]s belonging to this finalizer alive. If an
	/// entry gets collected, the finalizer is not run when the
	/// [FinalizerEntry.value] is collected.
	///
	/// TODO(http://dartbug.com/47777): For native finalizers, what data structure
	/// can we use that we can modify in the VM. So that we don't have to send a
	/// message to Dart to clean up entries for which the GC has run.
	///
	/// Requirements for data structure:
	/// 1. Keeps entries reachable. Entries that are collected will never run
	/// the GC.
	/// 2. Atomic insert in Dart on `attach`. GC should not run in between.
	/// 3. Atomic remove in Dart on `detach`. multiple GC tasks run in parallel.
	/// 4. Atomic remove in C++ on value being collected. Multiple GC tasks run in
	/// parallel.
	///
	/// For Dart finalizers we execute the remove in Dart, much simpler.
	@pragma("vm:recognized", "other")
	@pragma('vm:prefer-inline')
	external Set<FinalizerEntry> get _allEntries;

Here it also matters what happens on Isolate.exit, even if you don't send the finalizer. Will it trigger, stay alive, or quietly go away with its isolate?

It will trigger. We don't want it to quietly go away, it would lead to leaking native resources.

Sending (merging) the native finalizer to another isolate with Isolate.exit shouldn't change how it behaves much, over not sending it, but it does allow you to interact and update the object during sending.

I don't think we can achieve that sending or not sending doesn't change the behavior. We have specified for both Finalizer and NativeFinalizer that if the finalizer object gets GCed that the callbacks never get run not sending is not an option. To prevent native resources from leaking, a NativeFinalizer that is reachable in an isolate that gets shut down is run eagerly.

So sending the native finalizer to a new isolate prevents the native callbacks from being run eagerly. So the behavior is different between sending and not sending.

(Having shared variables from the shared memory multithreading would do what you want! Then it would be the same sending or not sending it.)

Why does a native finalizer need an isolate reference? Is the native code run in a way that makes it being to that isolate?

1. To clear up the data structures in the heap after the native callback has been run. See the same code as linked above. We could investigate changing the data structure so that we do not have to run Dart code (in a specific isolate!) to clean up data structures in the Dart heap. E.g. by making elements removable from a Dart Set in C++ VM code or by not using a Set but still keeping all the operations O(1). Thinking about it, maybe _allEntries could be converted into a doubly linked list, I'd need to think about it a bit more.
2. To run the native finalizers on isolate shutdown.

Whether an exit message is handled shouldn't matter. It exists, and is delivered. Whether someone looks at it after that is up to them. If it languishes in the buffer of a paused SteamSubscription, that's what someone asked for.

I guess if a message is not handled, but stays in flight forever, it does keep all the objects in such message alive, including the NativeFinalizer. So any objects attached that are GCed will have their native callback run.

If we change the _allEntries data structure to a doubly linked list, then we could change the isolate pointer on an entry (and add the finalizer to the isolate.finalizers linked list) to the target isolate immediately. This should have the effect that if a message is not handled before the receiving isolate shutdown, the native callbacks are run on the receiving isolate shutdown. (I'd need to double check that the not handled messages stay alive until the finalizers are run on isolate shutdown.)

The adopt functionality seems reasonable. Biggest question is whether later additions to the adoptee should be forwarded to the adopter too.

That is a good question. Alternatively, calling attach or detach on a NativeFinalizer that has been adopted should throw. I don't really have a strong opinion about this.
(Note we can't really add this to normal Finalizers, as we can't check that the callback closures are equal. Or would it be enough to check that the signatures are equal?)

High level idea then:

• Convert _allEntries to be a doubly linked list, then we don't have to worry about having to clean up the data structure in the Dart heap anymore.
• We'd still need an isolate pointer in a native finalizer (and a list of native finalizers in the isolate) to ensure the callbacks are run for native finalizers that are reachable on isolate shutdown.
• When a native finalizer is sent through Isolate.exit the isolate pointer is immediately set to the target isolate (and the finalizer is immediately added to the target isolate finalizers and removed from the sending isolate finalizers).
• Add adopt to NativeFinalizer API.

[dcharkes]

From a discussion with @HosseinYousefi:

• Migrating a NativeFinalizer to the longest living isolate is a leaky abstraction. We can’t encapsulate JObject properly. Users would need to know about the NativeFinalizer and send it to the longest living isolate with Isolate.exit. It is undesirable to burden users with this.
• Refcounting requires us to add a finalizer on messages send with SendPort.send not delivered. This can also not be encapsulated in package:jni and package:jnigen. The user would need to write the native finalizer argument in the SendPort.send, or we would need to provide JObject.send(SendPort). Both don’t enable writing abstractions (larger object graphs containing JObjects).
  • We could introduce a interface class MessageNotDeliveredNativeFinalizer with a get NativeFinalizerFunction callback. Then if the message is not delivered we have a O(N) traversal of the full graph reachable from the message to see which objects implement this. (We cannot do this on message send, because deeply immutables on send because are O(1).)
• We could consider annotating NativeFinalizers with @pragma('vm:sharable') and allow users to mark static fields with @pragma(‘vm:shared’). Basically we would need to make all operations on NativeFinalizers concurrency safe. (We have some Set and Expando operations internally.)
  • Currently (without shared), we cannot say a native finalizer in an isolate group is going to stay alive forever, because a Dart code writer would not be able to say that this native finalizer object itself can be GCed. With a shared/@pragma('vm:shared') we can.
  • This is basically implementing a subset of Shared Memory Multithreading language#3531, but we would not allow users to annotate things @pragma('vm:sharable').
  • It would be safe to annotate Finalizables with @pragma('vm:deeply-immutable') only if the NativeFinalizer is in a static field marked @pragma('vm:shared').

[mkustermann]

Sending a NativeFinalizer to another isolate should have the effect of changing the isolate pointer to the receiving isolate.

If we have an object sharable across isolates, that object doesn't have an owning isolate. If this sharable object represents a resource, the resource is owned by the isolate group - not by an individual isolate.

=> We may want to disallow attaching native finalizers (with run-eagerly-on-isolate-shutdown semantics) to sharable objects.
=> We may want to introduce NativeFinalizer.sharable instead, which would have the semantics of run-eagerly-on-isolate-group-shutdown and one can only attach those to sharable objects.

[mkustermann]

=> We may want to disallow attaching native finalizers (with run-eagerly-on-isolate-shutdown semantics) to sharable objects.

This is somewhat of a separate issue, but may be worthwhile fixing.

Already today one can create deeply immutable objects (dart constants, but also deeply immutable typed data arrays). Attaching finalizers to them seems not a good idea. @dcharke could you make a CL to disallow that?

[dcharkes]

Okay, I think the conclusion is that we do not want to support sending NativeFinalizers to other isolates. Rather we should support #55062.