Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vm/ffi] TypedData, Pointer, multiple isolates, and releasing native resources #54885

Open
dcharkes opened this issue Feb 12, 2024 · 5 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi type-enhancement A request for a change that isn't a bug

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Feb 12, 2024

Pointers can be converted into external TypedData with the asTypedList.

When sending messages to other isolates:

  • Pointers are simply an address sent to another isolate.
  • external TypedDatas have their backing buffer copied and a free-finalizer attached.

This means that if a user calls asTypedData first, and then sends to another isolate, all native resources are cleaned up correctly both in the case the message is delivered as well as when the message is not delivered. We're assuming Dart takes ownership of the native memory and attaches a finalizer to free the buffer once it's done using it by calling asTypedList with a finalizer immediately on receiving the pointer from native code.

If the user sends the Pointer to another isolate and converts it to a typed-data on the other end, attaching a finalizer there, the native resource does not get finalized if the message never arrives. But it is faster, because no copies of the backing buffer are made. (The user could start sending messages back and forth between both isolates to acknowledge ownership is transferred from one isolate to the other, but that leads to the byzantine generals problem.)

A third strategy is to use a reference counter in native code and let every isolate call the finalizer. However, this is racy, because the source isolate could GC it's reference to the native resource before a reference on the receiving isolate is created increasing the reference count. Then you'd want to increase the reference account before sending, but that would lead to leaked resources if the message never arrives.

