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?

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

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.

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.

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

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.

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.

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

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

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.

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.

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

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

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

bz is this what you had in mind?

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 on attachment 8419366 [details] [diff] [review] Part 5 - Add the comments requested in comment 12 Yes, that looks great.

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

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.

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

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

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.