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

Update Dart flute WasmGC workload #53

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkustermann
Copy link

This updates the Dart flute benchmark. Apart from using a newer Dart2Wasm compiler this brings two important changes:

  • The compiler & standard library will now use JavaScript strings as it's only string implementation. It takes advantage of js-string builtins if available.

    We anticipate all browsers adopting the js-string builtin proposal and optimize the builtin methods.

    NOTE: We currently don't require the js-string builtin to be available. We polyfill the imported methods and we import string constants from the mjs file. The main reason for this is that JavaScriptCore doesn't support js-string builtin yet (filed [0]). Once JSC supports the proposal we want to require it as it allows using the much more efficient string constant import mechanism (where string constants are utf8-encoded in the wasm module instead of importing from JS file)

    [0] https://bugs.webkit.org/show_bug.cgi?id=287402

    (Before the compiler used 3 implementations: {One,Two}ByteString and JSString and copied the strings across the WasmGC<->JS boundary. But now that we get more performant JS strings we prefer using only JS strings due to avoiding the need to copy them across the boundary, working with JS RegExp, ...)

  • The flute benchmark is now compiled in -O2 mode that ensures Dart soundness. It will perform runtime type checks as required by the Dart language as well as throw catchable exceptions on out-of-bounds accesses.

    (Before the benchmark was compiled in -O4 mode which is unsound Dart, it didn't perform the type checks required by the Dart language specification and didn't perform index checks).

