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

[web] Avoid returning int from js interop classes. #37336

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

joshualitt
Copy link
Contributor

While ints can flow to JS on all backends, Dart2Wasm does not currently support returning ints from JS. The reason is because there is really no way to support this in the general case. This CL does not change any public APIs, but rather instead we introduce tiny trampolines to handle the conversion.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Nov 4, 2022
@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.

@Hixie
Copy link
Contributor

Hixie commented Nov 10, 2022

test-exemption: code refactor with no semantic change
test-exempt: this already tested, the code just doesn't run in Wasm mode today.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Some of these getters might be on a hot path. Do you know how toInt() behaves when the value is already an int? Is it a no-op?

Also, it's very easy to introduce int getters without adding a trampoline for them. Is there a way to detect or prevent that from happening? i.e. are the skwasm tests going to fail if an int is returned without doing toInt()?

@joshualitt
Copy link
Contributor Author

Some of these getters might be on a hot path. Do you know how toInt() behaves when the value is already an int? Is it a no-op?

Not entirely a noop, but almost. There is a size test, but because nearly all ints will be int32, branch prediction will nearly always be correct. Then there is an int | 0 instruction, but that too should be quite cheap. @yjbanov and I agreed the amount of overhead was negligible. However, it may still make sense to have these APIs return double or num someday, but really more for code cleanliness than performance.

Also, it's very easy to introduce int getters without adding a trampoline for them. Is there a way to detect or prevent that from happening? i.e. are the skwasm tests going to fail if an int is returned without doing toInt()?

Good point, in the longer term when Wasm tests are running on CI, then these issues will be caught. In addition, I'm going to propose 'compatibility mode' lints for JS interop, and if we have such a set of lints we can add one to catch this issue for users who want their JS interop code to work correctly on all backends.

@joshualitt joshualitt force-pushed the int-migrate branch 4 times, most recently from 2afc179 to c7b2651 Compare November 14, 2022 19:06
@joshualitt joshualitt merged commit 6976132 into flutter:main Nov 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 14, 2022
joshualitt added a commit that referenced this pull request Nov 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2022
joshualitt added a commit that referenced this pull request Nov 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 15, 2022
…115335)

* 6976132e5 [web] Switch to doubles (flutter/engine#37336)

* c3d7c5967 Roll Skia from a434f9b69660 to 33c62dafffc9 (9 revisions) (flutter/engine#37598)

* 91656a5ff Avoid segfault when converting no-op ColorFilter to ImageFilter (flutter/engine#37596)

* 965f87d31 Archive windows gen_snapshot.exe. (flutter/engine#35414)

* 16dba68e5 [Impeller] Only apply the rrect blur fast path for solid Colors (flutter/engine#37594)

* d366183c0 Roll Dart SDK from 7cbcf48157cf to 6f5478a58387 (2 revisions) (flutter/engine#37597)

* 88f56870b Roll Fuchsia Linux SDK from dRHIZSishiboEHMdw... to B0OuUvWOY24LI1WoF... (flutter/engine#37603)

* 28eeba518 [Multiwindow, macOS] FlutterCompositor::Present receives view_id (flutter/engine#37391)

* 8d815657c Roll Skia from 33c62dafffc9 to b474a43dcc34 (6 revisions) (flutter/engine#37606)

* 78ae24140 Revert "[web] Switch to doubles (#37336)" (flutter/engine#37609)
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…lutter#115335)

* 6976132e5 [web] Switch to doubles (flutter/engine#37336)

* c3d7c5967 Roll Skia from a434f9b69660 to 33c62dafffc9 (9 revisions) (flutter/engine#37598)

* 91656a5ff Avoid segfault when converting no-op ColorFilter to ImageFilter (flutter/engine#37596)

* 965f87d31 Archive windows gen_snapshot.exe. (flutter/engine#35414)

* 16dba68e5 [Impeller] Only apply the rrect blur fast path for solid Colors (flutter/engine#37594)

* d366183c0 Roll Dart SDK from 7cbcf48157cf to 6f5478a58387 (2 revisions) (flutter/engine#37597)

* 88f56870b Roll Fuchsia Linux SDK from dRHIZSishiboEHMdw... to B0OuUvWOY24LI1WoF... (flutter/engine#37603)

* 28eeba518 [Multiwindow, macOS] FlutterCompositor::Present receives view_id (flutter/engine#37391)

* 8d815657c Roll Skia from 33c62dafffc9 to b474a43dcc34 (6 revisions) (flutter/engine#37606)

* 78ae24140 Revert "[web] Switch to doubles (flutter#37336)" (flutter/engine#37609)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#115335)

* 6976132e5 [web] Switch to doubles (flutter/engine#37336)

* c3d7c5967 Roll Skia from a434f9b69660 to 33c62dafffc9 (9 revisions) (flutter/engine#37598)

* 91656a5ff Avoid segfault when converting no-op ColorFilter to ImageFilter (flutter/engine#37596)

* 965f87d31 Archive windows gen_snapshot.exe. (flutter/engine#35414)

* 16dba68e5 [Impeller] Only apply the rrect blur fast path for solid Colors (flutter/engine#37594)

* d366183c0 Roll Dart SDK from 7cbcf48157cf to 6f5478a58387 (2 revisions) (flutter/engine#37597)

* 88f56870b Roll Fuchsia Linux SDK from dRHIZSishiboEHMdw... to B0OuUvWOY24LI1WoF... (flutter/engine#37603)

* 28eeba518 [Multiwindow, macOS] FlutterCompositor::Present receives view_id (flutter/engine#37391)

* 8d815657c Roll Skia from 33c62dafffc9 to b474a43dcc34 (6 revisions) (flutter/engine#37606)

* 78ae24140 Revert "[web] Switch to doubles (flutter#37336)" (flutter/engine#37609)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs tests platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants