1073766 - Globals end up with resolve hooks, which makes it hard to optimize things on their proto chain

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1063878 comment 4.

In particular, globals pretty much always end up with a resolve hook to be able to call JS_ResolveStandardClass (unless they want to eagerly resolve them, which in workers we likely don't) and then things like IonBuilder::objectsHaveCommonPrototype start returning false and we end up not being able to do getter optimizations in IonMonkey.

Now maybe we can do something clever in objectsHaveCommonPrototype (e.g. if the objects with a resolve hook are singletons, because we've already resolved the property on them, right?) or maybe we can make the ClassHasResolveHook test return false in special cases where the name is a standard class name and all the resolve hook does is resolve standard classes.... or something.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, also we have the wonderful types::CanHaveEmptyPropertyTypesForOwnProperty thing, which basically means "deoptimize globals"....

Comment 2

[Johannes Schulte [:jschulte]]

Attachment: resolve_v1.patch

Comment 3

[Johannes Schulte [:jschulte]]

Attachment: lazy_types_v1.patch

This is based on bug 1063878, which should land soon.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8509497 [details] [diff] [review]
lazy_types_v1.patch

Review of attachment 8509497 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, right now there really isn't a way to to do this entirely using TI because of the way undefined global properties are represented and the implicit notion of an 'own' property in TI.  The simplest and still pretty fast thing here would be to include a shape guard on the global when optimizing things on its prototype chain, which will take care of both the TI undefined properties issue and the resolve hook issue.

::: js/src/jit/IonBuilder.cpp
@@ +8787,5 @@
>              // Test for isOwnProperty() without freezing. If we end up
>              // optimizing, freezePropertiesForCommonPropFunc will freeze the
>              // property type sets later on.
> +            // Baseline instantiated lazy propertysets on the global
> +            // for this name, so we don't have to check for that.

There are several problems with this chain of reasoning:

- Instantiating a property on a global (aka an object where CanHaveEmptyPropertyTypesForOwnProperty) will not cause its type set to be filled with an undefined type if the property is undefined.  See UpdatePropertyType in jsinfer.cpp.

- There is no guarantee that baseline has done anything at a given opcode, unless there is a baseline cache attached to it.

@@ +8831,5 @@
> +#ifdef DEBUG
> +            JSObject *singleton = type->singleton();
> +            if(!singleton || !singleton->is<GlobalObject>())
> +                MOZ_ASSERT(!property.isOwnProperty(constraints()));
> +#endif

The call to isOwnProperty() is effectful --- it is what does the freezing --- and can't be made DEBUG only here.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8509495 [details] [diff] [review]
resolve_v1.patch

Review of attachment 8509495 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +8782,5 @@
> +                // The resolve-hook on globals is JS_ResolveStandardClass,
> +                // which does nothing more than resolving some standard class-names.
> +                // If name is one of those, it has already been resolved in the Interpreter,
> +                // so we are safe to skip the resolve-hook here.
> +            }

For the same reason as in the other patch, there isn't any guarantee in IonBuilder that any opcodes have executed in either the interpreter or baseline, and that resolve hooks have run.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Now to be fair, the only callsite of objectsHaveCommonPrototype is IonBuilder::testCommonGetterSetter.  And that's only called if we did in fact find baseline stubs...  I agree that this is a bit fragile, though.  :(

Comment 7

[Johannes Schulte [:jschulte]]

Attachment: v2.patch

This way?

Comment 8

[Johannes Schulte [:jschulte]]

Btw, thanks for the quick feedback and explanations!
Moreover, feel free to take this bug, if that's less time-consuming for you than doing the reviews/feedbacks. I'd just be glad to see Bug 1063878 being not too pointless :)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8511112 [details] [diff] [review]
v2.patch

I think this has an issue similar to bug 1081274: if the global's shape changes after the first time we ion-compile we will enter a bailout loop.

The patch in that bug will make us at least avoid the bailout loop and just take a slower ion codepath, but ideally we'd update the receiverShape in the baseline stub changes...

We can do that as a followup once bug 1081274 lands.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8511112 [details] [diff] [review]
v2.patch

Review of attachment 8511112 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineIC.h
@@ +4784,5 @@
>  // Stub for calling an native getter on a native object when the getter is kept on the proto-chain.
>  class ICGetProp_CallNativePrototype : public ICGetPropCallPrototypeGetter
>  {
>      friend class ICStubSpace;
> +    static const uint16_t IS_GLOBAL_BIT = 0x1;

I think you can remove this and all the related changes, and determine whether the receiver is a global object with receiverShape->getObjectClass()->flags & JSCLASS_IS_GLOBAL.

::: js/src/jit/IonBuilder.cpp
@@ +8800,5 @@
> +            if (ClassHasResolveHook(compartment, clasp, name)) {
> +                if (!singleton || !singleton->is<GlobalObject>())
> +                    return false;
> +                *guardGlobal = true;
> +            }

Style nit: need a newline here

@@ +8820,5 @@
>              }
> +            if (singleton) {
> +                if (types::CanHaveEmptyPropertyTypesForOwnProperty(singleton)) {
> +                    if (!singleton->is<GlobalObject>())
> +                        return false;

This can MOZ_ASSERT(singleton->is<GlobalObject>()) instead.

@@ +8881,3 @@
>      // Check if all objects being accessed will lookup the name through foundProto.
> +    if (!objectsHaveCommonPrototype(types, name, isGetter, foundProto, &guardGlobal) ||
> +        (guardGlobal && !globalShape))

Style nit: needs {}

@@ +8884,2 @@
>          return nullptr;
> +    

Style nit: excess whitespace

::: js/src/jsinfer.cpp
@@ +1455,3 @@
>          if (CanHaveEmptyPropertyTypesForOwnProperty(obj))
>              return true;
>      }

Unfortunately the CanHaveEmptyPropertyTypesForOwnProperty abstraction isn't helping much here, since CanHaveEmptyPropertyTypes iff is<GlobalObject>() so this condition is a lot more complicated than it needs to be.  I think you can rename checkGlobal to allowEmptyTypesForGlobal here and on freezePropertiesForCommonPrototype (which will avoid negating the condition when calling freezePropertiesForCommonPrototype) and then simplify this to:

MOZ_ASSERT(CanHaveEmptyPropertyTypesForOwnProperty(obj) == obj->is<GlobalObject>());
if (obj && !allowEmptyTypesForGlobal) {
    if (CanHaveEmptyPropertyTypesForOwnProperty(obj))
        return true;
}

Comment 11

[Johannes Schulte [:jschulte]]

Attachment: v3.patch

Based on bug 1081274, which, I assume, will land first.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 8511541 [details] [diff] [review]
v3.patch

Review of attachment 8511541 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineInspector.cpp
@@ +523,2 @@
>              else
>                  MOZ_ASSERT(getter == nstub->getter());

These conditions are complicated and hard to follow.  I think this needs a new helper function:

static Shape *GlobalShapeForGetPropFunction(ICStub *stub)
{
    if (stub->isGetProp_CallNativePrototype()) {
        ICGetProp_CallNativePrototype *nstub =
            stub->toGetProp_CallNativePrototype();
        if (nstub->receiverShape()->getObjectClass()->flags & JSCLASS_IS_GLOBAL)
            return nstub->receiverShape();
    }
    return nullptr;
}

Then this block of conditions can just be:

if (!holder) {
    holder = nstub->holder();
    holderShape = nstub->holderShape();
    getter = nstub->getter();
    global = GlobalShapeForGetPropFunction(nstub);
} else if (nstub->holderShape() != holderShape || GlobalShapeForGetPropFunction(nstub) != global)
    return nullptr;
} else {
    ...
}

Comment 13

[Boris Zbarsky [:bzbarsky]]

So if something changes the global shape after we've first ion-compiled we won't take the fast path anymore with this patch, right?

Comment 14

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> So if something changes the global shape after we've first ion-compiled we
> won't take the fast path anymore with this patch, right?

Yeah.  I guess we could use the last global shape encountered in the baseline stubs instead, though if the global changes shape too many times we would probably stop attaching new stubs at some point.  An alternative would be to evict older stubs when a specific object associated with the stub changes shape, though that is also vulnerable to the object changing shapes many times.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah.  See bug 1081274 for a similar thing we're doing...

Comment 16

[Johannes Schulte [:jschulte]]

Attachment: v4.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9a0a9acab7a3

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87f42360c1bd

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87f42360c1bd

Comment 19

[Boris Zbarsky [:bzbarsky]]

Do we need a followup bug on comment 14?

Comment 20

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> Do we need a followup bug on comment 14?

This would be nice, but not I think necessary unless this problem is being observed in the wild.

Comment 21

[Boris Zbarsky [:bzbarsky]]

It's pretty easy to cause the problem (including by accident, given the resolve hook on globals), but it looks like when it happens we now fall back to an IC instead of generating the DOM fast path, which is slower but not catastrophically so.  For future reference, here's a worker testcase that shows a slowdown due to a shape change on the global:

  function f() {
    var start = performance.now();
    var count = 1000000;
    var end;
    for (var i = 0; i < count; ++i)
      end = performance.now();
    return (end - start) / count * 1e6;
  }

  postMessage(f());
  Date;
  postMessage(f());