Last Comment Bug 714280 - Make gcMaxBytes a hard limit
: Make gcMaxBytes a hard limit
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 714066
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-30 07:31 PST by Igor Bukanov
Modified: 2012-01-30 13:02 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (15.17 KB, patch)
2011-12-31 10:07 PST, Igor Bukanov
no flags Details | Diff | Review
v2 (18.40 KB, patch)
2012-01-05 02:18 PST, Igor Bukanov
anygregor: review+
Details | Diff | Review

Description Igor Bukanov 2011-12-30 07:31:59 PST
+++ 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 2011-12-31 10:07:16 PST
Created attachment 585153 [details] [diff] [review]
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] 2012-01-04 05:06:58 PST
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 2012-01-04 06:30:17 PST
(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 2012-01-05 02:18:09 PST
Created attachment 586013 [details] [diff] [review]
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] 2012-01-05 10:10:55 PST
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 2012-01-05 13:19:34 PST
(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] 2012-01-06 07:24:05 PST
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 9 Igor Bukanov 2012-01-30 13:02:24 PST
https://hg.mozilla.org/mozilla-central/rev/48928463f978 - this has landed some time ago

Note You need to log in before you can comment on or make changes to this bug.