engine
engine copied to clipboard
[web] Avoid using `js_util.{jsify,dartify}()` in dart2wasm for converting 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
DomFinalizationRegistryExtensionacceptJSAnytypes 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 callObject.toJSBoxdirectly -
avoids making
toJSAnyShallowdelegate totoJSAnyDeepin wasm =>toJSAnyDeepusesjs_util.jsify()which is slow => We cannot useObject.toJSBoxdue to it being slower to create JS boxes as it semantically does something different atm (see issue below) => Instead use conditional import ofdart:_wasmwhich provides the necessary primitives => Similar for going from JS to Dart. -
Avoid calling converting from Dart object to
JSAnymore than needed (we did the operation twice for each registration and once for unregistration)
Issue https://github.com/dart-lang/sdk/issues/55183
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.
Something fishy going on with the imports - possibly due to this sdk_rewriter.dart tool, I'll have a look later.
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 https://github.com/dart-lang/sdk/issues/55183 / https://github.com/dart-lang/sdk/issues/55187 - if a new API gets added, we can replace this PR by calling the new API.
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.
@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
Thanks, @eyebrowsoffire !