901290 - Cycle collect more than just at shutdown

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

I verified that we actually run the CC too.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a24cbd51b6f7

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a24cbd51b6f7

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 789894 [details] [diff] [review]
Patch

Low risk and verified fix for a 25 regression, approving for Aurora 25.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/00d1b86bdbb6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

Any updates here, Kyle?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for Aurora

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7354d3c16740