926678 - JSRuntime::gcMallocBytes is ignored if it doesn't trigger a GC the first time

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

JSRuntime::updateMallocCounter very carefully calls onTooMuchMalloc only when gcMallocBytes *crosses* the zero line, not whenever it is negative. However, this means that if that call to onTooMuchMalloc declines to request a GC, updateMallocCounter will never try again. Hence:

(gdb) p gcMallocBytes
$27 = -718199820

and very large shell processes.

(CC'ing bhackett and billm since they did bug 921809.)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug926678-runtime-malloc-count

How about we make js::TriggerGC() and TriggerZoneGC() return whether they have actually triggered a GC?  Then we can use this information to set gcMallocBytes to a suitably large value to stop retriggering before the GC happens and resets them.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 817101 [details] [diff] [review]
bug926678-runtime-malloc-count

Review of attachment 817101 [details] [diff] [review]:
-----------------------------------------------------------------

That sounds pretty good. I'm a little worried that some code might not expect gcMallocBytes to be so large, but I guess we'll see what happens :-).

Comment 3

[Jon Coppeard (:jonco)]

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

Comment 4

[Carsten Book [:Tomcat]]

had to backout this change, as discussed on irc we were seeing failures like

https://tbpl.mozilla.org/php/getParsedLog.php?id=29180484&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=29182531&tree=Mozilla-Inbound

that happened after that push, sorry

Comment 5

[Jon Coppeard (:jonco)]

I can't get this crashtest failure to reproduce locally, although it does reproduce on try.

GC is still happening with the patch applied - in fact when the failing test starts, GC has happened more recently in the build with the patch applied.  So this changes the pattern of GC, and I suspect that is somehow provoking the crash.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug926678-runtime-malloc-count

So the problem with my first patch was that I didn't spot that BudgetIncrementalGC() actually calls isTooMuchMalloc() on the runtime and on the zones to work out whether we need to do a non-incremental GC.  Changing the alloc count like I did messed this up, and led to the crashtest process holding on to 3+GB of memory, which lead to other things falling over, hence the failure.

Instead, let's add a simple flag to say whether we tried to trigger a GC already, and check that instead.

Also, we can make these atomics rather than volatile in line with the other places where we have state accessed by more than one thread (actually the alloc count in the zone wasn't even volatile).

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 828798 [details] [diff] [review]
bug926678-runtime-malloc-count

Review of attachment 828798 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/125d0f9767b5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/125d0f9767b5