418641 - ++ and -- cases in the interpreter generates too much code

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently in the interpreter loop all the code implementing various pre- and post-increment/decrement bytecodes uses NONINT_INCREMENT_OP_MIDDLE macro to implement non-integer case of the operation. Since the macro contains a lot of code, its expansion at three places leads to bloat in js_Interpret just to support this the rarely executed case.

It would be nice to replace the macro by a function to shrink the code.

Comment 1

[Igor Bukanov]

Attachment: test case to check the correctness of ++/-- operations

The test cases covers most of ++/-- bytecodes to ensure the operation correctness. The only exception is fast global bytecodes.

Comment 2

[Igor Bukanov]

Attachment: v1

The patch moves NONINT_INCREMENT_OP_MIDDLE into separated function NonIntIncDec and reorganizes fast ++/-- cases to share more code. 

With GCC 4.3 on Fedora-9 alpha the patch shrinks the size of jsinterp.o from 54756 to 52288 or by 2468 bytes (4.5%).

Comment 3

[Igor Bukanov]

To implement the bug 419632 the code would need stronger rooting. So I will update this patch so the new implementation of ++/-- can provide all the necessary strong roots for allocating the double numbers.

Comment 4

[Igor Bukanov]

Attachment: v2

the new version makes ++/-- code not to rely on weak rooting of numbers at all so that can be switched of. For that it was necessary to have extra 2 slots on the stack so the patch added JS_TMPSLOT2. The patch also changes the meaning of the extra slots to mean an extra space besides JSCodeSpec.uses that should exist on the stack. This is the reason the patch has added JS_TMPSLOT2 for ++name-like operations despite the fact that JSCodeSpec.uses is zero and JSCodeSpec.defs is one for them. The extra slot just makes sure that the space is available after nuses. If some of it will be used for defs, then fine.

Comment 5

[Igor Bukanov]

Attachment: v3

The new version re-syncs v2 with trunk changes.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 307360 [details] [diff] [review]
v3

>-    jsval *vp, lval, rval, ltmp, rtmp;
>+    jsval *vp, lval, rval, ltmp, rtmp, limit, delta, delta2;

Suggest putting novelties in blocks, if necessary blocks around numerous "cases" (computed goto target / computed goto opcode implementations). We have some evidence (IIRC you did) that top-level locals hurt optimization, and anyway these additions are a bit too special to deserve to take up such space at the top ;-).

Same for op2.

>+                    sp[-1] = v + delta;

Instead of delta, perhaps incr or adjust? Just a thought.

>+          finish_prop_incop:
>+            if (cs->nuses == 0) {
>+                /* sp[-1] already contains the result of name increment. */
>+            } else {
>+                rtmp = sp[-1];
>+                sp -= cs->nuses;
>+                sp[-1] = rtmp;
>+            }
>+            len = cs->length;
>             DO_NEXT_OP(len);

The vaguer convention for label name is end_*op and it can be a label at the very end, or at an epilog such as this. Perhaps finish_*op is better, but end_*op is the current convention. Your call.

>+            if (JS_LIKELY(JSVAL_IS_INT(rval) && rval != limit)) {

In the rest of the code, "max" is greatest possible value, limit is one beyond or fencepost. Suggest maxval rather than limit.

>+ * Find the results of incrementing or decrementing *vp when it is expected
>+ * that it will not fit JS int. On exit for pre-increments both vp and vp2

s/JS int/jsval int/

/be

Comment 7

[Igor Bukanov]

Attachment: v4

The new version addresses the nits and restricts the range of fast inc/dec operations to [-2*29, 2**29), not (-2*30, 2**30). Such int jsvals do not require overflow checks and can be tested via:

#define CAN_DO_FAST_INC_DEC(v)     (((((v) << 1) ^ v) & 0x80000001) == 1)

With GCC it saved 97 bytes of code as the new checks avoids loading of max/min constants for overflow detection.

Comment 8

[Igor Bukanov]

Attachment: v5

The new version adds static asserts to verify that CAN_DO_FAST_INC_DEC(v) is false when v is INT_TO_JSVAL(JSVAL_INT_MIN|MAX) or JSVAL_VOID.

Comment 9

[Brendan Eich [:brendan]]

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

>+#define CAN_DO_FAST_INC_DEC(v)     (((((v) << 1) ^ v) & 0x80000001) == 1)

Good to know it not de-optimize any increment ops we care about in at least one popular benchmark ;-).

>+JS_STATIC_ASSERT(JSVAL_INT == 1);
>+JS_STATIC_ASSERT(!CAN_DO_FAST_INC_DEC(JSVAL_VOID));
>+JS_STATIC_ASSERT(!CAN_DO_FAST_INC_DEC(INT_TO_JSVAL(JSVAL_INT_MIN)));
>+JS_STATIC_ASSERT(!CAN_DO_FAST_INC_DEC(INT_TO_JSVAL(JSVAL_INT_MAX)));
>+JS_STATIC_ASSERT(!CAN_DO_FAST_INC_DEC(JSVAL_VOID));

Righteous.

>+ * Find the results of incrementing or decrementing *vp. On exit for
>+ * pre-increments both vp and vp2 will contain the result. For post-increments

"For pre-increments, both *vp and *vp2 will contain the result on return." etc.

Need caffeine and I'll do another review, just wanted to note this -- great patch AFAICT at this point. Let's optimize with r+ if it passes mochitest and js tests.

/be

Comment 10

[Igor Bukanov]

Attachment: v6

The new version addresses the nits from the comment 9.

Comment 11

[Igor Bukanov]

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

The patch shrinks the interpreter by 2.5K and allows to implement faster path for allocating double values.

Comment 12

[Igor Bukanov]

The patch from the comment 10 passed mochi tests (Linux build) and the test suite in jsshell (thread-safe debug build).

Comment 13

[Damon Sicore (:damons)]

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

a1.9+=damons

Comment 14

[Igor Bukanov]

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

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

Comment 15

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-418641.js,v  <--  regress-418641.js
initial revision: 1.1

Comment 16

[Bob Clary [:bc] (inactive)]

v 1.9.0