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

Build infrastructure to allow changing implementation of native calls to use FFI calls instead of runtime calls #43889

Open
3 of 7 tasks
mkustermann opened this issue Oct 22, 2020 · 8 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi type-enhancement A request for a change that isn't a bug type-performance Issue relates to performance or code size

Comments

@mkustermann
Copy link
Member

mkustermann commented Oct 22, 2020

Native methods in Dart are currently using our runtime call mechanism to call from Dart into C code. There is a significant performance overhead due to this: The native dart function cannot be inlined, we go indirectly through stubs, we box all arguments and make an array on the stack (in order to provide the C function a pointer to an array of arguments) ...

To avoid some of this overhead, we would like to enable calling natives using the existing FFI calling mechanism. That would allow us to pass unboxed primitives (e.g. integers/doubles) directly using C calling convention as well as dart objects via auto-wrapping in handles.

We want to prototype this on natives in Dart's core libraries and later make Flutter use it in dart:ui for faster calls into C (e.g. skia calls).

This will require adding a symbol-lookup mechanism (either declaratively as we do with natives right now, or imperatively) to avoid depending on VM/Embedder symbols to be available at runtime.

Some tasks related to the Ffi Native support:

/cc @mraleph

@mkustermann mkustermann added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-performance Issue relates to performance or code size type-enhancement A request for a change that isn't a bug labels Oct 22, 2020
@mkustermann mkustermann assigned ghost Oct 22, 2020
@mkustermann
Copy link
Member Author

mkustermann commented Nov 3, 2020

To clarify this a bit more, I think we want a solution where:

  • There is a single symbol lookup mechanism
  • The embedder can control how symbols are resolved
  • The dart code for ffi natives should preferably simple, possibly declarative

These are the properties our current natives have and we probably want to have the same properties for ffi natives as well.

@mkustermann
Copy link
Member Author

mkustermann commented Nov 5, 2020

Some notes from offline discussions:

  • We would like to strive to a solution where we can eventually replace our old runtime calls to avoid the need to maintain two mechanisms that achieve the same goal (calling from Dart to C) where one is most likely superior to the other.

  • It might be nice to have a declarative way of defining such ffi natives. One option would be for example using a @FfiNative annotation plus a kernel transformer:

    // Old native in Dart
    int foo(FooBar arg, double arg2) native "Native_foo";
    
    // Old native in Kernel
    @ExternalName("Native_foo")  
    external int foo(FooBar arg, double arg2);
    
    // Ffi native in Dart
    @FfiNative<Int8 Function(Dart_Handle, double)>("Native_foo")
    external int foo(FooBar arg, double arg2);
  • We would like to have one mechanism to lookup symbols - where resolution can be controlled (to some extend) by the embedder.

@ghost
Copy link

ghost commented Nov 18, 2020

@mkustermann, @mraleph
Do we have any proposals for acquiring the corresponding native function pointer (i.e. without native runtime entry)?
Presumably this would involve the 'one mechanism to lookup symbols'.

One approach (which I'm stealing from Daco) is to add something like LoadNativePtrInstr which would do a lookup akin to NativeEntry::ResolveNative in NativeCallInstr.
This would then be responsible for loading the native function pointer, to enable the call to the FFI Trampoline.

One question I have though is whether we have the native function pointers at compile (IL -> asm.) time, or whether we need to do the lookup at runtime?

@mraleph
Copy link
Member

mraleph commented Nov 19, 2020

Do we have any proposals for acquiring the corresponding native function pointer (i.e. without native runtime entry)?

I have not been deeply involved in the discussions - so I am not sure if there are any strong reasons to avoid native runtime entries to begin with, e.g. why not simply have:

Pointer<Void> _resolve(String name) native "Ffi_resolve";

@mkustermann
Copy link
Member Author

Do we have any proposals for acquiring the corresponding native function pointer (i.e. without native runtime entry)?

I have not been deeply involved in the discussions - so I am not sure if there are any strong reasons to avoid native
runtime entries to begin with, e.g. why not simply have:

+1 To Slava's answer. It would be one mechanism to resolve any native function (1 <-> N relationship).

We could later on, go one step further and make symbol resolution faster as well as avoid relying on natives (which we may want to replace entirely in the future) by "injecting" a C function pointer into a global dart field - the dart code could then call this C function directly via FFI (avoiding usage of natives):

// Pointer to a `void* Lookup(Dart_Handle symbol_to_lookup)` function, which
// Dart code can call to resolve a native name.
@pragma('vm:entry-point')
Pointer<NativeFunction<Pointer<Void> Function(Dart_Handle)>> _resolver;

One question I have though is whether we have the native function pointers at compile (IL -> asm.) time, or
whether we need to do the lookup at runtime?

In JIT we could resolve the native function at compile time, though in AOT mode we cannot rely on this.

@ghost
Copy link

ghost commented Nov 25, 2020

Thank you for clarifying. My previous understanding was that natives (even for the resolve function) was off the table, so this was very helpful.

I've put together a PoC in 170092, for a naive kernel transform into calls to a single native resolve function.
The numbers from some quick benchmarking are however not very encouraging with a runtime of ~460% of the baseline.
Though a performance hit is entirely expected since the CL only adds overhead to do the native resolve and then the ffi call.

Since the bottleneck is the native call to 'resolve' on every ffi native function call, we could amortise that away by caching the resulting function pointer, but I think doing so would likely require a static for every ffi native function.
I'd like to double check that (as I believe has previously been mentioned) is also not an option we're willing to accept?

In the meantime I'll have a look at the ffi-based resolver.

@ghost
Copy link

ghost commented Nov 27, 2020

Since the bottleneck is the native call to 'resolve' on every ffi native function call, we could amortise that away by caching the resulting function pointer, but I think doing so would likely require a static for every ffi native function.
I'd like to double check that (as I believe has previously been mentioned) is also not an option we're willing to accept?

From off-thread follow-up:
We're ok with caching the function pointer in fields for every ffi native function as a first step. Though the long-term goal naturally is to get rid of this overhead through other mechanisms.

@ghost
Copy link

ghost commented Nov 30, 2020

Implementing a cached _resolver as mentioned above appears to roughly halve the overhead, though that it still more than 2x the baseline.
I'll start work on modifying the transform to cache the individual function pointers.

dart-bot pushed a commit that referenced this issue Jun 15, 2021
Adds support for marking external functions as @FfiNative's,
which will be called using fast FFI calls.

Resolution happens by calling out to a new embedder provided
Dart_FfiNativeResolver which the embedder can specify via
Dart_SetFfiNativeResolver.

TEST=vm/cc/DartAPI_FfiNativeResolver

Bug: #43889
Change-Id: I3cfff360b05314499a81444b90f4ea0a1b937b0b
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170092
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Jul 14, 2021
This change essentially exposes `asFunction`'s `isLeaf` in
`@FfiNative`, allowing us to declare FFI Natives as leaf calls.

TEST=Adds tests/ffi/ffi_native_test.dart

Bug: #43889
Change-Id: I2a396fae2ab28d21df282f3afb35fa401485ed52
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206375
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Jul 20, 2021
This makes it a compile-time error to add @FfiNative annotation
to any non-static function, such as an instance method.

TEST=tests/ffi/ffi_native_test.dart,pkg/analyzer/test/src/diagnostics/ffi_native_test.dart

Bug: #43889
Change-Id: Ib9ec61345bb47e735ed635c5ceea15ab643f65a6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/207306
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
dart-bot pushed a commit that referenced this issue Jul 28, 2021
Adds GetNativeField(o,i) to access NativeFieldWrapperClass{1-4}
native fields directly in Dart instead of through VM API.
This will allow us to pass `this` as a raw pointer instead of as
a Dart handle which needs to be unwrapped.
This will in turn allow ffi native calls for instance methods.

TEST=Adds vm/cc/DartAPI_NativeFieldAccess

Bug: #43889
Change-Id: I8006c735569fcbe5e49915a4c7e72c4ca85b356e
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/205482
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Aug 9, 2021
Previously the synthetic field that holds the FfiNative
function pointer was injected into the current library.
This change makes sure we instead add the field to the
relevant parent - Class or Library.

TEST=Added regression test for name collision.

Bug: #43889
Change-Id: Ifbf2d70de00e4748c179fe7d626c495675c2b338
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208502
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Aug 9, 2021
Adds a convenience function for working with FfiNatives
where we often end up doing:

class SomeClass extends NativeFieldWrapperClass1 {
  void doThing() => _doThing(Pointer.fromAddress(getNativeField(this)))
  @FfiNative<Void Function(Pointer)>
  external static void _doThing(Pointer self);
}

TEST=Existing.

Bug: #43889
Change-Id: I983ba882574732b71b7ceafcb98b8ce5339e9003
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208503
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
dart-bot pushed a commit that referenced this issue Aug 9, 2021
This reverts commit 2e17bb2.

Reason for revert: Breaks precomp. due to import of dart:ffi.

Original change's description:
> [VM] Adds getNativeFieldPtr(..)
>
> Adds a convenience function for working with FfiNatives
> where we often end up doing:
>
> class SomeClass extends NativeFieldWrapperClass1 {
>   void doThing() => _doThing(Pointer.fromAddress(getNativeField(this)))
>   @FfiNative<Void Function(Pointer)>
>   external static void _doThing(Pointer self);
> }
>
> TEST=Existing.
>
> Bug: #43889
> Change-Id: I983ba882574732b71b7ceafcb98b8ce5339e9003
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208503
> Commit-Queue: Clement Skau <cskau@google.com>
> Reviewed-by: Tess Strickland <sstrickl@google.com>

TBR=cskau@google.com,sstrickl@google.com

Change-Id: I7bc9376f05664fda4fcf27b584081295195d5562
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: #43889
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/209542
Reviewed-by: Clement Skau <cskau@google.com>
Commit-Queue: Clement Skau <cskau@google.com>
@ghost ghost removed their assignment May 16, 2022
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, and the AOT and JIT backends. library-ffi type-enhancement A request for a change that isn't a bug type-performance Issue relates to performance or code size
Projects
None yet
Development

No branches or pull requests

3 participants