Closed Bug 549793 Opened 15 years ago Closed 12 years ago

XPCJSRuntime::UnrootContextGlobals should not unroot globals for uncollectable documents

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: dbaron, Unassigned)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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.
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.
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
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.
Yeah, sorry, just dumped the description in this bug. I will fork off a bug (I don't even think its dependent actually).
Moved into 549806. This bug is strictly about less unrooting now.
Attached patch possible patch (obsolete) (deleted) — Splinter Review
Here's a patch that I think does what I described; I've only barely started testing it.
Attached patch possible patch (deleted) — Splinter Review
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.
Attachment #429937 - Attachment is obsolete: true
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.
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.
We still does this during gray marking.
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.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: