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

Conversation

@ghost
Copy link

@ghost ghost commented Nov 9, 2021

This change converts dart:ui native functions into FFI native functions.

For example:

String? _toImage(int width, int height, _Callback<_Image?> callback) native 'Scene_toImage';

Becomes:

@FfiNative<Handle Function(Pointer<Void>, Uint32, Uint32, Handle)>('Scene::toImage')
external String? _toImage(int width, int height, _Callback<_Image?> callback);

This makes use of the newly introduced FFI binding Tonic templates from c797abf

The benefit of using FFI native function calls instead of the old native function calls, is that FFI in many cases is faster, as seen in the benchmarks on https://dart-review.googlesource.com/c/sdk/+/218920.

It is however worth noting that there are certain cases where FFI native function calls (currently) have a larger overhead, due to known issues.
But in dart:ui there is only one function that falls in that category (namely SemanticsUpdateBuilder::updateNode with its 36 parameters). And benchmarking suggests that this has no measurable impact on actual app performance.

@google-cla google-cla bot added the cla: yes label Nov 9, 2021
@TKDev7
Copy link

TKDev7 commented Nov 9, 2021

Does this yield better performance?

@ghost
Copy link
Author

ghost commented Nov 10, 2021

Does this yield better performance?

Please note that this is a draft PR, for changes still being worked on.

But yes, in general, FFI tends to have less overhead compared to the old native functions.
Thought there are certain corner cases where there is known overhead that makes FFI slower, such as an abnormal amount of Handle function parameters.

For some quick benchmark numbers to get an idea of the difference, see:
https://dart-review.googlesource.com/c/sdk/+/218920

That said, end users are unlikely to see a noticeable impact on app performance from this improvement since the overhead of calling native functions in dart:ui completely pales in comparison to the cycles spent on everything else, including app code.

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

Hixie commented Feb 2, 2022

@cskau-g Are you still working on this PR?

@ghost
Copy link
Author

ghost commented Feb 7, 2022

@cskau-g Are you still working on this PR?

Yes. Though I've been out on leave for a while.

@ghost ghost requested a review from gspencergoog February 10, 2022 16:37
@ghost ghost marked this pull request as ready for review February 10, 2022 16:37
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@ghost
Copy link
Author

ghost commented Feb 10, 2022

@gspencergoog are you a good candidate to review this change? If not can you help nominate someone who might be?

@Hixie
Copy link
Contributor

Hixie commented Feb 11, 2022

@jason-simmons or @aam might be good reviewers

@dnfield
Copy link
Contributor

dnfield commented May 4, 2022

I think the iOS failure is because of CI getting confused about where this applies to. Fixing that probably requires a rebase. Rebasing this patch would be painful. I think we should ignore that failure on presubmit.

@dnfield
Copy link
Contributor

dnfield commented May 4, 2022

Wacky, it passed on a rerun.

@dnfield dnfield added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed Work in progress (WIP) Not ready (yet) for review! labels May 4, 2022
@fluttergithubbot fluttergithubbot merged commit a74a58f into flutter:main May 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2022
zanderso added a commit that referenced this pull request May 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2022
@jason-simmons
Copy link
Member

Note for when we reland this:
The Path.relativeArcToPoint and Path.relativeConicTo methods have incorrect argument counts in lib/ui/dart_ui.cc

@dnfield
Copy link
Contributor

dnfield commented May 6, 2022

Looks like we're missing test coverage of those two methods in the engine.

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

Labels

cla: yes needs tests waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants