462265 - TM: Perform .apply() in the interpreter loop bypassing js_Invoke()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andreas Gal :gal]

Attachment: patch

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 345394 [details] [diff] [review]
patch

>           BEGIN_CASE(JSOP_APPLY)
>             argc = GET_ARGC(regs.pc);
>             vp = regs.sp - (argc + 2);
>+            lval = *vp;
>+            if (!VALUE_IS_FUNCTION(cx, lval)) 
>+                goto do_call;
>+            obj = JSVAL_TO_OBJECT(lval);
>+            fun = GET_FUNCTION_PRIVATE(cx, obj);
>+            if (FUN_INTERPRETED(fun))
>+                goto do_call;
>+            if ((JSFastNative)fun->u.n.native != js_fun_apply &&
>+                (JSFastNative)fun->u.n.native != js_fun_call) 
>+                goto do_call;

Brace if condition takes more than one line, but see next point that may save bracing here:

>+            bool apply = (JSFastNative)fun->u.n.native == js_fun_apply;

Move apply up so you can test if (!apply && (JSFastNative)fun->u.n.native != js_fun_call) on one line.

Brace the case (half-indented) so bool apply does not leak into the rest of the interpreter, or else use the top-level cond variable instead.

>+            obj = JS_THIS_OBJECT(cx, vp);
>+            if (!obj || !OBJ_DEFAULT_VALUE(cx, obj, JSTYPE_FUNCTION, &vp[1]))
>+                goto error;
>+            jsval fval = vp[1];

Don't add more locals, at least not without bracing to minimize scope. You could use rval here, for instance.

>+                    

Trailing whitespace makes angels cry.

>+            if (!VALUE_IS_FUNCTION(cx, fval)) {
>+                str = JS_ValueToString(cx, fval);
>+                if (str) {
>+                    const char *bytes = js_GetStringBytes(cx, str);
>+
>+                    if (bytes) {
>+                        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                             JSMSG_INCOMPATIBLE_PROTO,
>+                                             js_Function_str, "apply",
>+                                             bytes);
>+                    }
>+                }
>+                goto error;
>+            }

This might be worth writing a common js_ReportNotApplicable or some such, living in jsfun.cpp for use here and in js_fun_apply.

>+                    

More angel tears.

>+            if (argc == 0) {
>+                /* Call fun with its global object as the 'this' param if no args. */
>+                obj = NULL;
>+            } else {
>+                /* Convert the first arg to 'this'. */
>+                if (!JSVAL_IS_PRIMITIVE(vp[2]))
>+                    obj = JSVAL_TO_OBJECT(vp[2]);
>+                else if (!js_ValueToObject(cx, vp[2], &obj))
>+                    goto error;
>+            }
>+
>+            if (!apply) {
>+                /* If we have a 1st arg, copy the rest as arguments */
>+                if (argc > 0) 
>+                    memmove(vp + 2, vp + 3, (argc - 1) * sizeof *vp);

From this point until the bottom of this hunk, obj is potentially unrooted (vp[2] used to reference it).

>+            } else {
>+                if (argc > 1) { /* Have to expand arguments? */
>+                    JSObject *aobj = NULL;
>+                    /* If the 2nd arg is null or void, call the function with 0 args. */
>+                    if (JSVAL_IS_NULL(vp[3]) || JSVAL_IS_VOID(vp[3])) {
>+                        argc = 0;
>+                    } else {
>+                        /* The second arg must be an array (or arguments object). */
>+                        JSBool arraylike = JS_FALSE;
>+                        jsuint length = 0;
>+                        if (!JSVAL_IS_PRIMITIVE(vp[3])) {
>+                            aobj = JSVAL_TO_OBJECT(vp[3]);
>+                            if (!js_IsArrayLike(cx, aobj, &arraylike, &length))
>+                                goto error;
>+                        }
>+                        if (!arraylike) {
>+                            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                                     JSMSG_BAD_APPLY_ARGS, "apply");
>+                            goto error;
>+                        }

I bet after this point, using aobj gets you GCC whinage about may be used before set.

>+                        jsuint apply_argc = argc;
>+                        argc = (uintN)JS_MIN(length, ARRAY_INIT_LIMIT - 1);
>+                            
>+                        /* Make room for another missing arguments to the right. */
>+                            
>+                        /* Allocate missing expected args adjacent to actuals. */
>+                        JSArena* a = cx->stackPool.current;
>+                        if (argc > apply_argc) {
>+                            jsval* newsp = vp + 2 + argc;
>+                            JS_ASSERT(newsp > regs.sp);
>+                            if ((jsuword) newsp <= a->limit) {
>+                                if ((jsuword) newsp > a->avail)
>+                                    a->avail = (jsuword) newsp;
>+                            } else {
>+                                /* Overflowing the current arena, get a new one. */
>+                                jsuword nbytes = (2 + argc) * sizeof(jsval);
>+                                JS_ARENA_ALLOCATE_CAST(newsp, jsval *, &cx->stackPool,
>+                                                       nbytes);
>+                                if (!newsp) {
>+                                    js_ReportOutOfScriptQuota(cx);
>+                                    goto bad_inline_call;
>+                                }
>+                                    

Angels.

>+                                vp = newsp;

Update regs.sp somewhere to cover everything.

>+                            }
>+                            JS_ASSERT((jsuword)(vp + 2 + argc) < a->limit);
>+                        }
>+
>+                        /* Expand array content onto the stack. */
>+                        jsval* sp = vp + 2;
>+                        for (jsuint i = 0;

You have top level jsint i in js_Interpret -- do not shadow it.

>+                                           i < argc; i++) {
>+                            ok = JS_GetElement(cx, aobj, (jsint)i, sp);
>+                            if (!ok)
>+                                goto error;
>+                            sp++;
>+                        }
>+                    }
>+                }
>+            }
>+                 
>+            vp[0] = fval;
>+            vp[1] = OBJECT_TO_JSVAL(obj);
>+

