Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Adds Tonic templates for Dart FFI bindings. #29405

Merged
8 commits merged into from Feb 8, 2022
Merged

Adds Tonic templates for Dart FFI bindings. #29405

8 commits merged into from Feb 8, 2022

Conversation

ghost
Copy link

@ghost ghost commented Oct 29, 2021

This change adds Tonic templates for automatically generating bindings for Dart FFI, as well as serialisation of the bindings.

  • Adds parallel FfiDispather templates to the existing DartDispatcher used for (old) native bindings.
  • Adds serialisation of the bindings to enable automatic conversion and verification.
  • Extends existing DartConverters with conversions to and from the FFI transport types.
  • Adds new test (ffi_native_unittest.cc) for the above.

This will allow us to replace the existing native functions in e.g. dart:ui with new Dart FFI native functions.

The benefit of FFI calls over the old native calls is that FFI automatically handles data conversion for a number of primitive types like int, double and bool, and thus does not require the overhead of calling Dart API within the native code to deal with Handles.
Additionally, for code that does not use Handles, we can avoid further overhead of setting up frames and transitioning VM state.

With FfiDispatchers in place to set up all the new entry points, the remaining steps to convert the existing native functions to FFI are: 1) setting up the native function resolver, and 2) converting the functions like:

  void moveTo(double x, double y) native 'Path_moveTo';
  /// \/ -- Becomes -- \/
  @FfiNative<Void Function(Pointer<Void>, Float, Float)>('Path::moveTo', isLeaf: true)
  external void moveTo(double x, double y);

The serialisation of the bindings will additionally allow us to automatically generate the above conversions, and subsequently validate all bindings in Dart code match their native counterparts.

Issues:

@google-cla google-cla bot added the cla: yes label Oct 29, 2021
@ghost ghost requested review from chinmaygarde, iskakaushik and dcharkes October 29, 2021 11:08
@ghost ghost marked this pull request as ready for review October 29, 2021 13:54
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.

First round of comments, I'll take a deeper dive later.

High level comment: The commit message should have a detailed explanation of why and how:

  • What is the goal of the FfiNatives, why do we want to start using them in Tonic.
  • How do the FfiNatives (roughly) work compared to the old natives.
  • (Which then explains the motivation for adding these to the Convertors.)
  • Motivate the serialization.

We'd want to sent this PR to a Flutter dev as well to review. And that description would help them (and me).

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.

Assuming that the 'why' is:

  1. Replace existing DartDispatcher with FfiDispatcher calls later.
  2. Generate bindings from all FfiDispatcher templates.

For 2. I would expect also a test or some code showing how that is done. (To prevent that we land something here that turns out to not work for the use case of generating code.)

After the comments are addressed, I'd like someone who understands the workings and/or use of the current DartDispatcher to take a look as well.

@ghost
Copy link
Author

ghost commented Nov 11, 2021

@chinmaygarde, @iskakaushik would the two of you like to review this change as well, or are you fine with Daco's review?

@chinmaygarde
Copy link
Member

I'll take a stab at this today but won't be able to fully review it. It requires a fair bit of context to be paged in.

@CaseyHillers CaseyHillers changed the base branch from master to main November 15, 2021 18:13
@ghost
Copy link
Author

ghost commented Nov 18, 2021

@chinmaygarde I'd be happy to set up a VC for us to discuss the changes, if that would be helpful?

@ghost ghost changed the base branch from master to main November 18, 2021 14:13
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

minor nits, but LGTM overall.

@chinmaygarde
Copy link
Member

@mkustermann Since @cskau-g is on leave, is there anyone else who can take this to completion? Since this has an LGTM already, perhaps we should land this?

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@mkustermann
Copy link
Member

@mkustermann Since @cskau-g is on leave, is there anyone else who can take this to completion? Since this has an LGTM already, perhaps we should land this?

Apologies for the late reply. I think @cskau-g can land it when he's back (~ 2 weeks)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 8, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants