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

Pointer.asTypedList should allow user to specify a finalizer. #50507

Closed
mraleph opened this issue Nov 18, 2022 · 8 comments
Closed

Pointer.asTypedList should allow user to specify a finalizer. #50507

mraleph opened this issue Nov 18, 2022 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi

Comments

@mraleph
Copy link
Member

mraleph commented Nov 18, 2022

Proposed extension:

XyzList Pointer<Xyz>.asTypedList(int length, {NativeFinalizerFunction finalizer})

You can't put typed list into NativeFinalizer because they don't extend Finalizable (even though they behave a bit like one - we never cache internal data pointer and let typed data die).

Users currently work around this limitation by creating a Finalizable object and linking its lifetime to that of typed data through Expando. It would be nice if such workarounds were unnecessary

We should take this into account when exposing external strings as well.

/cc @dcharkes
/cc @Haidar0096

@mraleph mraleph added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi labels Nov 18, 2022
@Haidar0096
Copy link
Contributor

Currently, the workaround to achieve zero-copying of an array which was created using native C code is something like this:

First I define:

/// Represents a memory resource on the heap which will be freed when the associated dart object is garbage-collected.
class _MagickRelinquishableResource implements ffi.Finalizable {
  static final ffi.NativeFinalizer _finalizer = ffi.NativeFinalizer(_dylib.lookup('magickRelinquishMemory'));

  /// Represents a map with weak keys. The keys of the map will be dart objects, and the values will be
  /// `_MagickRelinquishableResource` objects (which are also `Finalizable` objects). When the dart object (the key)
  /// is garbage-collected, the associated `_MagickRelinquishableResource` object (the value) will be
  /// garbage-collected, and the native cleanup function will be invoked, which will cause the native memory to be freed.
  static final Expando<_MagickRelinquishableResource> _relinquishables = Expando();

  _MagickRelinquishableResource._();

  /// Registers [ptr] to be freed when [obj] is garbage-collected.
  ///
  /// For more info on the other params, see [ffi.NativeFinalizer.attach]
  static void registerRelinquishable(TypedData obj, ffi.Pointer<ffi.Void> ptr,
      {ffi.Pointer<ffi.Void>? detach, int? externalSize}) {
    _MagickRelinquishableResource finalizable = _MagickRelinquishableResource._();
    _finalizer.attach(finalizable, ptr, detach: detach, externalSize: externalSize);
    _relinquishables[obj] = finalizable;
  }
}

Then I use it to zero-copy arrays like this:

  Uint8List? magickGetImageProfile(String name) {
    // call some native code to get a pointer ...
    final Uint8List profile = profilePtr.cast<ffi.Uint8>().asTypedList(length);
    _MagickRelinquishableResource.registerRelinquishable(profile, profilePtr.cast());
    return profile;
  }

@dcharkes
Copy link
Contributor

You can't put typed list into NativeFinalizer because they don't extend Finalizable (even though they behave a bit like one - we never cache internal data pointer and let typed data die).

It would also be kind of weird to attach a native finalizer to an arbitrary typed list, since one does not have access to the backing store through that API.

Proposed extension:

XyzList Pointer<Xyz>.asTypedList(int length, {NativeFinalizerFunction finalizer})

When making a typed list from a pointer we do have access to the backing store, so I agree that that is the right place to add this API.

@dcharkes dcharkes changed the title Pointer.asTypedList should allow user to specify an finalizer. Pointer.asTypedList should allow user to specify a finalizer. Apr 19, 2023
@dcharkes
Copy link
Contributor

dcharkes commented May 1, 2023

Passing a NativeFinalizerFunction has two issues:

  1. We need to create a new NativeFinalizer object for every asTypedList invocation. (The GC recognizes NativeFinalizer with it's FinalizerEntries, we wouldn't want to introduce a new data structure just for this use case.)
  2. The NativeFinalizers wouldn't be kept alive, unless we stick them in a global set, but then we'd never GC the finalizers.

Possibly a better design is to make the API:

XyzList Pointer<Xyz>.asTypedList(int length, {NativeFinalizer finalizer});

That would enable reuse of the finalizer object, and lets the just stick that single object in a global.

@mraleph
Copy link
Member Author

mraleph commented May 1, 2023

  1. We need to create a new NativeFinalizer object for every asTypedList invocation. (The GC recognizes NativeFinalizer with it's FinalizerEntries, we wouldn't want to introduce a new data structure just for this use case.)

I would have just expected us to create a finalizable handle with a peer that points to a struct which looks like this:

struct Peer {
  void* ptr;
  NativeFinalizerFunction_t finalizer;
};

and associated finalizer:

static void FinalizeHandle(void* whatever, void* peer) {
  auto p = static_cast<Peer*>(peer);
  (p->finalizer)(p->ptr);
  free(peer);
}

Even better we could consider making our finalizable persistent handles to support simple finalizer signatures (e.g. in addition to supporting void (*)(void*, void* peer) it could support void (*)(void*) out of the box and have a bit on the handle which determines which one is called. Then we don't need to allocate Peer indirection at all.

@dcharkes
Copy link
Contributor

dcharkes commented May 1, 2023

Ah I was thinking to reuse the logic in the heap/GC rather than finalizable handles.

Using finalizable handles would likely be slightly slower because it requires some VM state transitions. We optimzed away from runtime entries because it was slow [0]. Maybe we can do it via an FfiNative, which would be faster than a runtime entry. It cannot be a leaf call because we need Dart API access. WDYT about performance?

From an API point of view either option could work, either providing a NativeFinalizer or a NativeFinalizerFunction.
Using a NativeFinalizer would mean users can eagerly detach if they desire (which is likely not the case as they are probably passing the typed list into code they don't have control over).

Even better we could consider making our finalizable persistent handles to support simple finalizer signatures (e.g. in addition to supporting void (*)(void*, void* peer) it could support void (*)(void*) out of the box and have a bit on the handle which determines which one is called. Then we don't need to allocate Peer indirection at all.

Déjà vu, didn't I add such a bit in my first finalizer prototype where I reused the finalizable handles to handle a different signature? It feels very familiar.

[0] https://dart-review.googlesource.com/c/sdk/+/221360

@dcharkes
Copy link
Contributor

dcharkes commented May 2, 2023

We already have Dart_NewExternalTypedDataWithFinalizer. So I'm assuming all interaction from Dart code keeps typed data obljects alive, meaning that we do not have premature finalization issues. As such, the typed data does not have to be marked Finalizable, in neither of the two above implementation options.

@mraleph
Copy link
Member Author

mraleph commented May 2, 2023

Using finalizable handles would likely be slightly slower because it requires some VM state transitions. We optimzed away from runtime entries because it was slow [0]. Maybe we can do it via an FfiNative, which would be faster than a runtime entry. It cannot be a leaf call because we need Dart API access. WDYT about performance?

FWIW we can also use as singular NativeFinalizer stored in the static variable in the dart:typed_data to achieve a similar effect, e.g. it can either use an indirection similar to Peer described above or we can add some hidden internal capabilities to the finalizer where a single finaliser can call many different callbacks. Though FinalizerEntry is already pretty huge and it is not entirely clear to me how to fit one more pointer into it without making it bigger. On non-compressed pointers we could reuse detach - but on compressed pointers that would not have enough space.

@dcharkes
Copy link
Contributor

dcharkes commented May 2, 2023

FWIW we can also use as singular NativeFinalizer stored in the static variable in the dart:typed_data to achieve a similar effect, e.g. it can either use an indirection similar to Peer described above

Ah yes, that would work indeed.

or we can add some hidden internal capabilities to the finalizer where a single finaliser can call many different callbacks. Though FinalizerEntry is already pretty huge and it is not entirely clear to me how to fit one more pointer into it without making it bigger. On non-compressed pointers we could reuse detach - but on compressed pointers that would not have enough space.

Yeah, I'd rather not modify FinalizerEntry.

I think I'm leaning towards your suggestion of sticking a singular NativeFinalizer stored in the static variable in the dart:typed_data. Reusing the GC/heap based implementation seems better to me than using the Dart API based implementation because of the required state transitions in the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi
Projects
None yet
Development

No branches or pull requests

3 participants