462027 - (deepbail) Bail off trace when reentering interpreter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

The underlying cause of bug 460336 is that we currently assume reentrancy is trace-invariant, i.e. that if you try a builtin at record time and it doesn't reenter, then that builtin will never reenter on trace.  I don't think there's any nontrivial case where this is true.  That is, the deepAbort machinery is all bogus.

Here's my proposed fix:

* Always guard after a call to a fallible builtin, to check for the case where we reentered the interpreter, bailing off trace, but successfully completed the call and returned a value.  In that case the trace must side exit, boxing the successful return value, putting it on the stack, and arranging for the interpreter to resume seamlessly (nontrivial, but certainly doable, if what bailed off trace was e.g. an js_ObjectToString call in the middle of a fat opcode).

* Factor out the code in js_ExecuteTree that rebuilds the stack frames and calling that stuff from js_GetTopStackFrame (introduced in bug 460865) instead of asserting. js_GetTopStackFrame also needs to set a flag so that the guard described above will fail when we return to the trace.

* Eyeball the existing infallible builtins to make sure they absolutely can't reenter.

Once we get there, tracing arbitrary JSFastNatives will be in reach.

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)

> * Always guard after a call to a fallible builtin, to check for the case where
> we reentered the interpreter, bailing off trace, but successfully completed the
> call and returned a value.

We guard fallible builtin calls already. Can we avoid adding another guard, combine guards with one failure test and leave a bread-crumb for the side exit machinery to notice and distinguish OOM (e.g.) from this case?

> In that case the trace must side exit, boxing the
> successful return value, putting it on the stack, and arranging for the
> interpreter to resume seamlessly (nontrivial, but certainly doable, if what
> bailed off trace was e.g. an js_ObjectToString call in the middle of a fat
> opcode).

You wrote "call to a fallible builtin" but those are from JSOP_CALL, not from the middle of a fat opcode like JSOP_ADD. Scripted conversion hooks that get trace-JITed should be handled by the fix for bug 456511, which this bug should not duplicate.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to comment #1)
> We guard fallible builtin calls already. Can we avoid adding another guard,
> combine guards with one failure test and leave a bread-crumb for the side exit
> machinery to notice and distinguish OOM (e.g.) from this case?

Yes, I think so, maybe cx->builtinExitCode.

> You wrote "call to a fallible builtin" but those are from JSOP_CALL, not from
> the middle of a fat opcode like JSOP_ADD. Scripted conversion hooks that get
> trace-JITed should be handled by the fix for bug 456511, which this bug should
> not duplicate.

OK, I won't worry about those until I hear more about how you plan to fix 456511.

What I had in mind, fwiw, was to just write the intermediate jsval back to its stack slot and retry the instruction in the interpreter.  For example, in (string + object) if the object.toString() call reenters and completes successfully, the side exit could then store the result on the top of the stack, overwriting the object, and resume in the interpreter, which would then do (string + string).  Maybe trampolines are less work.

Comment 3

[Jason Orendorff [:jorendorff]]

Trampolines do have the advantage that if you do it right, you can stay on trace.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #2)
> (In reply to comment #1)
> > We guard fallible builtin calls already. Can we avoid adding another guard,
> > combine guards with one failure test and leave a bread-crumb for the side exit
> > machinery to notice and distinguish OOM (e.g.) from this case?
> 
> Yes, I think so, maybe cx->builtinExitCode.

Andreas suggested using the InterpState storage -- a bit in the FrameInfo stored at state.rp would be enough.

Trampolines or whatever they should be called would let us keep on JITting, which is critical for performance in some hot benchmarks.

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Trampolines do have the advantage that if you do it right, you can stay on
> trace.

Sorry, mid-aired and plowed ahead. Yes, this is the hope for bug 456511.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: work in progress

This asserts on one test, but imacros should fix that.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Any update here?  Should this be a blocker?

Comment 8

[Brendan Eich [:brendan]]

I thought I nominated this. Yes, we need this safety fix. The last blocker in the list is bad, and there are many others of its ilk.

/be

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

OK, but no movement on it in a month -- can we get an update?

Comment 10

[Brendan Eich [:brendan]]

Motion is in the dependency bugs -- left-most one in list was updated 12/4.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

String_p_match_obj indirectly calls js_ValueToString on an object.  That is one case of this bug.

js_FastValueToIterator indirectly calls js_GetProperty.  Same deal.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to comment #6)
> v1
> 
> This asserts on one test, but imacros should fix that.

Imacros did not fix that.  Filed bug 468782 since the patch here is already big.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v1

This introduces a new class of builtins that can "deep bail", i.e. fall off trace in native code and proceed to finish the native call, possibly GCing, accessing JS stack frames, or calling back into the interpreter along the way.  After a deep bail, the native eventually returns back to JITted code, which detects the deep bail and returns to the interpreter.  Instead of retrying the current instruction, the interpreter resumes at the next instruction--builtins of this kind always run to completion, and can have side effects, so they must not be retried.  Details are in a new comment in jsbuiltins.h.

This patch fixes all the interpreter-reentry problems I know of in natives so far (Array.p.join and friends).  However, it does not fix bug 468782 ("TM: js_FastValueToIterator and js_FastCallIteratorNext can reenter") which is another common source of re-enter bugs now.

Comment 14

[Brendan Eich [:brendan]]

Looks ok, could you attach a rebased patch? Also, any idea of perf effects?

/be

Comment 15

[Brendan Eich [:brendan]]

I've found (VMSideExit.block) that adding to js_ExecuteTree hurts perf. The bail from native that entered interpreter case is rare. Could we move some of the cost from js_ExecuteTree into the side exit code for only the bail-guards, somehow?

/be

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to comment #15)
> I've found (VMSideExit.block) that adding to js_ExecuteTree hurts perf. The
> bail from native that entered interpreter case is rare. Could we move some of
> the cost from js_ExecuteTree into the side exit code for only the bail-guards,
> somehow?

No, the new code in js_ExecuteTree (I think you mean the check for cx->builtinStatus & JSBUILTIN_BAILED?) must happen just before resuming in the interpreter, not at the time of the side exit.  In the case of a deep bail, the two are distinct and can be far apart.

Perf numbers coming.

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - v2

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - fixing traceable natives that return jsval - v1

SunSpider:
  baseline    - 2434ms
  with part 1 - 2453ms (0.8% slower than baseline)
  with part 2 - 2468ms (1.4% slower than baseline)

There are plenty of reasons for part 2 to lose a bit.  I fixed the brokenness in push and pop by giving them _FAIL behavior; this adds instructions before and after the call to the native (about 5 instructions total, including one guard).  I can make them fast again at least for dense arrays.  Also, I fixed bug 463320 while I was in there, but not in a particularly smart way.

Part 1 I'm not so sure about.  It could be the new code in js_ExecuteTree--or something else.  I'll profile it.

Comment 19

[Jason Orendorff [:jorendorff]]

I didn't get around to profiling until just now-- and I can't reproduce the slowdown today.  :-|  I'll look again tomorrow.

Sayrer asked me for a feature: make it so that other recorder methods can also emit code to call builtins that have _FAIL behavior.  This is initially attractive but turns out to be really difficult, and wouldn't be any better for perf than what sayrer ended up doing (see bug 466781).  So I won't attempt it. _FAIL builtins must only be called via record_JSOP_CALL.

Comment 20

[Jason Orendorff [:jorendorff]]

I'm still not reproducing this slowdown.

  baseline    - 2363ms
  with part 1 - 2352ms (0.5% faster than baseline)
  with part 2 - 2372ms (0.4% slower than baseline)

I speculate that every checkin randomly changes T_SunSpider by as much as 1-2%, just by changing some magical property of the compiled code that g++ doesn't optimize, like e.g. alignment of branch targets.  If I happen to test my patches against a particularly hot revision, I see a slowdown.  Otherwise I don't.  Between comment 18 and comment 19, I rebased to tip.

Or maybe there's non-random noise on my machine that I should eliminate.  Is there an especially clean way on Mac to temporarily turn off all inessential services?  (I mean stuff like networking, the Bluetooth daemon, Growl, and Spotlight.)

Comment 21

[Brendan Eich [:brendan]]

Not sure if you have the fix for bug 472791 (tip == tracemonkey in comment 20?); Andreas found it reduced noise significantly.

/be

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 356073 [details] [diff] [review]
part 1 - v2

Again marking these r? since I can't find anything wrong with their perf.

Comment 23

[Jason Orendorff [:jorendorff]]

There is a known bug in this patch.

functionCall() takes a snapshot just before the call to a _FAIL native, capturing the pre-call state of the stack.  This snapshot is used if we deep-bail from that native.  But functionCall() does not emit a guard here.

Therefore the stack is not considered "live" at that point, and a StackFilter will remove writes to the (native) stack immediately before the call.  These writes must not be dropped; when they are dropped, and we deep-bail from the native, we copy garbage from the native stack to the interpreter stack and will probably crash.

For now I am working around this by commenting out the two StackFilters in nanojit/Assembler.cpp.  This works but we don't want to check in something like that.

Comment 24

[Brendan Eich [:brendan]]

Should I review anyway? It all looked good save nits last I scanned. Testing is the thing, clearly.

Is the snapshot without guard issue being dealt with in a separate bug?

/be

Comment 25

[Andreas Gal :gal]

I will file a blocking bug and take care of it. Brendan, go ahead and review in parallel.

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 356073 [details] [diff] [review]
part 1 - v2

>@@ -810,16 +821,23 @@ struct JSContext {
> 
>     /* Flag to indicate that we run inside gcCallback(cx, JSGC_MARK_END). */
>     JSPackedBool        insideGCMarkCallback;
> 
>     /* Exception state -- the exception member is a GC root by definition. */
>     JSPackedBool        throwing;           /* is there a pending exception? */
>     jsval               exception;          /* most-recently-thrown exception */
> 
>+    /*
>+     * Used by _FAIL builtins; see jsbuiltins.h. The builtin sets the
>+     * JSBUITLIN_BAILED bit if it bails off trace and the JSBUILTIN_ERROR bit

Typo: JSBUITL...

>+     * if an error or exception occurred. Cleared on side exit.
>+     */
>+    uint32              builtinStatus;

#ifdef JS_TRACER?

Comment might start by saying why this is early in JSContext (it is early in order to use a small immediate offset from cx, I'm guessing/hoping -- otherwise it could be at the end with the other new members).

>+
>+    /*
>+     * State for the current tree execution.  bailExit is valid if the tree has
>+     * called back into native code via a fallible builtin and has not yet
>+     * bailed, else garbage (NULL in debug builds).
>+     */
>+    JSExecuteTraceInfo  *traceRun;
>+    VMSideExit          *bailExit;

#ifdef JS_TRACER too?

> };
> 
> #ifdef JS_THREADSAFE
> # define JS_THREAD_ID(cx)       ((cx)->thread ? (cx)->thread->id : 0)
> #endif
> 
> #ifdef __cplusplus
> /* FIXME(bug 332648): Move this into a public header. */
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -1829,19 +1829,19 @@ const char js_defineGetter_str[] = "__de
> const char js_defineSetter_str[] = "__defineSetter__";
> const char js_lookupGetter_str[] = "__lookupGetter__";
> const char js_lookupSetter_str[] = "__lookupSetter__";
> #endif
> 
> JS_DEFINE_TRCINFO_1(obj_valueOf,
>     (3, (static, JSVAL, Object_p_valueOf, CONTEXT, THIS, STRING,                  0, 0)))
> JS_DEFINE_TRCINFO_1(obj_hasOwnProperty,
>-    (3, (static, BOOL_FAIL, Object_p_hasOwnProperty, CONTEXT, THIS, STRING,       0, 0)))
>+    (3, (static, BOOL_RETRY, Object_p_hasOwnProperty, CONTEXT, THIS, STRING,       0, 0)))
> JS_DEFINE_TRCINFO_1(obj_propertyIsEnumerable,
>-    (3, (static, BOOL_FAIL, Object_p_propertyIsEnumerable, CONTEXT, THIS, STRING, 0, 0)))
>+    (3, (static, BOOL_RETRY, Object_p_propertyIsEnumerable, CONTEXT, THIS, STRING, 0, 0)))

May as well line up all columns, existing lines change too.

> JS_DEFINE_CALLINFO_2(extern, BOOL,   js_EqualStrings, STRING, STRING,                       1, 1)
> JS_DEFINE_CALLINFO_2(extern, INT32,  js_CompareStrings, STRING, STRING,                     1, 1)
> 
> JS_DEFINE_TRCINFO_1(str_toString,
>-    (2, (extern, STRING_FAIL,      String_p_toString, CONTEXT, THIS,                        1, 1)))
>+    (2, (extern, STRING_FAIL,       String_p_toString, CONTEXT, THIS,                        1, 1)))
> JS_DEFINE_TRCINFO_2(str_substring,
>-    (4, (static, STRING_FAIL,      String_p_substring, CONTEXT, THIS_STRING, INT32, INT32,   1, 1)),
>-    (3, (static, STRING_FAIL,      String_p_substring_1, CONTEXT, THIS_STRING, INT32,        1, 1)))
>+    (4, (static, STRING_RETRY,      String_p_substring, CONTEXT, THIS_STRING, INT32, INT32,   1, 1)),
>+    (3, (static, STRING_RETRY,      String_p_substring_1, CONTEXT, THIS_STRING, INT32,        1, 1)))
> JS_DEFINE_TRCINFO_1(str_charAt,
>-    (3, (extern, STRING_FAIL,      js_String_getelem, CONTEXT, THIS_STRING, INT32,           1, 1)))
>+    (3, (extern, STRING_RETRY,      js_String_getelem, CONTEXT, THIS_STRING, INT32,           1, 1)))
> JS_DEFINE_TRCINFO_1(str_charCodeAt,
>-    (2, (extern, INT32_FAIL,       js_String_p_charCodeAt, THIS_STRING, INT32,               1, 1)))
>+    (2, (extern, INT32_RETRY,       js_String_p_charCodeAt, THIS_STRING, INT32,               1, 1)))
> JS_DEFINE_TRCINFO_4(str_concat,
>-    (3, (static, STRING_FAIL,      String_p_concat_1int, CONTEXT, THIS_STRING, INT32,        1, 1)),
>-    (3, (extern, STRING_FAIL,      js_ConcatStrings, CONTEXT, THIS_STRING, STRING,           1, 1)),
>-    (4, (static, STRING_FAIL,      String_p_concat_2str, CONTEXT, THIS_STRING, STRING, STRING, 1, 1)),
>-    (5, (static, STRING_FAIL,      String_p_concat_3str, CONTEXT, THIS_STRING, STRING, STRING, STRING, 1, 1)))
>+    (3, (static, STRING_RETRY,      String_p_concat_1int, CONTEXT, THIS_STRING, INT32,        1, 1)),
>+    (3, (extern, STRING_RETRY,      js_ConcatStrings, CONTEXT, THIS_STRING, STRING,           1, 1)),
>+    (4, (static, STRING_RETRY,      String_p_concat_2str, CONTEXT, THIS_STRING, STRING, STRING, 1, 1)),
>+    (5, (static, STRING_RETRY,      String_p_concat_3str, CONTEXT, THIS_STRING, STRING, STRING, STRING, 1, 1)))
> JS_DEFINE_TRCINFO_2(str_match,
>-    (4, (static, JSVAL_FAIL,       String_p_match, CONTEXT, THIS_STRING, PC, REGEXP,         1, 1)),
>-    (4, (static, JSVAL_FAIL,       String_p_match_obj, CONTEXT, THIS, PC, REGEXP,            1, 1)))
>+    (4, (static, JSVAL_FAIL,        String_p_match, CONTEXT, THIS_STRING, PC, REGEXP,         1, 1)),
>+    (4, (static, JSVAL_FAIL,        String_p_match_obj, CONTEXT, THIS, PC, REGEXP,            1, 1)))
> JS_DEFINE_TRCINFO_3(str_replace,
>-    (4, (static, STRING_FAIL,      String_p_replace_str, CONTEXT, THIS_STRING, REGEXP, STRING, 1, 1)),
>-    (4, (static, STRING_FAIL,      String_p_replace_str2, CONTEXT, THIS_STRING, STRING, STRING, 1, 1)),
>-    (5, (static, STRING_FAIL,      String_p_replace_str3, CONTEXT, THIS_STRING, STRING, STRING, STRING, 1, 1)))
>+    (4, (static, STRING_RETRY,      String_p_replace_str, CONTEXT, THIS_STRING, REGEXP, STRING, 1, 1)),
>+    (4, (static, STRING_RETRY,      String_p_replace_str2, CONTEXT, THIS_STRING, STRING, STRING, 1, 1)),
>+    (5, (static, STRING_RETRY,      String_p_replace_str3, CONTEXT, THIS_STRING, STRING, STRING, STRING, 1, 1)))
> JS_DEFINE_TRCINFO_1(str_split,
>-    (3, (static, OBJECT_FAIL_NULL, String_p_split, CONTEXT, THIS_STRING, STRING,             0, 0)))
>+    (3, (static, OBJECT_RETRY_NULL, String_p_split, CONTEXT, THIS_STRING, STRING,             0, 0)))
> JS_DEFINE_TRCINFO_1(str_toLowerCase,
>-    (2, (extern, STRING_FAIL,      js_toLowerCase, CONTEXT, THIS_STRING,                     1, 1)))
>+    (2, (extern, STRING_RETRY,      js_toLowerCase, CONTEXT, THIS_STRING,                     1, 1)))
> JS_DEFINE_TRCINFO_1(str_toUpperCase,
>-    (2, (extern, STRING_FAIL,      js_toUpperCase, CONTEXT, THIS_STRING,                     1, 1)))
>+    (2, (extern, STRING_RETRY,      js_toUpperCase, CONTEXT, THIS_STRING,                     1, 1)))

Ditto about columnation being worth the overlarge change.

>+struct JSExecuteTraceInfo
>+{

Nit: prevailing style is evolving for C++ but the left brace goes on its own only if the struct or class inherits from another.

>+    double              *stackBuffer;
>+    uintN               *inlineCallCountp;
>+    VMSideExit          **innermostNestedGuardp;
>+    JSObject            *globalObj;
>+    double              *gp;

Disparity in name length is striking -- is gp the best name?

>+    uint32 st = 0;

Suggest more canonical names: "bs" if short is important (and irreverence is a bonus), or just "status"?

>@@ -3845,22 +3915,23 @@ TraceRecorder::monitorRecording(JSContex
>         js_AbortRecording(cx, "deep abort requested");
>         return JSMRS_STOP;
>     }
> 
>     if (tr->walkedOutOfLoop()) {
>         if (!js_CloseLoop(cx))
>             return JSMRS_STOP;
>     } else {
>-        // Clear one-shot state used to communicate between record_JSOP_CALL and post-
>-        // opcode-case-guts record hook (record_FastNativeCallComplete).
>-        tr->pendingTraceableNative = NULL;
>-
>-        // In the future, handle dslots realloc by computing an offset from dslots instead.
>-        if (tr->global_dslots != tr->globalObj->dslots) {
>+    // Clear one-shot state used to communicate between record_JSOP_CALL and post-
>+    // opcode-case-guts record hook (record_FastNativeCallComplete).
>+    tr->pendingTraceableNative = NULL;
>+    tr->pendingBailExit_ins = NULL;
>+
>+    // In the future, handle dslots realloc by computing an offset from dslots instead.
>+    if (tr->global_dslots != tr->globalObj->dslots) {
>             js_AbortRecording(cx, "globalObj->dslots reallocated");
>             return JSMRS_STOP;
>         }
> 
>         jsbytecode* pc = cx->fp->regs->pc;
> 
>         /* If we hit a break, end the loop and generate an always taken loop exit guard. For other
>            downward gotos (like if/else) continue recording. */

Indentation is off here -- accidental change when merging?

>@@ -6201,16 +6273,23 @@ next_specialization:;
>         ABORT_TRACE("can't trace native constructor");
>     ABORT_TRACE("can't trace unknown constructor");
> 
> success:
> #if defined _DEBUG
>     JS_ASSERT(args[0] != (LIns *)0xcdcdcdcd);
> #endif
> 
>+    if (JSTN_ERRTYPE(known) == FAIL_STATUS) {
>+        pendingBailExit_ins = snapshot(INTERPRETER_EXIT);
>+        GuardRecord* rec = (GuardRecord *) pendingBailExit_ins->payload();
>+        JS_ASSERT(rec->exit);
>+        lir->insStorei(INS_CONSTPTR(rec->exit), cx_ins, offsetof(JSContext, bailExit));
>+    }
>+
>     LIns* res_ins = lir->insCall(known->builtin, args);

The snapshot will not include missing arguments, but that should be ok. Is this the one that's problematic still?

/be

Comment 27

[Jason Orendorff [:jorendorff]]

Bug 473880 is the new blocking bug mentioned in comment 23 and comment 25.

Thank you for the great second review--the patch needs them.

> >+    double              *stackBuffer;
> >+    uintN               *inlineCallCountp;
> >+    VMSideExit          **innermostNestedGuardp;
> >+    JSObject            *globalObj;
> >+    double              *gp;
>
> Disparity in name length is striking -- is gp the best name?

Probably not, but this is fodder for a second patch, as all these names are taken directly from the existing code.  In this patch I want to minimize the changes due to factoring out LeaveTrace.

> Indentation is off here -- accidental change when merging?

Yep.  Thanks.

> >+    if (JSTN_ERRTYPE(known) == FAIL_STATUS) {
> >+        pendingBailExit_ins = snapshot(INTERPRETER_EXIT);
> >+        GuardRecord* rec = (GuardRecord *) pendingBailExit_ins->payload();
> >+        JS_ASSERT(rec->exit);
> >+        lir->insStorei(INS_CONSTPTR(rec->exit), cx_ins, offsetof(JSContext, bailExit));
> >+    }
> >+
> >     LIns* res_ins = lir->insCall(known->builtin, args);
>
> The snapshot will not include missing arguments, but that should be ok.
> Is this the one that's problematic still?

Well, I've learned a few things since this patch.

I was using the same VMSideExit for the later guard on cx->builtinStatus.  That was a terrible idea, as the stack changes from pre-call to post-call between here and there.  I got rid of pendingBailExit_ins and it's better now.

Second, there's another bug (unrelated to bug 473880).  I add some code at the end of js_ExecuteTree for the case where we deep-bailed, called LeaveTree (which restored the interpreter stack in pre-call state, *pc==JSOP_CALL or APPLY), and continued with the native code, and the call succeeded.

If we get there, we now need to change the interpreter stack from pre-call state to post-call state.  The only hard part is copying the return value from the native stack to the interpreter stack.  To do this you need three addresses: fp->sp - 1; the corresponding address within the typemap; the corresponding address within the native stack.

The code I wrote to do this is *ahem* naive.  There are lots of cases.  Still sorting through it.  Help is welcome.

(On IRC last night, I cast doubt on the code in functionCall that stores JSVAL_BOXED, but that code is innocent--the bug is in my new code.)

Comment 28

[Jason Orendorff [:jorendorff]]

Even my code to detect whether we deep-bailed is wrong (in the case of a nested exit, we see NESTED_EXIT, not INTERPRETER_EXIT).

The right way to do this is, don't try to detect in js_ExecuteTree whether we deep-bailed or not.  Call LeaveTree unconditionally.  In LeaveTree, just detect if we already left the tree (i.e. we deep-bailed) and do different recovery if so.  LeaveTree already has all the complexity I need.

Comment 29

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v2 (both parts) to v3 (unified)

Comment 30

[Jason Orendorff [:jorendorff]]

Attachment: v3

This combines "part 1 - v2" and "part 2 - v1" with the new fixes in "interdiff v2 to v3".

Having two separate parts was bogus -- as I got deeper into it, I realized part 1 didn't quite work right without part 2.

I removed the hacks I previously added to String_p_match{,_obj} for bug 453564.  They're no longer needed now that those functions return JSVAL_FAIL.

This is a work in progress.  Much closer though.

Comment 31

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v3 to v4

Two small fixes.

1. The top-of-stack calculation had another trick up its sleeve.

2. Now that I'm calling LeaveTree twice, it needs to distinguish between the first time and the second time; hence DEEP_BAIL_EXIT and STATUS_EXIT.  We never actually emit a real guard with exit type DEEP_BAIL_EXIT. If LeaveTree sees that, we're deep bailing.

Incidentally, I think the default on-exit semantics--"don't try to extend here"--are correct for these new exit codes, but please check.

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: v4

With this and the other two patches (coming soon to bug 462042 and bug 468782), all regression tests again pass.  Trace-tests reveal two remaining bugs:

1.  Stats aren't being collected properly:

  > $JS -j trace-test.js createMandelSet
  createMandelSet : passed
  createMandelSet : passed
  ...
  recorder: started(158), aborted(97), completed(99), different header(0),
    trees trashed(0), slot promoted(0), unstable loop variable(20), breaks(0),
    returns(15), unstableInnerCalls(10)
  monitor: triggered(361438), exits(361299), type mismatch(0), global mismatch(18)

Can it be correct that aborted + completed > started?  Or triggered > exits?

2. Many tests fail.  They get the right answer but wrong stats:

  testRUNLOOPCorrectness : FAILED: expected number ( 3 )
    [recorderStarted: 1 recorderAborted: 0 traceTriggered: 1]
    != actual number ( 3 )
    [recorderStarted: 2 recorderAborted: 0 traceTriggered: 0] 

The JIT, it seems, stops working after a while.  As if to confirm this, createMandelSet brings everything to a grinding halt.

However, if I run the tests individually, they pass, even createMandelSet. A handful "fail" with very minor stats changes, like this:

  >>> ./d-objdir/js -j trace-test.js testAddAnyInconvertibleObject
  testAddAnyInconvertibleObject : FAILED: expected string ( "pass" )
    [recorderStarted: 1 recorderAborted: 0 sideExitIntoInterpreter: 93]
    != actual string ( "pass" )
    [recorderStarted: 1 recorderAborted: 0 sideExitIntoInterpreter: 92] 
  >>> ./d-objdir/js -j trace-test.js testAddInconvertibleObjectAny
  testAddInconvertibleObjectAny : FAILED: expected string ( "pass" )
    [recorderStarted: 1 recorderAborted: 0 sideExitIntoInterpreter: 93]
    != actual string ( "pass" )
    [recorderStarted: 1 recorderAborted: 0 sideExitIntoInterpreter: 92]

Comment 33

[Jeff Walden [:Waldo]]

The testAddMumbleMumble and testBitOrMumbleMumble tests, in addition to having overlarge side-exit counts per bug 471214, for some reason have different side-exit counts when run individually versus when run all together.  I'm not sure what the reason for this is, but I think those few test anomalies are best left for bug 471214 to address, unless you can demonstrate that they are different from what happens in an unpatched tree.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> They get the right answer but wrong stats:

I should note that for all but the simplest of tests some play in those sorts of numbers would be expected.  That said, testRUNLOOPCorrectness is one of these simple tests where the expected numbers really should be hit.

> A handful "fail" with very minor stats changes, like this

That one we should just adjust the stats in the test.  If the side exits jumped up a lot that would be a real fail, imo, but a change by 1 is ok.

Comment 35

[Jason Orendorff [:jorendorff]]

Found it - dumb bug in my patch for bug 462042, caused by hg qpush resolving a conflict in a crazy way the last time I rebased.

Comment 36

[Jason Orendorff [:jorendorff]]

Attachment: v5

Transposed with the patch in bug 468782, since I want to land that one first.  The only other change here is a fix for a dumb bug I introduced in v4 (cx->builtinStatus wasn't always getting cleared).

This passes all regression tests and trace-test.js.

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 357837 [details] [diff] [review]
v5

Looks great -- thanks!

/be

Comment 38

[Jason Orendorff [:jorendorff]]

Attachment: v6

Rebase on top of recent changes to typemaps.  The few parts that changed in the rebase should probably get a spot review from gal or dvander:

- the hunk in snapshot() under `if (resumeAfter)`
- the top of js_ExecuteFrame
- and particularly the top-of-stack calculations in LeaveTree, at line 4018.

v6 also renames traceRun.gp to traceRun.global, as brendan suggested a while ago.

Comment 39

[Jason Orendorff [:jorendorff]]

Attachment: v7

Er, v6 was garbage.  This adds two new tests, thanks to graydon, and simplifies the troublesome top-of-stack calculation a bit.

Comment 38 still applies, so I'm asking Andreas for review.

Comment 40

[Andreas Gal :gal]

Looks got. The LeaveTree TOS calculation is hairy. I suggest running a DEBUG browser against dep unit and see if the asserts ever get hit.

Comment 41

[Andreas Gal :gal]

Comment on attachment 359149 [details] [diff] [review]
v7

Looks good (I reviewed the interdiff to v6, not the whole thing).

Comment 42

[Jason Orendorff [:jorendorff]]

Attachment: v8

New in this rev:

- fixes needed to close bug 475761

- fix so that a debug message in MatchRegExp doesn't cause us to end up in LeaveTree. :-)

- rebase on top of recent changes to the semantics of JS_ON_TRACE.

Still to do:

- Make sure that between a deep bail and the return of a _FAIL native, any temporary gcthings used by that native are GC-reachable.  This is not true yet for e.g. the ids in {Get,Set}{Property,Element}_tn, so I think reentering there could be a problem.  I'll add a test that calls gc() from a getter or something.

Comment 43

[Andreas Gal :gal]

Attachment: v9, identical to v8, but merge JSExecuteTraceInfo into InterpState

This patch fails the same two trace-tests: FAILED: testInterpreterReentry6,testInterpreterReentry7

It is also a 120ms perf loss over TIP (just like v8).

Comment 44

[Andreas Gal :gal]

Correction. This is a 90ms perf loss. Another 30ms were lost somewhere else. Will investigate that separately.

Comment 45

[Jason Orendorff [:jorendorff]]

Quite apart from the perf issues, I discovered today that any code that allocates stack space must be avoided while on trace.  Tracking this down.

Comment 46

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v9 to v10

This adds changes to fix bug 475761.

This is in the Try Server's queue.  On my machine, `make check` aborts with an assertion that has nothing to do with me.  I can reproduce the assertion even without my patch, so I'll file that as a separate bug.  Spinning a build here for mochitesting.

Comment 47

[Jason Orendorff [:jorendorff]]

Attachment: v10

(Due to some rebasing, v10 is not strictly the sum of v9 and the interdiff.)

Comment 48

[Andreas Gal :gal]

The try-server build is still slow on fasta. Investigating.

http://hg.mozilla.org/try/rev/949044628792

whale:src gal$ ./Darwin_DBG.OBJ/js -j t/string-fasta.js  
recorder: started(17), aborted(10), completed(15), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(3), breaks(3), returns(0), unstableInnerCalls(2)
monitor: triggered(111986), exits(111986), type mismatch(0), global mismatch(0)

Comment 49

[Andreas Gal :gal]

Attachment: set pcHint inside ObjectToIterator_tn and CallIteratorNext_tn

Set pcHint in ObjectToIterator_tn and CallIteratorNext_tn. This fixes the perf regression in fasta. Performance is now identical to pre-patchset. Marking r? to remind jorendorff to merge this with his patch.

Comment 50

[Andreas Gal :gal]

Attachment: v11, with my patch merged in (fast again on fasta)

Comment 51

[Andreas Gal :gal]

interdiff is totally messed up, don't know why. I will push to the try server. Brendan's makefile mess seems to have killed the v10 try server attempt.

Comment 52

[Andreas Gal :gal]

I pushed a try a baseline TM tip and v11 to the try server:

baseline: http://hg.mozilla.org/try/rev/07be1f190a3d
v11: http://hg.mozilla.org/try/rev/8a487bfc5d2d

Comment 53

[Jason Orendorff [:jorendorff]]

interdiff is messed up because you had to rebase.

That patch (v10 to v11) stinks, but it's the expedient thing.

I'm now completely convinced that we can and should get rid of Detecting and the other bytecode inspection in jsobj.cpp.  Not this week, but soon I hope.

Comment 54

[Jesse Ruderman]

Testing 8a487bfc5d2d (pulled from the try server), jsfunfuzz found this crash [@ LeaveTree]:

var e = <x><y/></x>;
for (var j = 0; j < 4; ++j) { +[e]; }

Comment 55

[Jesse Ruderman]

That was the only problem jsfunfuzz hit in 8 hours, btw :)

Comment 56

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v11 to v12 (fixes comment 54)

Comment 57

[Jason Orendorff [:jorendorff]]

Attachment: v12

Comment 58

[Jason Orendorff [:jorendorff]]

Attachment: v13

Changing this back made bitops-bitwise-and measurably faster (barf).

 struct InterpState
 {
     double        *sp;                  // native stack pointer, stack[0] is spbase[0]
+    void          *rp;                  // call stack pointer
     double        *global;              // global frame pointer
-    void          *rp;                  // call stack pointer
     JSContext     *cx;                  // current VM context handle
     double        *eos;                 // first unusable word after the native stack
     void          *eor;                 // first unusable word after the call stack

Comment 59

[Jason Orendorff [:jorendorff]]

Attachment: v14 - unbitrotted

Patches get old fast around here!  Rebasing on top of Waldo's backout.

Comment 60

[Andreas Gal :gal]

Comment on attachment 360397 [details] [diff] [review]
v13

I suggest removing the DEBUG-specific on-trace code. Could make failures in OPT hard to debug. We might be able to eliminate the recorder special case for charAt but thats a separate bug. InterpState should probably be part of the trace monitor, thats also a separate bug.

Comment 61

[Jason Orendorff [:jorendorff]]

After a little discussion on IRC, we're keeping the DEBUG-specific code for now:

<jorendorff> gal: I have JS_ASSERT(cx->bailExit) in js_GetTopStackFrame
<jorendorff> since I am not going to null that field, the assert becomes ineffective
<gal> yeah
<gal> well leave the debug trace code in
<gal> for fuzzing
<gal> lets take it out down the road
<gal> it might help jesse finding stuff
<jorendorff> ok, in for now
<gal> I see the value of that specific case
<gal> perfect

Pushed.

http://hg.mozilla.org/tracemonkey/rev/435d0fe86a78

Comment 62

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/435d0fe86a78

Comment 63

[Bob Clary [:bc] (inactive)]

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

Comment 64

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6633e255fd7c

Comment 65

[Robert Sayre]

(In reply to comment #64)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6633e255fd7c

Wrong changeset.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b34543c2edbe

Comment 66

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2