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

[dart2wasm] High overhead getting JS wrapper for WasmGC objects in new interop API #55183

Closed
mkustermann opened this issue Mar 13, 2024 · 6 comments
Labels
area-dart2wasm Issues for the dart2wasm compiler.

Comments

@mkustermann
Copy link
Member

The current way to pass Dart objects to JavaScript with the new interop API is this via Object.toJSBox:

@patch
extension ObjectToJSBoxedDartObject on Object {
  @patch
  JSBoxedDartObject get toJSBox {
    if (this is JSValue) {
      throw 'Attempting to box non-Dart object.';
    }
    final box = JSObject();
    js_helper.JS<WasmExternRef?>('(o,s,v) => o[s] = v', box.toExternRef,
        _jsBoxedDartObjectProperty.toExternRef, jsObjectFromDartObject(this));
    return box as JSBoxedDartObject;
  }
}

The reasoning behind it seems to be safety when mixing dart objects from different dart applications running in the same browser context:

/// Embedded global property for wrapped Dart objects passed via JS interop.
///
/// This is a Symbol so that different Dart applications don't share Dart
/// objects from different Dart runtimes. We expect all [JSBoxedDartObject]s to
/// have this Symbol.

=> Our main user of Dart2Wasm is Flutter4Web where almost certainly only one Dart app runs
=> This makes us pay a high price for something that's very uncommon.

We need to change this. Either

  • ignore that (multi-dart-app) aspect entirely (and leave it up to app developers to not mix objects across apps)
  • we make it some sort of opt-in (e.g. compiler flag)
  • provide public APIs users can use that are faster and don't have this overhead (if a user knows they aren't going to mix objects from different Dart apps)

Interestingly enough js_util.jsify() is high overhead due to many type tests and recursive behavior, but it does not perform the complex mechanism of obtaining JS wrappers. Instead it makes the simple & fast way to get JS wrapper, namely WasmAnyRef.fromObject(object).externalize().toJS.

/cc @srujzs

@mkustermann mkustermann added the area-dart2wasm Issues for the dart2wasm compiler. label Mar 13, 2024
mkustermann added a commit to mkustermann/engine that referenced this issue Mar 13, 2024
…ting to JS wrappers

The `js_util.jsify()` related code shows up in CPU profile of wonderous.

=> Any `SkwasmObjectWrapper` object invokes this logic in the
constructor and dispose method.

This PR

* makes `DomFinalizationRegistryExtension` accept JSAny types instead of
  Dart types and internally converting
  => Callsites can call more precise `<>.toJS*` extension methods
  => Avoids extra type checks on the objects

* avoids making `toJSAnyShallow` delegate to `toJSAnyDeep` in wasm
  => `toJSAnyDeep` uses `js_util.jsify()` which is slow
  => We cannot use `Object.toJSBox` due to it being slower to create JS
  boxes as it semantically does something different atm (see issue below)
  => Instead use conditional import of `dart:_wasm` which provides the
  necessary primitives
  => Similar for going from JS to Dart.

* Avoid calling converting from Dart object to JSAny more than needed
  (we did the operation twice for each registration and once for
  unregistration)

Issue dart-lang/sdk#55183
@srujzs
Copy link
Contributor

srujzs commented Mar 13, 2024

Our plan is to do the third option: #55187. We've had some internal discussion a while back and agreed to add this, but we haven't had any external need for this until recently, so this should be a higher priority.

@mkustermann
Copy link
Member Author

Our plan is to do the third option: #55187. We've had some internal discussion a while back and agreed to add this, but we haven't had any external need for this until recently, so this should be a higher priority.

As mentioned on the other issue, passing objects to JS and back again should use the fast mechanism by-default, we shouldn't encourage users to use a slow mechanism and hide the fast one dart:js_interop_unsafe - when in reality there's no safety concerns for them at all.

/cc @sigmundch

mkustermann added a commit to mkustermann/engine that referenced this issue Mar 14, 2024
…ting to JS wrappers

The `js_util.jsify()` related code shows up in CPU profile of wonderous.

=> Any `SkwasmObjectWrapper` object invokes this logic in the
constructor and dispose method.

This PR

* makes `DomFinalizationRegistryExtension` accept JSAny types instead of
  Dart types and internally converting
  => Callsites can call more precise `<>.toJS*` extension methods
  => Avoids extra type checks on the objects

* avoids making `toJSAnyShallow` delegate to `toJSAnyDeep` in wasm
  => `toJSAnyDeep` uses `js_util.jsify()` which is slow
  => We cannot use `Object.toJSBox` due to it being slower to create JS
  boxes as it semantically does something different atm (see issue below)
  => Instead use conditional import of `dart:_wasm` which provides the
  necessary primitives
  => Similar for going from JS to Dart.

* Avoid calling converting from Dart object to JSAny more than needed
  (we did the operation twice for each registration and once for
  unregistration)

Issue dart-lang/sdk#55183
mkustermann added a commit to mkustermann/engine that referenced this issue Mar 14, 2024
…ting to JS wrappers

The `js_util.jsify()` related code shows up in CPU profile of wonderous.

=> Any `SkwasmObjectWrapper` object invokes this logic in the
constructor and dispose method.

This PR

* makes `DomFinalizationRegistryExtension` accept JSAny types instead of
  Dart types and internally converting
  => Callsites can call more precise `<>.toJS*` extension methods
  => Avoids extra type checks on the objects

* avoids making `toJSAnyShallow` delegate to `toJSAnyDeep` in wasm
  => `toJSAnyDeep` uses `js_util.jsify()` which is slow
  => We cannot use `Object.toJSBox` due to it being slower to create JS
  boxes as it semantically does something different atm (see issue below)
  => Instead use conditional import of `dart:_wasm` which provides the
  necessary primitives
  => Similar for going from JS to Dart.

* Avoid calling converting from Dart object to JSAny more than needed
  (we did the operation twice for each registration and once for
  unregistration)

Issue dart-lang/sdk#55183

change condition

move
mkustermann added a commit to mkustermann/engine that referenced this issue Mar 18, 2024
…ting to JS wrappers

The `js_util.jsify()` related code shows up in CPU profile of wonderous.

=> Any `SkwasmObjectWrapper` object invokes this logic in the
constructor and dispose method.

This PR

* makes `DomFinalizationRegistryExtension` accept JSAny types instead of
  Dart types and internally converting
  => Callsites can call more precise `<>.toJS*` extension methods
  => Avoids extra type checks on the objects

* avoids making `toJSAnyShallow` delegate to `toJSAnyDeep` in wasm
  => `toJSAnyDeep` uses `js_util.jsify()` which is slow
  => We cannot use `Object.toJSBox` due to it being slower to create JS
  boxes as it semantically does something different atm (see issue below)
  => Instead use conditional import of `dart:_wasm` which provides the
  necessary primitives
  => Similar for going from JS to Dart.

* Avoid calling converting from Dart object to JSAny more than needed
  (we did the operation twice for each registration and once for
  unregistration)

Issue dart-lang/sdk#55183

change condition

move
mkustermann added a commit to flutter/engine that referenced this issue Mar 18, 2024
…ting to JS wrappers (#51375)

The `js_util.jsify()` related code shows up in CPU profile of wonderous.

=> Any `SkwasmObjectWrapper` object invokes this logic in the
constructor and dispose method.

This PR

* makes `DomFinalizationRegistryExtension` accept `JSAny` types instead
of Dart types and internally converting
  => Callsites can call more precise `<>.toJS*` extension methods
=> Will avoids extra type checks on the objects when we can call
`Object.toJSBox` directly

* makes us use a `toJSWrapper` / `fromJSWrapper` which will not delegate to a recursive `jsify()` but simply externalize the wasm gc object
=> We cannot use `Object.toJSBox` due to it being slower to create JS
boxes as it semantically does something different atm (see issue below)
=> Instead use conditional import of `dart:_wasm` which provides the
necessary primitives
  => Similar for going from JS to Dart.

* Avoid converting from Dart object to `JSAny` more than needed
(we did the operation twice for each registration and once for
unregistration)

Issue dart-lang/sdk#55183
@mkustermann
Copy link
Member Author

We now have Object.toExternalReference and ExternalDartReference.toDartObject extension methods, so I'm going to close this issue.

Thanks @srujzs for adding those!

@mkustermann
Copy link
Member Author

mkustermann commented Apr 18, 2024

Interestingly enough js_util.jsify() is high overhead due to many type tests and recursive behavior, but it does not perform the complex mechanism of obtaining JS wrappers. Instead it makes the simple & fast way to get JS wrapper, namely WasmAnyRef.fromObject(object).externalize().toJS.

@srujzs This seems to be still the case. It may be a discrepancy between dart2js/ddc and dart2wasm. Should this get aligned?

@srujzs
Copy link
Contributor

srujzs commented Apr 19, 2024

I believe it's aligned as it is:

. All three compilers return an ExternalDartReference when given an unrelated Dart object. In DDC and dart2js, both this operation and toExternalReference are a no-op. In dart2wasm, they both externalize the Dart object.

There's still the intention of disallowing unrelated Dart objects from being passed to jsify that we discussed here: #55222.

@mkustermann
Copy link
Member Author

All three compilers return an ExternalDartReference when given an unrelated Dart object. In DDC and dart2js, both this operation and toExternalReference are a no-op. In dart2wasm, they both externalize the Dart object.

If we disallow sending unrelated dart objects, then that's fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler.
Projects
None yet
Development

No branches or pull requests

2 participants