Boris Zbarsky [:bzbarsky] Reporter
	Description • 11 years ago

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?

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 2 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 10 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 10 years ago

(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?

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 10 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 6 • 10 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 7 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 8 • 10 years ago

> 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.

	Jan de Mooij [:jandem] Assignee
	Comment 9 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 10 • 10 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 11 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 12 • 10 years ago

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.

	Eric Faust [:efaust]
	Comment 13 • 10 years ago

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"?

	Eric Faust [:efaust]
	Comment 14 • 10 years ago

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!

	Eric Faust [:efaust]
	Comment 15 • 10 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 16 • 10 years ago

(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).

	Jan de Mooij [:jandem] Assignee
	Comment 17 • 10 years ago

(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.

	Jan de Mooij [:jandem] Assignee
	Comment 18 • 10 years ago

bz is this what you had in mind?

	Jan de Mooij [:jandem] Assignee
	Comment 19 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 20 • 10 years ago

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

Yes, that looks great.

	Jan de Mooij [:jandem] Assignee
	Comment 21 • 10 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 22 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 10 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 25 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 10 years ago

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