This updates the Dart flute benchmark. Apart from using a newer
Dart2Wasm compiler this brings two important changes:

  * The compiler & standard library will now use JavaScript strings as
    it's only string implementation. It takes advantage of `js-string`
    builtins if available.

    We anticipate all browsers adopting the `js-string` builtin
    proposal and optimize the builtin methods.

    NOTE: We currently don't *require* the `js-string` builtin to be
    available. We polyfill the imported methods and we import string
    constants from the mjs file. The main reason for this is that
    JavaScriptCore doesn't support `js-string` builtin yet (filed [0]).
    Once JSC supports the proposal we want to require it as it allows
    using the much more efficient string constant import mechanism
    (where string constants are utf8-encoded in the wasm module instead
    of importing from JS file)

    [0] https://bugs.webkit.org/show_bug.cgi?id=287402

    (Before the compiler used 3 implementations: `{One,Two}ByteString`
    and `JSString` and copied the strings across the WasmGC<->JS
    boundary. But now that we get more performant JS strings we prefer
    using only JS strings due to avoiding the need to copy them across
    the boundary, working with JS RegExp, ...)

  * The flute benchmark is now compiled in `-O2` mode that ensures Dart
    soundness. It will perform runtime type checks as required by the
    Dart language as well as throw catchable exceptions on out-of-bounds
    accesses.

    (Before the benchmark was compiled in `-O4` mode which is unsound
    Dart, it didn't perform the type checks required by the Dart language
    specification and didn't perform index checks).

    We intend to switch Flutter web to also use `-O2` mode.
@mkustermann
Copy link
Author

/cc @kmiller68 @eqrion @danleh

@danleh
Copy link
Contributor

danleh commented Mar 6, 2025

LGTM

I also briefly measured before and after on d8: It's a tiny bit slower now (I assume because of -O2 / soundness mode), namely Wall time 9.3s / Total score 7.2 (this PR) vs. Wall time 8.9s / Total score 7.5 (current main). I also took a profile / flame graph, looks roughly the same before and after, no big surprises.

@eqrion
Copy link

eqrion commented Mar 6, 2025

@mkustermann

The flute benchmark is now compiled in -O2 mode that ensures Dart soundness. It will perform runtime type checks as required by the Dart language as well as throw catchable exceptions on out-of-bounds accesses.

What mode does Dart recommend users compile with and why? -O2 or -O4? And is that the mode that most users use?

@mkustermann
Copy link
Author

What mode does Dart recommend users compile with and why? -O2 or -O4? And is that the mode that most users use?

The tl;dr

  • dart compile js and dart compile wasm default to -O1
  • flutter build web defaults currently to -O4 (but we intend to switch to -O2)

The levels are roughly

  • -O0: Unoptimized
  • -O1: Optimization turned on (in wasm case, also use binaryen)
  • -O2: Minify/Obfuscate (makes runtime types obfuscated, exceptions have less detailed information, ...)
    => This is the smallest & fastest mode we have that maintains Dart semantics.
  • -O3: Unsound: Omit implicit type checks
  • -O4: Unsound: Omit explicit (user-written) type checks

The very long version:

Dart 1 was an unsound language with optional types (they were ignored at runtime). The switch to Dart 2 made Dart a soundly typed language - the reified generics of Dart 2 require the runtime to perform type checks in order to maintain soundness. (The switch to Dart 3 has introduced nullability in the type system - but this isn't relevant here).

  • Dart VM (JIT and AOT) always adhered to Dart language semantics and from Dart 2 onwards performs any type checks at runtime needed for soundness.
  • Dart2js also does that, but offered an option to get smaller code size & higher performance by assuming no type errors are thrown at runtime (somewhat similar to wasm-opt/binaryen's --traps-never-happen) - which is enabled in -O3 / -O4 modes. Many customers used this immediately, so dart2js never had big enough incentive of optimizing the runtime type checks.

When flutter added a web backend it could only use dart2js and, because type checks weren't optimized yet, they used -O4 as default (which is unsound Dart). This was arguably a terrible choice - we should always let end users opt-into the unsound behavior, instead of defaulting to it. When we added dart2wasm support we wanted eye-to-eye performance comparison numbers, so we also added a similar -O4 mode that skips runtime type checks.

Though for a variety of reasons we have now strong incentive to a) optimize dart2js type check performance b) switching to -O2 mode in dart2wasm (and possibly also dart2js). Among the reasons is that flutter web apps that exhibit undefined behavior in dart2js may lead to benign exceptions that can be caught, but the undefined behavior dart2wasm leads often to uncatchable wasm traps (so apps that "kind of worked - despite exhibiting undefined behavior" in dart2js may trap in wasm now). We want users to switch to dart2wasm and therefore want to avoid this difference in "undefined behavior" semantics of just trapping.

For dart2wasm we have optimized those type checks a lot over the last year which made switching to sound -O2 mode now feasible. We intend to switch flutter to use -O2 in dart2wasm in the not too distant future. As the benchmarks we add to WebKit/JetStream should be things that last for many years to come, I think it makes sense to use the -O2 version here. (If strongly preferred I can delay this PR (or the -O change only) until flutter also switches - but I want to ensure we can land this before the next version of JetStream is finalized)

Switching the flute benchmark from -O4 to -O2 costs around 10% code size and 8% (**) performance in my measurements - which is acceptable to us and is still much better than dart2js.

(**) Measured on D8 as I happen to have numbers for all optimization modes on D8 but not JSC/Shell.

Cloning into 'wasm_gc_benchmarks'...
cf32ca4 Recompile all benchmarks
f80892d Update Dart SDK, switch wasm to -O2, recompile performance benchmarks
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you manually edit this? This is an output file from build.sh normally so next time someone updates they'll see the old log again.

Is there a build with the bits you're uploading here saved somewhere? Maybe we can update that script to pull from there?

Copy link
Author

Choose a reason for hiding this comment

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

This particular line is the abbreviated commit hash and commit message. See hash & commit message from mkustermann/wasm_gc_benchmarks@f80892d

That being said I removed one line from the generated build log: git clone outputed something that's to be interpreted by a terminal rather than normal text (git clone shows this Updating ... (x/y) where it constantly updates the x/y information of the same line). It wasn't there in the old build.log and isn't normal text, so I removed that line

@kmiller68
Copy link
Contributor

@mkustermann Thanks for the detailed explanation!

Overall seems reasonable to me (modulo comment on build.log). If Dart is planning to default to -O2 we should probably test that. We plan on implementing js-string builtins and given it's polyfillable I imagine folks will use it on the web soon anyway. So the extra 🔥 to get us to implement it sooner rather than later seems reasonable to me.

@mkustermann
Copy link
Author

We plan on implementing js-string builtins and given it's polyfillable I imagine folks will use it on the web soon anyway. So the extra 🔥 to get us to implement it sooner rather than later seems reasonable to me.

@kmiller68 As mentioned on the original commit message, it's not entirely polyfill'able because of the magic utf8-encode imports. I'd actually really make this rely on js-string builtin being there. If WebKit will get support soon, I can also hold this PR until then. wdyt?

@mkustermann
Copy link
Author

flutter build web defaults currently to -O4 (but we intend to switch to -O2)

Correction: As I just found out, flutter has already switched to -O2 mode now (~ 4 weeks ago in flutter/flutter@30b8eb6)

@kmiller68
Copy link
Contributor

@kmiller68 As mentioned on the original commit message, it's not entirely polyfill'able because of the magic utf8-encode imports. I'd actually really make this rely on js-string builtin being there.

What's the magic utf8-encode imports? IIUC, js-string builtins is just a list of host provided imports. Is there some other part I'm missing?

If WebKit will get support soon, I can also hold this PR until then. wdyt?

I don't think we can promise any timeline (per Apple policy). That said, happy to go either way you decide.

@mkustermann
Copy link
Author

mkustermann commented Mar 11, 2025

What's the magic utf8-encode imports? IIUC, js-string builtins is just a list of host provided imports. Is there some other part I'm missing?

Specifically I'm referring to this part of the specification:

This proposal adds an extension to the JS-API compile routine to support optimized 'imported string constants'
to address this use-case.

The WebAssemblyCompileOptions dictionary is extended with a USVString? importedStringConstants flag.
...
Example

let instance = WebAssembly.instantiate(bytes, {importedStringConstants: "XXX"});

// The global is automatically populated with the string constant
assertEq(instance.exports.constant.value, "my string constant");
(module
 (global (import "XXX" "my string constant") (ref extern))
 (export "constant" (global 0))
)

This is a really useful mechanism

  • it shrinks the combined wasm+js code size
  • it improves startup latency - as less JavaScript code has to be parsed
  • it may allow wasm2wasm optimizers that are aware of js-string builtin semantics to do a better job optimizing wasm
  • ...

I don't think we can promise any timeline (per Apple policy). That said, happy to go either way you decide.

The main thing we care about is that this will be included in the next version of JetStream. Is there a timeline when the next version may be published?

@eqrion
Copy link

eqrion commented Mar 12, 2025

@mkustermann Thanks for that explanation, that makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants