416439 - assignments to ok leads to extra generated code in the interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently the interpreter uses a pattern like:

ok = action();
if (!ok)
    goto out;

With GCC on x86 this leads to suboptimal code. In particular, replacing that with

if (!action())
   goto error;

can shrink the size of js_Interpret by at least 1K as the following patch demonstrates for the thread-safe optimized with -Os build of jsinterpr.c. it would be nice to switch the interpreter to this more efficient pattern.

Comment 1

[Igor Bukanov]

Attachment: v1

Under GCC 4.1.2 for Linux with a thread-safe optimized build the patch shrinks the code size of js_Interpret from by 1257 bytes from 45219 to 43962 making the code for the function smaller by the factor of 2.8%.

I will test the patch more before asking for a review.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Created an attachment (id=302187) [details]
> v1

The patch consists mostly of replacements like

ok = action();
if (!ok)
    goto out;

->

if (!action())
    goto error;

In addition there are the following changes:

1. The inline return code is moved towards the end of js_Interpret for lesser number of jumps.

2. The application hook called from the exception handling code is called with the address of a temporary boolean, not with the address of "ok". That change alone shrinks the code by 200 bytes or so. It seems with the address of &ok leaked GCC assume that any function call can access/modify it.

3. I renamed the label "out" as "exit" to make sure that goto out in any pending patches would trigger compilation error.

4. The are few other code movements to minimize to shrink the code. 

Comment 3

[Brendan Eich [:brendan]]

Is this with -Os on Mac? How about -O2 just for grins?

Cc'ing sayrer -- wondering what ICC does.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Is this with -Os on Mac? How about -O2 just for grins?

The numbers where from -Os, the default now days for SpiderMonkey.

Comment 5

[Igor Bukanov]

Here is the data for the size of js_Interpret code in bytes for a thread-safe optimized build of jsinterp.o compiled with GCC 4.1.2 with -O2:

before  after   shrink factor
52256   50640   1616   1.03

Comment 6

[Robert Sayre]

icc -O2 stripped size:

911000 --> 906900

Comment 7

[Robert Sayre]

er, that was the dylib.

jsinterp.o

86032 -> 83120

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> icc -O2 stripped size:

Does icc have an equivalent of -Os ?

Comment 9

[Robert Sayre]

jsinterp.o at -Os

70940 -> 70076

Comment 10

[Igor Bukanov]

Attachment: just removing &ok

This is small patch to measure the cost of &ok in the interpreter. The numbers at -O2 for the size of js_Interpret:

trunk  small  full   
52256  52137  50640

To Robert: can you try this patch with icc -Os as well?

Comment 11

[Robert Sayre]

Comment on attachment 302248 [details] [diff] [review]
just removing &ok

This patch, "just removing &ok" increases code size for icc -Os.

70940 -> 71228

Comment 12

[Igor Bukanov]

Attachment: v2

The new patch removes that hack not pass &ok to the hook as with the other changes that has no influence on the code size. Plus the patch moves if (!ok) code to re-enable the cache after errors under the error label to avoid taking an extra check.

Comment 13

[Igor Bukanov]

Attachment: v3

The new version syncs the patch with the trunk.

Comment 14

[Igor Bukanov]

Attachment: v4

The previous versions do not handle properly returns from traps, so here is a version that does not move inline return block.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 302344 [details] [diff] [review]
v4

Looks good to me. A bit worrying that we used to set ok to true all over, and counted on that -- but I suppose GCC would warn if its tiny brain detected any hint of use before set?

