1296639 - There should be only one periodic full GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

At the moment GCRuntime::maybePeriodicFullGC will trigger a full GC every 20 seconds if MaybeGC is called (which is called after running scripts from the browser).  However the browser also triggers its own full GC every 60 seconds.

We should remove one of these.

Comment 1

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

The one in DOMGCSliceCallback is triggered when doing a compartmental GC. What does maybePeriodicFullGC do? Or when is it called?

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1296639-remove-periodic-full-gc

Remove SM's periodic full GC.  The embedding can still trigger such GCs if it decides it needs to (and gecko still does of course).

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Olli Pettay [:smaug] from comment #1)
> The one in DOMGCSliceCallback is triggered when doing a compartmental GC.
> What does maybePeriodicFullGC do? Or when is it called?

maybePeriodicFullGC triggers a full (all-zones) GC every 20 seconds if there has been significant GC allocation or there are enough arenas to decommit.  

We already have other allocation based triggers though and we now decommit arenas at the end of GC anyway so I don't think this is required any more.  Telemetry shows this happens pretty rarely in practice too (0.17%).

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c82bf507f68b
Remove SpiderMonkey's periodic full GC r=sfink

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c3c2cb3b92
Remove decommit threshold prefs r=terrence

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32c3c2cb3b92

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1296639-beta

Approval Request Comment
[Feature/regressing bug #]: Not a regression, this removes an unnecessary GC trigger.
[User impact if declined]: Requested by RyanVM to reduce intermittent test failures in bug 1293057.
[Describe test coverage new/current, TreeHerder]: On m-c since August 23rd.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8795287 [details] [diff] [review]
bug1296639-beta

Fixes an intermittent (verified by RyanVM), Beta50+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1bff8281c912
https://hg.mozilla.org/releases/mozilla-beta/rev/0f6060afb0ed