462482 - TM: Trace apply and call

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Andreas Gal :gal]

Attachment: patch (untested, for reference)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

The attached patch traces all uses of apply and call that stay in the same interpreter. We no longer depend on the gross Aray_1str hack. However, we should probably optimize for that case. The example below produces a FastNewArray and store and a read from it. All that should go away using fairly simple escape analysis. 

var q = [];
for (var i = 0; i < 10; ++i)
    Array.prototype.push.apply(q, [5]);
print(q.join(","));

Comment 3

[Andreas Gal :gal]

Attachment: v3

Comment 4

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/1f5cc90151f5

Comment 5

[Andreas Gal :gal]

Aehm wrong bug. Sorry for the confusion. Ignore #4.

Comment 6

[Brendan Eich [:brendan]]

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

>@@ -4899,36 +4899,38 @@ js_Interpret(JSContext *cx)
>                                  * state with undefineds for all arguments we 
>                                  * didn't unpack yet, so we leave it at that. 
>                                  */
>                                 goto error;
>                             }
>                             sp++;
>                         }
>                         
>-                        goto do_call_with_specified_vp_and_argc;
>+                        TRACE_1(ApplyComplete, argc);
>+                        goto do_call_with_argc;
>                     }
>                 } else
>                     argc = 0;

Sorry I missed this -- please brace else if then has braces (if then or the if condition take multiple lines).

This seems lost down here after the big honking then clause, but it's the right order for prefetched/predicted performance. PGO would have to swap the else and then if you wrote if (argc < 2) { argc = 0; } else { ... }.

>     static JSTraceableNative knownNatives[] = {
>         { (JSFastNative)js_Array,  &js_FastNewArray_ci,   "pC", "",    FAIL_NULL | JSTN_MORE },
>         { (JSFastNative)js_Array,  &js_Array_1int_ci,     "pC", "i",   FAIL_NULL | JSTN_MORE },
>         { (JSFastNative)js_Array,  &js_Array_2obj_ci,     "pC", "oo",  FAIL_NULL | JSTN_MORE },
>         { (JSFastNative)js_Array,  &js_Array_3num_ci,     "pC", "ddd", FAIL_NULL | JSTN_MORE },
>         { (JSFastNative)js_Object, &js_FastNewObject_ci,  "fC", "",    FAIL_NULL | JSTN_MORE },
>         { (JSFastNative)js_Date,   &js_FastNewDate_ci,    "pC", "",    FAIL_NULL },
>     };
> 
>+    LIns* arg1_ins = NULL;
>+    jsval arg1 = JSVAL_VOID;

You can get rid of these now, and the code that uses them.

>+    jsval thisval = tval;

Just use tval, no need for rename.

>+
>+    /* 
>+     * If this is apply, and the argument is too long and would need
>+     * a separate stack chunk, do a heavy-weight apply. 
>+     */
>+    if (apply && argc >= 2) {
>+        if (JSVAL_IS_PRIMITIVE(vp[3]))
>+            ABORT_TRACE("arguments parameter of apply is primitive");
>+        aobj = JSVAL_TO_OBJECT(vp[3]);
>+        aobj_ins = get(&vp[3]);

Blank line here, please.

>+        /* 
>+         * We expect a dense array for the arguments (the other
>+         * frequent case is the arguments object, but that we
>+         * don't trace at the moment). 
>+         */
>+        guard(false, lir->ins_eq0(aobj_ins), MISMATCH_EXIT);
>+        if (!guardDenseArray(aobj, aobj_ins))
>+            ABORT_TRACE("arguments parameter of apply is not a dense array");

Ditto.

>+        /*
>+         * Make sure the array has the same length at runtime.
>+         */
>+        length = jsuint(aobj->fslots[JSSLOT_ARRAY_LENGTH]);
>+        guard(true, lir->ins2i(LIR_eq, 

One angel tear! :-)

>+                               stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH),
>+                               length), BRANCH_EXIT);

Nit: don't string args together and then when they overflow leave the third hiding. Break after true and underhang:

>+        guard(true,
>+              lir->ins2i(LIR_eq, stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH), length),
>+              BRANCH_EXIT);

All the lir->ins2 args fit on one line this way, bonus!

Blank line here.

>+        /* dslots must not be NULL */

Sentence for comment taking one or more lines.

>+        dslots_ins = lir->insLoad(LIR_ldp, aobj_ins, offsetof(JSObject, dslots));
>+        guard(false,
>+              lir->ins_eq0(dslots_ins),
>+              MISMATCH_EXIT);

blank line and comment nit:

>+        /* Guard array capacity */
>+        guard(true,
>+              lir->ins2(LIR_ult,
>+                        lir->insImm(length),
>+                        lir->insLoad(LIR_ldp, dslots_ins, 0 - (int)sizeof(jsval))),
>+              MISMATCH_EXIT);

(Righteous layout :-)

