714280 - Make gcMaxBytes a hard limit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #714066 comment 6 +++

With the tracer JIT removed JSRuntime::gcMaxBytes can be changed back to be a hard limit that JSRuntime::gcBytes never exceeds. Currently gcMaxBytes  is just a soft limit that the allocation code can breach and that only periodically queried from the operation callback or when running the GC. With gcBytes becoming a hard limit those queries can be removed.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch moves the gcMaxByte check into Chunk::allocateArena where the JSRuntime::gcBytes counter is updated making OOM reporting code in the operation calback no longer necessary. In the patch I removed JS_ATOMIC_ADD calls when updating the gcBytes counters - these are not necessary as we always update the counters under the lock. I also changed the gcByte counters types to size_t for consistency with other counters.

Comment 2

[Gregor Wagner [:gwagner]]

Comment on attachment 585153 [details] [diff] [review]
v1


>-#ifdef JS_THREADSAFE
>-    if (rt->gcBytes >= rt->gcMaxBytes) {
>-        AutoLockGC lock(rt);
>-        cx->runtime->gcHelperThread.waitBackgroundSweepEnd();
>-    }
>-#endif

Does this change the current behavior? Where do we wait for the background 
thread to finish before we are OOM?

Comment 3

[Igor Bukanov]

(In reply to Gregor Wagner [:gwagner] from comment #2)
> Does this change the current behavior? Where do we wait for the background 
> thread to finish before we are OOM?

Good catch. There is a wait in ArenaLists::allocateFromArena, but it only happens when the background finalization for a particular arena free list has not finished, there is no wait when allocating a new arena indeed.

Comment 4

[Igor Bukanov]

Attachment: v2

It turned out that problem of not waiting for the background finalization to finish before reporting OOM during the GC allocation is per-existing one. This happens when OOM is real and comes from the failure to allocate more chunks. In that case we just report OOM without any waiting.

The patch fixes by changing ArenaLists::refillFreeList to always try to allocate arena twice, the second time after the background finalization finishes. As a bonus this simplified ArenaLists::allocateFromArena.

Comment 5

[Gregor Wagner [:gwagner]]

Comment on attachment 586013 [details] [diff] [review]
v2

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
>@@ -715,29 +715,32 @@ Chunk::fetchNextFreeArena(JSRuntime *rt)
>     return aheader;
> }
> 
> ArenaHeader *
> Chunk::allocateArena(JSCompartment *comp, AllocKind thingKind)
> {
>     JS_ASSERT(hasAvailableArenas());
> 
>     JSRuntime *rt = comp->rt;
>+    JS_ASSERT(rt->gcBytes <= rt->gcMaxBytes);
>+    if (rt->gcMaxBytes - rt->gcBytes < ArenaSize)
>+        return NULL;

Can you explain the logic behind this?
We might have empty arenas in already allocated chunks 
and still report OOM? Shouldn't we check this when we allocate chunks?

Comment 6

[Igor Bukanov]

(In reply to Gregor Wagner [:gwagner] from comment #5)
> Can you explain the logic behind this?
> We might have empty arenas in already allocated chunks 
> and still report OOM? Shouldn't we check this when we allocate chunks?

Without the patch we also report OOM when rt->gcBytes > rt->gcMaxBytes while we have empty chunks and committed free arenas. The patch just moves that check from the operation callback to the place where the counter is increased.

Also I have a patch in work that will remove all empty chunks and decommit all empty arenas on OOM or when rt->gcBytes breaches rt->gcMaxBytes. In that case rt->gcMaxBytes becomes a very useful notion on the amount of used memory that can only be released with the GC.

Comment 7

[Gregor Wagner [:gwagner]]

Comment on attachment 586013 [details] [diff] [review]
v2

>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp
>+++ b/js/src/jscntxt.cpp
>@@ -778,19 +778,19 @@ PopulateReportBlame(JSContext *cx, JSErr
>  * type message, and then hope the process ends swiftly.
>  */
> void
> js_ReportOutOfMemory(JSContext *cx)
> {
>     cx->runtime->hadOutOfMemory = true;
> 
>     /* AtomizeInline can cal this indirectly when it creates the string. */

Good opportunity to fix "cal"


Nice!

Comment 8

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48928463f978

Comment 9

[Igor Bukanov]

https://hg.mozilla.org/mozilla-central/rev/48928463f978 - this has landed some time ago