I think we have multiple things we should address here.

  1. We should document the difference between sending Pointers to other isolates and sending TypedDatas created from pointers to other isolates.
  2. We could consider adding NativeFinalizers to sending messages that get run if the message is not delivered. That would enable users to implement the refcounter properly.
  3. We could consider introducing something like a IsolateGroupFinalizable which allows one to attach a NativeFinalizer which will run once every copy of the IsolateGroupFinalizable object has been GCed. This would avoid the need for users to implement their own refcounter. (I feel like we discussed this before, but I can't find the relevant GitHub issue.)

cc @mkustermann @mraleph

I think this is a rather common question that has to be dealt with as soon as somewhat longer FFIcalls are involved that users would like to run on a helper isolate. @craiglabenz

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi labels Feb 12, 2024
@a-siva a-siva added the type-enhancement A request for a change that isn't a bug label Feb 14, 2024
@dcharkes
Copy link
Contributor Author

Notes from discussion with @mkustermann:

  • If the object is not mutable, we don't need ref-counting for IsolateGroupFinalizable, instead we can set the ImmutableBit.
  • I'll go and explore a marker interface for IsolateGroupFinalizable that forces (a) the implementors must be sealed classes, (b) all their fields must be final, (c) all their fields must be sharable (maybe limit to int and Pointer for now).
  • IsolateGroupFinalizable implements Finalizable so that we can attach NativeFinalizers. Wait: that doesn't work, because Finalizables are not sendable. We probably need to a BaseFinalizable that's implemented by both IsolateGroupFinalizable and Finalizable and change the NativeFinalizer API to work on BaseFinalizable (names subject to change)

@mraleph This sounds related to Sharables. But likely not all Sharables need NativeFinalizers attached. So, I don't think we should conflate the two concepts. Hence I will go with a different name for now.

@dcharkes
Copy link
Contributor Author

Some more observations together with @mkustermann:

  • Finalizable not being allowed to send through SendPort.send is a runtime check, not a type check. This means we can have deeply immutable objects implement Finalizable. The ImmutableBit should be checked before the implements-Finalizable-check.
  • The name for this marker interface or pragma should not be Shareable. Shareable is a way to indicate that something is implemented in a thread-safe way in Shared Memory Multithreading language#3531. Shareables are not copied but shared across isolates even though mutations on other threads are visible. Shareables are only shared with SendPort.share, not with SendPort.send. DeeplyImmutables are also shared on SendPort.send. The fact that these are shared on send is "invisible" to users because of the immutability. It's not fully invisible to users though, because it can be observed through native finalizers.

@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 27, 2024

Looking at what the VM currently marks as Immutable, some of the types are not marked sealed.

abstract interface class RegExp implements Pattern {

abstract interface class SendPort implements Capability {

abstract interface class Capability {

This means that objects of these types are immutable, but any subtypes are not.
Consequently, this means transitive immutability cannot be inferred by looking at field-types statically.

@lrhn Do we have some documentation somewhere about RegExp, SendPort, and Capability of why we allow implementing them? Should we consider marking them final so that only the dart: libraries can produce instances of these types? Edit: found some documentation https://dart-review.googlesource.com/c/sdk/+/288201, SendPort is implemented in some places.

@lrhn
Copy link
Member

lrhn commented Feb 27, 2024

It's probably that SendPort is implemented in precisely one place, package:isolate, written by me, and which is no longer officially maintained.

If we really want to make SendPort a final class, it's not impossible that we could.

RegExp is likely the same story, there might be implementations. Not absolutely sure, but I know I have written something like ConstRegExp as an example, and someone might have chosen to use it.

I don't think a "deeply immutable" marker interface, which subclasses must then satisfy, is a good idea in general. Having it for specific native-twisted types is probably fine, and having it on final classes doesn't affect anybody else.

Being deeply immutable is an implementation detail, not a property of a type. It's very likely that RegExp is not actually immutable, it can easily have done caching. I know the web implementation does.

@dcharkes
Copy link
Contributor Author

Thanks @lrhn !

It's probably that SendPort is implemented in precisely one place, package:isolate, written by me, and which is no longer officially maintained.

If we really want to make SendPort a final class, it's not impossible that we could.

RegExp is likely the same story, there might be implementations. Not absolutely sure, but I know I have written something like ConstRegExp as an example, and someone might have chosen to use it.

Okay, that's good to know for the future if we come up with use cases.

For now I will just disallow fields typed Capability, SendPort, and RegExp in deeply immutable objects.

I don't think a "deeply immutable" marker interface, which subclasses must then satisfy, is a good idea in general. Having it for specific native-twisted types is probably fine, and having it on final classes doesn't affect anybody else.

Being deeply immutable is an implementation detail, not a property of a type. It's very likely that RegExp is not actually immutable, it can easily have done caching. I know the web implementation does.

I have gone for a pragma so far. And indeed it would be quite ugly to have to add it to double/int/bool/null etc. So let's stick to a pragma.

I presume we say that behavior only observable through Finalizers or NativeFinalizers is considered an "implementation detail" due to the fact that GC is considered an implementation detail from a Dart-language-POV.
(Note that such implementation details are very important when it comes to interop with native code and releasing native resources. Which is an interop-POV and the reason for this GitHub issue.)

copybara-service bot pushed a commit that referenced this issue Mar 7, 2024
This CL introduces a way to mark all instances of a class as deeply
immutable.

In order to statically verify that all instances of a deeply immutable
class are immutable, a deeply immutable classes must have the following
properties:

1. All instance fields must
   1. have a deeply immutable type,
   2. be final, and
   3. be non-late.
2. The class must be `final` or `sealed`. This ensures no
   non-deeply-immutable subtypes are added by external code.
3. All subtypes must be deeply immutable. This ensures 1.1 can be
   trusted.
4. The super type must be deeply immutable (except for Object).

Note that instances of some classes in the VM are deeply immutable
while their class cannot be marked immutable.

* SendPort, Capability, RegExp, and StackTrace are not `final` and
  can be implemented by external code.
* UnmodifiableTypedDataViews do not have a public type. (It was
  recently deprecated.)

See runtime/docs/deeply_immutable.md for more details.

Use case:

This enables attaching a `Dart_FinalizableHandle` to a deeply immutable
object and the deeply immutable object with other isolates in the same
isolate group.

(Note that `NativeFinalizer`s live in an isolate, and not an isolate
group. So this should currently _not_ be used with `NativeFinalizer`s.
See #55062 for making a
`NativeFinalizer.shared(` that would live in an isolate group instead
of in an isolate.)

Implementation details:

Before this CL, the `ImmutableBit` in the object header was only ever
set to true for predefined class ids (and for const objects). After
this CL, the bit can also be set to true for non const instances of
user-defined classes. The object allocation and initialization code has
been changed to deal with this new case. The immutability of a class is
saved in the class state bits. On object allocation and initialization
the immutability bit is read from the class for non-predefined class
ids.

TEST=runtime/tests/vm/dart/isolates/fast_object_copy2_test.dart
TEST=runtime/vm/isolate_reload_test.cc
TEST=tests/lib/isolate/deeply_immutable_*

Bug: #55120
Bug: #54885
Change-Id: Ib97fe589cb4f81673cb928c93e3093838d82132d
Cq-Include-Trybots: luci.dart.try:vm-aot-android-release-arm64c-try,vm-aot-android-release-arm_x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-aot-obfuscate-linux-release-x64-try,vm-aot-optimization-level-linux-release-x64-try,vm-appjit-linux-debug-x64-try,vm-asan-linux-release-x64-try,vm-checked-mac-release-arm64-try,vm-eager-optimization-linux-release-ia32-try,vm-eager-optimization-linux-release-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-fuchsia-release-x64-try,vm-kernel-linux-debug-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-reload-rollback-linux-debug-x64-try,vm-ubsan-linux-release-x64-try
Cq-Include-Trybots: dart-internal/g3.dart-internal.try:g3-cbuild-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354902
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants