964915 - Make window.foo fast by doing the get on the inner instead

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It would be nice to eliminate this footgun.  Luke suggested a plan yesterday.

The basic idea is that if we're doing a getprop (so we know the id is not an array index, right?) and the object the get is happening on is an outer window proxy then we are in fact guaranteed that the get will return the same thing as a get on the inner.

So what we could do is check during JIT compilation that we have an outer window and the inner is our global and if so just guard on the object coming through being the same JSObject* or something and do the get on the global.  We'd need to discard jitcode when the current inner changes, presumably.

I sort of poked at this, but the various getPropTry* things all independently reget the object the get is happening on, looks like, which is a bit annoying...

Thoughts?

Comment 1

[Jan de Mooij [:jandem]]

Attachment: WIP

This works, but I want to write some tests to make sure the brain transplant case is handled correctly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So first of all, this is pretty awesome.  I'm seeing very nice improvements here for value prop gets of the form window.whatever and same for DOM getters.

One thing to be really careful with is other getters defined on the window.  In particular, something like:

  Object.defineProperty(window, "foo",
    { get: Object.prototype.valueOf }
  );

or 

  Object.defineProperty(window, "foo",
    { get: function() { return this; } }
  );

Right now these are safe because:

1)  We don't output a fast path for these in Ion because baseline doesn't have an IC for these sort of gets on proxies (bug 944014 is kinda related), so in IonBuilder::getPropTryCommonGetter when we call inspector->commonGetPropFunction() we get null and hence don't do things like inline the scripted caller.

2)  We then generate an Ion IC, but for the latter case there is no IC stub for calling scripted getters and for the former case we explicitly check in GetPropertyIC::tryAttachNative (via IsCacheableGetPropCallNative) whether our native is OK with a non-outerized this, and if it's not we don't attach an IC stub.

But that all seems pretty fragile.  :(  We should at least add comments in the IC code about attaching calls to scripted getters needing to watch out for this.  And it would be good to explicitly test for this situation in IonBuilder::getPropTryCommonGetter if we can.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and also, we may not want to innerize in the case when doing that would just cause us to not output fast paths.  Should only be relevant for getters/setters without the relevant jitinfo.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Oh, and also, we may not want to innerize in the case when doing that would
> just cause us to not output fast paths.  Should only be relevant for
> getters/setters without the relevant jitinfo.

I don't understand the last sentence. You mean if there's a getter/setter without jitinfo, we could fail to use a fast path if we innerize?

We could change the patch to do this:

(1) jsop_getprop behaves just like it does without the patch, until it gets to getPropTryCache.

(2) Right before we call getPropTryCache, we add a new getPropTryInnerize call. This one tries to innerize, and if it manages to do that it calls the following methods with the inner window as object:

- getPropTryConstant
- getPropTryDefiniteSlot
- getPropTryCommonGetter, with a new "innerized = true" argument. If innerized == true, getPropTryCommonGetter only does the MGetDOMMember/MGetDOMProperty optimization and nothing else.

If all this fails, we return to jsop_getprop and do the getPropTryCache on the outer window as usual.

This way we (a) never pass the innerized object to the IC or VM call and (b) only pass it to DOM getters, not other getters. For DOM getters this is always safe right?

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove cx from innerObject hook

This patch removes the "cx" argument from GetInnerObject and the innerObject hook. The only innerObject hook we have is proxy_innerObject and it doesn't use the cx. This makes it easier to call this hook during Ion compilation.

I also removed JSObjectOp and used js::ObjectOp instead, and added a new js::InnerObjectOp.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Pass obj as argument to getProp* methods

This patch pops obj in jsop_getprop, then passes it explicitly to all getProp* methods. This is consistent with jsop_setprop and jsop_getelem and makes it easier to do what I described in comment 4.

The patch also inlines jsop_arguments_length into its only caller, getPropTryArgumentsLength.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> This way we (a) never pass the innerized object to the IC or VM call and (b)
> only pass it to DOM getters, not other getters. For DOM getters this is
> always safe right?

I just found JSJitInfo::needsOuterizedThisObject, it returns false for getters and setters, so we can MOZ_ASSERT(!jitInfo->needsOuterizedThisObject()), good.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> You mean if there's a getter/setter without jitinfo, we could fail to use a fast path if
> we innerize?

Yes.  If we have a JSNative getter/setter without jitinfo, and the get is happening as window.foo as opposed to bareword foo, then I'm pretty sure that without this patch we will emit an IC.  With this patch we won't.

Now in practice, this situation never arises, I expect.  So far, at least.

> For DOM getters this is always safe right?

It's safe for JSNatives with jitinfo that don't have needsOuterizedThisObject().

I'm pretty sure all things that are JSJitInfo::Getter return false from that method, but explicitly checking can't hurt.

Comment 9

[Jan de Mooij [:jandem]]

What I described in comment 4 doesn't work; in most cases we don't have type information for the global properties (because Baseline doesn't have this optimization and didn't instantiate type info for them), and getPropTryCommonGetter doesn't work because Baseline does not add a getter stub in this case.

The new plan is for getPropTryInnerize to just call getPropTryCache with the inner object. Longer-term we can add this optimization to Baseline too and then we can use our full GETPROP optimizations, but for now this should be fine.

Then we also don't have to worry about getPropTryCommonGetter, the IC just needs a comment to warn about scripted getters as bz pointed out.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, that explains why I get a GetDOMProperty for a bareword "performance" but GetPropertyCacheV for "window.performance" with this patch!

The plan from comment 9, with a followup bug for baseline and a comment in the code pointing to that bug sounds great.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Optimize GETPROP

This optimizes the GETPROP case; will do something similar for SETPROP when this is in.

feedback? from bhackett for a sanity check of the TI parts, efaust and bz for the rest.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8418854 [details] [diff] [review]
Part 3 - Optimize GETPROP

This looks great.  r=me

One other set of comments I'd like us to add: right now we have no baseline stub for the case of a getname/gname with a scripted getter.  See the !scripted check in TryAttachGlobalNameStub in BaselineIC.cpp and the fact that TryAttachScopeNameStub only tries to IsCacheableGetPropReadSlot and doesn't handle getters at all.

What that means is that in ion we don't generate any code via getPropTryCommonGetter for bareword access on the window, falling through to the ion IC instead.  We should comment in the baseline code that if stubs for this case are ever added at either of those two places the ion code in getPropTryCommonGetter will need to be checked carefully for its handling of non-outerized this objects.

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8418680 [details] [diff] [review]
Part 1 - Remove cx from innerObject hook

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

Looks solid. r=me

::: js/src/shell/js.cpp
@@ +3509,1 @@
>              args.rval().setObjectOrNull(op(cx, parent));

Unrelated, but while we're here is it worth writing an analagous GetOuterObject(cx, obj) so that callsistes like this don't really have to "embrace the ugliness"?

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8418691 [details] [diff] [review]
Part 2 - Pass obj as argument to getProp* methods

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

This is cool as we move towards having increasing numbers of the "getprop-like" ops using the getprop optimization paths.

r=me

::: js/src/jit/IonBuilder.cpp
@@ +6586,5 @@
>          return true;
>  
>      types::TemporaryTypeSet *types = bytecodeTypes(pc);
> +    MDefinition *globalObj = constant(ObjectValue(*obj));
> +    if (!getPropTryCommonGetter(&succeeded, globalObj, name, types))

Boy, am I glad to see that cleaned up. This is so much better!

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 8418854 [details] [diff] [review]
Part 3 - Optimize GETPROP

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

This looks great. r=me

::: js/src/jit/IonBuilder.cpp
@@ +9129,5 @@
> +    // Note: the Baseline ICs don't know about this optimization, so it's
> +    // possible the global property's HeapTypeSet has not been initialized
> +    // yet. In this case we'll fall back to getPropTryCache for now.
> +    //
> +    // Also note that we don't call getPropTryCommonGetter below, because

OK, so in theory we could pass to appropriate native getters, right? Ones without needsOuterizedWindow(), etc, etc. So if we get the BC ICs to know about OuterWinodwProxies more directly (by doing the DOM stuff in general), we should readdress this point and get getPropTryCommonGetter in on the game.

If so, can you file a followup to come back to this with bug 944014 as a blocker and CC the usual suspects?

::: js/src/jit/IonCaches.cpp
@@ +1164,5 @@
> +    //
> +    // Be careful when adding support for other getters here: for outer window
> +    // proxies, IonBuilder can innerize and pass us the inner window (the global),
> +    // see IonBuilder::getPropTryInnerize. This is fine for native getters because
> +    // IsCacheableGetPropCallNative checks they can handle both the inner and

Thanks for the footgun avoidance.

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Use GetOuterObject in Parent

(In reply to Eric Faust [:efaust] from comment #13)
> Unrelated, but while we're here is it worth writing an analagous
> GetOuterObject(cx, obj) so that callsistes like this don't really have to
> "embrace the ugliness"?

GetOuterObject already exists; this patch calls it in Parent. I also looked for other places that could use it but this seems to be the only one. Also added a null-check, because the hook is fallible (at least in theory).

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #15)
> If so, can you file a followup to come back to this with bug 944014 as a
> blocker and CC the usual suspects?

Filed bug 1007631; will update the comment to point to that bug as well.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Add the comments requested in comment 12

bz is this what you had in mind?

Comment 19

[Jan de Mooij [:jandem]]

Parts 1 and 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52120db36f78
https://hg.mozilla.org/integration/mozilla-inbound/rev/66eb8cc0d73a

Try: https://tbpl.mozilla.org/?tree=Try&rev=0bbc2ea214bc

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8419366 [details] [diff] [review]
Part 5 - Add the comments requested in comment 12

Yes, that looks great.

Comment 21

[Jan de Mooij [:jandem]]

And revert a small part of part 2, because regressing Octane-raytrace by 60% was not the plan:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f222fa5f7e2

Comment 22

[Jan de Mooij [:jandem]]

And parts 3 and 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2232f06e4ef0
https://hg.mozilla.org/integration/mozilla-inbound/rev/dea73fdb063c

Link to Try push in comment 19.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52120db36f78
https://hg.mozilla.org/mozilla-central/rev/66eb8cc0d73a
https://hg.mozilla.org/mozilla-central/rev/8f222fa5f7e2

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/2232f06e4ef0
https://hg.mozilla.org/mozilla-central/rev/dea73fdb063c

Comment 25

[Jan de Mooij [:jandem]]

And part 4 (part 5 landed with part 3):

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac29f3acef71

Filed bug 1009447 for the SETPROP part.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac29f3acef71