549793 - XPCJSRuntime::UnrootContextGlobals should not unroot globals for uncollectable documents

Status: RESOLVED INCOMPLETE

(Core :: XPConnect, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was having a discussion with Andreas about cycle collector performance, and I started wondering why we traverse so many JS objects given the optimization in  nsXPConnect::Traverse:

    // There's no need to trace objects that have already been marked by the JS
    // GC. Any JS objects hanging from them will already be marked. Only do this
    // if DEBUG_CC is not defined, else we do want to know about all JS objects
    // to get better graphs and explanations.
    if(!cb.WantAllTraces() && type == GCMarked)
        return NS_OK;

It occurs to me that this optimization would be much more effective if, in XPCJSRuntime::UnrootContextGlobals, we didn't unroot the globals for any global object for which the check in nsGlobalWindow::cycleCollection::Traverse:

  if (tmp->mDoc && nsCCUncollectableMarker::InGeneration(
                     cb, tmp->mDoc->GetMarkedCCGeneration())) {
    return NS_SUCCESS_INTERRUPTED_TRAVERSE;
  }

determined that the document was uncollectable.


I think this would prevent us from putting the bulk of the JS GC heap into the cycle collector graph.

Comment 1

[Andreas Gal :gal]

This bug touches all the relevant parts of the context global clearing.

https://bugzilla.mozilla.org/show_bug.cgi?id=458099

I think one of the mistakes is that we clear the globals prior to the JS GC run. I assume this is an optimization attempt in order to avoid rescanning the JS GC heap again, however, we have to do that anyway to collect the cycle collector edges. Another potential benefit is that we immediately dispose of JS objects participating in a cycle the cycle collector drops. However, we pay a couple hefty fines for this:

1. CC becomes non-interruptible and non-abortable. We have to sweep the XPCOM heap to discover JSObjects that are held live through XPCOM and then mark them since we bypassed this marking by clearing out the context globals.
2. (1) muddles the boundary between GC and CC and results in a "GC marking done, but not really" state where the JS GC thinks its done marking, but the CC will revive additional objects.

I would like to undo this optimization attempt and run a regular JS GC and then CC only on the XPCOM side. We still have to explore the JS heap to a degree to identify cycles, but the only cycle resolution we engage in is removing roots of cycles at the XPCOM->JSHeap boundary. Once that root is dropped the next JS GC will clear the cycle on the JS side, unwinding the cycle on the XPCOM side. This has a number of benefits:

1. Simple, clearly defined GC protocol. Marking, Sweep, Done. No intermediate and mark after IsAboutToBeFinalized calls.
2. CC becomes interruptible and abortable, i.e. if the user moves the mouse or presses a key or we get a network update, we just stop CCing and do it later. With this we can CC more often, but essentially with 0 latency (minus battery life issues, but that's for stuart to worry about).
3. The JS engine can start sweeping its own heap once its done marking, concurrently to cycle collection since cycle collection will no longer revive dead objects (or try to touch dead objects). We spend more time sweeping than marking, so we might be able to hide the time of CC in the sweep time.

Comment 2

[Andreas Gal :gal]

A bit of archeology shows that way back we decided to intertwine GC and CC to avoid a redundant scan/mark. That's basically exactly what I propose we try now, since we can make it concurrent and interruptible. We will need some experimenting with this to see whether comment #1 is faster, or the decision back in 2007 was right after all. Doesn't look super hard to hack that up though (at least to he point where we can benchmark it), since its mostly undoing changes.

"So if I understand correctly XPCOM objects would use the API from bug 379220 to
keep JS objects alive. Instead of running the JS GC and use gcThingCallback,
we'd call JS_TraceRuntime to get refcounts for JS objects and we'd use tracing
to reconstruct the graph (like in attachment 261948 [details] [diff] [review]). If that's correct, then
we would duplicate a bunch of work between the JS GC (which we'll still need to
run) and the refcounting trace?"

https://bugzilla.mozilla.org/show_bug.cgi?id=377884#c9

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Can we put that in a separate bug from what I described in comment 0, and leave this bug about what its summary says?  If this behaves the way I think it will, I think we'd want to take it on branches.

Comment 4

[Andreas Gal :gal]

Yeah, sorry, just dumped the description in this bug. I will fork off a bug (I don't even think its dependent actually).

Comment 5

[Andreas Gal :gal]

Moved into 549806. This bug is strictly about less unrooting now.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible patch

Here's a patch that I think does what I described; I've only barely started testing it.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible patch

Here's the current status.  Last I checked (a while ago) it was adding more stuff that's in JS components to the CC graph than I expected it to.  I think mrbkap explained it to me, but I don't remember the explanation, and it doesn't make sense to me given that mozJSComponentLoader::GlobalForLocation calls JS_AddNamedRoot... unless the global variables actually end up on something other than the global object.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A somewhat more merged version of this patch is at:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/tip/unapplied.optimize-away-js-traversal
although it probably doesn't build anymore.

Comment 9

[Andrew McCreight [:mccr8]]

Does anybody know if this is still possibly relevant?  I don't see a function UnrootContextGlobals anymore, but maybe the equivalent code is just elsewhere now.

Comment 10

[Andreas Gal :gal]

We still does this during gray marking.

Comment 11

[Andrew McCreight [:mccr8]]

Okay, thanks.  I want to look into which documents these huge CC graph globs are associated with, and it sounds like this is one possible source.

Comment 12

[Andrew McCreight [:mccr8]]

We optimize out most of the JS heap nowadays so this doesn't seem relevant.  We mark at least some uncollectable documents black during the GC, so maybe this is even a dupe.