1007631 - Make IonBuilder::getPropTryCommonGetter work with the innerize-window optimization

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

In bug 964915 I'm adding an optimization for GETPROP/SETPROP on outer window proxies by doing them on the inner window instead.

After we add getter/setter stubs to Baseline that perform the same optimization, we can call getPropTryCommonGetter (which requires a Baseline stub) in getPropTryInnerize.

We have to be careful to do this only for getters that have needsOuterizedThisObject == false though, see the comment in getPropTryInnerize.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I was poking at this a bit in baseline.  It looks to me like there we can't rely on type information, right?  So do we have to create some sort of IC that basically guards on the object being the outer window proxy for our global and then duplicates what the normal getprop ICs do?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #1)
> So I was poking at this a bit in baseline.  It looks to me like there we
> can't rely on type information, right?  So do we have to create some sort of
> IC that basically guards on the object being the outer window proxy for our
> global and then duplicates what the normal getprop ICs do?

Yes. Instead of duplicating it might be easier to use the normal getprop ICs and add a bool to the ICStubCompiler. If it's set, we emit the extra outer/inner window check. The bool should also be added to the value returned by the ICStubCompiler's getKey() method (the "key" is used to share Baseline IC code between stubs, two stubs with the same key should have the same JIT code).

Or maybe adding a new stub kind is simpler, if it doesn't duplicate too much code.

Comment 3

[Johannes Schulte [:jschulte]]

Hey Boris,
I've also been looking into this a bit, and have a somewhat working patch ready. I will have to see, whether it really works and passes the tests, but this should be done soon.
So if you haven't started working on this and it's not too urgent, we can let Jan first look over my patch and then decide, whether the approach works or you take the bug :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

I haven't started on this, and this is not too urgent.  If you're willing to do this, that would be totally awesome from my point of view!

Comment 5

[Johannes Schulte [:jschulte]]

Attachment: v1.patch

Gives me a 10x speedup on baseline-only and 33% speedup on ion on this microbenchmark:

var start = new Date;
for (var i = 0; i < 100000; ++i) {
  window.performance.now();
}
var end = new Date;
document.body.textContent = end - start;

Do we want to add the optimization for slot-reads in baseline, too? (added complexity vs. speedup)

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8525574 [details] [diff] [review]
v1.patch

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

Sorry for the delay. Even after working on the Ion optimization I still have to look up how the inner/outer window thing works.

Looks good, thanks for working on this! Main issue is that we're missing a guard in the generated code, let me know if you have any questions about that.

::: js/src/jit/BaselineIC.cpp
@@ +6443,5 @@
> +        inner = GetInnerObject(obj);
> +        MOZ_ASSERT(script->global().isNative());
> +        if (inner != &script->global())
> +            return true;
> +        // ICGetProp_CallNative*::Compiler::generateStubCode depends on this

Nit: add a period at the end.

@@ +6523,5 @@
>          *attached = true;
>          return true;
>      }
>  
> +    

Nit: some trailing whitespace here; remove this line.

