379758 - Replace JSOP_SETSP via stack and scope unwind outside the interpreter loop.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

See bug 375695 comment 12.

/be

Comment 1

[Igor Bukanov]

(In reply to comment #0)
> See bug 375695 comment 12.

That was not well-thought suggestions. setsp has to be able to recover from an unpredictable exception event. As such it is inevitably slower then specialized leaveblock, exititer etc. that transfer the stack and the scope chain from one well known state into another.

Moreover, with the patch from bug 375695 comment 10 the only purpose of setsp left is to restore the stack after the exception. As such this bytecode can be removed and the corresponding implementation moved to the part of the interpreter that prepares for the exception capturing. The only non-trivial change for that would be to record the stack depth in the exception table instead of inserting it into setsp bytecode and alter the decompiler correspondingly. 

Moreover, such arrangement allows to address the bug 349326 and insert the missing enditer on exceptions without any slowdown during normal execution. For that the exception table can be extended with information about the iterators so during the search for the exception handler the interpreter can also discover the pending enditer calls.


 Moreover, In this way it would be also easy to implement missing calls to enditer without any overhead for non-exception case.    

Comment 2

[Igor Bukanov]

Changing the title to reflect the intention.

Comment 3

[Igor Bukanov]

Attachment: implementation v1

Summary of changes:

1. SET_SP is replaced by stack/scope unwind during exception capturing. To support that I added stackDepth member to JSTryNote.

2. The exception capturing code now pushes (true, exception) pair to the stack before transfering control to the finally. It allowed to remove [gosub] that currently is inserted before finally and simplify [gosub] and [gosubx] that now just push (false, pc-index) pair. The pair format replaces the current (exception, 0)/(hole, pc-index) as I think it is easy to follow.

3. the patch adds JSTryNote.kind so the exception capturing code can quickly distinguish between finally and catch cases. The member is JSTryNoteKind enum with 2 values, JSTN_FINALLY and JSTN_CATCH, not a simple boolean flag, to anticipate the changes to fix bug 349326 which would add something like JSTN_ITER to the enum.  

4. The patch removes JSTryNote.catchStart as the above changes make finally bytecode to follow immediately try/catchEnd for try-catch-finally case or try/tryEnd for try-finally case. In this way both catch and finally begins immediately after the area they protect and the try note end is the handler start.

5. To safe space required by try notes and removes the code that counts them in few places, they are allocated as [length, notes[length]] JSTryNoteArray struct, not as notes[length] notes_terminator since notes_terminator takes more space then length.

6. There is a cleanup in jsscript.c to avoid code duplication when allocating JSScript and initializing JSScript.tryNotes.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 264685 [details] [diff] [review]
implementation v1

>+    fprintf(gOutFile, "\nException table:\n"
>+            "kind      stack  start  end\n");
>+
>+    tn = script->trynotes->notes;
>+    tnlimit = tn + script->trynotes->length;
>+    do {
>+        kind = (tn->kind == JSTN_FINALLY)
>+               ? "finally"
>+               : (JS_ASSERT(tn->kind == JSTN_CATCH), "catch");
>+        fprintf(gOutFile, " %-10s%-7u%-7u%-7u\n",

Good to lose the tabs (;-) but I would leave a space between each format specifier in case of a large number. Also, historically Unix tools and I agree that %-ns formats should be for strings, and numbers should be right justified so you can align less significant digits across rows.

>         /* Emit finally handler if any. */
>+        finallyStart = 0;   /* to quell GCC unitilized warnings */

Typo: "unitilized"

>             /*
>-             * The stack budget must be balanced at this point.  All [gosub]
>-             * calls emitted before this point will push two stack slots, one
>-             * for the pending exception (or JSVAL_HOLE if there is no pending
>-             * exception) and one for the [retsub] pc-index.
>+             * The stack depth at the begining of finally must match the try
>+             * depth plus 2 slots. The interpreter uses these two slots to
>+             * either push (true, exception) pair when it transfer control to

"when it transfers"

Might also write "control flow" instead of "control" and wrap nicely.

>+             * the finally after capturing an exception or to push

Put a comma before " or to push".

>+             * (false, pc-index) when it calls finally from [gosub]. The first
>+             * element of the pair indicates for [retsub] that it should
>+             * either rethrow the pending exception or transfer the control
>+             * back to the caller of finally.

This reminds me that JUMPX_OFFSET_{MIN,MAX} should be altered to reflect the encoding of a jump offset as an int jsval (this seems better to me than enforcing lower bounds only for gosub/gosubx emission).

> out:
>+    JS_ASSERT(script->code <= pc);
>+    JS_ASSERT(pc < script->code + script->length);

I doubt these will botch, but you could combine them using JS_UPTRDIFF. But if one or the other could botch it would be nicer to know which way pc was out of bounds, so this comes down to source lines spent on assertions vs. ease of debugging a rare case.

> #if JS_HAS_GENERATORS
>-            if (JS_LIKELY(cx->exception != JSVAL_ARETURN)) {
>-                SCRIPT_FIND_CATCH_START(script, pc, pc);
>-                if (!pc)
>-                    goto no_catch;
>-            } else {
>-                pc = js_FindFinallyHandler(script, pc);
>-                if (!pc) {
>-                    cx->throwing = JS_FALSE;
>-                    ok = JS_TRUE;
>-                    fp->rval = JSVAL_VOID;
>+                    /* Catch can not intercept closing of a generator. */

Nit: "cannot"

>+            /*
>+             * The catch or finally begines right after the code they protect.

Typo: "begines"

>+             */
>+            pc = (script)->main + tn->start + tn->length;
>+
>+            /*
>+             * When failed during the scope recovery, restart the exception

"When failing"

>+
>+/*
>+ * JSTryNoteArray is allocated after script notes and an extra gap to ensure
>+ * that JSTryNoteArray is alligned on sizeof(uint32) boundary, the maximum
>+ * size of JSTryNoteArray.length and JSTryNote fields.
>+ */
>+JS_STATIC_ASSERT(sizeof(JSTryNote) == 3 * sizeof(uint32));
>+JS_STATIC_ASSERT(sizeof(JSTryNoteArray) == 4 * sizeof(uint32));

Nit: blank line here for legibility.

>+#define JSTRYNOTE_ALIGNMASK     (sizeof(uint32) - 1)
[snip]
>+        uint8  kind = tn->kind;
>+        uint16 stackDepth = tn->stackDepth;
>+        uint32 start = tn->start;
>+        uint32 length = tn->length;
>+
>+        if (!JS_XDRUint8(xdr, &kind) ||
>+            !JS_XDRUint16(xdr, &stackDepth) ||

These pad to 32 bit unsigned units in the XDR stream, so could be combined -- or to future-proof, explicitly zero-extend to uint32 temps and XDR those. I say combine, we're future-proof via JSXDR_BYTECODE_VERSION ;-).


>+/*
>+ * Exception handling record.
>  */
> struct JSTryNote {
>-    ptrdiff_t    start;         /* start of try statement */
>-    ptrdiff_t    length;        /* count of try statement bytecodes */
>-    ptrdiff_t    catchStart;    /* start of catch block (0 if end) */
>+    JSTryNoteKind   kind: 8;
>+
>+    /* Stack depth upon exception handler entry. */
>+    uint16          stackDepth;

I hear roc's blog (http://weblogs.mozillazine.org/roc) talked about how MSVC will not pack a signed with an unsigned int, and I bet it won't pack enum (signed int) with unsigned either. Suggest you use uint8 for kind and add an explicit uint8 padding; member.

/be

Comment 5

[Igor Bukanov]

Attachment: implementation v2

Here is a new patch to address the nits and suggestions from the previous comment.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 264858 [details] [diff] [review]
implementation v2

>+JS_STATIC_ASSERT(JSTN_CATCH == 0);
>+JS_STATIC_ASSERT(JSTN_FINALLY == 1);
>+static const char* const TryNoteNames[] = { "catch", "finally" };

Same nit as the last time in the other place: blank line between static assertions and a data definition.

>-        /* Count the trynotes. */
>-        if (script->trynotes) {
>-            while (script->trynotes[ntrynotes].catchStart)
>-                ntrynotes++;
>-            ntrynotes++;        /* room for the end marker */
>-        }
>+        ntrynotes = script->trynotes ? script->trynotes->length : 0;
>     }
> 
>     if (!JS_XDRUint32(xdr, &length))
>         return JS_FALSE;
>     if (magic >= JSXDR_MAGIC_SCRIPT_2) {
>         if (!JS_XDRUint32(xdr, &prologLength))
>             return JS_FALSE;
>@@ -645,61 +688,59 @@ js_XDRScript(JSXDRState *xdr, JSScript *
>             script->filename = filename;
>             filenameWasSaved = JS_TRUE;
>         }
>         script->lineno = (uintN)lineno;
>         script->depth = (uintN)depth;
> 
>         if (magic < JSXDR_MAGIC_SCRIPT_4) {

This reminds me: although we use JSXDR_BYTECODE_VERSION to invalidate any saved fastload files (actually IIRC they're thrown away at the drop of a hat, so we have multiple layers of defense), some SpiderMonkey embedding that XDRs (many do) may fail to do that, relying on JSXDR_MAGIC_SCRIPT_n instead. We could add _5, which is pure pain and contorts the code.

Probably the right answer here is a release note for JS1.7. Bclary, should we file a bug to track that?


>+        /*
>+         * We combine kind and stackDepth when serializing as XDR is not
>+         * efficient when serializing small integer types.

Just like Sun XDR, btw.

>+         */
>+        JSTryNote *tn = &script->trynotes->notes[--ntrynotes];
>+        uint8  kind = tn->kind;
>+        uint16 stackDepth = tn->stackDepth;
>+        uint32 kindAndDepth = ((uint32)kind << 16) | (uint32)stackDepth;
>+        uint32 start = tn->start;
>+        uint32 length = tn->length;
>+
>+        if (!JS_XDRUint32(xdr, &kindAndDepth) ||
>+            !JS_XDRUint32(xdr, &start) ||
>+            !JS_XDRUint32(xdr, &length)) {
>             goto error;
>         }
>-        tn->start = (ptrdiff_t) start;
>-        tn->length = (ptrdiff_t) catchLength;
>-        tn->catchStart = (ptrdiff_t) catchStart;
>+        tn->kind = (uint8)(kindAndDepth >> 16);
>+        tn->stackDepth = (uint16)kindAndDepth;
>+        tn->start = start;
>+        tn->length = length;

Why use start and length temps instead of just XDR'ing &tn->start and &tn->length? That's the symmetric pattern that XDR favors to avoid mismatched encode/decode bugs.

r=me with this and the nit addressed.

/be

Comment 7

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

(In reply to comment #6)
> >+         * We combine kind and stackDepth when serializing as XDR is not
> >+         * efficient when serializing small integer types.
> 
> Just like Sun XDR, btw.

Yeah! Totally not my fault.

Comment 8

[Igor Bukanov]

Attachment: implementation v3

The new patch address the nits from comment 6. In the patch I use the JS_STATIC_ASSERT to record the assumptions about JSTryNote.kind and JSTryNote.stackDepth. But such usage is not supported with MSVC IIRC and the current definition of the macro. So the patch tries another approach.

Hopefully MSVC likes it better. If not, I will remove the static assert and restore the macro to the previous state.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> MSVC will not pack a signed with an unsigned int, and I bet it won't pack
> enum (signed int) with unsigned either. Suggest you use uint8 for kind and
> add an explicit uint8 padding; member.

Close -- it packs types which are the same, ignoring signedness; see bug 374988 comment 3.  I'm in the process of getting this added to the portability docs.

Also, the type of an enum isn't guaranteed to be a signed integer, at least in C99 6.7.2.2 paragraph 4 (seems thus unlikely to have been that way in C89):

> Each enumerated type shall be compatible with char, a signed integer type,
> or an unsigned integer type. The choice of type is implementation-defined,
> but shall be capable of representing the values of all the members of the
> enumeration.

Note the "choice of type is implementation-defined"; the compiler could optimize to char here if it wanted.  Thus, having a bitfield of an enum is theoretically not safely packable with anything but another bitfield of the same enum, if we accept that the packing bug exists in the first place.

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)
>
> Probably the right answer here is a release note for JS1.7. Bclary, should we
> file a bug to track that?

bug 380901 tracks releasing SpiderMonkey 1.7. If someone can give me the text, I can make sure it is added to the release notes.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 264946 [details] [diff] [review]
implementation v3

Looks good, hope MSVC likes it.

/be

Comment 12

[Igor Bukanov]

Attachment: implementation v4

This is the previous patch without JS_STATIC_ASSERT changes as that was done as a part of bug 381236.

Comment 13

[Igor Bukanov]

I committed the patch from comment 12 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.143; previous revision: 3.142
done
Checking in jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.90; previous revision: 3.89
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.251; previous revision: 3.250
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.75; previous revision: 3.74
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.219; previous revision: 3.218
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.348; previous revision: 3.347
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.245; previous revision: 3.244
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.94; previous revision: 3.93
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.143; previous revision: 3.142
done
Checking in jsscript.h;
/cvsroot/mozilla/js/src/jsscript.h,v  <--  jsscript.h
new revision: 3.35; previous revision: 3.34
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.30; previous revision: 1.29
done

Comment 14

[Josh Aas]

This seems to have caused a whole bunch of random mochitest failures. I backed it out locally and the failures went away.

Comment 15

[Josh Aas]

I backed this out to fix mochitests.

Comment 16

[Josh Aas]

Actually, the backout failed because I can't check in to js. Leaving reopened until this is resolved.

Comment 17

[Igor Bukanov]

(In reply to comment #14)
> This seems to have caused a whole bunch of random mochitest failures. I backed
> it out locally and the failures went away.

Can I have a log of the failures?

Comment 18

[Josh Aas]

I don't have the log any more, but among other things the FUEL tests were failing sometimes.  The tbox unit tests went bad first, I reproduced locally, then backing out this patch fixed them. Now the tboxes are fine, so who knows...

Comment 19

[Igor Bukanov]

I looked at failed mochitests, and the failures started even before I committed the patch. For example, exactly the same number test failed before I committed the patch as with after the patch with the same AFAICS errors. To witness compare  the log for "qm-rhel02 dep" tinderbox before and after the commit:

Before commit:

L- C
TUnit
245/3
reftest
601/0/46
mochitest
23178/0/2132
chrome
32/0/0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1179991920.7293.gz

After the commit:
L- C
TUnit
247/3
reftest
601/0/46
mochitest
23178/0/2132
chrome
32/0/0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1179993743.13272.gz

The same tests continue to fail later. I also looked at tparticular failures, and they happens on the code paths that were not affected by the patch. Thus I mark this as fixed.