Closed Bug 926678 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jimb, Assigned: jonco)



(1 file, 1 obsolete file)

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.)
Attached patch bug926678-runtime-malloc-count (obsolete) — Splinter Review
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.
Attachment #817101 - Flags: review?(wmccloskey)
Comment on attachment 817101 [details] [diff] [review]

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 :-).
Attachment #817101 - Flags: review?(wmccloskey) → review+
had to backout this change, as discussed on irc we were seeing failures like

that happened after that push, sorry
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.
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).
Assignee: nobody → jcoppeard
Attachment #817101 - Attachment is obsolete: true
Attachment #828798 - Flags: review?(wmccloskey)
Comment on attachment 828798 [details] [diff] [review]

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

Attachment #828798 - Flags: review?(wmccloskey) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.