The default bug view has changed. See this FAQ.

Make gcMaxBytes a hard limit

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
18.40 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
+++ 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.
(Assignee)

Updated

5 years ago
No longer blocks: 702251
(Assignee)

Comment 1

5 years ago
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.
Assignee: general → igor
Attachment #585153 - Flags: review?(anygregor)
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?
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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.
Attachment #585153 - Attachment is obsolete: true
Attachment #585153 - Flags: review?(anygregor)
Attachment #586013 - Flags: review?(anygregor)
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?
(Assignee)

Comment 6

5 years ago
(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 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!
Attachment #586013 - Flags: review?(anygregor) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/48928463f978
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/48928463f978 - this has landed some time ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.