Skip to content
This repository has been archived by the owner on Jan 17, 2024. It is now read-only.

Expose pointer to free from allocators #203

Merged
merged 6 commits into from
Aug 8, 2023
Merged

Expose pointer to free from allocators #203

merged 6 commits into from
Aug 8, 2023

Conversation

mraleph
Copy link
Contributor

@mraleph mraleph commented Aug 7, 2023

This enables to write code like this:

final buf = malloc.allocate<Uint8>(len).asTypedList(len, finalizer: malloc.nativeFree);

@mraleph mraleph requested a review from dcharkes August 7, 2023 09:46
@dcharkes
Copy link
Contributor

dcharkes commented Aug 7, 2023

Closes: dart-lang/native#910

@dcharkes
Copy link
Contributor

dcharkes commented Aug 7, 2023

@mraleph How about the interplay of Arena and the two allocators you've added the functionality to?

Are we expecting devs to mix and match allocators?

  using((arena) {
    final str = "myString".toNativeUtf8(allocator: arena);
    final buf = malloc<Uint8>(len).asTypedList(len, finalizer: malloc.nativeFree);
  });

In general we have a question about combining finalizers and eager finalization (dart-lang/sdk#49906), but in this specific case, the native finalizer can't be cancelled, so there's no reason to use the arena for the list backing the typed data.
It feels weird, using two allocators, but, I think it's the right thing to do.

Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with comments.

stdlib.lookup('CoTaskMemFree');
final WinCoTaskMemFree winCoTaskMemFree = winCoTaskMemFreePtr.asFunction();

abstract class AllocatorExposingNativeFinalizer implements Allocator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add interface keyword.

We could consider making it just a marker interface and not implementing Allocator and making the allocators types in their own right (then the name would be NativeFreeable). But having it implement the allocator makes it useful for specifying the type of malloc and calloc. So that's probably better.

lib/src/allocation.dart Outdated Show resolved Hide resolved
* Bump version and update CHANGELOG.md
@mraleph
Copy link
Contributor Author

mraleph commented Aug 7, 2023

Are we expecting devs to mix and match allocators?

I think this change specifically addresses non-arena based usage, if you have malloc allocated memory and want to make sure you automatically free it (e.g. when you pass data you get through FFI to a consumer which works with typed data).

@dcharkes
Copy link
Contributor

dcharkes commented Aug 7, 2023

cc @lrhn for a potentially better name for NativeFreeableAllocator.

lgtm from my side.

Copy link

@lrhn lrhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, this feels a little to much like a hack, more than a coherent design.
You need a function, so you make the existing class, which happens to know about the function, expose it, in its public API.

I have many ideas for how to completely redesign Allocator, but a more conservative design would be to expose Malloc and Calloc classes (final and with private constructors), and have them expose the native free function as a static getter.

Then you have

final class Malloc implements Allocator {
  Pointer(T) alloc<T>(int count) { ... }
  void free(Pointer allocation) { ... }
  static final Pointer<NativeFunction<Void Function(Pointer)>> nativeFree = ...;
  static final Pointer<NativeFunction<Pointer Function(IntPointer)>> nativeAlloc = ...;
}
// ....
const Malloc malloc = MallocAllocator();

and similarly for CallocAllocator where nativeAlloc takes two arguments.

That gives you a link between the Allocator instance and its functions, without exposing them from the object itself.

Or if you prefer, drop the NativeFreeableAllocator type, and expose CallocAllocator and MallocAllocator types that each have instance getters for nativeFree and nativeAlloc, but not shared supertype.

Then you can't abstract over the NativeFreeableAllocator and get to its nativeFree, but it's an abstraction that doen't work for alloc, and which feels glued on the Allocator API. I'd see if it could work before introducing a shared supertype just for nativeFree.

lib/src/allocation.dart Outdated Show resolved Hide resolved
lib/src/allocation.dart Show resolved Hide resolved
lib/src/allocation.dart Outdated Show resolved Hide resolved
lib/src/allocation.dart Outdated Show resolved Hide resolved
lib/src/allocation.dart Outdated Show resolved Hide resolved
@dcharkes
Copy link
Contributor

dcharkes commented Aug 7, 2023

I have many ideas for how to completely redesign Allocator, but a more conservative design would be to expose Malloc and Calloc classes (final and with private constructors), and have them expose the native free function as a static getter.

Hm, I had the same thought in the comments (but with a marker interface in case someone would want to write generic code).

Then you can't abstract over the NativeFreeableAllocator and get to its nativeFree, but it's an abstraction that doen't work for alloc, and which feels glued on the Allocator API.

The use cases that we have don't require a shared super type.

final buf = malloc.allocate<Uint8>(len).asTypedList(len, finalizer: malloc.nativeFree);

Making a Malloc and Calloc type would work for these use cases.

@mraleph
Copy link
Contributor Author

mraleph commented Aug 8, 2023

I have many ideas for how to completely redesign Allocator, but a more conservative design would be to expose Malloc and Calloc classes (final and with private constructors), and have them expose the native free function as a static getter.

I am not sure I like this proposal - it makes the code look strange: allocate and free are also effectively static, but they are still instance methods because Dart does not allow to abstract over static methods. So you end up with the code like this:

final x = malloc.allocate<Uint8>(len).asTypedList(len, finalizer: Malloc.nativeFree);

Notice there is malloc and Malloc in the same line which looks somewhat weird to me.

Or if you prefer, drop the NativeFreeableAllocator type, and expose CallocAllocator and MallocAllocator types that each have instance getters for nativeFree and nativeAlloc, but not shared supertype.

I can do this - I don't think any other major changes are worth it.

Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a test in test/allocation_test.dart.

lib/src/allocation.dart Show resolved Hide resolved
@dcharkes dcharkes merged commit e2c01a9 into main Aug 8, 2023
4 checks passed
@dcharkes dcharkes deleted the expose-free branch August 8, 2023 09:14
@dcharkes
Copy link
Contributor

dcharkes commented Aug 8, 2023

Thanks @mraleph !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants