Closed Bug 508140 Opened 13 years ago Closed 13 years ago

TM: remove reserved objects and doubles gunk

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Depends on: 506125
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #392380 - Attachment is patch: true
Attachment #392380 - Attachment mime type: application/octet-stream → text/plain
Attachment #392380 - Flags: review?(jorendorff)
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.
Blocks: 505308
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.
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 on attachment 392380 [details] [diff] [review]
patch

Forwarding review; see comment 5.
Attachment #392380 - Flags: review?(jorendorff) → review?(brendan)
Comment on attachment 392380 [details] [diff] [review]
patch

I want to do some more work on this.
Attachment #392380 - Flags: review?(brendan)
Attached patch patch (obsolete) — Splinter Review
Rely on conservative stack scanning and avoid the need for reserve lists.
Attachment #392364 - Attachment is obsolete: true
Attachment #392380 - Attachment is obsolete: true
Depends on: 516832
No longer depends on: 506125
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.
Attachment #404010 - Flags: review?(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
(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
Attachment #404010 - Flags: review?(zweinberg)
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
(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.
The patch is buggy. Instead of the don't mark flag we need a "incomplete, mark conservatively" flag. WIP will post new patch today.
(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 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).
Attachment #404010 - Flags: review?(zweinberg) → review-
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #404010 - Attachment is obsolete: true
Attachment #404010 - Flags: review?(brendan)
Depends on: 521472
No longer depends on: 516832
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #411839 - Attachment is obsolete: true
Attachment #412046 - Flags: review?(dvander)
The patch is a 9ms speedup on SS.
Attached patch patchSplinter Review
Attachment #412046 - Attachment is obsolete: true
Attachment #412098 - Flags: review?(dvander)
Attachment #412046 - Flags: review?(dvander)
Attachment #412098 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/c159b69e34a1
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/c159b69e34a1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 557140
You need to log in before you can comment on or make changes to this bug.