508140 - TM: remove reserved objects and doubles gunk

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

We currently reserve a bunch of doubles and objects to make sure recovery is infallible. This requires a lot of ugly code in 2 hot paths: NewGCThing/NewDouble and ExecuteTree. This patch gets rid of all of that code and instead simply allows the GC heap to grow beyond its maximum sizes while unwinding (soft quota reached). If we hit a hard quota (out of memory), we crash safely. This should be an extremely rare case, and is usually followed by a browser crash anyway. We will remove this safe crash point once bug 500346 is fixed and we can GC while unwinding.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Crash in a way that gives us a stack and mmap larger chunks on macosx and zero them to compensate for the crazy poor mmap performance on mac (the patch minimally changes allocation behavior, moving the allocation of a page to a little later, which arbitrarily affects sunspider scores).

Comment 3

[Andreas Gal :gal]

Note: If the GC fails to free up pages because they are partially allocated we might end up growing our GC heap in definitively until we run out of memory eventually.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 392380 [details] [diff] [review]
patch

>+#ifdef __APPLE__
>+        if (p != MAP_FAILED)
>+            memset(p, 0, js_gcArenasPerChunk << GC_ARENA_SHIFT);
>+#endif

So, what's that about?

>-# define js_gcArenasPerChunk 7
>+# define js_gcArenasPerChunk 31

OK, but can you put this in a separate changeset from the rest? This little change affects the behavior of a lot of code I don't know very well.

>         if (doGC
> #ifdef JS_TRACER
>-            && !JS_ON_TRACE(cx) && !JS_TRACE_MONITOR(cx).useReservedObjects
>+            && !JS_ON_TRACE(cx)
> #endif
>             ) {

You can lose the #ifdef now.

>+static void
>+crash()
>+{
>+    *(int*)0 = 0;
> }

Can we just call abort()? I just have no faith in GCC to emit code that does exactly what you obviously intend here.

Comment 5

[Jason Orendorff [:jorendorff]]

So much for the narrow technical review.

But the main question here is, is it ok to crash on OOM?

>+        if (!ok) // FIXME: can be removed once we scan the native stack during gc (bug 500346)
>+            crash();

Just to clarify the issue here: even if we could GC on trace, this call could still theoretically fail with OOM (if GC fails to free anything and the OS can't produce another page). Whether it could happen in practice (vs. whether the OS would just kill the process first) depends on the OS. I don't see why not, particularly if physical memory exceeds user-land virtual address space; but I've never personally seen the right conditions arise. It's something I really know very little about.

The patch is a very nice simplification, but it's a requirements/policy question.

We've gone in circles on this issue before. I am not the one to cut the knot. Cc-ing Brendan.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 392380 [details] [diff] [review]
patch

Forwarding review; see comment 5.

Comment 7

[Andreas Gal :gal]

Comment on attachment 392380 [details] [diff] [review]
patch

I want to do some more work on this.

Comment 8

[Andreas Gal :gal]

Attachment: patch

Rely on conservative stack scanning and avoid the need for reserve lists.

Comment 9

[Andreas Gal :gal]

Comment on attachment 404010 [details] [diff] [review]
patch

Brendan, are you ok with the CRASH() point in there? It will only be hit if we are completely out of memory and GC doesn't free up anything.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (From update of attachment 404010 [details] [diff] [review])
> Brendan, are you ok with the CRASH() point in there? It will only be hit if we
> are completely out of memory and GC doesn't free up anything.

Comment 0 said it would go away when bug 506117 (bug 500346 in comment 0 but that bug was a dup of 506117) is fixed. But is that true?

SpiderMonkey can OOM after a GC, and it unwinds. Firefox may or may not. With the Electrolysis work we have a better plan, but I'm not sure this patch is aligned with it. Cc'ing cjones.

/be

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 404010 [details] [diff] [review] [details])
> > Brendan, are you ok with the CRASH() point in there? It will only be hit if we
> > are completely out of memory and GC doesn't free up anything.
> 
> Comment 0 said it would go away when bug 506117 (bug 500346 in comment 0 but
> that bug was a dup of 506117) is fixed.

Er, or is the right bug now bug 500346 ? How many such bugs do we need? :-P

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 404010 [details] [diff] [review]
patch

>@@ -2946,16 +2899,19 @@ gc_lock_traversal(JSDHashTable *table, J
>                               JSVAL_TRACE_KIND(_v));                          \
>             }                                                                 \
>         }                                                                     \
>     JS_END_MACRO
> 
> void
> js_TraceStackFrame(JSTracer *trc, JSStackFrame *fp)
> {
>+    if (fp->flags & JSFRAME_DONT_MARK)
>+        return;

The flag is misnamed in light of the "Trace" GC-heap-tracing API. Suggest JSFRAME_INCOMPLETE or JSFRAME_DONT_GC_YET.

But is it really safe to not mark an incomplete frame's roots?

>+/*
>+ * If we are trying to unwind a native frame and run out of memory, and a GC cannot
>+ * resolve the situation, we can't recover and crash. This is a very rare situation
>+ * and should be avoided through proper management of the heap size by calling
>+ * MaybeGC() with sufficient frequency during periods of native code execution.
>+ */
>+void CRASH()
>+{
>+    *(void **)0 = 0;
>+}

We need something safer and more certain cross-platform. As jorendorff suggested, calling abort will do the job on Unixy systems. Is it on Windows?

Zack Weinberg looked into safe bad address to dereference cross-platform. r?'ing Zack for his thoughts here.

>@@ -6616,16 +6518,17 @@ LeaveTree(InterpState& state, VMSideExit
> 
>         /*
>          * Synthesize a stack frame and write out the values in it using the
>          * type map pointer on the native call stack.
>          */
>         SynthesizeFrame(cx, *fi, callee);
>         int slots = FlushNativeStackFrame(cx, 1 /* callDepth */, (*callstack)->get_typemap(),
>                                           stack, cx->fp, 0);
>+        cx->fp->flags &= ~(JSFRAME_DONT_MARK);

Don't parenthesize primary-expression operands of unary operators.

>@@ -6750,26 +6653,31 @@ LeaveTree(InterpState& state, VMSideExit
> #ifdef DEBUG
>     int slots =
> #endif
>         FlushNativeStackFrame(cx, innermost->calldepth,
>                               innermost->stackTypeMap(),
>                               stack, NULL, ignoreSlots);
>     JS_ASSERT(unsigned(slots) == innermost->numStackSlots);
> 
>+    /* Allow the GC to mark the now complete frames. */
>+    for (JSStackFrame* fp = cx->fp; fp && (fp->flags & JSFRAME_DONT_MARK); fp = fp->down)
>+        fp->flags &= ~JSFRAME_DONT_MARK;

Here, you didn't ;-).

>@@ -1163,17 +1163,17 @@ class TraceRecorder {
>                                                  jsval* rval);
>     JS_REQUIRES_STACK RecordingStatus newString(JSObject* ctor, uint32 argc, jsval* argv,
>                                                   jsval* rval);
>     JS_REQUIRES_STACK RecordingStatus interpretedFunctionCall(jsval& fval, JSFunction* fun,
>                                                                 uintN argc, bool constructing);
>     JS_REQUIRES_STACK void propagateFailureToBuiltinStatus(nanojit::LIns *ok_ins,
>                                                            nanojit::LIns *&status_ins);
>     JS_REQUIRES_STACK RecordingStatus emitNativeCall(JSSpecializedNative* sn, uintN argc,
>-                                                       nanojit::LIns* args[], bool rooted);
>+                                                       nanojit::LIns* args[]);

Fix over-indented by two space second line.

/be

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 404010 [details] [diff] [review] [details])
> > Brendan, are you ok with the CRASH() point in there? It will only be hit if we
> > are completely out of memory and GC doesn't free up anything.
>
> SpiderMonkey can OOM after a GC, and it unwinds. Firefox may or may not. With
> the Electrolysis work we have a better plan, but I'm not sure this patch is
> aligned with it. Cc'ing cjones.
> 

It depends.

For Electrolysis we'll probably try to play games with soft rlimits to detect excessive memory usage.  In that world, it appears our soft-rlimit safety net might trigger CRASH(), which probably isn't what anyone wants.

If SM could unwind into a Firefox call site known to be fallible, we could invoke our OOM code there.  Another option is for SM to directly call "Firefox's" OOM-handling code instead of CRASH().  (Firefox's OOM code currently just crashes too.)

But all that's far enough off that I'm not sure it's worth worrying about now.

Comment 14

[Andreas Gal :gal]

The patch is buggy. Instead of the don't mark flag we need a "incomplete, mark conservatively" flag. WIP will post new patch today.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #12)
> But is it really safe to not mark an incomplete frame's roots?

(In reply to comment #14)
> The patch is buggy. Instead of the don't mark flag we need a "incomplete, mark
> conservatively" flag. WIP will post new patch today.

So the answer to my question is that it's not safe? Good to know -- wasn't sure how it could work, thought maybe there was something in the gcthings/sprops that the trees hold that saved us.

Still want Zack's thoughts on best safe crash technique. I also need more time to think through the consequences of CRASH. Chris Jones is right to suggest it might bite at a soft-rlimit scenario when we don't want it to happen. Worse, this patch comes in well before Electrolysis, so we don't have any rendering process to restart mostly-transparently.

Murphy was an optimist...

/be

Comment 16

[Zack Weinberg (:zwol)]

Comment on attachment 404010 [details] [diff] [review]
patch

(In reply to comment #12)
> >+void CRASH()
> >+{
> >+    *(void **)0 = 0;
> >+}
> 
> We need something safer and more certain cross-platform. As jorendorff
> suggested, calling abort will do the job on Unixy systems. Is it on Windows?
> 
> Zack Weinberg looked into safe bad address to dereference cross-platform.
> r?'ing Zack for his thoughts here.

*(void **)0 = 0 is definitely *not* safe for this use case -- a compiler is within its rights to *silently delete* an operation that it can prove always writes to NULL (the effect is undefined, and surely the programmer does not *mean* to cause a crash...)

My "safe bad address" research was for situations where the address is not visibly dereferenced at compile time, so doesn't really apply here; also, I was not able to find any such address on 32-bit OSX, and I'm not enough of a Windows guru to validate the address I picked for 32-bit Windows with high confidence (still looking for help there...)

I do not know whether abort() is reliable on Windows, but if it is, I would go for that.  With gcc on any platform (and probably also with LLVM on OSX, since they copied most of gcc's extensions) another option is __builtin_trap(), which compiles to an illegal instruction.  If abort() is *not* reliable on Windows, for MSVC I'd suggest using inline assembly to insert an illegal instruction (__asm { ud2 } or __asm { hlt }, for instance).

Comment 17

[Zack Weinberg (:zwol)]

BTW, in case anyone thought I was kidding about compilers silently deleting code that they can prove triggers undefined behavior: http://gcc.gnu.org/ml/gcc/2009-10/msg00136.html shows a situation where it lets you delete tons of code as dead.

Comment 18

[Andreas Gal :gal]

Attachment: patch

This patch removes the reserved objects/doubles list by temporarily waiving the GC heap quota while LeaveTree runs. If we run into a malloc() failure during LeaveTree, bad things happen. I will address that in the next iteration.

Comment 19

[Andreas Gal :gal]

If we run into malloc() failure in LeaveTree, bad things happen already (bug 521472). We will fix that issue there and land this patch as is.

Comment 20

[Andreas Gal :gal]

Attachment: patch

Comment 21

[Andreas Gal :gal]

The patch is a 9ms speedup on SS.

Comment 22

[Andreas Gal :gal]

Attachment: patch

Comment 23

[Andreas Gal :gal]

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

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c159b69e34a1