Slow interop type tests: foo.isA<Foo> or foo.isInstanceOfString("Foo")

[mkustermann]

Currently it seems the following code

import 'dart:js_interop';

@JS()
external JSAny get object;

@JS("Window")
extension type Window(JSAny? obj) {}

main() {
  print(object.instanceOfString('Window'));
  // or
  print(object.isA<Window>());
}

get compiled to something like this

main() {
    print(JSAnyUtilityExtension|instanceOfString(object));
}

which is implemented in sdk/lib/js_interop/js_interop.dart

  bool instanceOfString(String constructorName) {
    if (constructorName.isEmpty) return false;
    final parts = constructorName.split('.');
    JSObject? constructor = globalContext;
    for (final part in parts) {
      constructor = constructor?[part] as JSObject?;
      if (constructor == null) return false;
    }
    return instanceof(constructor as JSFunction);
  }

  // Implemented as
  //   JS("(o, c) => o instance of c",
  //       obj.toExternRef,
  //       constructor.toExternRef)`
  external bool instanceof(JSFunction constructor);

This is very inefficient as we do the instanceOfString logic (splitting string by ., looking up in the global context) for every type test.

A simple optimization would be to just cache the constructor in a global variable, so any following type checks will simply call the JS <obj> instanceof <constructor>. Maybe there's other ways to optimize it more, but at least this we should do.

One may also consider compiling this to per-type JS helper functions, e.g. object.isA<Window> to (o) => o instanceof Window instead of a generic (o, c) => o instanceof c -- if that's beneficial for performance

In some DOM related code (e.g. jaspr) this shows up on the profile and I think we should optimize this.

/cc @srujzs @osa1 Could one of you take a look at this?
One could

[srujzs]

Similar issue: #56130

Caching the constructor isn't always safe as the the class could possibly change since the last isA call.

Some options:

• Avoid the instanceOfString logic if we don't have a type with .s and use instanceof directly (this will avoid the string processing at least).
• Maybe use functions like these:

bool _specialInstanceOfForNoDots(String s) { // instanceof Foo
  return JS("(o, gc, s) => o instanceof gc[s]", o.toExternRef, globalContext.toExternRef, s.toJS.toExternRef);
}

bool _specialInstanceOfForOneDot(String s1, String s2) { // instanceof Foo.Bar
  return JS("(o, gc, s1, s2) => o instanceof gc[s1][s2]", o.toExternRef, globalContext.toExternRef, s1.toJS.toExternRef, s2.toJS.toExternRef);
}

Using string interpolation within the JS string won't work for dart2wasm, but maybe it's a possibility for dart2js/ddc for further optimization.

[mkustermann]

Caching the constructor isn't always safe as the the class could possibly change since the last isA call.

Just like in other situations (see e.g. #55183) it seems to me that we are paying a high price for an uncommon thing.

It's probably very rare that a isA<X>() will answer one way and later on in the program execution answers in a different way (i.e. the resolution of name to constructor yields a different constructor). So I would say that if we want to expose a way to handle this uncommon case, we should make it harder to use and make the common thing easy to use & fast. For example dynamicIsA<T> could do the slow version and resolve on each call and isA<X> could optimize it at-will (even resolve before main runs if we decided to).

(We may also put a warning on the isInstanceOfString to tell users to prefer using isA<T>)

If we don't do this, then we should tell users that suffer performance issues to resolve constructors manually and use the unsafe low-levelinstanceof mechanism (if we expose this?)

[mkustermann]

From a simple not very scientific benchmark

import 'dart:js_interop';
import 'dart:js_interop_unsafe';

@JS('globalThis')
external JSObject get window;

@JS("Array") // so it works with D8
extension type Window(JSAny a) {}

JSFunction? getConstructor(String constructorName) {
  if (constructorName.isEmpty) return null;
  final parts = constructorName.split('.');
  JSObject? constructor = globalContext;
  for (final part in parts) {
    constructor = constructor?[part] as JSObject?;
    if (constructor == null) return null;
  }
  return constructor as JSFunction;
}

final windowConstructor = getConstructor('Array')!;

main() {
  for (int i = 0; i < 5; ++i) {
    measure<bool>(
      i == 4,
      'window.instanceOfString("Window")',
      () => window.instanceOfString('Window'),
    );
    measure<bool>(i == 4, 'window.isA<Window>()', () => window.isA<Window>());
    measure<bool>(
      i == 4,
      'window.instanceOf',
      () => window.instanceof(windowConstructor),
    );
  }
}

T measure<T>(bool doPrint, String name, T Function() fun) {
  final sw = Stopwatch()..start();
  late T value;
  final count = 1000000;
  for (int i = 0; i < count; ++i) {
    value = fun();
  }
  if (doPrint) {
    print('${sw.elapsed.inMicroseconds / count} us/call for $name');
  }
  return value;
}

seems to indicate

% pkg/dart2wasm/tool/compile_benchmark  -O2 -g test.dart test.wasm
...
% pkg/dart2wasm/tool/run_benchmark test.wasm
0.379 us/call for window.instanceOfString("Window")
0.547 us/call for window.isA<Window>()
0.095 us/call for window.instanceOf

=> It's much faster to use cached constructor (if I measured it right?)

[srujzs]

I don't necessarily mind doing the potentially unsafe thing here as long as we document it. It just means there's a slim chance of something breaking when we make that change.

Alternatively, I did a quick mockup of the second option in #60344 (comment), and the benchmarks are a lot closer (I changed everything to use Array to be consistent FYI):

0.261 us/call for jsArray.instanceOfString("Array")
0.263 us/call for jsArray.isA<JSArray>
0.018 us/call for jsArray.instanceOf
0.048 us/call for specialInstanceOfForNoDots

The benefit is naturally avoiding that slim chance of breakage but also avoiding adding any logic into the compilers to cache constructors correctly. I assume the slightly higher cost compared to the cached constructor is the extra property access on globalThis. It may also further go up with types nested in namespaces e.g. @JS('a.b.c'), but those tend to be rarer.

[mkustermann]

Alternatively, I did a quick mockup of the second option in #60344 (comment), and the benchmarks are a lot closer (I changed everything to use Array to be consistent FYI):

Sounds promising that it would become 5x faster!
Though it seems using constructors would still gain an additional 2x?

We have it as a goal to allow performant Dart web frameworks based on DOM manipulation - any such framework would probably have such type checks in the hot path. So they want it to be as fast as possible.

The question is whether we ask those users to rewrite their code to avoid using isA<>() and instanceOfString and instead get constructor manually and use instanceof -- Or we make our existing APIs fast. In our docs we do recommend people to use isA<>() -- it would be hard to tell people to avoid it if they want fast performance. Which is why I'd be very much in favor of making the easy&common thing (e.g. isA<>()) to be optimized as much as possible and have another mechanism for uncommon cases.

Using string interpolation within the JS string won't work for dart2wasm, but maybe it's a possibility for dart2js/ddc for further optimization.

Our compiler sees the isA<>() and isInstanceOfString call sites and their constant arguments so we can specialize this somehow.

I don't necessarily mind doing the potentially unsafe thing here as long as we document it. It just means there's a slim chance of something breaking when we make that change.

To be honest I find that this "unsafe code" if caching but "safe code" if not caching a fallacy: If we assume that the type check result can change at any time during execution of the program then one cannot really write "safe programs". Imagine:

JSObject node;
if (node.isInstanceOfString("HtmlElement")) {
  foo();
  use((node as Foo).someHtmlElementProperty);
  ...;
}

The call to foo() could call out to JavaScript and mess around with prototype chains, modify globalThis - it could have any side-effect. So we could say that this code - i.e. the node as Foo cast - is not safe (irrespective of whether we cache the constructor lookup or re-resolve it every time).

It's true that foo() is user-written and so users theoretically have control over any side-effect causing operations, but practically users will not know all side-effects of code they call and they will still assume that the type check result doesn't change.

My point is that the code that uses isA<>() / isInstanceOfString / instanceof will in practice always assume that once it gets its answer that this answer doesn't change anymore and any following code can assume it has that type that was checked against and supports those properties/methods/...

In general I view the new JS interop as being static: We declare JS type and their properties/methods statically using extension types -- we assume therefore that at runtime properties/methods aren't added/removed to those types. Similarly I'd view the is<A>() as doing a check against a statically known JS type - and assume that the type doesn't change (just as we assume that the statically declared properties/methods don't change)

[gmpassos]

I don't necessarily mind doing the potentially unsafe thing here as long as we document it. It just means there's a slim chance of something breaking when we make that change.

To be honest I find that this "unsafe code" if caching but "safe code" if not caching a fallacy: If we assume that the type check result can change at any time during execution of the program then one cannot really write "safe programs". Imagine:

Just to point out that DOM types, like HTMLDivElement, are predefined by the browser and usually have a well-defined lifecycle. In other words, these DOM types are not meant to be changed or dynamically modified at runtime. I’m not even sure if most browsers support that, as it could completely break DOM integration with JavaScript.

IMHO, this kind of optimization with DOM types is "safe enough."