Closed Bug 490592 Opened 13 years ago Closed 13 years ago

Possible to GC way too much during shutdown due to XUL and XBL prototypes


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: bzbarsky, Assigned: peterv)



(Keywords: fixed1.9.1)


(1 file, 3 obsolete files)

If shutdown takes more than 2 seconds, so that whatever GC timer we set as we tear down the windows actually fires and we once again have a null gc timer, then nsJSContext::Unlink has the possibility of triggering GC.  There's a boolean member that controls whether it should destroy its JSContext using JS_DestroyContext or JS_DestroyContextNoGC, and this boolean is set to not gc for the nsJSContext of an nsGlobalWindow that's a subframe... but set to GC for the nsJSContext of a XUL or XBL prototype document, and for non-subframe windows.

This triggers one GC per XBL or XUL proto doc if the GC timer fires during shutdown.  Over here, with a "start firefox on default start page and then immediately quit" test, that gives me the following stats for shutdown:

  21 GC calls when unlinking the nsJSContext for an XBL proto doc
   7 GC calls when unlinking the nsJSContext for a XUL proto doc
   3 GC calls when unlinking the nsJSContext for an nsGlobalWindow
   1 GC call from the timer triggering CC
   1 GC call from nsXREDirProvider::Shutdown
   1 GC call from nsDOMScriptObjectFactory::Observe
   1 GC call from mozJSComponentLoader::Observe
   3 GC calls from shutting down cycle collector
   1 GC call from mozJSComponentLoader::UnloadModules
   1 GC call on the safe context from ~nsXPConnect
   1 GC call on the temp context it creates from ~nsXPConnect

An obvious question: can we get away with not doing GC when destroying XBL and XUL proto JSContexts?  The common case is that this is all chrome and so cached until shutdown in the proto caches anyway, right?  In the rare case of non-chrome XUL and XBL, the proto docs could go away when the main doc referencing them does, but in that case we'd schedule a GC for that main doc going away anyway, right?   I suppose that GC could be what's freeing the proto docs... but I still think it's ok to not eagerly GC for those; perhaps just set up the GC timer if needed.

David, could this be what you were seeing in terms of shutdown being hit by a lot of GC calls?
Flags: blocking1.9.2?
Does the patch in bug 475737 help?
I'll try to test tomorrow, but I don't see why it would: the relevant issue is that all of those nsJSContexts have the "gc on destruction" boolean set to true and we destroy them all during shutdown.
You talk about unlinking in comment 0 and that patch sets the "gc on destruction" boolean to false when unlinking.
Oh, I see.  About 1/3 of the Unlink calls came from cycle collector directly; the rest came via ~nsJSContext, so aren't affected by that patch.
Attached patch v1 (obsolete) — Splinter Review
Haven't tested this yet.
Assignee: nobody → peterv
We could also inverse the logic and have context holders ask explicitly for GC on destruction.
Assignee: peterv → nobody
I think we should also remove the GC calls from
nsDOMScriptObjectFactory::Observe, mozJSComponentLoader::Observe and
Assignee: nobody → peterv
Maybe not the one in nsXREDirProvider::DoShutdown (but I wonder if that should really do a CC).
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #375049 - Attachment is obsolete: true
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #375253 - Attachment is obsolete: true
Attached patch v1.1Splinter Review
Attachment #375254 - Attachment is obsolete: true
Attachment #375256 - Flags: superreview?(bzbarsky)
Attachment #375256 - Flags: review?(bzbarsky)
Comment on attachment 375256 [details] [diff] [review]

This seems to work, I'm now down to 7 js_GC calls on shutdown (down from 42).
Attachment #375256 - Flags: superreview?(bzbarsky)
Attachment #375256 - Flags: superreview+
Attachment #375256 - Flags: review?(bzbarsky)
Attachment #375256 - Flags: review+
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #375256 - Flags: approval1.9.1?
Comment on attachment 375256 [details] [diff] [review]

I think we should take this on branch. Together with bug 475737 this decreased Tshutdown on Linux by about 9%. OS X also showed a decrease when this landed, but it regressed in the meantime (I backed out briefly to make sure this patch wasn't implicated in the regression).
Attachment #375256 - Flags: approval1.9.1? → approval1.9.1+
Keywords: fixed1.9.1
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Depends on: 494027
Flags: blocking1.9.2? → blocking1.9.2+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.