Last Comment Bug 665044 - Don't garbage collect while we hold the nsCycleCollectorRunner mutex
: Don't garbage collect while we hold the nsCycleCollectorRunner mutex
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 664506
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-17 10:40 PDT by Andrew McCreight [:mccr8]
Modified: 2011-06-28 02:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
lift the CC GC outside of the runner mutex (1.22 KB, patch)
2011-06-21 15:08 PDT, Andrew McCreight [:mccr8]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-06-17 10:40:05 PDT
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!) 2011-06-17 10:57:11 PDT
To be clear, it's the nsCycleCollectorRunner's mutex. The cycle collector doesn't have a mutex.
Comment 2 Andrew McCreight [:mccr8] 2011-06-17 11:15:33 PDT
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] 2011-06-21 15:05:32 PDT
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] 2011-06-21 15:08:40 PDT
Created attachment 540907 [details] [diff] [review]
lift the CC GC outside of the runner mutex
Comment 5 Andrew McCreight [:mccr8] 2011-06-22 10:58:33 PDT
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!) 2011-06-22 17:29:17 PDT
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] 2011-06-27 10:38:17 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/771d948c8779
Comment 8 Marco Bonardo [::mak] 2011-06-28 02:25:12 PDT
http://hg.mozilla.org/mozilla-central/rev/771d948c8779

Note You need to log in before you can comment on or make changes to this bug.