Closed Bug 416439 Opened 15 years ago Closed 15 years ago

assignments to ok leads to extra generated code in the interpreter

Categories

(Core :: JavaScript Engine, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: memory-footprint)

Attachments

(2 files, 19 obsolete files)

v12
126.01 KB, patch
igor
: review+
Details | Diff | Splinter Review
8.18 KB, text/plain
Details
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.
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #302187 - Flags: review?(brendan)
(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. 
Is this with -Os on Mac? How about -O2 just for grins?

Cc'ing sayrer -- wondering what ICC does.

/be
(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.
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
icc -O2 stripped size:

911000 --> 906900
er, that was the dylib.

jsinterp.o

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

Does icc have an equivalent of -Os ?
jsinterp.o at -Os

70940 -> 70076
Attached patch just removing &ok (obsolete) — Splinter Review
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 on attachment 302248 [details] [diff] [review]
just removing &ok

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

70940 -> 71228
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #302187 - Attachment is obsolete: true
Attachment #302252 - Flags: review?(brendan)
Attachment #302187 - Flags: review?(brendan)
Attached patch v3 (obsolete) — Splinter Review
The new version syncs the patch with the trunk.
Attachment #302248 - Attachment is obsolete: true
Attachment #302252 - Attachment is obsolete: true
Attachment #302300 - Flags: review?(brendan)
Attachment #302252 - Flags: review?(brendan)
Attached patch v4 (obsolete) — Splinter Review
The previous versions do not handle properly returns from traps, so here is a version that does not move inline return block.
Attachment #302300 - Attachment is obsolete: true
Attachment #302344 - Flags: review?(brendan)
Attachment #302300 - Flags: review?(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
Attachment #302344 - Flags: review?(brendan)
Attachment #302344 - Flags: review+
Attachment #302344 - Flags: approval1.9+
I prefer to land this after landing the bug 416601 to have a smaller patch.
Depends on: 416601
Attached patch v5 (obsolete) — Splinter Review
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.
Attachment #302344 - Attachment is obsolete: true
Attachment #303541 - Flags: review?(brendan)
(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);
                     }

Attached patch v6 (obsolete) — Splinter Review
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.
Attachment #303541 - Attachment is obsolete: true
Attachment #303544 - Flags: review?(brendan)
Attachment #303541 - Flags: review?(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
Attachment #303544 - Flags: review?(brendan)
Attachment #303544 - Flags: review+
Attachment #303544 - Flags: approval1.9+
Attached patch v7 (merging trunk change) (obsolete) — Splinter Review
v6 required a merge with the trunk due to the latest changes in jsinterp.c. This sync does not yet address any nits.
Attachment #303544 - Attachment is obsolete: true
Attached file v6-v7 plain diff (obsolete) —
The diff shows all trivial changes to merge with the trunk.
Attached patch v7 for real (obsolete) — Splinter Review
I have not saved the updated jsinterp.c when making the previous diff.
Attachment #303690 - Attachment is obsolete: true
Attachment #303691 - Attachment is obsolete: true
Attached file v6-v7 plain diff for real (obsolete) —
Attached patch v8 (obsolete) — Splinter Review
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.
Attached patch v7-v8 delta (obsolete) — Splinter Review
Attachment #303692 - Attachment is obsolete: true
Attachment #303693 - Attachment is obsolete: true
Attached patch v9 (obsolete) — Splinter Review
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.
Attachment #303700 - Attachment is obsolete: true
Attachment #303701 - Attachment is obsolete: true
Attached patch v7-v9 delta (obsolete) — Splinter Review
Attached patch v10 (obsolete) — Splinter Review
Here is one more comment fix.
Attachment #303703 - Attachment is obsolete: true
Attachment #303705 - Attachment is obsolete: true
Attached patch v7-v10 delta (obsolete) — Splinter Review
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.
Attachment #303707 - Flags: review?(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
Attachment #303707 - Flags: review?(brendan)
Attachment #303707 - Flags: review+
Attachment #303707 - Flags: approval1.9+
Attached patch v11 (merge and addressing nits) (obsolete) — Splinter Review
The patch required a trivial merge with trunk due to jsinterp.c changes.
Attachment #303707 - Attachment is obsolete: true
Attachment #303710 - Attachment is obsolete: true
Attached file v10-v11 plain diff (obsolete) —
Attachment #304105 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 304105 [details]
v10-v11 plain diff

Setting the flags based on the previous approvals and the triviality of the merge.
Attachment #304105 - Flags: review+
Attachment #304105 - Flags: approval1.9+
Attached patch v12Splinter Review
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 */
Attachment #304104 - Attachment is obsolete: true
Attachment #304105 - Attachment is obsolete: true
Attached file v11-v12 plain diff
Attachment #304105 - Flags: approval1.9+
Attachment #304105 - Flags: review+
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.
Attachment #304195 - Flags: review+
Attachment #304195 - Flags: approval1.9+
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Yay! I have patches in flight that need adjusting, but it's great to get this in.

/be
Depends on: 418456
Depends on: 418572
Keywords: footprint
Depends on: 418614
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.