415455 - Faster integer conversions in the interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Igor Bukanov]

Currently the macro FETCH_INT from jsinterp.c contains the following code:

#define FETCH_INT(cx, n, i)                                                   \
    JS_BEGIN_MACRO                                                            \
        jsval v_ = FETCH_OPND(n);                                             \
        if (JSVAL_IS_INT(v_)) {                                               \
            i = JSVAL_TO_INT(v_);                                             \
        } else {                                                              \
            SAVE_SP_AND_PC(fp);                                               \
            ok = js_ValueToECMAInt32(cx, v_, &i);                             \
            if (!ok)                                                          \
                goto out;                                                     \
        }                                                                     \
    JS_END_MACRO

That is, the code calls a relatively slow js_ValueToECMAInt32 even for doubles values. 

Since the various bit manipulation bytecodes contains uses the macro, it means that a script with the corresponding bit operations would be penalized whenever the highest bit of the number would be set. Such numbers are converted to doubles in SpiderMonkey and FETCH_INT will take a slow path to access them. The same is applicable to FETCH_UINT macro.

It would be nice if SpiderMonkey would optimize the macros for the case of double values holding integers that fits 32 bits.

Comment 1

[Igor Bukanov]

Attachment: v1

The fix optimized FETCH_(INT|UINT) to check for double values. Another optimization is in jsnum.c to take a fast path for double values holding int32 numbers. That optimization assumes that when d == -0.0, then in

    int32 i;
...
    i = (int32) d;
    if ((jsdouble) i == d)
        return i;

the if check would return true. It definitely works on Linux, but I would like to know if MSVC does the proper job here as well.

Comment 2

[Igor Bukanov]

Attachment: sunspider in js shell (thread-safe build)

Here is SunSpider stats to show the difference with the patch applied. It has lines like:

md5:               1.23x as fast
sha1:              1.24x as fast  

On the other hand there is a slowdown as well. To know if this is a real slowdown (perhaps due to increased code-size of the interpreter) or just a noise, it would be interesting to know the results on other computers.

Comment 3

[Mike Schroepfer]

Yea - we'd like something like this

Comment 4

[Brendan Eich [:brendan]]

Want the win but not the loss -- should be able to pinpoint why some tests slowed down, and maybe reorder jsval tag tests. Tell me if this is the wrong thing and I will r+, but for now I'm hoping for more data and a pure win.

/be

Comment 5

[Moh Haghighat]

Attachment: Sunspider Coverage of part of jsinterp.c

Sunspider coverage of jsinterp.c is attached. Lines 243-263 show the coverage of the macro FETCH_INT. I had to convert the macro to a function and tweak it a bit for the coverage tool to show the macro coverage details. Basically, ~96.8% of the cases (~67.2M/69M) are integers, ~2.81% of the cases (2.2M/69M) are doubles, and the rest ~0.4% (285K/69M) require js_ValueToNumber call. So, roughly 3% of the cases would be affected by this change, and 87% of this 3% goes through the suggested "fast" path. The absolute value of the affected cases is not a lot, it's < 2M altogether, but the imbalance between the branch probability makes it worth further investigation. I applied the patch to an older source base that I have and got almost the same results with md5 and sha1 the top winners ~1.3x while having some slowdowns. I also moved & replicated js_DoubleToECMAInt32 above to avoid some stores/loads - not much help. I'll investigate it further.

- moh

Comment 6

[Igor Bukanov]

Comment on attachment 301136 [details] [diff] [review]
v1

A better patch will follow.

Comment 7

[Igor Bukanov]

Attachment: v2

Here is an updated patch. It tries to minimize the code bloat. In particular, it does not try to inline into the interpreter loop ToInt32(v) conversions when v is a double. Rather it changes the signatures of To(Int|Uint)32 conversion functions to return the converted value directly: 

extern int32
js_ValueToECMAInt32(JSContext *cx, jsval *vp);

extern uint32
js_ValueToECMAUint32(JSContext *cx, jsval *vp);

For error reporting the code uses *vp which is set to JSVAL_NULL when the conversion fails. This way the compiler can keep the results in the registers.

Comment 8

[Igor Bukanov]

Attachment: sunspider results (Pentium-M 1.1 GHz, Fedora-8, threadsafe jsshell)

These are the results comparing the trunk versus the effects of the patches from comment 7 and from bug 418641 comment 8. The total win is 2%.

Without the patch from bug 418641 the changes introduced in the patch for this bug adversely influence some benchmarks that never touches the opcode affected by this patch so there is no total win. The code shrink from bug 418641 removes that effect.

Comment 9

[Igor Bukanov]

Attachment: v3

The previous version has a static assert right in the middle of the interpreter loop, this version moves it before js_Interpret.

Comment 10

[Brendan Eich [:brendan]]

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

>@@ -229,25 +229,22 @@ js_GetLengthProperty(JSContext *cx, JSOb
>         *lengthp = obj->fslots[JSSLOT_ARRAY_LENGTH];
>         return JS_TRUE;
>     }
> 
>     JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>     id = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);
>     ok = OBJ_GET_PROPERTY(cx, obj, id, &tvr.u.value);
>     if (ok) {
>-        /*
>-         * Short-circuit, because js_ValueToECMAUint32 fails when called
>-         * during init time.
>-         */
>         if (JSVAL_IS_INT(tvr.u.value)) {
>             i = JSVAL_TO_INT(tvr.u.value);
>             *lengthp = (jsuint)i;       /* jsuint cast does ToUint32 */
>         } else {
>-            ok = js_ValueToECMAUint32(cx, tvr.u.value, (uint32 *)lengthp);
>+            *lengthp = (jsuint) js_ValueToECMAUint32(cx, &tvr.u.value);

Nit: no need for (jsuint) cast above, and it looks odd (though follows the more common style of space after cast) compared to the cast in the then clause.

> js_DoubleToECMAUint32(jsdouble d)
> {
>+    int32 i;
>     JSBool neg;
>-    jsdouble two32 = 4294967296.0;
>+    jsdouble two32;
> 
>-    if (!JSDOUBLE_IS_FINITE(d) || d == 0)
>+    if (!JSDOUBLE_IS_FINITE(d))
>         return 0;
> 
>+    /*
>+     * We check if d fits int32, not uint32 as all but ">>>" bit manipulation
>+     * bytecode stores the result as int, not uint. When the result does not
>+     * fit int jsval, it will be stored as a negative double.

s/check if/check whether/
s/not uint32/not uint32,/
s/all but/all but the/

r=me and go for approval1.9? with these picked. Thanks,

/be

Comment 11

[Igor Bukanov]

Attachment: v4

The new version addresses the nits from comment 10. Asking for a+ as it improves on Linux sunspider results by 2%.

Comment 12

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

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

You don't need approval, this is a blocker: land at will!

Comment 13

[Igor Bukanov]

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

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