665044 - Don't garbage collect while we hold the nsCycleCollectorRunner mutex

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

The cycle collector calls the JS garbage collector while holding a mutex.  Bug 664506 separates the JS GC call a bit from the rest of the CC, so bent would like to look into moving the JS GC call outside of the mutex.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

To be clear, it's the nsCycleCollectorRunner's mutex. The cycle collector doesn't have a mutex.

Comment 2

[Andrew McCreight [:mccr8]]

I can work on this, but I'd like to discuss with somebody what exactly the runner mutex is supposed to do.

Comment 3

[Andrew McCreight [:mccr8]]

Looking over the code, the only thing it looks like the main thread and the CC thread will touch in GCIfNeeded is the GC status bits, but the only time the CC thread read that info is when it is in BeginCollection, and that will only happen when the main thread is sitting between mRequest.Notify() and mReply.Wait(), where there's no code running, so it looks okay to me.

One thing I'm not sure about is that lifting the GC above |if (!mCollector->PrepareForCollection(&whiteNodes))|, but that means the GC will happen before obs->NotifyObservers(nsnull, "cycle-collector-begin", nsnull), instead of after.  Is that okay?  I don't know what it does.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: lift the CC GC outside of the runner mutex

Comment 5

[Andrew McCreight [:mccr8]]

Try run was good on this. (I redid the Android failure and it came out okay, despite not showing up on the chart.)
http://tbpl.mozilla.org/?tree=Try&rev=440d90d06dc1

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 540907 [details] [diff] [review]
lift the CC GC outside of the runner mutex

Review of attachment 540907 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 7

[Andrew McCreight [:mccr8]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/771d948c8779

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/771d948c8779