Suggest last_exit or final_exit instead of exit2, just as a nicer name (not that I've been so nice in the past ;-).

Best to get this in quickly, but first: does it build and run the suite on both Mac or Linux *and* Windows (which lacks computed goto)?

/be

Comment 16

[Igor Bukanov]

I prefer to land this after landing the bug 416601 to have a smaller patch.

Comment 17

[Igor Bukanov]

Attachment: v5

The new patch is adjusted for the changes from bug 416601. In addition the patch makes sure that fp->sp is always initialized to have simpler logic in js_TraceFrame and to improve assert coverage. Plus js_PutBlockObjects modifies the scope chain itself for smaller code.

Comment 18

[Igor Bukanov]

(In reply to comment #15)
> (From update of attachment 302344 [details] [diff] [review])
> Looks good to me. A bit worrying that we used to set ok to true all over, and
> counted on that -- but I suppose GCC would warn if its tiny brain detected any
> hint of use before set?

To make sure that GCC warns about v5 contains the following:

                     if (hook) {
+                        /*
+                         * Do not pass &ok directly as exposing the address
+                         * inhibits optimizations and uninitialised warnings.
+                         */
                         SAVE_SP_AND_PC(fp);
-                        hook(cx, fp, JS_FALSE, &ok, hookData);
+                        status = ok;
+                        hook(cx, fp, JS_FALSE, &status, hookData);
+                        ok = status;
                         LOAD_INTERRUPT_HANDLER(cx);
                     }

Comment 19

[Igor Bukanov]

Attachment: v6

The new version fixes an existing DEBUG-only bug when the code has accessed op to report the tracing info before it could be initialized. This was reported by GCC when compiling with JS_THREADED_INTERP defined to 0. Here is a delta:

--- /home/igor/m/ff/mozilla/quilt.v31313/js/src/jsinterp.c	2008-02-15 17:47:13.000000000 +0100
+++ js/src/jsinterp.c	2008-02-15 17:47:00.000000000 +0100
@@ -6443,16 +6443,21 @@ interrupt:
     advance_pc:
         pc += len;
 
 #ifdef DEBUG
         if (tracefp) {
             intN ndefs, n;
             jsval *siter;
 
+            /*
+             * op may be invalid here when a catch or finally handler jumps to
+             * advance_pc.
+             */
+            op = pc[-len];
             ndefs = js_CodeSpec[op].ndefs;
             if (ndefs) {
                 SAVE_SP_AND_PC(fp);
                 if (op == JSOP_FORELEM && sp[-1] == JSVAL_FALSE)
                     --ndefs;
                 for (n = -ndefs; n < 0; n++) {
                     char *bytes = js_DecompileValueGenerator(cx, n, sp[n],
                                                              NULL);

The test suite passes with the patch in both JS_THREADED_INTERP=0,1 cases.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 303544 [details] [diff] [review]
v6

>           BEGIN_CASE(JSOP_STOP)
>+            /*
>+             * When the inlined frame exist with an exception or an error,

s/exist/exists/

>@@ -6696,17 +6563,17 @@ interrupt:
>             ok = UnwindScope(cx, fp, tn->stackDepth, JS_TRUE);
>             JS_ASSERT(fp->sp == fp->spbase + tn->stackDepth);
>             sp = fp->sp;

RESTORE_SP(fp);

>     ok &= UnwindScope(cx, fp, 0, ok || cx->throwing);
>     JS_ASSERT(fp->sp == fp->spbase);
>     sp = fp->sp;

RESTORE_SP(fp);

>+     * We must not be in an inline frame. The check above ensures that
>      * for the error case and for a normal return the code jumps directly to
>      * parent's frame pc.

Rewrap this comment as if by Q2j in vim when cursor is on first line.

r/a=me with these.

/be

Comment 21

[Igor Bukanov]

Attachment: v7 (merging trunk change)

v6 required a merge with the trunk due to the latest changes in jsinterp.c. This sync does not yet address any nits.

Comment 22

[Igor Bukanov]

Attachment: v6-v7 plain diff

The diff shows all trivial changes to merge with the trunk.

Comment 23

[Igor Bukanov]

Attachment: v7 for real

I have not saved the updated jsinterp.c when making the previous diff.

Comment 24

[Igor Bukanov]

Attachment: v6-v7 plain diff for real

Comment 25

[Igor Bukanov]

Attachment: v8

This version addresses the nits from comment 20 and moves newsp local into the block that starts the inlined call. That variable is not necessary for the initial stack initialization.

Comment 26

[Igor Bukanov]

Attachment: v7-v8 delta

Comment 27

[Igor Bukanov]

Attachment: v9

The new version fixes pre-existing stalled comments that refers to nested invocations of js_Interpret by the xml-filter. Since the bug 309894 was fixed this is no longer applicable.

Comment 28

[Igor Bukanov]

Attachment: v7-v9 delta

Comment 29

[Igor Bukanov]

Attachment: v10

Here is one more comment fix.

Comment 30

[Igor Bukanov]

Attachment: v7-v10 delta

Comment 31

[Igor Bukanov]

Comment on attachment 303707 [details] [diff] [review]
v10

The number of changes due to merge (attachment 303693 [details] shows the difference) and fixes besides addressing the nits (see attachment 303710 [details] [diff] [review]) makes necessary to ask for another review.

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 303707 [details] [diff] [review]
v10

>+     * We must not be in an inline frame. The check above ensures that for the
>+     * error case and for a normal return the code jumps directly to parent's
>+     * frame pc.

Comma after "normal return" before "the code jumps directly to ...".

Thanks for the interdiff and trailing whitespace cleanup -- r/a=me with the comma nit picked.

/be

Comment 33

[Igor Bukanov]

Attachment: v11 (merge and addressing nits)

The patch required a trivial merge with trunk due to jsinterp.c changes.

Comment 34

[Igor Bukanov]

Attachment: v10-v11 plain diff

Comment 35

[Igor Bukanov]

Comment on attachment 304105 [details]
v10-v11 plain diff

Setting the flags based on the previous approvals and the triviality of the merge.

Comment 36

[Igor Bukanov]

Attachment: v12

Here is another merge with the trunk. The previous patch failed at one hunk that required to adjust it so that part of the patch looks like:

--- /home/igor/m/ff/mozilla/quilt.OKGnoW/js/src/jsinterp.c	2008-02-19 11:24:58.000000000 +0100
+++ js/src/jsinterp.c	2008-02-19 11:22:12.000000000 +0100
@@ -4179,37 +4179,32 @@ interrupt:
 #if JS_HAS_XML_SUPPORT
                 /* Special-case XML object method lookup, per ECMA-357. */
                 if (OBJECT_IS_XML(cx, obj)) {
                     JSXMLObjectOps *ops;
 
                     ops = (JSXMLObjectOps *) obj->map->ops;
                     obj = ops->getMethod(cx, obj, id, &rval);
                     if (!obj)
-                        ok = JS_FALSE;
+                        goto error;
                 } else
 #endif
-                if (JS_LIKELY(aobj->map->ops->getProperty == js_GetProperty)) {
-                    ok = js_GetPropertyHelper(cx, aobj, id, &rval, &entry);
-                } else {
-                    ok = OBJ_GET_PROPERTY(cx, obj, id, &rval);
+                if (JS_LIKELY(aobj->map->ops->getProperty == js_GetProperty)
+                    ? !js_GetPropertyHelper(cx, aobj, id, &rval, &entry)
+                    : !OBJ_GET_PROPERTY(cx, obj, id, &rval)) {
+                    goto error;
                 }
-                if (!ok)
-                    goto out;
                 STORE_OPND(-1, OBJECT_TO_JSVAL(obj));
                 STORE_OPND(-2, rval);
-                if (!ComputeThis(cx, JS_FALSE, sp)) {
-                    ok = JS_FALSE;
-                    goto out;
-                }
+                if (!ComputeThis(cx, JS_FALSE, sp))
+                    goto error;
             } else {
                 JS_ASSERT(obj->map->ops->getProperty == js_GetProperty);
-                ok = js_GetPropertyHelper(cx, obj, id, &rval, &entry);
-                if (!ok)
-                    goto out;
+                if (!js_GetPropertyHelper(cx, obj, id, &rval, &entry))
+                    goto error;
                 STORE_OPND(-1, lval);
                 STORE_OPND(-2, rval);
             }
 
           end_callname:
             /* Wrap primitive lval in object clothing if necessary. */
             if (JSVAL_IS_PRIMITIVE(lval)) {
                 /* FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=412571 */

Comment 37

[Igor Bukanov]

Attachment: v11-v12 plain diff

Comment 38

[Igor Bukanov]

Comment on attachment 304195 [details] [diff] [review]
v12

Setting the flags based on triviality of the merge of the already approved patch with the trunk.

Comment 39

[Igor Bukanov]

I checked in the patch from comment 36 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1203423318&maxdate=1203423421&who=igor%25mir2.org

Comment 40

[Brendan Eich [:brendan]]

Yay! I have patches in flight that need adjusting, but it's great to get this in.

/be