Blank line before major comment not starting after open {:

>+        /*
>+         * The interpreter uses a heavy-weight apply that re-enters the
>+         * interpreter if we are about to cross a stack chunk, and we
>+         * can't trace that case.

Suggest avoiding heavyweight or variations, that has a specific meaning (functions that use eval or otherwise can be analyzed by the compiler to know that they need a Call object).

Same nit applies to jsinterp.cpp code from which this was copied.

Also "The interpreter ... that re-enters the interpreter" is a bit awkward. How about:

"The interpreter deoptimizes if we are about to cross a stack chunk by re-entering itself indirectly from js_Invoke, and we can't trace that case."

...
>+    LIns* callee_ins = get(&vp[1]);
>+    LIns* this_ins = NULL;
>+    if (argc > 0) {
>+        this_ins = get(&vp[2]);
>+        if (JSVAL_IS_PRIMITIVE(vp[2]))
>+            ABORT_TRACE("apply with primitive this");
>+    } else
>+        this_ins = lir->insImm(0);

Brace else if then is braced.

Blank line before this next one would be nice:

>+    LIns** argv;
>+    if (!apply) {
>+        --argc;
>+        argv = (LIns**)alloca(sizeof(LIns*) * argc);
>+        for (jsuint n = 0; n < argc; ++n)
>+            argv[n] = get(&vp[3 + n]); /* skip over the this parameter */
>+    } else {
>+        if (argc >= 2) {
>+            /*
>+             * We already established that argments is a dense array
>+             * and we know its length and we know dslots is not NULL
>+             * and the length is not beyond the dense array's capacity. 
>+             */
>+            argc = length;
>+            argv = (LIns**)alloca(sizeof(LIns*) * argc);
>+            for (unsigned n = 0; n < argc; ++n) {
>+                /* Load the value and guard on its type to unbox it. */
>+                LIns* v_ins = lir->insLoadi(dslots_ins, n * sizeof(jsval));
>+                /* 
>+                 * We can't "see through" a hole to a possible Array.prototype property, so
>+                 * we abort here and guard below (after unboxing).
>+                 */
>+                jsval* vp = &aobj->dslots[n];
>+                if (*vp == JSVAL_HOLE)
>+                    ABORT_TRACE("can't see through hole in dense array");
>+                if (!unbox_jsval(*vp, v_ins))
>+                    return false;
>+                if (JSVAL_TAG(*vp) == JSVAL_BOOLEAN) {
>+                    // Optimize to guard for a hole only after untagging, so we know that
>+                    // we have a boolean, to avoid an extra guard for non-boolean values.
>+                    guard(false, lir->ins2(LIR_eq, v_ins, INS_CONST(JSVAL_TO_BOOLEAN(JSVAL_HOLE))),
>+                          MISMATCH_EXIT);
>+                }
>+                argv[n] = v_ins;
>+            }
>+        } else
>+            argc = 0;

Same braced-else nit as for the interpreter.

/be

Comment 7

[Andreas Gal :gal]

Attachment: v4

Comment 8

[Brendan Eich [:brendan]]

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

>+        guard(true, lir->ins2i(LIR_eq,
>+                               stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH),
>+                               length), 
>+                               BRANCH_EXIT);

Still indented oddly -- really want this:

        guard(true,
              lir->ins2i(LIR_eq, stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH), length), 
              BRANCH_EXIT);

>+ 
>+        /* 
>+         * Make sure dslots is not NULL and guard the array's capacity.
>+         */
>+        dslots_ins = lir->insLoad(LIR_ldp, aobj_ins, offsetof(JSObject, dslots));
>+        guard(false,
>+              lir->ins_eq0(dslots_ins),
>+              MISMATCH_EXIT);
>+        guard(true,
>+              lir->ins2(LIR_ult,
>+                        lir->insImm(length),
>+                        lir->insLoad(LIR_ldp, dslots_ins, 0 - (int)sizeof(jsval))),
>+              MISMATCH_EXIT);

These look good still :-).


>+        
>+        /*
>+         * The interpreter deoptimizes if we are about to cross a stack chunk by
>+         * re-entering itself indirectly from js_Invoke, and we can't trace that 
>+         * case.

Please steal the revised jsinterp.cpp version of this comment.

>+        if (argc >= 2) {
>+            /*
>+             * We already established that argments is a dense array
>+             * and we know its length and we know dslots is not NULL
>+             * and the length is not beyond the dense array's capacity. 
>+             */
>+            argc = length;
>+            argv = (LIns**)alloca(sizeof(LIns*) * argc);
>+            for (unsigned n = 0; n < argc; ++n) {
>+                /* Load the value and guard on its type to unbox it. */
>+                LIns* v_ins = lir->insLoadi(dslots_ins, n * sizeof(jsval));

Blank line here.

>+                /* 
>+                 * We can't "see through" a hole to a possible Array.prototype property, so
>+                 * we abort here and guard below (after unboxing).
>+                 */
>+                jsval* vp = &aobj->dslots[n];
>+                if (*vp == JSVAL_HOLE)
>+                    ABORT_TRACE("can't see through hole in dense array");
>+                if (!unbox_jsval(*vp, v_ins))
>+                    return false;
>+                if (JSVAL_TAG(*vp) == JSVAL_BOOLEAN) {
>+                    // Optimize to guard for a hole only after untagging, so we know that
>+                    // we have a boolean, to avoid an extra guard for non-boolean values.
>+                    guard(false, lir->ins2(LIR_eq, v_ins, INS_CONST(JSVAL_TO_BOOLEAN(JSVAL_HOLE))),

This line fits the tw=99 limit but it might be more readable to break after "false," as the other guards above do.

>+                          MISMATCH_EXIT);

r=me with these nits picked. Thanks!

/be

Comment 9

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/594ec832d9a8

Comment 10

[Andreas Gal :gal]

Backed out due to redness.

http://hg.mozilla.org/tracemonkey/rev/1b4453d693d8

Comment 11

[Andreas Gal :gal]

Attachment: refreshed patch

Comment 12

[Andreas Gal :gal]

Was landed on TM: http://hg.mozilla.org/tracemonkey/rev/d63efc717c3f

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9b3388f9465c

Comment 14

[Bob Clary [:bc] (inactive)]

test included in js1_8_1/trace/trace-test.js 
http://hg.mozilla.org/mozilla-central/rev/8f967a7729e2

Comment 15

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2