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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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?

Comment 1

[Peter Van der Beken [:peterv]]

Does the patch in bug 475737 help?

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 3

[Peter Van der Beken [:peterv]]

You talk about unlinking in comment 0 and that patch sets the "gc on destruction" boolean to false when unlinking.

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Haven't tested this yet.

Comment 6

[Peter Van der Beken [:peterv]]

We could also inverse the logic and have context holders ask explicitly for GC on destruction.

Comment 7

[Peter Van der Beken [:peterv]]

I think we should also remove the GC calls from
nsDOMScriptObjectFactory::Observe, mozJSComponentLoader::Observe and
nsXREDirProvider::DoShutdown.

Comment 8

[Peter Van der Beken [:peterv]]

Maybe not the one in nsXREDirProvider::DoShutdown (but I wonder if that should really do a CC).

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 12

[Peter Van der Beken [:peterv]]

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).

Comment 13

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/5e867032abe5

Comment 14

[David Baron :dbaron: (away October 5-21)]

Looks like peterv backed this out: http://hg.mozilla.org/mozilla-central/rev/0f55d91d7724

Comment 15

[Peter Van der Beken [:peterv]]

Relanded: http://hg.mozilla.org/mozilla-central/rev/392148f89c32.

Comment 16

[Peter Van der Beken [:peterv]]

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).

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 375256 [details] [diff] [review]
v1.1

a191=beltzner

Comment 18

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a481e6b47493