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

Compile dart2wasm modules using the JS runtime exported compileStreaming #51488

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Mar 18, 2024

With dart-lang/sdk@44c2e17 dart2wasm started using the new Wasm built-in string import wasm:js-string.

With dart-lang/sdk@4cd6096 dart2wasm started exporting compile and compileStreaming from the generated JS runtime to compile dart2wasm-generated modules with the wasm:js-string built-in, polyfilling it when not available.

Update Wasm compilation code in Flutter to use compileStreaming from the JS runtime, enabling fast jwasm:js-string module when available.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Mar 18, 2024
@osa1
Copy link
Contributor Author

osa1 commented Mar 18, 2024

@eyebrowsoffire is this code path tested on the CI?

On the CI stringBuiltinSupported will be false, locally can I run the tests that use this code with a custom Chrome with extra command line flags, so that I can test the case where stringBuiltinSupported is true? We need to run a recent Chrome with --js-flags=--experimental-wasm-imported-strings for that variable to be true.

@osa1 osa1 requested a review from eyebrowsoffire March 18, 2024 13:28
@osa1 osa1 marked this pull request as ready for review March 18, 2024 13:28
@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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

Copy link
Member

@mkustermann mkustermann 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 wait for @eyebrowsoffire


const stringBuiltinSupported = this._detectWasmStringBuiltin();

const dartModulePromise = WebAssembly.compileStreaming(
Copy link
Member

Choose a reason for hiding this comment

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

We may consider in the future to make the .mjs file export 3 things:

  • (new) Compile API wrapping WebAssembly.compile*
  • Instantiate API wrapping WebAssembly.instantiate* (we already provide this)
  • Invoke main API (we already provide this)

That would encapsulate this logic here in the dart2wasm compiler instead of making a user of the compiler aware of what builtins the compiled code requires.

There's an aspect to think about: The current way allows flutter JS code to be loaded in parallel to the mjs file, then we'd have a dependency from former to the latter - which may harm startup latency. This may especially be noticeable if we use imported strings (which may make the mjs file much bigger). So we'd have to think whether it would make sense to do that or not.

Anyways, that's not needed right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want separate compilation and instantiation steps? Currently we don't do anything with the compiled module other than instantiating it. We could combine both steps in a single export.

Copy link
Member

Choose a reason for hiding this comment

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

We can also offer this in the mjs api:

  • (new) Compile&Instantiate API wrapping WebAssembly.compile* and WebAssembly.instantiate*

The setup should be optimized for startup time / initial page load. So we should try to do as much in parallel we can. So one advantage of separating the two, is that it allows the browser to download the wasm file & do some work in the streaming compile step in parallel to downloading/parsing/running the mjs file (which may become bigger once we have imported strings).

I don't know if Chrome/V8 does a lot of work in the compile step or most in instantiation step. May also differ between browsers.

This would need to be measured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So one advantage of separating the two, is that it allows the browser to download the wasm file & do some work in the streaming compile step in parallel to downloading/parsing/running the mjs file (which may become bigger once we have imported strings).

If the compile function is provided by the mjs file so you can't compile the Wasm and download mjs file with lots of strings in parallel, right?

Maybe strings can be in a separate file that we download in parallel.

@goderbauer
Copy link
Member

(triage): I spoke to @eyebrowsoffire and he said he is going to take a look at this one soon.

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.

Sorry this took a million years for me to actually review. LGTM for now. I do think, as Martin suggests, some of this logic should be built into the .mjs file rather than done in the flutter bootstrapping code, but I think this works in the meantime.

@osa1
Copy link
Contributor Author

osa1 commented Aug 21, 2024

I'm moving the compilation logic to the generated runtime blob in https://dart-review.googlesource.com/c/sdk/+/381581.

I'll get back to this once that's merged and rolled into the engine.

@osa1 osa1 marked this pull request as draft August 21, 2024 08:07
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 21, 2024
To be able to control how dart2wasm-generated modules are compiled, add
a `WebAssembly.compile` and `WebAssmebly.compileStreaming` wrapper to
the generated runtime blob.

These wrappers do js-string built-in detection and enable the built-in
when it's available.

Once the SDK is rolled into engine we will update the engine dart2wasm
module loader to use `compileStreaming` from the runtime blob, in
flutter/engine#51488.

This will allow enabling the built-in without duplicating the feature
detection and compilation code in the engine.

Change-Id: Icb939f16a2b0ae1d348ec5a814371e9ffde4fd68
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/381581
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Ömer Ağacan <[email protected]>
@osa1 osa1 changed the title Update dart2wasm module compilation to use Wasm string builtins Compile dart2wasm modules using the JS runtime exported compileStreaming Aug 26, 2024
@osa1 osa1 marked this pull request as ready for review August 26, 2024 11:15
@osa1
Copy link
Contributor Author

osa1 commented Aug 27, 2024

CI is failing with unrelated macos issue. Should we merge this? @mkustermann @eyebrowsoffire

@eyebrowsoffire
Copy link
Contributor

CI is failing with unrelated macos issue. Should we merge this? @mkustermann @eyebrowsoffire

I reran the CI step and it passed, so I think we're good to go!

@osa1 osa1 merged commit 671c277 into flutter:main Aug 28, 2024
27 checks passed
@osa1 osa1 deleted the dart2wasm_string_builtins branch August 28, 2024 18:00
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2024
zanderso added a commit to flutter/flutter that referenced this pull request Aug 29, 2024
…154316)