@@ +7306,5 @@
>          obj = masm.extractObject(R0, ExtractTemp0);
> +        if (innerize_) {
> +            ValueOperand val = regs.takeAnyValue();
> +            Register tmp = regs.takeAny();
> +            masm.loadPtr(Address(obj, ProxyDataOffset + offsetof(ProxyDataLayout, values)), tmp);

At this point we don't know |obj| is a proxy. You can either (1) guard it's a proxy or (2) store the outer window proxy JSObject* in the stub and guard |obj| is the same object.

::: js/src/jit/BaselineIC.h
@@ +4675,5 @@
>  
>        public:
>          Compiler(JSContext *cx, ICStub::Kind kind, ICStub *firstMonitorStub,
> +                 HandleObject obj, HandleObject holder, HandleFunction getter, uint32_t pcOffset,
> +                 bool innerize = false)

Nit: why is innerize here optional but not below? Making it not optional everywhere may be clearer.

Comment 7

[Johannes Schulte [:jschulte]]

Attachment: v2.patch

Comment 8

[Jan de Mooij [:jandem]]

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

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

Sorry again for the delay. This looks great, mostly nits.

::: js/src/jit/BaselineIC.cpp
@@ +6595,5 @@
> +        MOZ_ASSERT(script->global().isNative());
> +        if (inner != &script->global())
> +            return true;
> +        // ICGetProp_CallNative*::Compiler::generateStubCode depends on this.
> +        MOZ_ASSERT(&((GetProxyDataLayout(obj)->values->privateSlot).toObject()) == GetInnerObject(obj));

Nit: GetInnerObject(obj) can be just |inner|.

@@ +6601,5 @@
>  
>      bool isCallProp = (JSOp(*pc) == JSOP_CALLPROP);
>  
>      ICStub *monitorStub = stub->fallbackMonitorStub()->firstMonitorStub();
>      if (!isDOMProxy && IsCacheableGetPropReadSlot(obj, holder, shape)) {

IsCacheableGetPropReadSlot calls IsCacheableProtoChain, which asserts obj->isNative(). How does that work in this case? Is shape null so that IsCacheableGetPropReadSlot returns early?

Maybe change this if to:

if (!isDOMProxy && obj->isNative() && ...)

Also, can we move the code that sets |inner| down to right before the code below that uses it?

@@ +6697,5 @@
> +
> +    if (inner) {
> +        obj = inner;
> +        if (!EffectlesslyLookupProperty(cx, obj, name, &holder, &shape, &isDOMProxy,
> +                                    &domProxyShadowsResult, &domProxyHasGeneration))

Nit: indentation here looks wrong. Maybe tabs?

@@ +6716,2 @@
>      // Try handling JSNative getters.
> +    if (cacheableCall && !isScripted && !!inner == innerize) {

Instead of !!inner == innerize, can you change the previous if to:

if (inner && (!callee || !callee->jitInfo() || callee->jitInfo()->needsOuterizedThisObject())
    return true;

::: js/src/jit/IonBuilder.cpp
@@ +10049,1 @@
>      // object to scripted getters etc. See bug 1007631.

Nit: this comment should be updated.

Comment 9

[Jan de Mooij [:jandem]]

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

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

::: js/src/jit/BaselineIC.cpp
@@ +7564,5 @@
> +            Register tmp = regs.takeAny();
> +            masm.branchTestObjectIsProxy(false, objReg, tmp, &failure);
> +            masm.loadPtr(Address(objReg, ProxyDataOffset + offsetof(ProxyDataLayout, values)), tmp);
> +            masm.loadValue(Address(tmp, offsetof(ProxyValueArray, privateSlot)), val);
> +            objReg = masm.extractObject(val, ExtractTemp0);

I just asked efaust about this on IRC. Other proxies can use the same slot for their own stuff, so this value is not guaranteed to be an object.

Can you pass the object's Class* (obj->getClass()) to the stub compiler, and use branchTestObjClass here? Then you can remove the branchTestObjectIsProxy check, because it's implied by the Class check.

Comment 10

[Johannes Schulte [:jschulte]]

Attachment: v3.patch

Comment 11

[Johannes Schulte [:jschulte]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> @@ +6601,5 @@
> >  
> >      bool isCallProp = (JSOp(*pc) == JSOP_CALLPROP);
> >  
> >      ICStub *monitorStub = stub->fallbackMonitorStub()->firstMonitorStub();
> >      if (!isDOMProxy && IsCacheableGetPropReadSlot(obj, holder, shape)) {
> 
> IsCacheableGetPropReadSlot calls IsCacheableProtoChain, which asserts
> obj->isNative(). How does that work in this case? Is shape null so that
> IsCacheableGetPropReadSlot returns early?
> 
> Maybe change this if to:
> 
> if (!isDOMProxy && obj->isNative() && ...)

Yeah, EffectlesslyLookupProperty sets the shape to null and then returns early on non-native non-DOM-proxy objects. Still added the checks for the sake of clarity.

Comment 12

[Jan de Mooij [:jandem]]

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

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

Awesome, thanks :) r=me with nits below addressed.

::: js/src/jit/BaselineIC.cpp
@@ +6611,5 @@
>          return true;
>      }
>  
>      bool isScripted = false;
> +    bool cacheableCall = obj->isNative() ? IsCacheableGetPropCall(cx, obj, holder, shape, &isScripted,

Nit: I'd use obj->isNative() && IsCacheableGetPropCall(...);

@@ +6678,5 @@
> +    if (!isDOMProxy && !obj->isNative()) {
> +        outerClass = obj->getClass();
> +#ifdef DEBUG
> +        JSObject *outer = obj;
> +#endif

Nit: instead of this #ifdef you can do: DebugOnly<JSObject*> outer = obj;

(see mfbt/DebugOnly.h)

@@ +6698,5 @@
> +    RootedFunction callee(cx);
> +    if (shape && shape->hasGetterValue())
> +        callee = &shape->getterObject()->as<JSFunction>();
> +    else
> +        return true;

Nit: I'd write this as:

if (!shape || !shape->hasGetterValue())
    return true;

RootedFunction callee(cx, &shape->getterObject()->as<JSFunction>());

::: js/src/jit/BaselineIC.h
@@ +4726,5 @@
>      static size_t offsetOfPCOffset() {
>          return offsetof(ICGetPropCallGetter, pcOffset_);
>      }
>  
> +

Nit: remove the extra newline here.

Comment 13

[Johannes Schulte [:jschulte]]

Attachment: v4.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9fa31d3e2a9

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97ccac9529a6

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97ccac9529a6