Closed
Bug 901290
Opened 11 years ago
Closed 11 years ago
Cycle collect more than just at shutdown
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: khuey, Assigned: khuey)
References
Details
(Whiteboard: [MemShrink])
Attachments
(2 files)
(deleted),
patch
|
mccr8
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Right now the only time we ever run the CC on workers is at shutdown. We need to CC more often than just that.
Comment 1•11 years ago
|
||
Do we have some GC callback thing in workers? And/or do we manually trigger GC occasionally?
I think it might enough to trigger CC (asynchronously) during those times, at least for now.
Assignee | ||
Comment 2•11 years ago
|
||
I verified that we actually run the CC too.
Comment 3•11 years ago
|
||
Comment on attachment 789894 [details] [diff] [review]
Patch
Review of attachment 789894 [details] [diff] [review]:
-----------------------------------------------------------------
How long do those CCs take? You should be able to see by defining COLLECT_TIME_DEBUG in nsCycleCollector.cpp, plus you probably want to not print anything in TimeLog::Checkpoint if you are on the main thread...
Attachment #789894 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #3)
> How long do those CCs take?
Well nothing (except ImageData) is using it, so not very long ... I get 0 ms.
Comment 5•11 years ago
|
||
Great. I just wanted to make sure we didn't somehow end up with freakish amounts of gray objects. I'm not sure how that would happen though.
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 789894 [details] [diff] [review]
Patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 845545
User impact if declined: Without this patch it is possible to accumulate garbage in long lived worker threads that will not be collected until the worker shuts down. This could potentially lead to out-of-memory scenarios.
Testing completed (on m-c, etc.): Simple patch, tested manually in a debugger.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #789894 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Whiteboard: [MemShrink]
Comment 8•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 9•11 years ago
|
||
Comment on attachment 789894 [details] [diff] [review]
Patch
Low risk and verified fix for a 25 regression, approving for Aurora 25.
Attachment #789894 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•11 years ago
|
||
status-firefox25:
--- → fixed
status-firefox26:
--- → fixed
Comment 11•11 years ago
|
||
Backed out for cycle collector crashes.
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1848f275f6
https://tbpl.mozilla.org/php/getParsedLog.php?id=26831836&tree=Mozilla-Aurora
Comment 12•11 years ago
|
||
Any updates here, Kyle?
Assignee | ||
Comment 13•11 years ago
|
||
On Aurora the cycle collector runner was not removed, so calling nsCycleCollector_collect after nsCycleCollector_shutdownThreads explodes. We actually end up recursively invoking the cycle collector whenever the cycle collector decides to GC. The CC has internal protections against that that save us, except when we're already shutdown and just crash.
The solution is to move the flag that 'we shouldn't try to CC anymore' up a bit. I got r=mccr8 over IRC to make that change.
I confirmed this fixed the problem locally, although I did not test on try.
Attachment #803412 -
Flags: review+
Flags: needinfo?(khuey)
Assignee | ||
Updated•11 years ago
|
Keywords: branch-patch-needed
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•