Roll Flutter Engine from 8d248aead383 to f48ecf5b49f6 (40 revisions)

flutter/engine@8d248ae...f48ecf5

2024-08-29 [email protected] [Impeller] Use multiple command
buffers for blur submission. (flutter/engine#54846)
2024-08-29 [email protected] Roll Skia from 0d8d9d2974fa to
e37b6b198016 (1 revision) (flutter/engine#54854)
2024-08-29 [email protected] Remove
`--disable-dart-dev` across `flutter/engine`. (flutter/engine#54845)
2024-08-28 [email protected] Roll Fuchsia Test Scripts from
vIJGWtHj4Rdku9Ayv... to NWpblL_DFACOx_Spi... (flutter/engine#54852)
2024-08-28 [email protected] [Impeller] fix clip culling with exp
canvas. (flutter/engine#54701)
2024-08-28 [email protected] Roll Dart SDK from
bc3dad16b2d3 to fed5ce7ea2ad (2 revisions) (flutter/engine#54851)
2024-08-28 [email protected] Roll Skia from d55406ca32e9 to
0d8d9d2974fa (4 revisions) (flutter/engine#54850)
2024-08-28 [email protected] [skwasm] Always do backdrop filter
operation even if empty. (flutter/engine#54844)
2024-08-28 [email protected]
Migrate`header_guard_check` to `package:test`. (flutter/engine#54811)
2024-08-28 [email protected] Roll Fuchsia GN SDK from
OKGFjciA5Vd0TQks4... to ALNKvSVWQSpw1uxPy... (flutter/engine#54848)
2024-08-28 [email protected] Roll Skia from cd3d3daafe55 to
d55406ca32e9 (10 revisions) (flutter/engine#54847)
2024-08-28 [email protected] [Impeller] ensure that srcOver to
src conversion takes stroke coverage into account.
(flutter/engine#54817)
2024-08-28 [email protected] Roll Fuchsia GN SDK from
ALNKvSVWQSpw1uxPy... to OKGFjciA5Vd0TQks4... (flutter/engine#54840)
2024-08-28 [email protected] Remove scorecards and
other bading we are no longer tracking/links are borked
(flutter/engine#54839)
2024-08-28 [email protected] Compile dart2wasm modules using the JS
runtime exported compileStreaming (flutter/engine#51488)
2024-08-28 [email protected] Roll Dart SDK from
183b9e21b706 to bc3dad16b2d3 (1 revision) (flutter/engine#54838)
2024-08-28 [email protected] Ignore generated fixture
`.dill.deps` files. (flutter/engine#54836)
2024-08-28 6844906[email protected]
[fuchsia] use the api-level from gn-sdk (flutter/engine#54740)
2024-08-28 [email protected] [Impeller] port clip stack fixes to
new canvas. (flutter/engine#54727)
2024-08-28 [email protected] [Impeller] fall back to path
rendering on thick paths. (flutter/engine#54822)
2024-08-28 [email protected] Roll Fuchsia Linux SDK from
BCqzoTS_Sz6-AaSii... to ZL8AvfXX5LFIH1LYN... (flutter/engine#54834)
2024-08-28 [email protected] Roll Skia from ca108745b1de to
cd3d3daafe55 (1 revision) (flutter/engine#54832)
2024-08-28 [email protected] Roll Dart SDK from
42ddf2278114 to 183b9e21b706 (1 revision) (flutter/engine#54830)
2024-08-28 [email protected] Roll Dart SDK from
b519f85c3076 to 42ddf2278114 (1 revision) (flutter/engine#54829)
2024-08-28 [email protected] Roll Fuchsia GN SDK from
OKGFjciA5Vd0TQks4... to ALNKvSVWQSpw1uxPy... (flutter/engine#54827)
2024-08-28 [email protected] Roll Skia from 41cb13f65fe6 to
ca108745b1de (1 revision) (flutter/engine#54828)
2024-08-28 [email protected] Roll Skia from 259010335a55 to
41cb13f65fe6 (2 revisions) (flutter/engine#54826)
2024-08-28 [email protected] Roll Skia from 505fb55cd044 to
259010335a55 (1 revision) (flutter/engine#54823)
2024-08-28 [email protected] Roll Dart SDK from
8334290a421b to b519f85c3076 (1 revision) (flutter/engine#54821)
2024-08-28 [email protected] Roll Skia from 84e4a69da303 to
505fb55cd044 (1 revision) (flutter/engine#54819)
2024-08-27 [email protected] [Impeller] Increase host buffer
arena count to 4. (flutter/engine#54808)
2024-08-27 [email protected] Synchronize accounting for render op depths
(flutter/engine#54794)
2024-08-27 [email protected] Fix broken links
in `docs/` (flutter/engine#54815)
2024-08-27 [email protected] [Impeller] Don't override user
specification on Vulkan validation in unopt. (flutter/engine#54816)
2024-08-27 [email protected] Manual roll Dart SDK from
b81b344a194f to 8334290a421b (12 revisions) (flutter/engine#54813)
2024-08-27 [email protected] Roll Skia from 77017d30a455 to
84e4a69da303 (3 revisions) (flutter/engine#54812)
2024-08-27 [email protected] [Impeller] Clarify where to put the
metadata in the manifest. (flutter/engine#54814)
2024-08-27 [email protected] [Impeller] Use infinite swapchain
present timeouts to avoid logspam. (flutter/engine#54810)
2024-08-27 [email protected] Roll Skia from 2e1eea538014 to
77017d30a455 (2 revisions) (flutter/engine#54809)
2024-08-27 [email protected] Roll Skia from a2e2eb292492 to
2e1eea538014 (4 revisions) (flutter/engine#54806)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from BCqzoTS_Sz6- to ZL8AvfXX5LFI

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
...

---------

Co-authored-by: Zachary Anderson <[email protected]>
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…lutter#154316)

Roll Flutter Engine from 8d248aead383 to f48ecf5b49f6 (40 revisions)

flutter/engine@8d248ae...f48ecf5

2024-08-29 [email protected] [Impeller] Use multiple command
buffers for blur submission. (flutter/engine#54846)
2024-08-29 [email protected] Roll Skia from 0d8d9d2974fa to
e37b6b198016 (1 revision) (flutter/engine#54854)
2024-08-29 [email protected] Remove
`--disable-dart-dev` across `flutter/engine`. (flutter/engine#54845)
2024-08-28 [email protected] Roll Fuchsia Test Scripts from
vIJGWtHj4Rdku9Ayv... to NWpblL_DFACOx_Spi... (flutter/engine#54852)
2024-08-28 [email protected] [Impeller] fix clip culling with exp
canvas. (flutter/engine#54701)
2024-08-28 [email protected] Roll Dart SDK from
bc3dad16b2d3 to fed5ce7ea2ad (2 revisions) (flutter/engine#54851)
2024-08-28 [email protected] Roll Skia from d55406ca32e9 to
0d8d9d2974fa (4 revisions) (flutter/engine#54850)
2024-08-28 [email protected] [skwasm] Always do backdrop filter
operation even if empty. (flutter/engine#54844)
2024-08-28 [email protected]
Migrate`header_guard_check` to `package:test`. (flutter/engine#54811)
2024-08-28 [email protected] Roll Fuchsia GN SDK from
OKGFjciA5Vd0TQks4... to ALNKvSVWQSpw1uxPy... (flutter/engine#54848)
2024-08-28 [email protected] Roll Skia from cd3d3daafe55 to
d55406ca32e9 (10 revisions) (flutter/engine#54847)
2024-08-28 [email protected] [Impeller] ensure that srcOver to
src conversion takes stroke coverage into account.
(flutter/engine#54817)
2024-08-28 [email protected] Roll Fuchsia GN SDK from
ALNKvSVWQSpw1uxPy... to OKGFjciA5Vd0TQks4... (flutter/engine#54840)
2024-08-28 [email protected] Remove scorecards and
other bading we are no longer tracking/links are borked
(flutter/engine#54839)
2024-08-28 [email protected] Compile dart2wasm modules using the JS
runtime exported compileStreaming (flutter/engine#51488)
2024-08-28 [email protected] Roll Dart SDK from
183b9e21b706 to bc3dad16b2d3 (1 revision) (flutter/engine#54838)
2024-08-28 [email protected] Ignore generated fixture
`.dill.deps` files. (flutter/engine#54836)
2024-08-28 6844906[email protected]
[fuchsia] use the api-level from gn-sdk (flutter/engine#54740)
2024-08-28 [email protected] [Impeller] port clip stack fixes to
new canvas. (flutter/engine#54727)
2024-08-28 [email protected] [Impeller] fall back to path
rendering on thick paths. (flutter/engine#54822)
2024-08-28 [email protected] Roll Fuchsia Linux SDK from
BCqzoTS_Sz6-AaSii... to ZL8AvfXX5LFIH1LYN... (flutter/engine#54834)
2024-08-28 [email protected] Roll Skia from ca108745b1de to
cd3d3daafe55 (1 revision) (flutter/engine#54832)
2024-08-28 [email protected] Roll Dart SDK from
42ddf2278114 to 183b9e21b706 (1 revision) (flutter/engine#54830)
2024-08-28 [email protected] Roll Dart SDK from
b519f85c3076 to 42ddf2278114 (1 revision) (flutter/engine#54829)
2024-08-28 [email protected] Roll Fuchsia GN SDK from
OKGFjciA5Vd0TQks4... to ALNKvSVWQSpw1uxPy... (flutter/engine#54827)
2024-08-28 [email protected] Roll Skia from 41cb13f65fe6 to
ca108745b1de (1 revision) (flutter/engine#54828)
2024-08-28 [email protected] Roll Skia from 259010335a55 to
41cb13f65fe6 (2 revisions) (flutter/engine#54826)
2024-08-28 [email protected] Roll Skia from 505fb55cd044 to
259010335a55 (1 revision) (flutter/engine#54823)
2024-08-28 [email protected] Roll Dart SDK from
8334290a421b to b519f85c3076 (1 revision) (flutter/engine#54821)
2024-08-28 [email protected] Roll Skia from 84e4a69da303 to
505fb55cd044 (1 revision) (flutter/engine#54819)
2024-08-27 [email protected] [Impeller] Increase host buffer
arena count to 4. (flutter/engine#54808)
2024-08-27 [email protected] Synchronize accounting for render op depths
(flutter/engine#54794)
2024-08-27 [email protected] Fix broken links
in `docs/` (flutter/engine#54815)
2024-08-27 [email protected] [Impeller] Don't override user
specification on Vulkan validation in unopt. (flutter/engine#54816)
2024-08-27 [email protected] Manual roll Dart SDK from
b81b344a194f to 8334290a421b (12 revisions) (flutter/engine#54813)
2024-08-27 [email protected] Roll Skia from 77017d30a455 to
84e4a69da303 (3 revisions) (flutter/engine#54812)
2024-08-27 [email protected] [Impeller] Clarify where to put the
metadata in the manifest. (flutter/engine#54814)
2024-08-27 [email protected] [Impeller] Use infinite swapchain
present timeouts to avoid logspam. (flutter/engine#54810)
2024-08-27 [email protected] Roll Skia from 2e1eea538014 to
77017d30a455 (2 revisions) (flutter/engine#54809)
2024-08-27 [email protected] Roll Skia from a2e2eb292492 to
2e1eea538014 (4 revisions) (flutter/engine#54806)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from BCqzoTS_Sz6- to ZL8AvfXX5LFI

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
...

---------

Co-authored-by: Zachary Anderson <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants