Add specialized JS-interop method calls

[yjbanov]

In Flutter we see quite a bit of overhead in JS-interop method calls when using JsObject.callMethod. Currently on our CanvasKit rendering backend the call overhead can reach up to 80% of the total time it takes to do all the rendering work in our paint phase.

Here's a sample profile (notice the blue gaps of activity in between the yellow/green of useful work):

Here's a zoom in into a sequence of value conversions that are happening:

As you can see there are a lot of conversions happening, particularly around wrapping and unwrapping argument lists, and unwrapping return values.

In most cases we know exactly the method we call and the number of arguments we pass, so there's no need to wrap arguments in a list. In many cases there is no return value (void) and so there is no need to wrap it.

Proposal

Add specialized callMethodX and callVoidX methods to JsObject with the following signatures:

dynamic callMethod0(method);
dynamic callMethod1(method, arg1);
dynamic callMethod2(method, arg1, arg2);
dynamic callMethod3(method, arg1, arg2, arg3);
dynamic callMethod4(method, arg1, arg2, arg3, arg4);
...
dynamic callMethod20(method, arg1, arg2, arg3, arg4);

void callVoid0(method);
void callVoid1(method, arg1);
void callVoid2(method, arg1, arg2);
void callVoid3(method, arg1, arg2, arg3);
void callVoid4(method, arg1, arg2, arg3, arg4);
...
void callVoid20(method, arg1, arg2, arg3, arg4);

These methods should all be efficient and inlineable.

Examples of Flutter engine code that does a lot of JS interop:

• https://github.com/flutter/engine/blob/master/lib/web_ui/lib/src/engine/compositor/path.dart
• https://github.com/flutter/engine/blob/master/lib/web_ui/lib/src/engine/compositor/canvas.dart

/cc @sigmundch @rakudrama @jakemac53 @natebosch @jonahwilliams

[sigmundch]

I wonder if we could use a type parameter to combine callMethodX and callVoidX for example:

T callMethodX<T>(String name, dynamic arg1, ..., dynamic argX);

On an offline discussion @jakemac53 was suggesting to do a kernel transformer to make this more transparent. We'd add all the methods above, but a transformer would convert

callMethod(name, [1, 2, 3]);
var y = callMethod(name, [1, 2])

into

callVoid3(name, 1, 2, 3);
var y = callMethod2(name, 1, 2);

Because at the kernel level we can generate code accessing private members, we could in fact add all these methods as private members instead. This would mean that our changes would not be visible and it would be possible to change it again in the future without a breaking change if we are not satisfied with it.

[yjbanov]

I wonder if we could use a type parameter to combine callMethodX and callVoidX

I'm not sure. The effect I'm looking for is that we do not call _convertToDart. Can a type parameter do that?

[sigmundch]

I'm not sure. The effect I'm looking for is that we do not call _convertToDart. Can a type parameter do that?

I'm am not saying I like this, but one could do:

  return T == Null ? null :  _convertToDart(r);

The decision is made at runtime at that point.

[sigmundch]

At that point we might as well add a named parameter to indicate whether you want the return value converted :)

[jakemac53]

Adding a <T> could be helpful in terms of more eager casting into the expected type, and it allows void to express no return value.

[sigmundch]

Adding a could be helpful in terms of more eager casting into the expected type, and it allows void to express no return value.

unfortunately, I believe we can't use void in an expression (e.g. T == void)

[jakemac53]

ya it would require some compiler magic

[rakudrama]

Adding callMethod2 and friends would be a reasonable way to avoid processing the arguments via a List. It would not avoid calls to _convertToJs to convert the arguments.

I would be careful relying on Type.==. The latest nnbd spec requires some form of type canonicalization before comparison (to avoid unusable behaviour across opt-in/legacy). We can make it faster than walking the type tree, but it is not a simple operation.

Do we need all the features of JsObject? It is a wrapper, along with JsFunction and JsArray. There is support for 'proxy' wrappers to insulate JavaScript from 'seeing' the Dart representation of objects. If this is unnecessary, I think we can do much better via the more declarative @JS() annotation, where the methods may be declared with types, and would avoid wrapping the JavaScript objects, avoid wrapping the Dart pass-through objects, and have no calls to _convertToJs and _convertToDart.

[sigmundch]

I too would prefer we go down the route of making it possible to use @JS from dart:ui.

The minimum change here would be to move @JS to a dart:* package so it can be used from within dart:ui, and reexport it in package:js. Separately, we'd like to do a better job inlining the js-interop calls in dart2js, but that's not required (what we have today would be already a lot better than what you have via the JsObject APIs.)

@yjbanov - do @JS annotations work in your case? If for example the APIs you called via JsObject also exposed from the dart:html (e.g. CanvasElement/RenderContext2D), then @JS might not work out of the box and we'd need some compiler changes to support it.

[yjbanov]

@hterkelsen just checked, and @JS seems to produce the best output we could hope for, better than my original proposal in this issue (e.g. it bypasses Function.prototype.apply entirely and just calls the method). We can verify how well this works as soon as it lands.

[sigmundch]

We can verify how well this works as soon as it lands.

Can it actually land in our current state of affairs? That is, @JS cannot be used within the SDK today, so if there are scripts that convert package:web_ui to dart:ui, you will run into trouble at this time.

[jonahwilliams]

I think the proposal was to create a dart:_js which defined the annotation and re-exported it for package:js? Or something similar.

Then we could rewrite the package:js usage when creating the SDK.

[sigmundch]

Yes - I just misunderstood @yjbanov message and thought he was implying that you were planning on landing it before we do the work to move things into the dart:_package_js library.

[yjbanov]

Sorry for the confusion. What I meant was we're poised to adopt this new feature as soon as it's available. Currently JS interop is the single biggest performance killer for our WASM backend.

We're basically just sitting and refreshing this page waiting for the status of this issue to go from open to fixed 😃

[yjbanov]

BTW, the more I look at our profiles the more I'm convinced that moving @JS under dart:* is the right solution. I just discovered that JsObject.jsify can be outrageously slow too. For example, calling the following seemingly primitive makeSkRRect function can take >1ms on a Xeon workstation:

js.JsObject makeSkRect(ui.Rect rect) {
  return js.JsObject(canvasKit['LTRBRect'],
      <double>[rect.left, rect.top, rect.right, rect.bottom]);
}

js.JsObject makeSkRRect(ui.RRect rrect) {
  return js.JsObject.jsify({
    'rect': makeSkRect(rrect.outerRect),
    'rx1': rrect.tlRadiusX,
    'ry1': rrect.tlRadiusY,
    'rx2': rrect.trRadiusX,
    'ry2': rrect.trRadiusY,
    'rx3': rrect.brRadiusX,
    'ry3': rrect.brRadiusY,
    'rx4': rrect.blRadiusX,
    'ry4': rrect.blRadiusY,
  });
}

Profile:

[vsmenon]

@sigmundch @rakudrama @natebosch -

(1) Would moving package:js to dart:_js and re-exporting in package:js work? Any problems you can think of?

(2) We'd need to modify DDC and Dart2JS to special case dart:_js. I don't think this would be hard in the DDC case. Dart2JS?

[natebosch]

Any problems you can think of?

If we decide to not break existing JS interop, and we we decide that the future of JS interop looks slightly different then moving it now will add technical debt that we need to live with until Dart 3. In any place we need to understand the presence of an @JS annotation we will need to consider whether it is either the one from package:js or dart:_js.

If the exported version of the annotation becomes our golden path, we risk breaking old usages if we forget to check for both sources somewhere. We won't have test coverage of the old package:js versions in google3.

[sigmundch]

(1) Would moving package:js to dart:_js and re-exporting in package:js work?

Yes, I don't see any reason we couldn't make it work.

As long as we keep the dart:_package_js*1 private, we can move it back to package:js in the future without ever exposing it as a breaking change.

@natebosch is correct that we'll have some tax to lookup either annotation to preserve the backward compatibility with the old package:js, but I feel that's a small price to pay.

*1 note: there is a different dart:_js today, so we would have to decide whether to use that or a different name :)

If the exported version of the annotation becomes our golden path, we risk breaking old usages if we forget to check for both sources somewhere. We won't have test coverage of the old package:js versions in google3.

True, we could invest in adding a way to test in the sdk some code with the old package:js (using an old pinned copy)

I imagine the new package:js could be a patch release with an sdk constraint, so that users can easily move to the new version.

(2) We'd need to modify DDC and Dart2JS to special case dart:_js. I don't think this would be hard in the DDC case. Dart2JS?

Same - I don't expect this to be a hard change.

[vsmenon]

(1) Would moving package:js to dart:_js and re-exporting in package:js work?
*1 note: there is a different dart:_js today, so we would have to decide whether to use that or a different name :)

I've always been bad with names. :-)

(2) We'd need to modify DDC and Dart2JS to special case dart:_js. I don't think this would be hard in the DDC case. Dart2JS?

Same - I don't expect this to be a hard change.

For DDC, it may suffice to just add an additional test for dart:_package_js here:

https://github.com/dart-lang/sdk/blob/master/pkg/dev_compiler/lib/src/kernel/js_interop.dart#L14

[sigmundch]

quick follow up - in a design discussion today we made a determination: rather than reexporting them from package:js, we prefer to start by duplicating the package:js annotations inside a dart:_js_annotations library.

The compiler work to support both annotations is needed regardless to be backwards compatible, but creating a second copy makes it easier to be future compatible and avoid a breaking change until we decide what the future of package:js will be.

[yjbanov]

Thanks for the follow-up. Will the duplicate annotations' API surface and semantics be identical to that of package:js? Because we currently exist both as a package:ui and dart:ui what we plan to do is use package:js in package:ui and dart:_js_annotations in dart:ui. Will that work?

[sigmundch]

Yes, that's would be the goal - Since the library is private, for a long time you'll be the only user of those annotations (from dart:ui), so we'll make sure it works for you :)

[vsmenon]

@sigmundch @natebosch @rakudrama - can we list the breaking changes we are considering? E.g., support for dynamic dispatch, extending JS, implementing JS, etc.

[sigmundch]

Thanks for the reminder @vsmenon

We are planning changes for js-interop in the next few months. We still have to iron out the design, so we don't plan to enforce any new checks at this time. However, it would be great if any uses you have in dart:ui/package:web_ui follows this guideline:

• no is: don't use is checks on JS-interop types, it is ok to use as casts though.
• all anonymous: consider always using @anonymous for any js-interop class. This is a bit safer because today DDC supports is checks on non-annonymous classes, but dart2js doesn't. In the future we may drop all support for is checks in DDC as well.
• no dynamic invocation: avoid dynamic calls of js-interop members. To do so, ensure the receiver of those calls is typed properly. In the future we may require all js-interop calls to be known statically.
• no subtypes: Do not implement a js-interop type with a Dart class (e.g. no mocks) and only extend js-interop types with other js-interop types. In the future we may seal js-interop types and not allow extending or implementing them from Dart.

[yjbanov]

Those restrictions look fine to me.

What's the difference between the "no is" and "all anonymous" restrictions?

[sigmundch]

Almost no practical difference, but we may be removing bits that affect each one separately.

non-anonymous js-interop classes expose a constructor name to dart, both dart2js and ddc use it to construct the instances, and only ddc uses it for is checks. Avoiding them will make it easier to ensure that you are under the same semantics in ddc and dart2js today. We haven't decided whether we intend to keep the constructor capability in the future.

For is checks, anonymous js-interop classes have a funny property that they will say true if you ask is Foo for any other anonymous type (and this is also the case for non-anonymous classes in dart2js). I'm guessing in the future we'll try to statically detect those is checks and possibly require that you don't use them (use is JavaScriptObject instead or something general that includes all js-interop types).

[yjbanov]

Closing this as @JS has been implemented. However, I filed #42522 to improve the performance of @JS annotation (it is 7x slower than it could be).