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

[web] Avoid using js_util.{jsify,dartify}() in dart2wasm for converting to JS wrappers #51375

Merged
merged 7 commits into from Mar 18, 2024

Conversation

mkustermann
Copy link
Member

@mkustermann mkustermann commented Mar 13, 2024

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

  • 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

@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.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Mar 13, 2024
@mkustermann
Copy link
Member Author

Something fishy going on with the imports - possibly due to this sdk_rewriter.dart tool, I'll have a look later.

@mkustermann
Copy link
Member Author

Not sure why some things fail here and not sure whether it's worthwhile to debug.

Web/Interop team seems open to my suggestion on dart-lang/sdk#55183 / dart-lang/sdk#55187 - if a new API gets added, we can replace this PR by calling the new API.

…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
Copy link
Member Author

It appears some existing flutter web engine code uses thetoJSAnyShallow/toObjectShallow API despite it actually wanting a deep conversion (it happens to work in dart2js - as dart objects are JS objects as well, but not in dart2wasm)

So I've kept the old implementation and added a toJSWrapper / fromJSWrapper that we use for the finalizer code.

@mkustermann
Copy link
Member Author

@eyebrowsoffire I've made the PR now only affect finalizer code (which I saw on cpu profile), ( thereby not affecting other .toJSAnyShallow call sites that may actually require deep). Let me know if it looks ok to you

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!

@mkustermann
Copy link
Member Author

Thanks, @eyebrowsoffire !

@mkustermann mkustermann merged commit fda5c69 into flutter:main Mar 18, 2024
23 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 18, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 18, 2024
…145365)

flutter/engine@89df726...0ee413e

2024-03-18 chinmaygarde@google.com A native Android unit-testing harness. (flutter/engine#51479)
2024-03-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Fuchsia] only download fuchsia deps when necessary (#51439)" (flutter/engine#51500)
2024-03-18 kustermann@google.com [web] Avoid using `js_util.{jsify,dartify}()` in dart2wasm for converting to JS wrappers (flutter/engine#51375)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@mkustermann mkustermann deleted the jsify branch April 24, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-web Code specifically for the web engine
Projects
None yet
3 participants