You could lose this blank line. I hope obj is protected in the apply case from premature GC.

>+            goto do_call_with_specified_vp_and_argc;
>+          
>+          BEGIN_CASE(JSOP_CALL)
>+          BEGIN_CASE(JSOP_EVAL)
>+          do_call:
>+            argc = GET_ARGC(regs.pc);
>+            vp = regs.sp - (argc + 2);

Blank line here would be best.

>+          do_call_with_specified_vp_and_argc:
>             lval = *vp;
>             if (VALUE_IS_FUNCTION(cx, lval)) {
>                 obj = JSVAL_TO_OBJECT(lval);
>                 fun = GET_FUNCTION_PRIVATE(cx, obj);
> 
>                 /* Clear frame flags since this is not a constructor call. */
>                 flags = 0;
>                 if (FUN_INTERPRETED(fun))

/be

Comment 2

[Andreas Gal :gal]

Attachment: patch, seems to work but crashes after a long run (gc?)

Comment 3

[Andreas Gal :gal]

Fails this testcase:

function f(a,b) {
    //print("a=" + a + " b=" + b + " arguments=" + arguments[0] + arguments[1] + arguments[2] + arguments[3] + arguments[4]);
}
for (var i = 0; i < 500000; ++i)
    f.apply(this, [1,2,3,4,5,6,7,8]); //, [1, 2]);
f.apply();
f.apply(this);
f.apply(this,[1]);
f.apply(this,[]);

Comment 4

[Andreas Gal :gal]

Attachment: v3

Comment 5

[Brendan Eich [:brendan]]

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

>+          BEGIN_CASE(JSOP_APPLY)
>+          {
>+            argc = GET_ARGC(regs.pc);
>+            vp = regs.sp - (argc + 2);
>+            lval = *vp;
>+            if (!VALUE_IS_FUNCTION(cx, lval)) 

(Angels cry!)

>+                goto do_call;
>+            obj = JSVAL_TO_OBJECT(lval);
>+            fun = GET_FUNCTION_PRIVATE(cx, obj);
>+            if (FUN_INTERPRETED(fun))
>+                goto do_call;
>+            
>+            bool apply = (JSFastNative)fun->u.n.native == js_fun_apply;

IIRC you will be warned about goto over init -- hoist bool apply; to top of block.

>+            if (!apply && (JSFastNative)fun->u.n.native != js_fun_call) 

(Wahh!)

>+                goto do_call;
>+
>+            /* If this is apply, and the argument is too long and would need
>+               a separate stack chunk, do a heavy-weight apply. */

Prevailing comment style is

    /*
     * I am a major comment and I have a lot to say, without running over the
     * sacred 80th column.
     */

but this is a small nit.

>+            if (apply && argc >= 2 && !JSVAL_IS_PRIMITIVE(vp[3])) {
>+                /* This is only a problem if 2nd array is array-like */

"This is a problem only if the second argument is array-like."

>+                JSBool arraylike = JS_FALSE;
>+                jsuint length = 0;
>+                JSObject* aobj = JSVAL_TO_OBJECT(vp[3]);
>+                if (js_IsArrayLike(cx, aobj, &arraylike, &length)) {
>+                    jsuint argc2 = (uintN)JS_MIN(length, ARRAY_INIT_LIMIT - 1);

Nit: I found apply_argc (or newargc?) better -- number 2 as suffix usually is unclear, not mnemonic or readable.

>+                    jsval* newsp = vp + 2 + argc2;
>+                    JS_ASSERT(newsp >= vp + 2);
>+                    JSArena *a = cx->stackPool.current;
>+                    if (jsuword(newsp) > a->limit)
>+                        goto do_call; /* do a heavy-weight apply */
>+                }

But hey, argc2 is single-use -- eliminate it if you can stand to.

>+            }
>+            
>+            

No double blank lines please.

>+            if (!apply) {
>+                /* If we have a 1st arg, copy the rest as arguments */
>+                if (argc > 0) 
>+                    memmove(vp + 2, vp + 3, (--argc) * sizeof *vp);

No need for parens around --argc, but it's kinda hidden. Could give it its own statement before the call to memset, at the price of a block.

Blank line (one only) before comment that takes one or more lines please.

>+                /* obj is no longer rooted since its not held in vp[2]
>+                   any more. We will put it into vp[1] momentarily. */
>+            } else {
>+                if (argc >= 2) { /* Have to expand arguments? */
>+                    JSObject *aobj = NULL;
>+                    /* If the 2nd arg is null or void, call the function with 0 args. */
>+                    if (JSVAL_IS_NULL(vp[3]) || JSVAL_IS_VOID(vp[3])) {
>+                        argc = 0;
>+                    } else {
>+                        /* The second arg must be an array (or arguments object). */

One-time only uber-nit: 1st/2nd are written first/second in comments in most code around here -- you did a mix above (copy-paste?).

>+                        JSBool arraylike = JS_FALSE;
>+                        jsuint length = 0;
>+                        if (!JSVAL_IS_PRIMITIVE(vp[3])) {
>+                            aobj = JSVAL_TO_OBJECT(vp[3]);
>+                            if (!js_IsArrayLike(cx, aobj, &arraylike, &length)) 
>+                                goto error;
>+                        }
>+                        if (!arraylike) {
>+                            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                                     JSMSG_BAD_APPLY_ARGS, 
>+                                                     apply ? "apply" : "call");

js_apply_str and js_call_str instead of repeated string constants (same for earlier error I snipped).

>+                            goto error;
>+                        }
>+                        jsuint orig_argc = argc;
>+                        argc = (uintN)JS_MIN(length, ARRAY_INIT_LIMIT - 1);

Saw this movie already :-P. Can you hoist and consolidate?

>+                            
>+                        /* Make room for another missing arguments to the right. */
>+                        if (argc > orig_argc) {
>+                            jsval* newsp = vp + 2 + argc;
>+                            JS_ASSERT(newsp > regs.sp);
>+                            JSArena *a = cx->stackPool.current;
>+                            JS_ASSERT(jsuword(newsp) <= a->limit); /* we checked for this */
>+                            if ((jsuword) newsp > a->avail)
>+                                a->avail = (jsuword) newsp;

Blank line here on account of comment on its own line(s) after.

>+                            /* Clear any stack cells we just added. */
>+                            memset(vp + 2 + orig_argc, 0, (argc - orig_argc) * sizeof(jsval));

s/Clear/Null/

>+                        }
>+
>+                        /* Place callee/this and bump regs.sp early for GC safety. */
>+                        vp[0] = rval;
>+                        vp[1] = OBJECT_TO_JSVAL(obj);
>+                        if (argc > 0) {
>+                            /* The last stack cell holds a pointer to the arguments
>+                               array to make sure its rooted. */

"make sure it is rooted during OBJ_GET_PROPERTY calls"

>+                            vp[2 + argc - 1] = vp[3]; /* aobj */
>+                        }
>+                        regs.sp = vp + 2 + argc;
>+
>+                        /* Expand array content onto the stack. */
>+                        jsval* sp = vp + 2;
>+                        for (i = 0; i < jsint(argc); i++) {
>+                            id = INT_TO_JSID(i);
>+                            if (!OBJ_GET_PROPERTY(cx, aobj, id, sp)) {
>+                                /* There is no good way to restore the original stack 
>+                                   state here, but its in a reasonable state with

s/its/it is/

>+                                   undefineds for all arguments we didn't unpack
>+                                   yet so we leave it at that. */

Nit: comma after "yet".

Can I review a final patch? Thanks,

/be

Comment 6

[Andreas Gal :gal]

Attachment: v4

Comment 7

[Brendan Eich [:brendan]]

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

>+            } else {
>+                if (argc >= 2) { /* Have to expand arguments? */
>+                    JSObject *aobj = NULL;

Blank line here. (Note how comment on one or more lines after open brace at end of previous line does *not* need a blank line, according to prevailing style -- just observing, not complaining: the patch follows this fine point perfectly!)

>+                    /* 
>+                     * If the second arg is null or void, call the function 
>+                     * with 0 args. 

(Angels!)

>+                     */
>+                    if (JSVAL_IS_NULL(vp[3]) || JSVAL_IS_VOID(vp[3])) {
>+                        argc = 0;
>+                    } else {
>+                        /* 
>+                         * The second arg must be an array (or arguments 
>+                         * object). 

(Angels^2)

Also I don't see the need for parens, and this:

>+                        /* 
>+                         * The second arg must be an array or arguments object.
>+                         */

..12345678901234567890123456789012345678901234567890123456789012345678901234567890

looks tidier.

>+                         */
>+                        JSBool arraylike = JS_FALSE;
>+                        jsuint length = 0;
>+                        if (!JSVAL_IS_PRIMITIVE(vp[3])) {
>+                            aobj = JSVAL_TO_OBJECT(vp[3]);
>+                            if (!js_IsArrayLike(cx, aobj, &arraylike, &length)) 
>+                                goto error;
>+                        }
>+                        if (!arraylike) {
>+                            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                                     JSMSG_BAD_APPLY_ARGS, 
>+                                                     apply ? js_apply_str : "call");
>+                            goto error;
>+                        }

Blank line here would let things breathe a bit.

>+                        jsuint orig_argc = argc;
>+                        argc = (uintN)JS_MIN(length, ARRAY_INIT_LIMIT - 1);
>+                            
>+                        /* 
>+                         * Make room for another missing arguments to the 

Angels! At this point (see below, "we checked for this" in-line comment) you are gonna go past column 80 so why not fit the comment on one line?

>+                         * right. 
>+                         */
>+                        if (argc > orig_argc) {
>+                            jsval* newsp = vp + 2 + argc;
>+                            JS_ASSERT(newsp > regs.sp);
>+                            JSArena *a = cx->stackPool.current;
>+                            JS_ASSERT(jsuword(newsp) <= a->limit); /* we checked for this */
>+                            if ((jsuword) newsp > a->avail)
>+                                a->avail = (jsuword) newsp;
>+                            
>+                            /* Null any stack cells we just added. */
>+                            memset(vp + 2 + orig_argc, 0, (argc - orig_argc) * sizeof(jsval));
>+                        }
>+
>+                        /* 
>+                         * Place callee/this and bump regs.sp early for GC 
>+                         * safety. 

(Angels^2!)

In view of memset line length visible above, a one-line comment works here too, unless you want to say what the GC safety issue is here and not foreshadow the bigger comment below:

>+                         */
>+                        vp[0] = rval;
>+                        vp[1] = OBJECT_TO_JSVAL(obj);
>+                        if (argc > 0) {
>+                            /* 
>+                             * The last stack cell holds a pointer to the arguments
>+                             * array to make sure its rooted during the 

(Angels!)

s/its/it is/

could elaborate about "during any GCs triggered from the"

>+                             * OBJ_GET_PROPERTY calls below. 
>+                             */
>+                            vp[2 + argc - 1] = vp[3]; /* aobj */
>+                        }
>+                        regs.sp = vp + 2 + argc;

r=me with final nits picked. Thanks!

/be

Comment 8

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/b513ecb09631