replace brand checks: was Investigate the performance impact of xsnap-lockdown `isMap`

[ivanlei]

What is the Problem Being Solved?

packages/xsnap-lockdown/lib/object-inspect.js line 333 isMap() generates a lot of debug spew suggesting it might be creating a lot of short-lived Error objects in otherwise normal code path which might be expensive and result in slow down of core scenarios.

Description of the Design

Security Considerations

Scaling Considerations

Test Plan

[mhofman]

I believe it's safe for us to switch to instanceof style checks in object-inspect.js as we don't have realms identity discontinuity type problems, and all intrinsics are frozen, which means the prototype properties have not been tempered with. cc @michaelfig

[raphdev]

Not just isMap - practically every isX function will cause one of these exceptions. For example:

xsnap/src/object-inspect.js:376: exception: Set.prototype.size: this is no Set instance!

Corresponding line in object-inspect.js (src):

setSize.call(x); // setSize = Object.getOwnPropertyDescriptor(Set.prototype, 'size').get;

In XS that will check whether x is a Set (src):

txSlot* fxCheckSetInstance(txMachine* the, txSlot* slot, txBoolean mutable)
{
    if (slot->kind == XS_REFERENCE_KIND) {
        txSlot* instance = slot->value.reference;
        if (((slot = instance->next)) && (slot->flag & XS_INTERNAL_FLAG) && (slot->kind == XS_SET_KIND) && (instance != mxSetPrototype.value.reference)) {
            if (mutable && (slot->flag & XS_DONT_SET_FLAG))
                mxTypeError("Set instance is read-only");
            return instance;
        }
    }
    mxTypeError("this is no Set instance");
    return C_NULL;
}

And what we see is the logged exception (which is printed when mxNoConsole is not set).

Worth noting mxTypeError results in slots being allocated, likely for the exception being thrown. We'll have to understand the extent to which this can impact performance. Given the volume of debug output, it's possible this can contribute to gc stress.

[michaelfig]

This can be fixed by changing all of packages/xsnap-lockdown/lib/object-inspect.js is* functions that use try-catch brand checks into just using instanceof instead.

The instanceof pattern doesn't work for cross-realm objects, but we don't have any of those in xsnap at this time.

[FUDCo]

In addition to the try/catch folderall, the various isX functions seem to also contain other boilerplate, e.g., isMap currently looks like:

function isMap(x) {
  if (!mapSize || !x || typeof x !== 'object') {
    return false;
  }
  try {
    mapSize.call(x);
    try {
      setSize.call(x);
    } catch (s) {
      return true;
    }
    return x instanceof Map; // core-js workaround, pre-v2.5.0
  } catch (e) {}
  return false;
}

Getting rid of the try/catch reduces it to:

function isMap(x) {
  if (!mapSize || !x || typeof x !== 'object') {
    return false;
  }
  return x instanceof Map;
}

which raises in my mind the question as to whether all that other stuff is necessary, e.g., could this simply be reduced to

function isMap(x) {
  return x instanceof Map;
}

My take is the !mapSize check is pre-flighting something that was needed for the try/catch test and is therefor now nugatory, while the other checks are trying to fast-path the failure case, but I expect close to 100% of the time the check function this is called we'll be in the success case (i.e., the thing we're checking to be sure is actually a Map actually is) so optimizing for the failure case is a net loss.

Opinions, anyone?

[FUDCo]

Also, isWeakRef uses the try/catch pattern without then running an instanceof check. Is there something weird about WeakRef or can this one also just be instanceof WeakRef?

[FUDCo]

Also also, isWeakRef tests for weakRef-ness by actually invoking the deref method, which I understand pins the ref for the rest of the turn, and therefor presumably has the potential to interfere with GC behavior, no?

[mhofman]

I expect close to 100% of the time the check function this is called we'll be in the success case (i.e., the thing we're checking to be sure is actually a Map actually is) so optimizing for the failure case is a net loss.

Let's verify that assumption. I believe once the value has been checked to be an object, it is then passed through a waterfall of isX check to find the sub-type.

Is there something weird about WeakRef or can this one also just be instanceof WeakRef?

Maybe because WeakRef may not exist on the global? Which would highlight the need for a check slightly more complex than x instanceof Foo.

Also also, isWeakRef tests for weakRef-ness by actually invoking the deref method, which I understand pins the ref for the rest of the turn, and therefor presumably has the potential to interfere with GC behavior, no?

Yikes, indeed. Another reason to get rid of this test.

[FUDCo]

Maybe because WeakRef may not exist on the global? Which would highlight the need for a check slightly more complex than x instanceof Foo.

Since the module gins up weakRefDeref by checking the global, I presume that checking for that will tell us if we're in a world with WeakRef or not. (And this would be an exception to the "let's get rid of all that test junk" proposal I made above.)

[FUDCo]

I believe once the value has been checked to be an object, it is then passed through a waterfall of isX check to find the sub-type.

Closer read of the code suggests that you are correct in this surmise.

[FUDCo]

If we're cascading through a bunch of checks that each start with !x || typeof x !== 'object' to make the individual checks faster, then perhaps the cascade itself should start with that check instead? Or at this point would that just be fitting the noise in the perf measurement?

[mhofman]

I am wondering if we couldn't get away with a switch(getPrototypeOf(x)) { case Map.prototype: ....