588016 - Avoid reporting OOM when background has not finished

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently SM reports OOM when system malloc or mmap returns NULL even when the background thread has not yet finished releasing the allocating memory. This is not good as the system should allow the background thread to finish its job before reporting OOM.

Comment 1

[Igor Bukanov]

In the bug 584688 it would be important to wait for the background sweep to finish before reporting OOM as string finalization may take substantial amount of time. So I make this bug to block that one to implement the necessary machinery here and minimize the patch there.

Comment 2

[Igor Bukanov]

Attachment: v1

The essence of the patch is to extend the background thread with signals to notify the listeners about the end of delayed free. A generic background thread was not very suitable for that so the patch replaces that with specialized code tailored for GC needs.

For simplicity the patch makes the GC to wait for the previous sweep to finish. That should not be an issue unless our GC heuristics are broken and schedule the GC way too often. But for compartment-level GC this would need to be reconsidered.

Comment 3

[Igor Bukanov]

Attachment: v2

In v2 I fixed I a race bug startBackgroundSweep/threadLoop. It should be the former, not latter, that sets the sweeping flag.

Comment 4

[Gregor Wagner [:gwagner]]

Is there a special reason why you moved everything from jstask into jsgc? jstask might not be the right name any more but that's not hard to change. I would prefer to keep it separate and make our jsgc file smaller.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Is there a special reason why you moved everything from jstask into jsgc?

This is due to its tight integration with the GC itself (using gcLock, GC waiting on its signals) and in anticipation of bug 584688.

Comment 6

[Gregor Wagner [:gwagner]]

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

>+JS_FRIEND_API(void *)
>+JSRuntime::onOutOfMemory(void *p, size_t nbytes, JSContext *cx)
>+{
>+    gcHelperThread.waitBackgroundSweepEnd(this);
>+    p = !p
>+        ? ::js_malloc(nbytes)
>+        : (p == reinterpret_cast<void *>(1))
>+        ? ::js_calloc(nbytes)
>+        : ::js_realloc(p, nbytes);
>+    if (!p && cx)
>+        js_ReportOutOfMemory(cx);
>+    return p;

I personally would prefer the more verbose version since it exceeds more than one "if".


>+
>+void
>+GCHelperThread::threadLoop(JSRuntime *rt)
>+{
>+    AutoLockGC lock(rt);
>+    while (!shutdown) {
>+        PR_WaitCondVar(wakeup, PR_INTERVAL_NO_TIMEOUT);
>+        if (sweeping) {
>+            AutoUnlockGC unlock(rt);
>+            doSweep();
>+        }
>+        sweeping = false;
>+        PR_NotifyAllCondVar(sweepingDone);
>+    }
>+}

Does this work? We lock with AutoLockGC and then wait?
Don't we need an extra lock and not the gcLock?


> 
>+    PRThread*         thread;
>+    PRCondVar*        wakeup;
>+    PRCondVar*        sweepingDone;
>+    bool              shutdown;
>+    bool              sweeping;

Do we need volatile here?


r=me with my 2 questions answered.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> >+    AutoLockGC lock(rt);
> >+    while (!shutdown) {
> >+        PR_WaitCondVar(wakeup, PR_INTERVAL_NO_TIMEOUT);
> >+        if (sweeping) {
> >+            AutoUnlockGC unlock(rt);
> >+            doSweep();
> >+        }
> >+        sweeping = false;
> >+        PR_NotifyAllCondVar(sweepingDone);
> >+    }
> 
> Does this work? We lock with AutoLockGC and then wait?

This works since the wait unlocks the corresponding lock until the PRCondVar is signaled.

> > 
> >+    PRThread*         thread;
> >+    PRCondVar*        wakeup;
> >+    PRCondVar*        sweepingDone;
> >+    bool              shutdown;
> >+    bool              sweeping;
> 
> Do we need volatile here?

No - the variables are read under the lock and volatile would not help with that.

Comment 8

[Igor Bukanov]

Attachment: v3

patch with nits addressed

Comment 9

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/e2e1ea2a39ce

Comment 10

[Robert Sayre]

had to back this out because of hangs during make check.

http://hg.mozilla.org/tracemonkey/rev/afe3db59ae35

Comment 11

[Igor Bukanov]

relanded - http://hg.mozilla.org/tracemonkey/rev/6659fdc25196

The hang from the comment 10 was due to a missing check for the sweeping flag in GCHelperThread::threadLoop. Without the check the first GC on the main thread can finish and call startBackgroundSweep before the background thread enters its loop. This lead to a missing PR_NotifyCondVar signal and a deadlock.

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/6659fdc25196