1214961 - Sweep XPConnect incrementally

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Myself and several others have recently been seeing long GC pauses caused by the CC getting impatient and finishing the GC. It took a couple weeks of browsing with 500+ tabs open, but I finally managed to catch this with the GC's logging output turned on.

The underlying cause here is that the GC is extending to 100's of slices and taking way longer than is justified, even with a massive heap. The cause for /this/ is that the fixed overhead in sweeping is taking up the entire budget and we are effectively only sweeping a single zone group in each slice. I've seen this before, but at the time we did not have fine enough grained diagnostics to pin it to a specific part of beginSweepingZoneGroup. Now, however, I've definitely trailed it down to something under the updateWeakPointers callbacks.

The main work done by this callback (at least in my current browsing session -- mostly treeherder and bugzilla with a few google properties) is sweeping of the Object2WrappedJSObjectMap. Mine is currently clocking in at 9MiB; only 50K or so entries, but apparently the per-entry overhead is enough to make this take just over 10ms, e.g. our slice budget. My guess would be that some heuristic in the browser, or possibly on the web, has made us more likely to be in an animation (reducing our budget from 40ms to 10ms) and thus making this problem more apparent.

The Object2WrappedJSObjectMap is responsible for holding the C++ reflectors of JS implemented XPCOM objects. Now, a C++ COM object can be QueryInterface()'d into any number of different specific interfaces, each of which needs to get its own C++ reflector. The underlying JSObject, however, needs to be the same for all of them. Thus, the JSObject* keys in this table refer to a list of nsXPCWrappedJS objects, one item each for each C++ nsIFoo that the JSObject has been QI'd into.

*Note*: The JS implementations can be QI'd to different interfaces in different compartments, resulting in the wrapper chain containing direct cross-compartment links. I'm honestly not sure why this is allowed, but that's the way it is right now. 

Because of these two things: we currently must visit the full table and all links at every slice of sweeping to ensure that things that are about to be finalized in a compartment are not reachable from a different compartment.

To figure out how intractable this is in practice I took some measurements. Of my 50K entries, ~90% have length 1, and thus reside in a single compartment. Of the remaining 10%, ~100% are length 2, 11 were length 3, and none were length 4. 50% of the 5K 2-length lists were single compartment and the remaining 2.5K contained cross-compartment edges.

This suggests a conceptually straightforward implementation strategy: split up single compartment lists into separate tables and only visit the table once, immediately before we finalize the compartment. Wrappers with cross-compartment links go in a separate table which we continue to visit, in full, at the start of each sweeping slice.

I've got a patch that implements this that, after three days of carving away incorrect assumptions, is able to boot and shutdown the browser without crashing. Do not expect this try run to be anything like green:
http//treeherder.mozilla.org/#/jobs?repo=try&revision=bcc1d4daeb66

Comment 1

[Terrence Cole [:terrence]]

Attachment: object_2_wrapped_map_by_zone-v0.diff

Try is what counts for green these days:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2cb0f637f72

Bobby, does the re-architecting here make sense to you? Is there a better way I could be doing this?

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8677155 [details] [diff] [review]
object_2_wrapped_map_by_zone-v0.diff

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

Andrew is the time-honored reviewer for XPConnect GC patches. :-)

Comment 3

[Terrence Cole [:terrence]]

I spent a few hours yesterday and today putting the probes to this in my large session and I'm declaring complete success, despite a small misunderstanding about where the overhead is. I would like to change one name in the GC changes though, so will upload a new patch today.

The problem with measuring performance here is twofold: on one hand, system activity can influence our min-spanning-zone-tree, which can result in a wildly varying overhead per zone-group and secondly, we can sweep multiple zone-groups in a single slice. In general, I was seeing all-zones GC's where the finalizer callbacks were fast (~14-15ms) and others where the finalizer callbacks were all long, and were spread out over many slices. I had assumed, I guess implicitly, that the long pauses were corresponding to large zone-groups and resumption of sweeping in a large zone group was long, but that's not the case. In general we're getting through at least one zone group per slice, by definition.

The way it breaks down is: if all zone-groups are being collected in one big go, we only have to call the weak callback once. On the other hand, if each zone is in its own group, we have to call the weak callback once per zone. Since calling it once is a third to half our slice time, regardless of how many zones we're collecting or the time it would take to collect any single zone, we get to sweep two or three zone groups per slice, max. Moreover, most zones have very low sweep overhead and it's only a handful that take more than a ms or two to sweep. Thus, sweeping all zones (minus the weak callback) is not substantially longer than sweeping our largest zone (minus the weak callback).

The GC's that were finishing fast, although maybe blowing out the (single) sweep slice budget a little were collecting all zones in a single group. The GC's that were rolling on forever were sweeping two or three zones per slice, resulting in #slices = #zones / (1 or 2.something). For my 1000's of tabs, at 16 slices per second, this was stretching out well past our "just finish it" threshold, depending on the specifics of what zones landed in what groups.

With this patch applied, all of the above still holds, except that the per-zone-group overhead is 1/10th what is was before -- somewhere between 250us and 350us on my fresh, large session, where it used to be 2-3ms. This brings us to a new bi-stable equilibrium. All-zones-in-a-single-zone-group GCs now take exactly the same amount of time as before, but with the time moved from the per-zone-group callback to the per-compartment callback. On the other hand, zone-per-group GC's should now take ~1/10th as many slices to complete, because we fit 10x more per-zone-group overheads into a slice.

I did not witness any particularly bad behavior, but that might just be because the session was fresh. Hard to tell. In any case, this is working exactly as expected, even though the result was surprising (to me at least).

Comment 4

[Terrence Cole [:terrence]]

Attachment: object_2_wrapped_map_by_zone-v1.diff

I'm just going to upgrade this to review, given the material success.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Andrew is the time-honored reviewer for XPConnect GC patches. :-)

I don't really know much about how the GC tracing works but I'm taking a look.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8677640 [details] [diff] [review]
object_2_wrapped_map_by_zone-v1.diff

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

Nice work figuring out this whole mess and coming up with a nice solution! I wonder if it would be worth having the GC somehow busting its budget if it notices that it is spending most of its time without making any forward progress. Having longer slices would be less bad than the catastrophic failures you were experiencing.

The terminology here is off to me. A wrapped JS itself cannot cross compartments, as it really does not exist in any particular compartment, as a C++ object, and there is no particular ordering relation between the different compartments as there is for CCWs. Plus, using different terminology to make things more distinct is probably a good idea. Maybe these could be "multi-compartment wrapped JS" and "single-compartment wrapped JS"? Does that make sense to you? Maybe I'm misunderstanding something. You probably know about as much as me about wrapped JS at this point. (This change in terminology will require a pass to rename comments and functions.)

Also, I think that the "wrapper chains" terminology you use doesn't quite match up with XPConnect. "wrapped JS" can just refer to the entire chain, even though really the chain is made up of elements of nsXPCWrappedJS.

Your commit message is a bit mangled. Also please add a paragraph or two of a high level overview of your changes.

r- because I want to see the revised version, mostly for the bits about memory reporting, but otherwise this looks good to me!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +941,2 @@
>  {
> +    // Called before each sweeping slice, after finishing marking, to clear out

"after finishing marking" to "after marking is finished" maybe? Marking is done in each sweeping slice?

@@ +941,4 @@
>  {
> +    // Called before each sweeping slice, after finishing marking, to clear out
> +    // any references to things that are about to be finalized and update any
> +    // moved pointers.

"update any moved pointers" Isn't this really more like "pointers to any moved objects?" I'm not sure what terminology you usually use in the GC.

@@ +952,5 @@
>  
> +/* static */ void
> +XPCJSRuntime::WeakPointerCompartmentCallback(JSRuntime* rt, JSCompartment* comp, void* data)
> +{
> +    // Called before each sweeping slice, after finishing marking, once for

Having this big comment that is almost identical to the one in the previous function doesn't feel great. The reader has to dig around to figure out what is different. Maybe something shorter like "Like WeakPointerZoneGroupCallback, but called with each compartment that is about to be finalized"?

@@ +2820,5 @@
>              // Note: cannot use amIAddonManager implementation at this point,
>              // as it is a JS service and the JS heap is currently not idle.
>              // Otherwise, we could have computed the add-on id at this point.
> +            extras->xpcPrivate = mallocSizeOf_(cp);
> +            extras->xpcWrappedJS = cp->SizeOfWrappedJS(mallocSizeOf_);

It seems a little odd to me that these are split up. Can you just combine them into a single category (including mentioning single-compartment wrapped JS in the description) and then write a SizeOfIncludingThis for CompartmentPrivate?

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +336,5 @@
>  
> +    xpc::CompartmentPrivate* rootComp = xpc::CompartmentPrivate::Get(rootJSObj);
> +    MOZ_ASSERT(rootComp);
> +
> +    // Find any existing wrapper.

Could you wrap all of this up into a method, maybe on CompartmentPrivate? GetNewOrUsed() is pretty giant already. Maybe called GetRootWrappedJS() or something.

@@ +338,5 @@
> +    MOZ_ASSERT(rootComp);
> +
> +    // Find any existing wrapper.
> +    JSObject2WrappedJSMap* rootMap = rootComp->GetWrappedJSMap();
> +    RefPtr<nsXPCWrappedJS> root = rootMap->Find(rootJSObj);

Would it be worth asserting that if something is in the compartment map that it isn't also in the global map?

@@ +342,5 @@
> +    RefPtr<nsXPCWrappedJS> root = rootMap->Find(rootJSObj);
> +    if (!root) {
> +        XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
> +        JSObject2WrappedJSMap* globalMap = rt->GetGlobalWrappedJSMap();
> +        MOZ_ASSERT(globalMap, "multi-compartment wrappedjs map missing");

Is there some reason to assert this for the global map but not the compartment map? It seems inconsistent.

@@ -348,5 @@
>              wrapper.forget(wrapperResult);
>              return NS_OK;
>          }
>      } else if (rootJSObj != jsObj) {
> -

Please leave this blank line.

@@ +395,5 @@
>      // that are subject to finalization. See the top of the file for more
>      // details.
>      NS_ADDREF_THIS();
>  
> +    XPCJSRuntime* xpcRt = nsXPConnect::GetRuntimeInstance();

Call this |rt| please. In the context of XPConnect, it is clear that refers to XPCJSRuntime.

@@ +400,2 @@
>      if (IsRootWrapper()) {
> +        MOZ_ASSERT(!CrossesCompartments());

I guess we're guaranteed this because mNext is null.

@@ +407,4 @@
>          NS_ADDREF(mRoot);
>          mNext = mRoot->mNext;
>          mRoot->mNext = this;
> +        if (mRoot->CrossesCompartments()) {

Maybe this is too cute, but you only have to check that the compartment of |mRoot| is different than the compartment of |this|, not the whole chain. The only case where you have to do something is if the WJS is currently single-compartment but is becoming multi-compartment. (If |this|'s compartment is the same as |mRoot| but different than some later thing in the chain, then |mRoot| must already be in the global table.) If you do that, please add a comment or an assert or both.

@@ +428,5 @@
> +
> +    // Wrapper chains that cross compartments are guaranteed to be in the
> +    // global table; wrapper chains that do not cross compartment bounds may
> +    // reside in either table, depending on whether they have ever had a cross-
> +    // compartment wrapper chain. Since removal requires a lookup anyway, we

Hopefully we won't have to deal with global wrapper table fragmentation at some point. :)

@@ +431,5 @@
> +    // reside in either table, depending on whether they have ever had a cross-
> +    // compartment wrapper chain. Since removal requires a lookup anyway, we
> +    // just do the remove on both tables in the latter case.
> +    GetGlobalWrappedJSMap()->Remove(wrapper);
> +    if (wrapper->CrossesCompartments()) {

If 90% are really single compartment, maybe just unconditionally remove it from the compartment table?

@@ +442,5 @@
> +}
> +
> +#ifdef DEBUG
> +static void
> +WrapperAssertionCallback(JSRuntime* rt, void* data, JSCompartment* comp)

This name is not very descriptive. Maybe NotHasWrapperAssertionCallback?

@@ +526,5 @@
>      }
>  }
>  
> +bool
> +nsXPCWrappedJS::CrossesCompartments() const

If you agree with my renaming reasoning, this would be something like IsMultiCompartment(). Otherwise, maybe rename this to IsCrossCompartment(). The "crosses compartments" phrasing sounds odd to me.

@@ +529,5 @@
> +bool
> +nsXPCWrappedJS::CrossesCompartments() const
> +{
> +    MOZ_ASSERT(IsRootWrapper());
> +    JSCompartment* comp = js::GetObjectCompartment(mJSObj);

super nit: Call this |c| or |compartment|, please.

::: js/xpconnect/src/xpcprivate.h
@@ +2454,5 @@
>      JSObject* GetJSObjectPreserveColor() const {return mJSObj;}
>  
> +    // Returns true if the wrapper chain crosses compartment boundaries.
> +    // If the wrapper chain crosses compartments, then it must be registered
> +    // on the XPCJSRuntime, otherwise, it must be registered in the

nit: probably "XPCJSRuntime. Otherwise," (period instead of the comma in the middle)

@@ +2456,5 @@
> +    // Returns true if the wrapper chain crosses compartment boundaries.
> +    // If the wrapper chain crosses compartments, then it must be registered
> +    // on the XPCJSRuntime, otherwise, it must be registered in the
> +    // CompartmentPrivate for the compartment of the root wrapper.
> +    bool CrossesCompartments() const;

Maybe something like "Returns true if a wrapper chain includes multiple compartments" etc. and then call this IsMultiCompartment()? If you can work something into the description about how this is for root wrappers that would be good.

::: js/xpconnect/src/xpcpublic.h
@@ +389,5 @@
>  
>      nsAutoCString jsPathPrefix;
>      nsAutoCString domPathPrefix;
>      nsCOMPtr<nsIURI> location;
> +    size_t xpcPrivate;

These two field names are not good. What are they measuring? sizeOfXPCPrivate and sizeOfXPCWrappedJS maybe?

Comment 7

[Andrew McCreight [:mccr8]]

(I only looked at the XPConnect changes.)

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8677640 [details] [diff] [review]
object_2_wrapped_map_by_zone-v1.diff

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

This all looks really good.

One question I have though is why are we splitting the map up by compartment?  I think splitting by zone would make more sense.  There's no XPConnect ZonePrivate though and maybe there are other reasons too.

It seems we could have the same issue with sweeping CPOWs too as these are all swept in one go.

::: js/src/jsapi.h
@@ +1734,5 @@
>   *  2) Their referent has been moved by a compacting GC
>   *
>   * To handle this, any part of the system that maintain weak pointers to
>   * JavaScript GC things must register a callback with
> + * JS_(Add,Remove)WeakPointer{Slice,Compartment}Callback(). This callback must

The comment says 'slice' instead of zone group (I guess you renamed this).

::: js/src/jsgc.cpp
@@ +1640,5 @@
> +GCRuntime::callWeakPointerZoneGroupCallbacks() const
> +{
> +    for (auto const& p : updateWeakPointerZoneGroupCallbacks) {
> +        p.op(rt, p.data);
> +    }

Nit: don't need braces around single line block, and again below.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> One question I have though is why are we splitting the map up by
> compartment?  I think splitting by zone would make more sense.  There's no
> XPConnect ZonePrivate though and maybe there are other reasons too.

FWIW, there is a zone equivalent of CompartmentPrivate, though I don't think there's a mainthread-only subclass like there is for compartments.

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Nice work figuring out this whole mess and coming up with a nice solution! I
> wonder if it would be worth having the GC somehow busting its budget if it
> notices that it is spending most of its time without making any forward
> progress. Having longer slices would be less bad than the catastrophic
> failures you were experiencing.

I've been meaning to look into doing something like this, but I've been holding off to focus on my studies. I want to look at it this weekend - I have some ideas for how to make it work, though I might get lost trying to figure out which work happens on every slice, and which happens only once.

Comment 11

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> Comment on attachment 8677640 [details] [diff] [review]
> object_2_wrapped_map_by_zone-v1.diff
> 
> Review of attachment 8677640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks really good.
> 
> One question I have though is why are we splitting the map up by
> compartment?  I think splitting by zone would make more sense.  There's no
> XPConnect ZonePrivate though and maybe there are other reasons too.

Yes, you are right. I had once again forgotten that Zone is a fully public interface and not a GC detail. I'll see how hard this is going to be to retrofit today.

> It seems we could have the same issue with sweeping CPOWs too as these are
> all swept in one go.

Yes, however, I measured times consistently below 50us here, so it seems less urgent. I guess CPOW are to support legacy addons (I thought including NoScript, which I am running) and their usage is strongly discouraged? I'm thinking this is going to be less and less of a problem in the future, but who knows?

> ::: js/src/jsapi.h
> @@ +1734,5 @@
> >   *  2) Their referent has been moved by a compacting GC
> >   *
> >   * To handle this, any part of the system that maintain weak pointers to
> >   * JavaScript GC things must register a callback with
> > + * JS_(Add,Remove)WeakPointer{Slice,Compartment}Callback(). This callback must
> 
> The comment says 'slice' instead of zone group (I guess you renamed this).

Thanks, fixed!

> ::: js/src/jsgc.cpp
> @@ +1640,5 @@
> > +GCRuntime::callWeakPointerZoneGroupCallbacks() const
> > +{
> > +    for (auto const& p : updateWeakPointerZoneGroupCallbacks) {
> > +        p.op(rt, p.data);
> > +    }
> 
> Nit: don't need braces around single line block, and again below.

Actually, MSVC chokes on any ranged for statement with a non-braced body statement. I opened bug 1218419 to remind us to clean this up as soon as MSVC is fixed and deployed.

Comment 12

[Terrence Cole [:terrence]]

I was not able to find anything at all like ZonePrivate in XPConnect. I'd like to add this, but it's going to be painful to do that and this at the same time, whereas it should be fairly simple once this is landed. We discussed this in the GC meeting and decided that it would be okay to land this per-compartment and move it in a separate patch.

Comment 13

[Andrew McCreight [:mccr8]]

JS_SetZoneUserData() is the callback used to set the "zone private". Right now, it is just a ZoneStringCache(), so a bit of work would be needed to make this work (as you don't want this XPConnect stuff off the main thread).

Comment 14

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Comment on attachment 8677640 [details] [diff] [review]
> object_2_wrapped_map_by_zone-v1.diff
> 
> Review of attachment 8677640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work figuring out this whole mess and coming up with a nice solution! I
> wonder if it would be worth having the GC somehow busting its budget if it
> notices that it is spending most of its time without making any forward
> progress. Having longer slices would be less bad than the catastrophic
> failures you were experiencing.

The problem is that the GC is organized in such a way that figuring out and acting on things that happen regularly is extremely difficult. It's certainly possible, but it would not be easy, would still have holes, and it wouldn't solve the fundamental problem. It would also add /even more complexity/ to the GC. Not that this doesn't, but at least it does address the root issue directly.

> The terminology here is off to me. A wrapped JS itself cannot cross
> compartments, as it really does not exist in any particular compartment, as
> a C++ object, and there is no particular ordering relation between the
> different compartments as there is for CCWs. Plus, using different
> terminology to make things more distinct is probably a good idea. Maybe
> these could be "multi-compartment wrapped JS" and "single-compartment
> wrapped JS"? Does that make sense to you? Maybe I'm misunderstanding
> something. You probably know about as much as me about wrapped JS at this
> point. (This change in terminology will require a pass to rename comments
> and functions.)

Nope, that's spot-on! I've renamed and rewritten as much as I could find.

> Also, I think that the "wrapper chains" terminology you use doesn't quite
> match up with XPConnect. "wrapped JS" can just refer to the entire chain,
> even though really the chain is made up of elements of nsXPCWrappedJS.

My understanding is that there is always a single JS implementation object (the key) which can be wrapped into many different interfaces (the chain). I think the new wording better reflects this. Please let me know if you see anything that could be clearer.

> r- because I want to see the revised version, mostly for the bits about
> memory reporting, but otherwise this looks good to me!

Yeah, sorry, that part was totally half-baked -- just enough for me to verify that it was working. I should have cleaned that up before asking for review.

> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ +941,2 @@
> >  {
> > +    // Called before each sweeping slice, after finishing marking, to clear out
> 
> "after finishing marking" to "after marking is finished" maybe? Marking is
> done in each sweeping slice?

Yes. Even though the marking phase is finished, barriers that have run inbetween the end of marking and when we start sweeping this zone group (potentially many slices later) may have put new entries in the mark buffer. Thus, in beginSweepingZoneGroup, we flush the mark buffer, which may well cause substantial amounts of marking during sweeping. I've tried to clarify this in the comment.

> @@ +941,4 @@
> >  {
> > +    // Called before each sweeping slice, after finishing marking, to clear out
> > +    // any references to things that are about to be finalized and update any
> > +    // moved pointers.
> 
> "update any moved pointers" Isn't this really more like "pointers to any
> moved objects?" I'm not sure what terminology you usually use in the GC.

Yes, my wording here was casual to the point of sloppiness.


> @@ +2820,5 @@
> >              // Note: cannot use amIAddonManager implementation at this point,
> >              // as it is a JS service and the JS heap is currently not idle.
> >              // Otherwise, we could have computed the add-on id at this point.
> > +            extras->xpcPrivate = mallocSizeOf_(cp);
> > +            extras->xpcWrappedJS = cp->SizeOfWrappedJS(mallocSizeOf_);
> 
> It seems a little odd to me that these are split up. Can you just combine
> them into a single category (including mentioning single-compartment wrapped
> JS in the description) and then write a SizeOfIncludingThis for
> CompartmentPrivate?

Yeah, good point. The size of the struct itself is always going to be dominated by the map anyway.

> ::: js/xpconnect/src/XPCWrappedJS.cpp
> @@ +336,5 @@
> >  
> > +    xpc::CompartmentPrivate* rootComp = xpc::CompartmentPrivate::Get(rootJSObj);
> > +    MOZ_ASSERT(rootComp);
> > +
> > +    // Find any existing wrapper.
> 
> Could you wrap all of this up into a method, maybe on CompartmentPrivate?
> GetNewOrUsed() is pretty giant already. Maybe called GetRootWrappedJS() or
> something.

Eh, I'm not so hot on that actually. This lookup is only used in one place, it would require CompartmentPrivate to access XPCJSRuntime, which it does not currently, and GetNewOrUsed fits on one screen already. And looking at it further, none of the intermediary values are even required, so we can trim it down to a very clear 4 lines trivially.

> @@ +338,5 @@
> > +    MOZ_ASSERT(rootComp);
> > +
> > +    // Find any existing wrapper.
> > +    JSObject2WrappedJSMap* rootMap = rootComp->GetWrappedJSMap();
> > +    RefPtr<nsXPCWrappedJS> root = rootMap->Find(rootJSObj);
> 
> Would it be worth asserting that if something is in the compartment map that
> it isn't also in the global map?

Good idea!

> @@ +342,5 @@
> > +    RefPtr<nsXPCWrappedJS> root = rootMap->Find(rootJSObj);
> > +    if (!root) {
> > +        XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
> > +        JSObject2WrappedJSMap* globalMap = rt->GetGlobalWrappedJSMap();
> > +        MOZ_ASSERT(globalMap, "multi-compartment wrappedjs map missing");
> 
> Is there some reason to assert this for the global map but not the
> compartment map? It seems inconsistent.

No, no good reason; it just already existed. Or rather, it was a dynamic check which returned failure. Which seemed absurd to me because absolutely nothing would work if the check failed, so recovery is not even an option. I changed it to an assert for sanity. Then when I added the new table access, I realized that this assertion doesn't really get us anything, but did not remove the existing one. Gone now.

> @@ +400,2 @@
> >      if (IsRootWrapper()) {
> > +        MOZ_ASSERT(!CrossesCompartments());
> 
> I guess we're guaranteed this because mNext is null.

Added a failure string to the assertion to that effect.

> @@ +407,4 @@
> >          NS_ADDREF(mRoot);
> >          mNext = mRoot->mNext;
> >          mRoot->mNext = this;
> > +        if (mRoot->CrossesCompartments()) {
> 
> Maybe this is too cute, but you only have to check that the compartment of
> |mRoot| is different than the compartment of |this|, not the whole chain.
> The only case where you have to do something is if the WJS is currently
> single-compartment but is becoming multi-compartment. (If |this|'s
> compartment is the same as |mRoot| but different than some later thing in
> the chain, then |mRoot| must already be in the global table.) If you do
> that, please add a comment or an assert or both.

In my 500 tab session, of the 50000 wrapper chains, I think I had 11 chains with a depth greater than 2. I don't think it's worth being too cute here.

> @@ +428,5 @@
> > +
> > +    // Wrapper chains that cross compartments are guaranteed to be in the
> > +    // global table; wrapper chains that do not cross compartment bounds may
> > +    // reside in either table, depending on whether they have ever had a cross-
> > +    // compartment wrapper chain. Since removal requires a lookup anyway, we
> 
> Hopefully we won't have to deal with global wrapper table fragmentation at
> some point. :)

Yeah. We could potentially defrag in the zonegroup sweep callback, although we still have the problem of getting a cx there.

> @@ +431,5 @@
> > +    // reside in either table, depending on whether they have ever had a cross-
> > +    // compartment wrapper chain. Since removal requires a lookup anyway, we
> > +    // just do the remove on both tables in the latter case.
> > +    GetGlobalWrappedJSMap()->Remove(wrapper);
> > +    if (wrapper->CrossesCompartments()) {
> 
> If 90% are really single compartment, maybe just unconditionally remove it
> from the compartment table?

Good idea! Done.

> @@ +526,5 @@
> >      }
> >  }
> >  
> > +bool
> > +nsXPCWrappedJS::CrossesCompartments() const
> 
> If you agree with my renaming reasoning, this would be something like
> IsMultiCompartment(). Otherwise, maybe rename this to IsCrossCompartment().
> The "crosses compartments" phrasing sounds odd to me.

Heh, I independently renamed it to IsMultiCompartment before even getting this far.

> @@ +529,5 @@
> > +bool
> > +nsXPCWrappedJS::CrossesCompartments() const
> > +{
> > +    MOZ_ASSERT(IsRootWrapper());
> > +    JSCompartment* comp = js::GetObjectCompartment(mJSObj);
> 
> super nit: Call this |c| or |compartment|, please.

|c| is right out, so |compartment| it is.

Comment 15

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> JS_SetZoneUserData() is the callback used to set the "zone private". Right
> now, it is just a ZoneStringCache(), so a bit of work would be needed to
> make this work (as you don't want this XPConnect stuff off the main thread).

Ah! So that's it. That's going to need enough work that I still feel justified landing on the compartment initially.

Comment 16

[Terrence Cole [:terrence]]

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

Comment 17

[Terrence Cole [:terrence]]

Attachment: object_2_wrapped_map_by_zone-v2.diff

I'll avoid checking in until after uplift.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8680127 [details] [diff] [review]
object_2_wrapped_map_by_zone-v2.diff

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

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +395,5 @@
>      // that are subject to finalization. See the top of the file for more
>      // details.
>      NS_ADDREF_THIS();
>  
> +    XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();

nit: rt isn't used, so just remove this line.

@@ +403,2 @@
>      } else {
> +        // We always start wrappers in the per-compartment table. If adding

nit: I think it makes sense to move this comment down to the part where you are actually adding it to the wrapped JS table.

@@ +506,5 @@
>              MOZ_ASSERT(cur, "failed to find wrapper in its own chain");
>          }
> +
> +        // Note: unlinking this wrapper may have changed us from a multi-
> +        // compartment wrapper to a single-compartment wrapper chain. We leave

"multi-compartment wrapper" -> "multi-compartment wrapper chain"?

::: js/xpconnect/src/xpcprivate.h
@@ +3804,5 @@
> +    JSObject2WrappedJSMap* GetWrappedJSMap() const { return mWrappedJSMap; }
> +    void UpdateWeakPointersAfterGC(XPCJSRuntime* runtime);
> +
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
> +    // FIXME: add memory reporting for this structure

What do you mean by "this structure"? Isn't there reporting for CompartmentPrivate?

Comment 19

[Terrence Cole [:terrence]]

Wow, I can't believe that I missed that FIXME twice; sorry about that. No, there was no reporting for CompartmentPrivate, previously. That is basically what I've added in this patch.

Comment 20

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecec6573ae9e323ea1f4ca401d160ad776405a6
Bug 1214961 - Sweep XPConnect incrementally; r=mccr8, r=jonco

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ecec6573ae9

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7ecec6573ae9

Comment 23

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion