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
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: bzbarsky, Assigned: peterv)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 3 obsolete files)
2.61 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•13 years ago
|
||
Does the patch in bug 475737 help?
![]() |
Reporter | |
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
You talk about unlinking in comment 0 and that patch sets the "gc on destruction" boolean to false when unlinking.
![]() |
Reporter | |
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
Haven't tested this yet.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•13 years ago
|
||
We could also inverse the logic and have context holders ask explicitly for GC on destruction.
Assignee | ||
Comment 7•13 years ago
|
||
I think we should also remove the GC calls from nsDOMScriptObjectFactory::Observe, mozJSComponentLoader::Observe and nsXREDirProvider::DoShutdown.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•13 years ago
|
||
Maybe not the one in nsXREDirProvider::DoShutdown (but I wonder if that should really do a CC).
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #375049 -
Attachment is obsolete: true
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #375253 -
Attachment is obsolete: true
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #375254 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #375256 -
Flags: superreview?(bzbarsky)
Attachment #375256 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•13 years ago
|
||
Comment on attachment 375256 [details] [diff] [review] v1.1 This seems to work, I'm now down to 7 js_GC calls on shutdown (down from 42).
![]() |
Reporter | |
Updated•13 years ago
|
Attachment #375256 -
Flags: superreview?(bzbarsky)
Attachment #375256 -
Flags: superreview+
Attachment #375256 -
Flags: review?(bzbarsky)
Attachment #375256 -
Flags: review+
Assignee | ||
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5e867032abe5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 14•13 years ago
|
||
Looks like peterv backed this out: http://hg.mozilla.org/mozilla-central/rev/0f55d91d7724
Assignee | ||
Comment 15•13 years ago
|
||
Relanded: http://hg.mozilla.org/mozilla-central/rev/392148f89c32.
Assignee | ||
Updated•13 years ago
|
Attachment #375256 -
Flags: approval1.9.1?
Assignee | ||
Comment 16•13 years ago
|
||
Comment on attachment 375256 [details] [diff] [review] v1.1 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).
Updated•13 years ago
|
Attachment #375256 -
Flags: approval1.9.1? → approval1.9.1+
Comment 17•13 years ago
|
||
Comment on attachment 375256 [details] [diff] [review] v1.1 a191=beltzner
Assignee | ||
Comment 18•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a481e6b47493
Updated•13 years ago
|
status1.9.2:
--- → beta1-fixed
Flags: blocking1.9.2? → blocking1.9.2+
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•