Web extension content scripts typically access the page over Xrays. Right now that's all very slow. Specific things we know to be slow: 1) Checking each Xray for the property. See also bug 1348099. 2) Handling of expando props; goes via the expando object. Ideally we'd end up with ICs covering both the "found on expando" case and the "found on some proto's holder" case... If it would help, I'm happy to sit down with someone and describe the object structures involved. One complication is that _getting_ the expando object is a huge pain in the behind. That might in fact make this whole approach not viable unless we change things so the expando object can be easily gotten from jitcode...

I looked at this a little a week or two ago and wrote a couple patches for xray ICs. This patch handles calling a getter on an xray when that getter is found on a DOM object wrapper in the target compartment (i.e. when content overwrites the getter property with something else then the IC won't hit). This doesn't handle expandos on the xray itself, as for some reason (I don't remember why) I didn't think that DOM wrapper xrays could have expando properties.

This patch adds an IC for indexed accesses on an xray for a nodelist object. Since content can't define new indexed properties on nodelists it seemed that we don't need to consider whether the xray or its target nodelist will fetch different indexed properties from each other. FWIW I wrote these patches based on the testcase in bug 1338942. Together these improve my time in a web extension from 13ms to 5ms; the same script running in a content compartment is 3ms.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #0) > One complication is that _getting_ the expando object is a huge pain in the > behind. That might in fact make this whole approach not viable unless we > change things so the expando object can be easily gotten from jitcode... So, I've been looking at how expandos work for xrays and, yeah, changing this would really make things smoother for integration into ICs. The main question I have: what is the relative importance of the sandboxed vs. non-sandboxed xray cases? When the xray's compartment is a sandbox then it does not share expando properties with xrays from any other compartments, and it is a lot easier to make things fast. We could just store the expando in the xray's compartment in a slot on the xray (there is a convenient unused GetProxyExtra slot for this), and add a strong reference from the target to the xray so that the xray is not collected until the C++ object is dead (to do this we would keep a strong reference from the target to the xray; I don't think this change would significantly increase the amount of stuff that is being kept alive nor cause problems for cycle collection, since JSSLOT_EXPANDO_EXCLUSIVE_GLOBAL is already a strong reference to the xray's compartment). This strategy wouldn't work in the non-sandboxed case, though, since multiple xrays can be associated with the same expando. I'm not sure what would be best in the non-sandboxed case, so I'm hoping it's not important for performance.

Apparently Chrome shares expandos between extension globals from the same extension, so we've talked about maybe using the expando sharing code for WebExtensions. It doesn't seem like a high priority though. Also, it's possible that in a future world all the extension globals will be in the same compartment. In that case, they would share the same Xray and so it wouldn't matter. And I don't think there's any reason for doing expando sharing in our own code. We probably shouldn't be using expandos anyway; WeakMaps give us more control. So once legacy extensions are gone, I think we won't need expando sharing any more.

On a call with bz and jorendorff, it sounds like we can also probably eliminate the expando object given some design changes we're making. More on that soon.

Are you still planning to work on this Brian? We need to figure out what to do in bug 1348099.

I stopped working on this after seeing comment 5. If the expandos are going to continue existing in their current form then I can get back to this and use the strategy outlined in comment 3; we would only be able to use ICs for xray accesses within sandboxed compartments (which don't share their expandos with any other compartments). Any IC infrastructure added here will be usable for the missing property case in bug 1348099.

(In reply to Brian Hackett (:bhackett) from comment #7) > expandos with any other compartments). Any IC infrastructure added here > will be usable for the missing property case in bug 1348099. I read the patches but don't understand how would it be usable for the missing property, could you shed me some light? So I can decide what else to address in bug 1348099. Thanks.

(In reply to Ting-Yu Chou [:ting] from comment #8) > (In reply to Brian Hackett (:bhackett) from comment #7) > > expandos with any other compartments). Any IC infrastructure added here > > will be usable for the missing property case in bug 1348099. > > I read the patches but don't understand how would it be usable for the > missing property, could you shed me some light? So I can decide what else to > address in bug 1348099. Thanks. The first patch here includes an IC that walks the proto chain of an xray. The shape checks done on the wrapped object are able to determine whether objects along the proto chain do or do not have the property being accessed. The example in comment 0 of that bug is I think the exact type of access which the first patch here caches (modulo correctness issues with expandos).

This patch modifies how expandos are stored per the outline in comment 3, and handles the testcase in bug 1338942. It doesn't handle the testcase in bug 1348099 (aka bug 613498), afaict because the global doing the proxy accesses in that bug has a "BackstagePass" global instead of a "Sandbox" global, which means that the global can share xray expandos with other globals and prevent us from being able to attach ICs. If I ignore this issue (so that ICs can be attached to code running against the BackstagePass global), then the patch seems to works well on the bug 1348099 testcase. I don't know how to measure precisely what happens when saving a session, but if I load the checkbox page and click on the last three checkboxes, the number of hits to XrayWrapper::getPropertyDescriptor goes from ~3 million to ~200 thousand. I don't know what the best way of fixing the issue above. The cleanest way would be to keep the expando pointer in the xray in sync for all xrays, not just those in sandboxes, but because there doesn't seem to be a way to find the incoming cross compartment edges for a compartment, doing this would require iterating over all compartments whenever creating an expando for a non-sandbox xray, which would have a performance cliff when creating lots of expandos from a non-sandbox extension when there are lots of compartments. An uglier way would be to keep some expando counter on each compartment, and an expected value on xrays into that compartment. The counter is updated whenever a non-sandbox expando is created in the compartment. If the counts are the same then the expando pointer is valid. An even simpler way would be to just change expando semantics for BackstagePass globals to resemble that of Sandbox globals, but I have no idea what the ramifications of such a change would be.

(In reply to Brian Hackett (:bhackett) from comment #10) > I don't know what the best way of fixing the issue above. > > The cleanest way would be to keep the expando pointer in the xray in sync > for all xrays, not just those in sandboxes, but because there doesn't seem > to be a way to find the incoming cross compartment edges for a compartment, > doing this would require iterating over all compartments whenever creating > an expando for a non-sandbox xray, which would have a performance cliff when > creating lots of expandos from a non-sandbox extension when there are lots > of compartments. > > An uglier way would be to keep some expando counter on each compartment, and > an expected value on xrays into that compartment. The counter is updated > whenever a non-sandbox expando is created in the compartment. If the counts > are the same then the expando pointer is valid. > > An even simpler way would be to just change expando semantics for > BackstagePass globals to resemble that of Sandbox globals, but I have no > idea what the ramifications of such a change would be. Would it be possible to have a flag on the global saying whether it wants to opt out of expando sharing? Then we could progressively set that flag on more and more of our JSMs, and get performance improvements that way. Generally I think it should be pretty easy to move away from expando sharing in our own code. I can't think of anything that uses expandos off the top of my head. Content scripts would also be a problem. Those use a TabChildGlobal, which is shared between all content scripts (including add-ons). We probably would have to wait until legacy add-ons are deprecated before we disable expando sharing for that global.

(In reply to Bill McCloskey (:billm) from comment #11) > Would it be possible to have a flag on the global saying whether it wants to > opt out of expando sharing? Then we could progressively set that flag on > more and more of our JSMs, and get performance improvements that way. > Generally I think it should be pretty easy to move away from expando sharing > in our own code. I can't think of anything that uses expandos off the top of > my head. Sure, this would be fine, though it should happen in a separate patch/bug I think. For now I'll just factor out the sandbox tests into a common function, but preserve the existing expando semantics.

Updated complete patch, I'll break this up for review.

Updated patch. I was going through and realized that we need to do something about cross-zone edges in these inline caches. I didn't think too much about this earlier because there is already an IC that includes cross-compartment shape etc. edges, but it is restricted to intra-zone accesses). This patch does the same thing and restricts the xray ICs to intra-zone accesses. This works for the test extension in bug 1338942, but does not work for the session store system in bug 1348099. Fixing this limitation will require either tracing these cross-zone edges, or ensuring/proving that the associated caches will not execute again.

This patch has the xray ICs and some plumbing so that the browser can describe information about xrays and nodelists to the JS engine.

This patch has structural changes to how xray expandos are stored so that they can be accessed efficiently from jitcode in globals that do not share their expandos with other globals. It also has the other side of the plumbing for describing xrays and nodelists to the JS engine (see the jsfriendapi.h changes in the previous patch).

Still sorting through this stuff, but why is NodeList special? At first glance, the only thing we're relying on is the "cannot be given new nonnegative integer expando properties" bit, and that should be true of anything with an indexed getter (and certainly anything with an indexed getter but no indexed setter).

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #17) > Still sorting through this stuff, but why is NodeList special? At first > glance, the only thing we're relying on is the "cannot be given new > nonnegative integer expando properties" bit, and that should be true of > anything with an indexed getter (and certainly anything with an indexed > getter but no indexed setter). There are a couple other things we're relying on: - Content can't give the object new nonnegative integer properties, or overwrite existing ones. - The object's proxy getProperty hook will tolerate being called from the wrong compartment (the IC code will unwrap the xray and call getProperty on the target while still being in the xray's compartment). I'm not sure how looking for indexed getter/setter would generalize this, because nodelists are proxies with opaque getProperty etc. hooks whose behavior can't really be reasoned about by the JS engine. If there are other array-like proxies which the above properties would hold for then we could expand it to include those, but I don't know which objects that would entail.

Hmm. So this is calling Proxy::get() while in the Xray compartment but "proxy" is the NodeList, not the Xray? That will land in NodeList's DOMProxyHandler::get and it's not at all clear to me that this tolerates being called with a compartment mismatch like that. Specifically, if the index is >= length, the get will fall through to the prototype, end up calling js::GetObjectProto and fail the same-compartment assert there, as far as I can tell. What we could probably do is change the codegen for this stuff to skip the "go to the proto" bits if the context compartment doesn't match our proxy; we already have asserts that the proxy itself is NOT an xray in DOMProxyHandler::get, and right now there are no cases in which it's called with a compartment mismatch, so this would be the only codepath affected... > If there are other array-like proxies Right, that was my point. There are. We can generalize this to them by having a virtual method on dom::DOMProxyHandler that indicates whether it has the behavior we want; codegen could then output implementations of this method. And IsNodeListProxy (somewhat renamed) could check the family, cast handler to mozilla::dom::DOMProxyHandler*, and call the virtual method). Peter, does the above sound sane?

Updated rolled up patch. This just removes the nodelist stuff. It doesn't seem valid to just ignore the prototype when calling from another compartment, since the xray's proto could presumably have indexed expando properties itself. Could DOMProxyHandler have a virtual method that is specialized for own-property indexed gets? Then we could call a stub with the xray that unwraps it, calls this virtual method, and if the result is undefined then it tries to get the property from the xray's proto. It is getting a little convoluted though and focusing on the nodelist stuff right now might not be the best idea; I wrote this optimization up after seeing it taking up time in the testcase in bug 1338942, but I don't know how representative that is of time sinks in addons or other xray-using globals in general.

This is the same as the earlier patch, minus the nodelist stuff.

This is the same as the earlier patch, minus the nodelist stuff.

This followup allows CacheIR to include guards on shapes, groups, and prototypes in other zones. Rather than pointing directly to GC things in other zones (which will complicate tracing/sweeping tremendously), these guards include pointers to CCWs of objects that refer to the GC thing to guard. The generated code unwraps these CCWs (the guard fails if the CCW has been nuked) to do the test, which is a little slower but should be fine.

Comment on attachment 8869601 [details] [diff] [review] JS changes >+GetPropIRGenerator::tryAttachXrayCrossCompartmentWrapper(HandleObject obj, ObjOperandId objId, I'm not sure why we're doing any testing of anything on "target" (e.g. its shape, its proto chain, etc). For example, why do we care whether we managed to look up "id" on "target"? It shouldn't matter for Xrays...

Comment on attachment 8869602 [details] [diff] [review] browser changes >+++ b/js/xpconnect/src/XPCJSRuntime.cpp >+ SetXrayJitInfo(&gXrayJitInfo); Isn't that in the "js" namespace? So js::SetXrayJitInfo? >+++ b/js/xpconnect/wrappers/XrayWrapper.cpp >+XrayTraits::getExpandoObjectInternal(JSContext* cx, HandleObject target, HandleObject exclusiveWrapper, Pretty sure this is over 80 chars. >- // Not found. Please leave the comment. >+// not be used while looking up the expando for the, wrapper), and keeping a s/the,/the/ >+XrayTraits::attachExpandoObject(JSContext* cx, HandleObject target, HandleObject exclusiveWrapper, Again, over 80 chars. >@@ -1250,38 +1305,49 @@ XrayTraits::cloneExpandoChain(JSContext* >@@ -1250,38 +1305,49 @@ XrayTraits::cloneExpandoChain(JSContext* >+ // The global containing this wrapper holder has an xray for |src| >+ // with expandos. Create an xray in the global for |dst| which >+ // will be associated with a clone of |src|'s expando object. >+ JSAutoCompartment ac(cx, UncheckedUnwrap(wrapperHolder)); >+ exclusiveWrapper = dst; >+ if (!JS_WrapObject(cx, &exclusiveWrapper)) >+ return false; >+ MOZ_ASSERT(IsXrayWrapper(exclusiveWrapper)); I'm not sure this assertion necessarily holds. In particular, we could be adopting from a content document into a chrome one, if chrome or extensions really goes out of its way to do that, I think... I'm not quite sure what the old code ended up doing in that case. It created an expando chain, but I'm not sure whether it then ended up doing anything useful with it. >+++ b/js/xpconnect/wrappers/XrayWrapper.h >+ bool getExpandoObjectInternal(JSContext* cx, JS::HandleObject target, JS::HandleObject exclusiveWrapper, >+ nsIPrincipal* origin, > JS::MutableHandleObject expandoObject); This no longer takes all its args in the same compartment. It should probably document what compartments things are in. >+ JSObject* attachExpandoObject(JSContext* cx, JS::HandleObject target, JS::HandleObject exclusiveWrapper, >+ nsIPrincipal* origin); Same here. r=me. Once we do jorendoff's compartment-merging thing we can simplify a bunch of this stuff, but for now this is the best we can do...

Comment on attachment 8869601 [details] [diff] [review] JS changes Review of attachment 8869601 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. This looks great, my main concern is that we should have tests that fail if we, say, comment out a random guard in CacheIR.cpp. Maybe we have xray tests covering some of this but those tests are usually not written with JITs/ICs in mind. ::: js/src/jit/BaselineCacheIRCompiler.cpp @@ +402,5 @@ > + Address holderAddress(scratch, sizeof(Value) * GetXrayJitInfo()->xrayHolderSlot); > + Address expandoAddress(scratch, NativeObject::getFixedSlotOffset(GetXrayJitInfo()->holderExpandoSlot)); > + > + if (hasExpando) { > + AutoScratchRegister scratch2(allocator, masm); This and |scratch| need to be moved before addFailurePath because they may change the register state the failure path expects (we should add assertions to catch this...). For this one you could use Maybe<AutoScratchRegister> and emplace() it if hasExpando is true. ::: js/src/jit/CacheIR.cpp @@ +787,5 @@ > + RootedShape shape(cx_); > + NativeGetPropCacheability type = CanAttachNativeGetProp(cx_, target, id, &targetHolder, &shape, > + pc_, canAttachGetter_, > + isTemporarilyUnoptimizable_); > + if (type != CanAttachCallGetter || !IsCacheableGetPropCallNative(target, targetHolder, shape)) Do we want a follow-up for plain data properties? I'm not sure which cases we care about here. @@ +811,5 @@ > + return false; > + } > + if (!holder || !IsProxy(holder) || !info->isCrossCompartmentXray(GetProxyHandler(holder))) > + return false; > + if (!prototypes.append(holder)) cx->recoverFromOutOfMemory() here since AutoObjectVector uses TempAllocPolicy. @@ +822,5 @@ > + if (!getter || !getter->is<JSFunction>()) > + return false; > + > + JSFunction& getterFun = getter->as<JSFunction>(); > + if (!getterFun.isNative() || getterFun.native() != shape->getterObject()->as<JSFunction>().native()) Could we check: if (getterFun != shape->getterObject()) here? ::: js/src/jit/IonCacheIRCompiler.cpp @@ +730,5 @@ > + FailurePath* failure; > + if (!addFailurePath(&failure)) > + return false; > + > + AutoScratchRegister scratch(allocator, masm); Move this one before addFailurePath too.

Comment on attachment 8869601 [details] [diff] [review] JS changes Review of attachment 8869601 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CacheIR.cpp @@ +833,5 @@ > + // Load the object wrapped by the CCW > + ObjOperandId wrapperTargetId = writer.loadWrapperTarget(objId); > + > + // Test the shape of the wrapped object against the lookup we performed. > + // Note that if the compartment does not match then these guards will fail. I forgot: wich guards in particular? Note that shapes are now shared across compartments within a zone.

Comment on attachment 8869603 [details] [diff] [review] cross-zone guards Review of attachment 8869603 [details] [diff] [review]: ----------------------------------------------------------------- Allocating 2 objects (one of them a wrapper even) each and every time we emit a cross-zone shape/group guard is pretty heavy weight and is the kind of thing that's going to show up in profiles. I really think we should consider/discuss other options here - at the very least we should have a map to reuse the wrapper when we guard on the same Shape/Group twice. ::: js/src/jit/BaselineCacheIRCompiler.cpp @@ +324,5 @@ > + masm.loadPtr(Address(scratch, ProxyObject::offsetOfReservedSlots()), scratch); > + > + // If the wrapper has been nuked then this address is no longer an object, > + // but we will load a null pointer into scratch and the branch below will > + // fail. Do we have tests for any of this?

> Do we want a follow-up for plain data properties? Probably yes. Specifically, it would be nice to make method calls cacheable. > if (getterFun != shape->getterObject()) No, because getterFun and shape->getterObject() are in different compartments: getterFun is in the Xray's compartment, and shape is in target's compartment. That said, it's still not clear to me why we're checking anything about "shape" at all, per above.

Jan, Brian, is this still waiting on Jan's review?

Sorry I forgot about the patch Brian emailed me :/ Keeping the needinfo, I'll take a look when I'm back (tomorrow).

Brian emailed this patch.

Comment on attachment 8880387 [details] [diff] [review] Complete patch bz should probably look at the test and the updated patch.

Comment on attachment 8880387 [details] [diff] [review] Complete patch I mostly didn't review the IC bits carefully in terms of checking all the slot offsets and whatnot. Jan, if you didn't either, we should figure out which of us will review all that. I also didn't double-check the "expando shape wrapper" setup. >+++ b/js/src/jit/CacheIR.cpp >+ // Test the wrapped object's shape. The properties held by xrays or their s/shape/class/ >+++ b/js/src/jsfriendapi.h >+ // Test whether xrays within a global object's compartment do not share The negative there is confusing. How about: // Test whether xrays with a global object's compartment have expandos of // their own, instead of sharing them with Xrays from other compartments. >+++ b/js/xpconnect/wrappers/XrayWrapper.cpp > expandoObject.set(head); >- return true; >+ break; I think it's a bit weird to break to a "// Not found." return here, since we fund something. Please leave the "return true" here as it was. >+++ b/js/xpconnect/wrappers/XrayWrapper.h >+ // |exclusiveWrapper| is optional and is any xray that has exclusive use of >+ // the expando. |target| is the object being wrapped. This must be called >+ // from the xray's compartment. > bool getExpandoObjectInternal(JSContext* cx, JS::HandleObject target, I'm not sure I follow this comment. exclusiveWrapper is optional only in the sense that it should be null when the compartment of cx does NOT want exclusive expandos, right? If it does, exclusiveWrapper must be provided? This still needs docs about compartments. "cx" is in the xray compartment. Presumably exclusiveWrapper is in the same compartment. "target" is in the target compartment, right? What compartment is expandoObject expected to be returned in? >+ // As above, but this must be called from |target|'s compartment. OK. So now cx and target are same-compartment, but exclusiveWrapper is in the xray compartment, right? We should document this clearly. r=me with those fixed, assuming Jan reviewed all the slot bits.

Just out of curiosity: Is this still being worked on?

Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/072f8d4a9964 Add IC for property reads on xrays, r=jandem,bz.

Too late for 55. Mark 55 won't fix.

Backed out from 56 for causing crashes with various extensions. https://hg.mozilla.org/releases/mozilla-release/rev/58e45fc51d63

Backed out from 57/58 as well. https://hg.mozilla.org/mozilla-central/rev/39aaf54972cb https://hg.mozilla.org/releases/mozilla-beta/rev/520e300d1266

I think we should be open to the idea of maybe uplifting this to 57 if we can fix the problem bug 1386490 ran into. I'll try to do some poking around and see whether I can figure out what's going on there.

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #41) > I think we should be open to the idea of maybe uplifting this to 57 if we > can fix the problem bug 1386490 ran into. I'll second this. X-ray overhead outside of sandboxes tends to completely dominate any profiles it shows up in. I've fixed a half dozen serious performance bugs just by making sure we avoid going through X-rays when we clone things. Without this patch, the amount of jank complex extension content scripts cause is huge. Given the focus on performance for the 57 release (and the Quantum branding...), I think shipping it without these improvements is a huge risk.

This was re-landed on 56 to avoid adding late risk to the release. https://hg.mozilla.org/releases/mozilla-release/rev/8fbf05f4b921

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e230e4de41e Add IC for property reads on xrays, r=jandem,bz.

Relanded this on inbound, because we believe the issues with it are all resolved.

Comment on attachment 8880387 [details] [diff] [review] Complete patch Approval Request Comment [Feature/Bug causing the regression]: None. [User impact if declined]: Significantly degraded WebExtension performance. [Is this code covered by automated tests?]: Sort of. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Maybe. It might be worth trying the various extensions mentioned as ending up with crashes and verifying that we don't crash anymore. [List of other uplifts needed for the feature/fix]: Bug 1396466 and bug 1404107 [Is the change risky?]: Somewhat [Why is the change risky/not risky?]: Well, it clearly caused problems the last time it landed. We are fairly certain we have found and squashed all of those, but "fairly" isn't "completely". Note that this patch is shipping in Firefox 56, so from a risk side what we're proposing is no worse than what we're shipping... [String changes made/needed]: None

Comment on attachment 8880387 [details] [diff] [review] Complete patch Must fix, glad we flagged this in time to uplift and bake nicely on 57(!!), Beta57+

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #47) ... > [Needs manual test from QE? If yes, steps to reproduce]: Maybe. It might be > worth trying the various extensions mentioned as ending up with crashes > and > verifying that we don't crash anymore. Hi Boris, could you please name a few extensions in order to verify this bug?

Greasemonkey, Tampermonkey, LastPass. In general, see https://crash-stats.mozilla.com/signature/?signature=js%3A%3AWrapperMap%3A%3Alookup#correlations and bug 1386490.

Thanks! Unfortunately, I couldn't reproduce the crash on my machine by following the STR from bug 1386490 comment 4 and bug 1386490 comment 5, using an affected Nightly build from 2017-07-30 and 56.0.2 (20171024165158). I've tried to reproduce it on Windows 10 x64, since most of the crashes were reported on this OS.