Don't garbage collect while we hold the nsCycleCollectorRunner mutex

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.22 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 664506
To be clear, it's the nsCycleCollectorRunner's mutex. The cycle collector doesn't have a mutex.
Summary: Don't garbage collect while we hold the cycle collector mutex → Don't garbage collect while we hold the nsCycleCollectorRunner mutex
(Assignee)

Comment 2

6 years ago
I can work on this, but I'd like to discuss with somebody what exactly the runner mutex is supposed to do.
Assignee: nobody → continuation
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 540907 [details] [diff] [review]
lift the CC GC outside of the runner mutex
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #540907 - Flags: review?(bent.mozilla)
Comment on attachment 540907 [details] [diff] [review]
lift the CC GC outside of the runner mutex

Review of attachment 540907 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #540907 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/771d948c8779
http://hg.mozilla.org/mozilla-central/rev/771d948c8779
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.