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

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: jschulte)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Oh, also we have the wonderful types::CanHaveEmptyPropertyTypesForOwnProperty thing, which basically means "deoptimize globals"....
(Assignee)

Comment 2

3 years ago
Created attachment 8509495 [details] [diff] [review]
resolve_v1.patch
Attachment #8509495 - Flags: feedback?(bobbyholley)
(Assignee)

Comment 3

3 years ago
Created attachment 8509497 [details] [diff] [review]
lazy_types_v1.patch

This is based on bug 1063878, which should land soon.
Attachment #8509497 - Flags: feedback?(bhackett1024)
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.
Attachment #8509497 - Flags: feedback?(bhackett1024) → feedback-
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.
Attachment #8509495 - Flags: feedback?(bobbyholley) → feedback-
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.  :(
(Assignee)

Comment 7

3 years ago
Created attachment 8511112 [details] [diff] [review]
v2.patch

This way?
Attachment #8509495 - Attachment is obsolete: true
Attachment #8509497 - Attachment is obsolete: true
Attachment #8511112 - Flags: feedback?(bhackett1024)
(Assignee)

Comment 8

3 years ago
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 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 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;
}
Attachment #8511112 - Flags: feedback?(bhackett1024) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8511541 [details] [diff] [review]
v3.patch

Based on bug 1081274, which, I assume, will land first.
Assignee: nobody → j_schulte
Attachment #8511112 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8511541 - Flags: review?(bhackett1024)
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 {
    ...
}
Attachment #8511541 - Flags: review?(bhackett1024) → review+
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?
(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.
Yeah.  See bug 1081274 for a similar thing we're doing...
(Assignee)

Comment 16

3 years ago
Created attachment 8515702 [details] [diff] [review]
v4.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9a0a9acab7a3
Attachment #8511541 - Attachment is obsolete: true
Attachment #8515702 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
OS: Mac OS X → All
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f42360c1bd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87f42360c1bd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Do we need a followup bug on comment 14?
Flags: needinfo?(bhackett1024)
(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.
Flags: needinfo?(bhackett1024)
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());
Depends on: 1101823
You need to log in before you can comment on or make changes to this bug.