dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.22k stars 1.57k forks source link

Provide a mechanism in VM that allows embedders to free native resources on isolate shutdown (to avoid relying on GC) #51387

Open mkustermann opened 1 year ago

mkustermann commented 1 year ago

Native resources (e.g. files) should ideally be managed by the developer and freed once not needed anymore. We do support letting those resources be freed automatically using weak persistent handles / finalizable handles. Doing so will make the GC eventually free up the native resources.

In context of isolates it may be desirable that unused native resources get eagerly freed on isolate shutdown - instead of waiting for the next GC to happen.

As such we could provide a mechanism in the VM that allows embedders (which e.g. implement dart:io) to free resources eagerly on isolate shutdown.

See for motivating use case: https://github.com/flutter/flutter/issues/120535

mkustermann commented 1 year ago

/cc @brianquinlan @aam

aam commented 1 year ago

Another suggestion that @rmacnak-google had was to move PersistentHandle(kept in ApiState) off isolate groups to isolates, so that associated finalizers are called when isolate that created a handle exits. There is one use of PersistentHandles that need to be adjusted if we do this: Isolate.exit(sendPort, object) uses such handle to pass produced value to the target isolate.

mkustermann commented 1 year ago

Another suggestion that @rmacnak-google had was to move PersistentHandle(kept in ApiState) off isolate groups to isolates, so that associated finalizers are called when isolate that created a handle exits.

As part of fixing https://github.com/dart-lang/sdk/issues/42312 we have settled on a design where we have auto-deleting finalizable handles and non-auto-deleting weak persistent handles. For the ladder: Once GC collects a weak persistent handle it will invoke the associated callback with peer pointer. That callback is allowed to call Dart_DeletePersistentHandle and Dart_DeleteWeakPersistentHandle. We'd break this if we made the handles isolate-specific: Since the GC runs with entered IG but has no active isolate, calls to those Dart_Delete*Handle() functions only have access to the isolate group (and we surely want to avoid linear scan to find out which isolate a handle belongs to).

The way one could work around this is to make the persistent handles store a back-pointer to the isolate. So the handle deletion functions would find the right isolate to delete the handle from at expense of an extra word for each handle.

It would make our isolates a bit more heavyweight (e.g. sizeof(dart::ApiState) == 3280 atm) and more cumbersome to use: Already today we have customers that rely on the handles being per-isolate group: It allows e.g. implementing a synchronous cache of immutable bytes (using our deeply immutable UnmodifiableUint8List) in C++ (using persistent handles) that many isolates can access synchronously via lookup/get-or-insert operations.

We could consider having both: handles that are per-isolate group and ones that are per-isolate. wdyt?

a-siva commented 1 year ago

It would make our isolates a bit more heavyweight (e.g. sizeof(dart::ApiState) == 3280 atm) and more cumbersome to use: Already today we have customers that rely on the handles being per-isolate group: It allows e.g. implementing a synchronous cache of immutable bytes (using our deeply immutable UnmodifiableUint8List) in C++ (using persistent handles) that many isolates can access synchronously via lookup/get-or-insert operations.

We could consider having both: handles that are per-isolate group and ones that are per-isolate. wdyt?

There seems to be some reservations about having per-isolate-group handles. Is this use case that you describe above of sharing deeply immutable objects in C++ using per-isolate-group-handles something that is used pervasively in some customer code? The same sharing could probably also be achieved by creaing per-isolate handles I would presume.

mkustermann commented 1 year ago

There seems to be some reservations about having per-isolate-group handles.

Could you elaborate a bit more on those reservations?

Is this use case that you describe above of sharing deeply immutable objects in C++ using per-isolate-group-handles something that is used pervasively in some customer code?

This is one use case I'm aware - there may be more I don't know about. Wouldn't call it "pervasive".

The same sharing could probably also be achieved by creating per-isolate handles I would presume.

The point is to share those unmodifiable bytes across isolates. The cache continues to exist and continues to have handles to bytes - even if an isolate dies and a newly created one performs lookups in the cache. So I don't think per-isolate handles would be sufficient here.

One could work around this by managing the memory manually by malloc() / free() & handing out raw pointers to dart isolates which can use dart:ffi s Uint8List bytes = Pointer<Uint8>.asTypedList() to access the shared bytes. Though that opens up possibility for bugs, since clearing a cache entry in C++ (and freeing the buffer) may result in use-after-free errors if Dart code still accesses it. i.e. one is solely reliant on manual memory management. It will also prevent our debugging tools to work correctly, since e.g. heapsnapshots will no longer contain information about the bytes.

If we had separate "isolate group handle"s one could restrict them to point only to deeply immutable objects (just as we only allow sharing deeply immutable objects in inter-isolate message passing).

a-siva commented 8 months ago

https://github.com/flutter/flutter/issues/120535 is blocked on this, we don't seem to have an owner for this issue.