Closed Bug 588016 Opened 10 years ago Closed 9 years ago

Avoid reporting OOM when background has not finished

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

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.
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.
Blocks: 584688
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #468060 - Flags: review?(anygregor)
Attached patch v2 (obsolete) — Splinter Review
In v2 I fixed I a race bug startBackgroundSweep/threadLoop. It should be the former, not latter, that sets the sweeping flag.
Attachment #468060 - Attachment is obsolete: true
Attachment #468164 - Flags: review?(anygregor)
Attachment #468060 - Flags: review?(anygregor)
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.
(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 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.
(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.
Attached patch v3Splinter Review
patch with nits addressed
Attachment #468164 - Attachment is obsolete: true
Attachment #470278 - Flags: review+
Attachment #468164 - Flags: review?(anygregor)
http://hg.mozilla.org/tracemonkey/rev/e2e1ea2a39ce
Whiteboard: fixed-in-tracemonkey
had to back this out because of hangs during make check.

http://hg.mozilla.org/tracemonkey/rev/afe3db59ae35
Whiteboard: fixed-in-tracemonkey
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.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/6659fdc25196
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.