1093307 - OOM calls ShrinkGCBuffers, but ShrinkGCBuffers does its work off-main-thread

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

ShrinkGCBuffers is an API called by gecko occasionally to do decommit: this happens off-main-thread and does not block, so effectively has no effect on the main thread. However, the OOM path in JSRuntime is trying to use it to free memory before re-allocating. We need a new API here that works on the main thread to synchronously toss everything that isn't nailed down.

Comment 1

[Terrence Cole [:terrence]]

Ah, actually looks like onOOM calls waitForBackgroundSweepOrAllocEnd -- didn't know that was public(!) -- so it's not actually totally busted. Still there's a ton more stuff we could be tossing here.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 0_stop_alloc_task_on_oom-v0.diff

There is more to do here. In particular we should not have to do racy tricks with the bg thread, or involve another thread at all here. This is an acceptable first step though.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 1_shrink_harder-v0.diff

And it looks like there's another serious facepalm with the shrinking GCs we trigger from the browser's low-memory signal. We currently do a full, non-incremental GC, then return to the mutator and sweep everything on the background thread. Of course, this means we'll only ever finish this work and actually free up memory far later, probably long after the browser has OOM'ed.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8516851 [details] [diff] [review]
1_shrink_harder-v0.diff

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

This is ok but we may need to change this again if we want to enable incremental compacting GC.

::: js/src/jsgc.cpp
@@ +5024,5 @@
>  
> +    sweepOnBackgroundThread = !lastGC &&
> +                              !TraceEnabled() &&
> +                              CanUseExtraThreads() &&
> +                              !shouldCompact() &&

shouldCompact() will return turn only if invocationKind == GC_SHRINK so this line is unnecessary with this change.

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/047b2012a5ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/367f7f0760fc

Comment 6

[Terrence Cole [:terrence]]

This probably also wants to do unconditional decommit on the foreground.

Comment 7

[Terrence Cole [:terrence]]

Attachment: 2_make_oom_paths_simpler-v0.diff

This adds a trivial decommit loop for use by the OOM path so that we don't have to rely on the expireChunksAndArenas viper-pit. This also unlocks a nice follow-up: now if we want to use a Vector to pass chunks to do background decommit, we can can fall back to this path on OOM to do the decommit in the foreground.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/047b2012a5ed
https://hg.mozilla.org/mozilla-central/rev/367f7f0760fc

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a96e15c810c

I feel much better about this path now.

Comment 10

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8b348242a2 for ggc bustage:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3742404&repo=mozilla-inbound

Comment 11

[Terrence Cole [:terrence]]

The build error was fixed in the other bug, so I think this should be fine now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2603e86d422

Comment 12

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

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