397239 - ActionMonkey: Remove "extra" parameter to JS_FN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The last parameter of JS_FN is called "extra" and causes extra slots to be created in the argv array.  These are for convenient rooting of local variables.  The technique is no longer necessary in ActionMonkey due to MMgc.

Brendan wrote in bug 388622 comment 14:

JS_FN is new enough that it has not shipped in a standalone SpiderMonkey
release or a XULRunner embedding release, so we could change it for Mozilla 2
and make the CVS version be a non-frozen prototype API. JS API compatibility
will be a "mostly" thing, with some changes (some opt-in via #ifdefs, I think,
but others that are "small enough" just incompatible changes). Comments?

Comment 1

[Igor Bukanov]

I think it could make sense to avoid the extra argument even on CVS trunk. Most of fast natives do not need local roots, so the support for local roots for them implies that an extra check before the call. On the other hand those fast natives that do require the local roots can use JSTempValueRoot.

Comment 2

[Jason Orendorff [:jorendorff]]

I don't think we can remove the |extra| field.  The JS_FN macro might be new, but |extra| has been around since (at least) April 1998, according to Bonsai.

Anyway, it doesn't exactly do an extra check before the call; js_Invoke does this:

========
        if (FUN_INTERPRETED(fun)) {
            native = NULL;
            script = fun->u.i.script;
            nvars = fun->u.i.nvars;
        } else {
            native = fun->u.n.native;
            script = NULL;
            nvars = 0;
-           nslots += fun->u.n.extra;
        }
========

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: for actionmonkey

Comment 4

[Igor Bukanov]

(In reply to comment #2)
> Anyway, it doesn't exactly do an extra check before the call; js_Invoke does
> this:
...

This is not true. Consider the bigger fragment:

================
        nslots = FUN_MINARGS(fun);
        nslots = (nslots > argc) ? nslots - argc : 0;
        if (FUN_INTERPRETED(fun)) {
            native = NULL;
            script = fun->u.i.script;
            nvars = fun->u.i.nvars;
        } else {
            native = fun->u.n.native;
            script = NULL;
            nvars = 0;
            nslots += fun->u.n.extra;
        }
     .....

    if (nslots != 0) {
================

There are 2 ifs hers, the first "nslots > argc" and the second is nslots != 0. With fun->u.n.extra == 0 that can be simplified to the single if:

================
        if (FUN_INTERPRETED(fun)) {
            native = NULL;
            script = fun->u.i.script;
            nvars = fun->u.i.nvars;
        } else {
            native = fun->u.n.native;
            script = NULL;
            nvars = 0;
        }
     .....

    nslots = FUN_MINARGS(fun);
    if (nslots > argc) {
        nslots -= argc;
...
================


The same extra check exists in JSOP_CALL case in the interpreter when it does the fast call dispatch:

================
if (fun->flags & JSFUN_FAST_NATIVE) {
    uintN nargs = JS_MAX(argc, fun->u.n.minargs);

    nargs += fun->u.n.extra;
    if (argc < nargs) {
================

With fun->u.n.extra == 0 that reduces to:

================
    if (argc < fun->u.n.minargs) {
================

Comment 5

[Igor Bukanov]

Comment on attachment 282536 [details] [diff] [review]
for actionmonkey

The patch does not take advantage of the fact that extra == 0 for fast natives in js_Interpret from jsinterp.cpp. Their in the JSOP_CALL case, when the interpreter dispatches the fast native call invocation, extra == 0 should simplify the code.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2 - for CVS trunk

This patch adds JSTempValueRooters to the 10 or so fast natives that need them.  In actionmonkey branch, of course, we'll have none of that.

Comment 7

[Igor Bukanov]

Comment on attachment 283774 [details] [diff] [review]
v2 - for CVS trunk

>Index: js/src/jsarray.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsarray.c,v
>retrieving revision 3.125
>diff -U 8 -p -r3.125 jsarray.c
>--- js/src/jsarray.c	18 Sep 2007 07:34:54 -0000	3.125
>+++ js/src/jsarray.c	5 Oct 2007 22:51:03 -0000
>@@ -806,41 +806,38 @@ array_join(JSContext *cx, uintN argc, js
>     }
>     return array_join_sub(cx, JS_THIS_OBJECT(cx, vp), TO_STRING, str, vp);
> }
> 
> static JSBool
> array_reverse(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *obj;
>-    jsval *argv, *tmproot, *tmproot2;
>+    jsval tmproot[2] = {JSVAL_NULL, JSVAL_NULL};
>+    JSTempValueRooter tvr;
>     jsuint len, half, i;
>     JSBool hole, hole2;
> 
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!js_GetLengthProperty(cx, obj, &len))
>         return JS_FALSE;
> 
>-    /*
>-     * Use argv[argc] and argv[argc + 1] as local roots to hold temporarily
>-     * array elements for GC-safe swap.
>-     */
>-    argv = JS_ARGV(cx, vp);
>-    tmproot = argv + argc;
>-    tmproot2 = argv + argc + 1;
>+    JS_PUSH_TEMP_ROOT(cx, 2, tmproot, &tvr);
>     half = len / 2;
>     for (i = 0; i < half; i++) {
>         if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-            !GetArrayElement(cx, obj, i, &hole, tmproot) ||
>-            !GetArrayElement(cx, obj, len - i - 1, &hole2, tmproot2) ||
>-            !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, *tmproot) ||
>-            !SetOrDeleteArrayElement(cx, obj, i, hole2, *tmproot2)) {
>+            !GetArrayElement(cx, obj, i, &hole, &tmproot[0]) ||
>+            !GetArrayElement(cx, obj, len - i - 1, &hole2, &tmproot[1]) ||
>+            !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, tmproot[0]) ||
>+            !SetOrDeleteArrayElement(cx, obj, i, hole2, tmproot[1])) {
>             return JS_FALSE;
>         }
>     }
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+
>     *vp = OBJECT_TO_JSVAL(obj);
>     return JS_TRUE;
> }

Here only one root is necessary since for the second *vp can be used.


> 
> typedef struct MSortArgs {
>     size_t       elsize;
>     JSComparator cmp;
>     void         *arg;
>@@ -1090,17 +1087,17 @@ JS_STATIC_ASSERT(JSVAL_NULL == 0);
> 
> static JSBool
> array_sort(JSContext *cx, uintN argc, jsval *vp)
> {
>     jsval *argv, fval, *vec, *mergesort_tmp;
>     JSObject *obj;
>     CompareArgs ca;
>     jsuint len, newlen, i, undefs;
>-    JSTempValueRooter tvr;
>+    JSTempValueRooter tvr, tvr2;
>     JSBool hole, ok;
> 
>     /*
>      * Optimize the default compare function case if all of obj's elements
>      * have values of type string.
>      */
>     JSBool all_strings;
> 
>@@ -1210,19 +1207,21 @@ array_sort(JSContext *cx, uintN argc, js
> 
>     /* Here len == 2 * (newlen + undefs + number_of_holes). */
>     if (all_strings) {
>         ok = js_MergeSort(vec, (size_t) newlen, sizeof(jsval),
>                           sort_compare_strings, cx, mergesort_tmp);
>     } else {
>         ca.context = cx;
>         ca.fval = fval;
>-        ca.localroot = argv + argc; /* local GC root for temporary string */
>+        JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr2);
>+        ca.localroot = &tvr2.u.value; /* local GC root for temporary string */
>         ok = js_MergeSort(vec, (size_t) newlen, sizeof(jsval),
>                           sort_compare, &ca, mergesort_tmp);
>+        JS_POP_TEMP_ROOT(cx, &tvr2);
>     }

Here there is no need to use the second tvr as the rooted location can be allocated in vec. 


> 
> static JSBool
> array_shift(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *obj;
>     jsuint length, i;
>-    JSBool hole;
>+    JSBool hole, ok;
>+    JSTempValueRooter tvr;
> 
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!js_GetLengthProperty(cx, obj, &length))
>         return JS_FALSE;
>     if (length == 0) {
>         *vp = JSVAL_VOID;
>     } else {
>         length--;
> 
>         /* Get the to-be-deleted property's value into vp ASAP. */
>         if (!GetArrayElement(cx, obj, 0, &hole, vp))
>             return JS_FALSE;
> 
>-        /*
>-         * Slide down the array above the first element, using our stack-local
>-         * GC root at vp[2].
>-         */
>+        /* Slide down the array above the first element. */
>+        ok = JS_TRUE;
>+        JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>         for (i = 0; i != length; i++) {
>             if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                !GetArrayElement(cx, obj, i + 1, &hole, &vp[2]) ||
>-                !SetOrDeleteArrayElement(cx, obj, i, hole, vp[2])) {
>-                return JS_FALSE;
>+                !GetArrayElement(cx, obj, i + 1, &hole, &tvr.u.value) ||
>+                !SetOrDeleteArrayElement(cx, obj, i, hole, tvr.u.value)) {
>+                ok = JS_FALSE;
>+                break;
>             }
>         }
>+        JS_POP_TEMP_ROOT(cx, &tvr);
>+        if (!ok)
>+            return JS_FALSE;
> 
>         /* Delete the only or last element when it exist. */
>         if (!hole && !DeleteArrayElement(cx, obj, length))
>             return JS_FALSE;
>     }
>     return js_SetLengthProperty(cx, obj, length);
> }
> 
> static JSBool
> array_unshift(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *obj;
>-    jsval *argv, *localroot;
>+    jsval *argv;
>     jsuint length, last;
>-    JSBool hole;
>+    JSBool hole, ok;
>+    JSTempValueRooter tvr;
> 
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!js_GetLengthProperty(cx, obj, &length))
>         return JS_FALSE;
>     if (argc > 0) {
>         /* Slide up the array to make room for argc at the bottom. */
>         argv = JS_ARGV(cx, vp);
>         if (length > 0) {
>             last = length;
>-            localroot = argv + argc;
>+            ok = JS_TRUE;
>+            JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>             do {
>                 --last;
>                 if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                    !GetArrayElement(cx, obj, last, &hole, localroot) ||
>+                    !GetArrayElement(cx, obj, last, &hole, &tvr.u.value) ||
>                     !SetOrDeleteArrayElement(cx, obj, last + argc, hole,
>-                                             *localroot)) {
>-                    return JS_FALSE;
>+                                             tvr.u.value)) {
>+                    ok = JS_FALSE;
>+                    break;
>                 }
>             } while (last != 0);
>+            JS_POP_TEMP_ROOT(cx, &tvr);
>+            if (!ok)
>+                return JS_FALSE;
>         }
> 
>         /* Copy from argv to the bottom of the array. */
>         if (!InitArrayElements(cx, obj, 0, argc, argv))
>             return JS_FALSE;
> 
>         length += argc;
>         if (!js_SetLengthProperty(cx, obj, length))
>@@ -1407,31 +1416,28 @@ array_unshift(JSContext *cx, uintN argc,
> 
>     /* Follow Perl by returning the new array length. */
>     return IndexToValue(cx, length, vp);
> }
> 
> static JSBool
> array_splice(JSContext *cx, uintN argc, jsval *vp)
> {
>-    jsval *argv, *localroot;
>+    jsval *argv;
>     JSObject *obj;
>     jsuint length, begin, end, count, delta, last;
>     jsdouble d;
>-    JSBool hole;
>+    JSBool hole, ok;
>     JSObject *obj2;
>+    JSTempValueRooter tvr;
> 
>-    /*
>-     * Nothing to do if no args.  Otherwise point localroot at our single
>-     * stack-local root and get length.
>-     */
>+    /* Nothing to do if no args.  Otherwise get length. */
>     if (argc == 0)
>         return JS_TRUE;
>     argv = JS_ARGV(cx, vp);
>-    localroot = argv + argc;
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!js_GetLengthProperty(cx, obj, &length))
>         return JS_FALSE;
> 
>     /* Convert the first argument into a starting index. */
>     if (!js_ValueToNumber(cx, *argv, &d))
>         return JS_FALSE;
>     d = js_DoubleToInteger(d);
>@@ -1460,163 +1466,183 @@ array_splice(JSContext *cx, uintN argc, 
>         else if (d > delta)
>             d = delta;
>         count = (jsuint)d;
>         end = begin + count;
>         argc--;
>         argv++;
>     }
> 
>-
>     /*
>      * Create a new array value to return.  Our ECMA v2 proposal specs
>      * that splice always returns an array value, even when given no
>      * arguments.  We think this is best because it eliminates the need
>      * for callers to do an extra test to handle the empty splice case.
>      */
>     obj2 = js_NewArrayObject(cx, 0, NULL);
>     if (!obj2)
>         return JS_FALSE;
>     *vp = OBJECT_TO_JSVAL(obj2);
> 
>+    /* After this, control must flow through label out: to exit. */
>+    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>+
>     /* If there are elements to remove, put them into the return value. */
>     if (count > 0) {
>         for (last = begin; last < end; last++) {
>-            if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                !GetArrayElement(cx, obj, last, &hole, localroot)) {
>-                return JS_FALSE;
>-            }
>+            ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) &&
>+                 GetArrayElement(cx, obj, last, &hole, &tvr.u.value);
>+            if (!ok)
>+                goto out;
> 
>-            /* Copy *localroot to new array unless it's a hole. */
>-            if (!hole && !SetArrayElement(cx, obj2, last - begin, *localroot))
>-                return JS_FALSE;
>+            /* Copy tvr.u.value to new array unless it's a hole. */
>+            if (!hole) {
>+                ok = SetArrayElement(cx, obj2, last - begin, tvr.u.value);
>+                if (!ok)
>+                    goto out;
>+            }
>         }
> 
>-        if (!js_SetLengthProperty(cx, obj2, end - begin))
>-            return JS_FALSE;
>+        ok = js_SetLengthProperty(cx, obj2, end - begin);
>+        if (!ok)
>+            goto out;
>     }
> 
>     /* Find the direction (up or down) to copy and make way for argv. */
>     if (argc > count) {
>         delta = (jsuint)argc - count;
>         last = length;
>         /* (uint) end could be 0, so can't use vanilla >= test */
>         while (last-- > end) {
>-            if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                !GetArrayElement(cx, obj, last, &hole, localroot) ||
>-                !SetOrDeleteArrayElement(cx, obj, last + delta, hole,
>-                                         *localroot)) {
>-                return JS_FALSE;
>-            }
>+            ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) &&
>+                 GetArrayElement(cx, obj, last, &hole, &tvr.u.value) &&
>+                 SetOrDeleteArrayElement(cx, obj, last + delta, hole,
>+                                         tvr.u.value);
>+            if (!ok)
>+                goto out;
>         }
>         length += delta;
>     } else if (argc < count) {
>         delta = count - (jsuint)argc;
>         for (last = end; last < length; last++) {
>-            if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                !GetArrayElement(cx, obj, last, &hole, localroot) ||
>-                !SetOrDeleteArrayElement(cx, obj, last - delta, hole,
>-                                         *localroot)) {
>-                return JS_FALSE;
>-            }
>+            ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) &&
>+                 GetArrayElement(cx, obj, last, &hole, &tvr.u.value) &&
>+                 SetOrDeleteArrayElement(cx, obj, last - delta, hole,
>+                                         tvr.u.value);
>+            if (!ok)
>+                goto out;
>         }
>         length -= delta;
>     }
> 
>     /* Copy from argv into the hole to complete the splice. */
>-    if (!InitArrayElements(cx, obj, begin, begin + argc, argv))
>-        return JS_FALSE;
>+    ok = InitArrayElements(cx, obj, begin, begin + argc, argv);
>+    if (!ok)
>+        goto out;
> 
>     /* Update length in case we deleted elements from the end. */
>-    return js_SetLengthProperty(cx, obj, length);
>+    ok = js_SetLengthProperty(cx, obj, length);
>+
>+out:
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+    return ok;
> }
> 
> /*
>  * Python-esque sequence operations.
>  */
> static JSBool
> array_concat(JSContext *cx, uintN argc, jsval *vp)
> {
>-    jsval *argv, *localroot, v;
>+    jsval *argv, v;
>     JSObject *nobj, *aobj;
>     jsuint length, alength, slot;
>     uintN i;
>-    JSBool hole;
>-
>-    /* Hoist the explicit local root address computation. */
>-    argv = JS_ARGV(cx, vp);
>-    localroot = argv + argc;
>+    JSBool hole, ok;
>+    JSTempValueRooter tvr;
> 
>     /* Treat our |this| object as the first argument; see ECMA 15.4.4.4. */
>-    --argv;
>+    argv = JS_ARGV(cx, vp) - 1;
>     JS_ASSERT(JS_THIS_OBJECT(cx, vp) == JSVAL_TO_OBJECT(argv[0]));
> 
>     /* Create a new Array object and root it using *vp. */
>     nobj = js_NewArrayObject(cx, 0, NULL);
>     if (!nobj)
>         return JS_FALSE;
>     *vp = OBJECT_TO_JSVAL(nobj);
> 
>+    /* After this, control must flow through label out: to exit. */
>+    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>+
>     /* Loop over [0, argc] to concat args into nobj, expanding all Arrays. */
>     length = 0;
>     for (i = 0; i <= argc; i++) {
>-        if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP))
>-            return JS_FALSE;
>+        ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP);
>+        if (!ok)
>+            goto out;
>         v = argv[i];
>         if (JSVAL_IS_OBJECT(v)) {
>             aobj = JSVAL_TO_OBJECT(v);
>             if (aobj && OBJ_GET_CLASS(cx, aobj) == &js_ArrayClass) {
>-                if (!OBJ_GET_PROPERTY(cx, aobj,
>+                ok = OBJ_GET_PROPERTY(cx, aobj,
>                                       ATOM_TO_JSID(cx->runtime->atomState
>                                                    .lengthAtom),
>-                                      localroot)) {
>-                    return JS_FALSE;
>-                }
>-                if (!ValueIsLength(cx, *localroot, &alength))
>-                    return JS_FALSE;
>+                                      &tvr.u.value);
>+                if (!ok)
>+                    goto out;
>+                ok = ValueIsLength(cx, tvr.u.value, &alength);
>+                if (!ok)
>+                    goto out;
>                 for (slot = 0; slot < alength; slot++) {
>-                    if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                        !GetArrayElement(cx, aobj, slot, &hole, localroot)) {
>-                        return JS_FALSE;
>-                    }
>+                    ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) &&
>+                         GetArrayElement(cx, aobj, slot, &hole,
>+                                         &tvr.u.value);
>+                    if (!ok)
>+                        goto out;
> 
>                     /*
>                      * Per ECMA 262, 15.4.4.4, step 9, ignore non-existent
>                      * properties.
>                      */
>-                    if (!hole &&
>-                        !SetArrayElement(cx, nobj, length + slot, *localroot)) {
>-                        return JS_FALSE;
>+                    if (!hole) {
>+                        ok = SetArrayElement(cx, nobj, length + slot,
>+                                             tvr.u.value);
>+                        if (!ok)
>+                            goto out;
>                     }
>                 }
>                 length += alength;
>                 continue;
>             }
>         }
> 
>-        if (!SetArrayElement(cx, nobj, length, v))
>-            return JS_FALSE;
>+        ok = SetArrayElement(cx, nobj, length, v);
>+        if (!ok)
>+            goto out;
>         length++;
>     }
> 
>-    return js_SetLengthProperty(cx, nobj, length);
>+    ok = js_SetLengthProperty(cx, nobj, length);
>+
>+out:
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+    return ok;
> }
> 
> static JSBool
> array_slice(JSContext *cx, uintN argc, jsval *vp)
> {
>-    jsval *argv, *localroot;
>+    jsval *argv;
>     JSObject *nobj, *obj;
>     jsuint length, begin, end, slot;
>     jsdouble d;
>-    JSBool hole;
>+    JSBool hole, ok;
>+    JSTempValueRooter tvr;
> 
>-    /* Hoist the explicit local root address computation. */
>     argv = JS_ARGV(cx, vp);
>-    localroot = argv + argc;
> 
>     /* Create a new Array object and root it using *vp. */
>     nobj = js_NewArrayObject(cx, 0, NULL);
>     if (!nobj)
>         return JS_FALSE;
>     *vp = OBJECT_TO_JSVAL(nobj);
> 
>     obj = JS_THIS_OBJECT(cx, vp);
>@@ -1651,25 +1677,35 @@ array_slice(JSContext *cx, uintN argc, j
>             }
>             end = (jsuint)d;
>         }
>     }
> 
>     if (begin > end)
>         begin = end;
> 
>+    /* After this, control must flow through label out: to exit. */
>+    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>+
>     for (slot = begin; slot < end; slot++) {
>-        if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-            !GetArrayElement(cx, obj, slot, &hole, localroot)) {
>-            return JS_FALSE;
>+        ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) &&
>+             GetArrayElement(cx, obj, slot, &hole, &tvr.u.value);
>+        if (!ok)
>+            goto out;
>+        if (!hole) {
>+            ok = SetArrayElement(cx, nobj, slot - begin, tvr.u.value);
>+            if (!ok)
>+                goto out;
>         }
>-        if (!hole && !SetArrayElement(cx, nobj, slot - begin, *localroot))
>-            return JS_FALSE;
>     }
>-    return js_SetLengthProperty(cx, nobj, end - begin);
>+    ok = js_SetLengthProperty(cx, nobj, end - begin);
>+
>+out:
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+    return ok;
> }
> 
> #if JS_HAS_ARRAY_EXTRAS
> 
> static JSBool
> array_indexOfHelper(JSContext *cx, JSBool isLast, uintN argc, jsval *vp)
> {
>     JSObject *obj;
>@@ -1990,45 +2026,45 @@ array_every(JSContext *cx, uintN argc, j
> static JSPropertySpec array_props[] = {
>     {js_length_str,   -1,   JSPROP_SHARED | JSPROP_PERMANENT,
>                             array_length_getter,    array_length_setter},
>     {0,0,0,0,0}
> };
> 
> static JSFunctionSpec array_methods[] = {
> #if JS_HAS_TOSOURCE
>-    JS_FN(js_toSource_str,      array_toSource,     0,0,0,0),
>+    JS_FN(js_toSource_str,      array_toSource,     0,0,0),
> #endif
>-    JS_FN(js_toString_str,      array_toString,     0,0,0,0),
>-    JS_FN(js_toLocaleString_str,array_toLocaleString,0,0,0,0),
>+    JS_FN(js_toString_str,      array_toString,     0,0,0),
>+    JS_FN(js_toLocaleString_str,array_toLocaleString,0,0,0),
> 
>     /* Perl-ish methods. */
>-    JS_FN("join",               array_join,         1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("reverse",            array_reverse,      0,0,JSFUN_GENERIC_NATIVE,2),
>-    JS_FN("sort",               array_sort,         0,1,JSFUN_GENERIC_NATIVE,1),
>-    JS_FN("push",               array_push,         1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("pop",                array_pop,          0,0,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("shift",              array_shift,        0,0,JSFUN_GENERIC_NATIVE,1),
>-    JS_FN("unshift",            array_unshift,      0,1,JSFUN_GENERIC_NATIVE,1),
>-    JS_FN("splice",             array_splice,       0,2,JSFUN_GENERIC_NATIVE,1),
>+    JS_FN("join",               array_join,         1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("reverse",            array_reverse,      0,0,JSFUN_GENERIC_NATIVE),
>+    JS_FN("sort",               array_sort,         0,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("push",               array_push,         1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("pop",                array_pop,          0,0,JSFUN_GENERIC_NATIVE),
>+    JS_FN("shift",              array_shift,        0,0,JSFUN_GENERIC_NATIVE),
>+    JS_FN("unshift",            array_unshift,      0,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("splice",             array_splice,       0,2,JSFUN_GENERIC_NATIVE),
> 
>     /* Python-esque sequence methods. */
>-    JS_FN("concat",             array_concat,       0,1,JSFUN_GENERIC_NATIVE,1),
>-    JS_FN("slice",              array_slice,        0,2,JSFUN_GENERIC_NATIVE,1),
>+    JS_FN("concat",             array_concat,       0,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("slice",              array_slice,        0,2,JSFUN_GENERIC_NATIVE),
> 
> #if JS_HAS_ARRAY_EXTRAS
>-    JS_FN("indexOf",            array_indexOf,      1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("lastIndexOf",        array_lastIndexOf,  1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("forEach",            array_forEach,      1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("map",                array_map,          1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("reduce",             array_reduce,       1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("reduceRight",        array_reduceRight,  1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("filter",             array_filter,       1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("some",               array_some,         1,1,JSFUN_GENERIC_NATIVE,0),
>-    JS_FN("every",              array_every,        1,1,JSFUN_GENERIC_NATIVE,0),
>+    JS_FN("indexOf",            array_indexOf,      1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("lastIndexOf",        array_lastIndexOf,  1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("forEach",            array_forEach,      1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("map",                array_map,          1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("reduce",             array_reduce,       1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("reduceRight",        array_reduceRight,  1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("filter",             array_filter,       1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("some",               array_some,         1,1,JSFUN_GENERIC_NATIVE),
>+    JS_FN("every",              array_every,        1,1,JSFUN_GENERIC_NATIVE),
> #endif
> 
>     JS_FS_END
> };
> 
> static JSBool
> Array(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>Index: js/src/jsbool.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsbool.c,v
>retrieving revision 3.30
>diff -U 8 -p -r3.30 jsbool.c
>--- js/src/jsbool.c	7 Aug 2007 07:29:32 -0000	3.30
>+++ js/src/jsbool.c	5 Oct 2007 22:51:04 -0000
>@@ -107,20 +107,20 @@ bool_toString(JSContext *cx, uintN argc,
> static JSBool
> bool_valueOf(JSContext *cx, uintN argc, jsval *vp)
> {
>     return js_GetPrimitiveThis(cx, vp, &js_BooleanClass, vp);
> }
> 
> static JSFunctionSpec boolean_methods[] = {
> #if JS_HAS_TOSOURCE
>-    JS_FN(js_toSource_str,  bool_toSource,  0, 0, JSFUN_THISP_BOOLEAN,0),
>+    JS_FN(js_toSource_str,  bool_toSource,  0, 0, JSFUN_THISP_BOOLEAN),
> #endif
>-    JS_FN(js_toString_str,  bool_toString,  0, 0, JSFUN_THISP_BOOLEAN,0),
>-    JS_FN(js_valueOf_str,   bool_valueOf,   0, 0, JSFUN_THISP_BOOLEAN,0),
>+    JS_FN(js_toString_str,  bool_toString,  0, 0, JSFUN_THISP_BOOLEAN),
>+    JS_FN(js_valueOf_str,   bool_valueOf,   0, 0, JSFUN_THISP_BOOLEAN),
>     JS_FS_END
> };
> 
> static JSBool
> Boolean(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>     JSBool b;
>     jsval bval;
>Index: js/src/jsdate.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsdate.c,v
>retrieving revision 3.90
>diff -U 8 -p -r3.90 jsdate.c
>--- js/src/jsdate.c	16 Aug 2007 06:23:06 -0000	3.90
>+++ js/src/jsdate.c	5 Oct 2007 22:51:04 -0000
>@@ -1975,70 +1975,70 @@ date_valueOf(JSContext *cx, uintN argc, 
> }
> 
> 
> /*
>  * creation and destruction
>  */
> 
> static JSFunctionSpec date_static_methods[] = {
>-    JS_FN("UTC",                 date_UTC,                2,MAXARGS,0,0),
>-    JS_FN("parse",               date_parse,              1,1,0,0),
>-    JS_FN("now",                 date_now,                0,0,0,0),
>+    JS_FN("UTC",                 date_UTC,                2,MAXARGS,0),
>+    JS_FN("parse",               date_parse,              1,1,0),
>+    JS_FN("now",                 date_now,                0,0,0),
>     JS_FS_END
> };
> 
> static JSFunctionSpec date_methods[] = {
>-    JS_FN("getTime",             date_getTime,            0,0,0,0),
>-    JS_FN("getTimezoneOffset",   date_getTimezoneOffset,  0,0,0,0),
>-    JS_FN("getYear",             date_getYear,            0,0,0,0),
>-    JS_FN("getFullYear",         date_getFullYear,        0,0,0,0),
>-    JS_FN("getUTCFullYear",      date_getUTCFullYear,     0,0,0,0),
>-    JS_FN("getMonth",            date_getMonth,           0,0,0,0),
>-    JS_FN("getUTCMonth",         date_getUTCMonth,        0,0,0,0),
>-    JS_FN("getDate",             date_getDate,            0,0,0,0),
>-    JS_FN("getUTCDate",          date_getUTCDate,         0,0,0,0),
>-    JS_FN("getDay",              date_getDay,             0,0,0,0),
>-    JS_FN("getUTCDay",           date_getUTCDay,          0,0,0,0),
>-    JS_FN("getHours",            date_getHours,           0,0,0,0),
>-    JS_FN("getUTCHours",         date_getUTCHours,        0,0,0,0),
>-    JS_FN("getMinutes",          date_getMinutes,         0,0,0,0),
>-    JS_FN("getUTCMinutes",       date_getUTCMinutes,      0,0,0,0),
>-    JS_FN("getSeconds",          date_getUTCSeconds,      0,0,0,0),
>-    JS_FN("getUTCSeconds",       date_getUTCSeconds,      0,0,0,0),
>-    JS_FN("getMilliseconds",     date_getUTCMilliseconds, 0,0,0,0),
>-    JS_FN("getUTCMilliseconds",  date_getUTCMilliseconds, 0,0,0,0),
>-    JS_FN("setTime",             date_setTime,            1,1,0,0),
>-    JS_FN("setYear",             date_setYear,            1,1,0,0),
>-    JS_FN("setFullYear",         date_setFullYear,        1,3,0,0),
>-    JS_FN("setUTCFullYear",      date_setUTCFullYear,     1,3,0,0),
>-    JS_FN("setMonth",            date_setMonth,           1,2,0,0),
>-    JS_FN("setUTCMonth",         date_setUTCMonth,        1,2,0,0),
>-    JS_FN("setDate",             date_setDate,            1,1,0,0),
>-    JS_FN("setUTCDate",          date_setUTCDate,         1,1,0,0),
>-    JS_FN("setHours",            date_setHours,           1,4,0,0),
>-    JS_FN("setUTCHours",         date_setUTCHours,        1,4,0,0),
>-    JS_FN("setMinutes",          date_setMinutes,         1,3,0,0),
>-    JS_FN("setUTCMinutes",       date_setUTCMinutes,      1,3,0,0),
>-    JS_FN("setSeconds",          date_setSeconds,         1,2,0,0),
>-    JS_FN("setUTCSeconds",       date_setUTCSeconds,      1,2,0,0),
>-    JS_FN("setMilliseconds",     date_setMilliseconds,    1,1,0,0),
>-    JS_FN("setUTCMilliseconds",  date_setUTCMilliseconds, 1,1,0,0),
>-    JS_FN("toUTCString",         date_toGMTString,        0,0,0,0),
>-    JS_FN(js_toLocaleString_str, date_toLocaleString,     0,0,0,0),
>-    JS_FN("toLocaleDateString",  date_toLocaleDateString, 0,0,0,0),
>-    JS_FN("toLocaleTimeString",  date_toLocaleTimeString, 0,0,0,0),
>-    JS_FN("toLocaleFormat",      date_toLocaleFormat,     0,0,0,0),
>-    JS_FN("toDateString",        date_toDateString,       0,0,0,0),
>-    JS_FN("toTimeString",        date_toTimeString,       0,0,0,0),
>+    JS_FN("getTime",             date_getTime,            0,0,0),
>+    JS_FN("getTimezoneOffset",   date_getTimezoneOffset,  0,0,0),
>+    JS_FN("getYear",             date_getYear,            0,0,0),
>+    JS_FN("getFullYear",         date_getFullYear,        0,0,0),
>+    JS_FN("getUTCFullYear",      date_getUTCFullYear,     0,0,0),
>+    JS_FN("getMonth",            date_getMonth,           0,0,0),
>+    JS_FN("getUTCMonth",         date_getUTCMonth,        0,0,0),
>+    JS_FN("getDate",             date_getDate,            0,0,0),
>+    JS_FN("getUTCDate",          date_getUTCDate,         0,0,0),
>+    JS_FN("getDay",              date_getDay,             0,0,0),
>+    JS_FN("getUTCDay",           date_getUTCDay,          0,0,0),
>+    JS_FN("getHours",            date_getHours,           0,0,0),
>+    JS_FN("getUTCHours",         date_getUTCHours,        0,0,0),
>+    JS_FN("getMinutes",          date_getMinutes,         0,0,0),
>+    JS_FN("getUTCMinutes",       date_getUTCMinutes,      0,0,0),
>+    JS_FN("getSeconds",          date_getUTCSeconds,      0,0,0),
>+    JS_FN("getUTCSeconds",       date_getUTCSeconds,      0,0,0),
>+    JS_FN("getMilliseconds",     date_getUTCMilliseconds, 0,0,0),
>+    JS_FN("getUTCMilliseconds",  date_getUTCMilliseconds, 0,0,0),
>+    JS_FN("setTime",             date_setTime,            1,1,0),
>+    JS_FN("setYear",             date_setYear,            1,1,0),
>+    JS_FN("setFullYear",         date_setFullYear,        1,3,0),
>+    JS_FN("setUTCFullYear",      date_setUTCFullYear,     1,3,0),
>+    JS_FN("setMonth",            date_setMonth,           1,2,0),
>+    JS_FN("setUTCMonth",         date_setUTCMonth,        1,2,0),
>+    JS_FN("setDate",             date_setDate,            1,1,0),
>+    JS_FN("setUTCDate",          date_setUTCDate,         1,1,0),
>+    JS_FN("setHours",            date_setHours,           1,4,0),
>+    JS_FN("setUTCHours",         date_setUTCHours,        1,4,0),
>+    JS_FN("setMinutes",          date_setMinutes,         1,3,0),
>+    JS_FN("setUTCMinutes",       date_setUTCMinutes,      1,3,0),
>+    JS_FN("setSeconds",          date_setSeconds,         1,2,0),
>+    JS_FN("setUTCSeconds",       date_setUTCSeconds,      1,2,0),
>+    JS_FN("setMilliseconds",     date_setMilliseconds,    1,1,0),
>+    JS_FN("setUTCMilliseconds",  date_setUTCMilliseconds, 1,1,0),
>+    JS_FN("toUTCString",         date_toGMTString,        0,0,0),
>+    JS_FN(js_toLocaleString_str, date_toLocaleString,     0,0,0),
>+    JS_FN("toLocaleDateString",  date_toLocaleDateString, 0,0,0),
>+    JS_FN("toLocaleTimeString",  date_toLocaleTimeString, 0,0,0),
>+    JS_FN("toLocaleFormat",      date_toLocaleFormat,     0,0,0),
>+    JS_FN("toDateString",        date_toDateString,       0,0,0),
>+    JS_FN("toTimeString",        date_toTimeString,       0,0,0),
> #if JS_HAS_TOSOURCE
>-    JS_FN(js_toSource_str,       date_toSource,           0,0,0,0),
>+    JS_FN(js_toSource_str,       date_toSource,           0,0,0),
> #endif
>-    JS_FN(js_toString_str,       date_toString,           0,0,0,0),
>-    JS_FN(js_valueOf_str,        date_valueOf,            0,0,0,0),
>+    JS_FN(js_toString_str,       date_toString,           0,0,0),
>+    JS_FN(js_valueOf_str,        date_valueOf,            0,0,0),
>     JS_FS_END
> };
> 
> static jsdouble *
> date_constructor(JSContext *cx, JSObject* obj)
> {
>     jsdouble *date;
> 
>Index: js/src/jsexn.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsexn.c,v
>retrieving revision 3.90
>diff -U 8 -p -r3.90 jsexn.c
>--- js/src/jsexn.c	16 Aug 2007 06:23:06 -0000	3.90
>+++ js/src/jsexn.c	5 Oct 2007 22:51:04 -0000
>@@ -865,57 +865,62 @@ exn_toString(JSContext *cx, uintN argc, 
> 
> #if JS_HAS_TOSOURCE
> /*
>  * Return a string that may eval to something similar to the original object.
>  */
> static JSBool
> exn_toSource(JSContext *cx, uintN argc, jsval *vp)
> {
>-    jsval *localroots;
>     JSObject *obj;
>     JSString *name, *message, *filename, *lineno_as_str, *result;
>+    jsval localroots[3] = {JSVAL_NULL, JSVAL_NULL, JSVAL_NULL};
>+    JSTempValueRooter tvr;
>+    JSBool ok;
>     uint32 lineno;
>     size_t lineno_length, name_length, message_length, filename_length, length;
>     jschar *chars, *cp;
> 
>-    localroots = JS_ARGV(cx, vp) + argc;
>-
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!OBJ_GET_PROPERTY(cx, obj,
>                           ATOM_TO_JSID(cx->runtime->atomState.nameAtom),
>                           vp)) {
>         return JS_FALSE;
>     }
>     name = js_ValueToString(cx, *vp);
>     if (!name)
>         return JS_FALSE;
>     *vp = STRING_TO_JSVAL(name);
> 
>-    if (!JS_GetProperty(cx, obj, js_message_str, &localroots[0]) ||
>-        !(message = js_ValueToSource(cx, localroots[0]))) {
>-        return JS_FALSE;
>-    }
>+    /* After this, control must flow through label out: to exit. */
>+    JS_PUSH_TEMP_ROOT(cx, 3, localroots, &tvr);

Use here JS_PUSH_TEMP_ROOT(cx, 0, localroots, &tvr) and later, when assigning to localroots, use
localroots[tvr.count++] =... It avoids unnecessary tvr initialization.

Comment 8

[Igor Bukanov]

The previous comments on the patch for CVS trunk got too much context, here is a compressed version:

(From update of attachment 283774 [details] [diff] [review])
>Index: js/src/jsarray.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsarray.c,v
> static JSBool
> array_reverse(JSContext *cx, uintN argc, jsval *vp)
> {
...
>+    jsval tmproot[2] = {JSVAL_NULL, JSVAL_NULL};
>+    JSTempValueRooter tvr;
...
>     *vp = OBJECT_TO_JSVAL(obj);
>     return JS_TRUE;
> }

Here only one root is necessary since for the second *vp can be used.


> static JSBool
> array_sort(JSContext *cx, uintN argc, jsval *vp)
> {

...
>-    JSTempValueRooter tvr;
>+    JSTempValueRooter tvr, tvr2;
>     JSBool hole, ok;
>+        JS_POP_TEMP_ROOT(cx, &tvr2);
>     }

Here there is no need to use the second tvr as the rooted location can be
allocated in vec. 


>Index: js/src/jsexn.c
> static JSBool
> exn_toSource(JSContext *cx, uintN argc, jsval *vp)
> {
>-    jsval *localroots;
>     JSObject *obj;
>     JSString *name, *message, *filename, *lineno_as_str, *result;
>+    jsval localroots[3] = {JSVAL_NULL, JSVAL_NULL, JSVAL_NULL};
>+    JSTempValueRooter tvr;
>+    JSBool ok;
>     uint32 lineno;
>     size_t lineno_length, name_length, message_length, filename_length, length;
>     jschar *chars, *cp;
> 
>-    localroots = JS_ARGV(cx, vp) + argc;
>-
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!OBJ_GET_PROPERTY(cx, obj,
>                           ATOM_TO_JSID(cx->runtime->atomState.nameAtom),
>                           vp)) {
>         return JS_FALSE;
>     }
>     name = js_ValueToString(cx, *vp);
>     if (!name)
>         return JS_FALSE;
>     *vp = STRING_TO_JSVAL(name);
> 
>-    if (!JS_GetProperty(cx, obj, js_message_str, &localroots[0]) ||
>-        !(message = js_ValueToSource(cx, localroots[0]))) {
>-        return JS_FALSE;
>-    }
>+    /* After this, control must flow through label out: to exit. */
>+    JS_PUSH_TEMP_ROOT(cx, 3, localroots, &tvr);

Use here JS_PUSH_TEMP_ROOT(cx, 0, localroots, &tvr) and later, when assigning
to localroots, use
localroots[tvr.count++] =... It avoids unnecessary tvr initialization.

Comment 9

[Jason Orendorff [:jorendorff]]

Thanks Igor.  Agreed on your first two comments, but I don't know about the last one.  I think the initialization is necessary regardless.  All three localroots slots are used (if no error occurs), and I think JS_GetProperty() requires them to be rooted before they can be populated (?).

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: diff v2 v3

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v3

Like v2, but adds Igor's first 2 suggestions.

No nice interdiff this time. :(  I'm not working with hg in this sandbox; next time I will.

Comment 12

[Igor Bukanov]

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

>===================================================================
>      */
>     if (len > (size_t)-1 / (2 * sizeof(jsval))) {
>         JS_ReportOutOfMemory(cx);
>         return JS_FALSE;
>     }

Here it is necessary to update the overflow check above to take into account the extra root.
> 
>     /*
>      * Allocate 2 * len instead of len, to reserve space for the mergesort
>-     * algorithm.
>+     * algorithm; plus 1 for an additional temporary root.
>      */
>-    vec = (jsval *) JS_malloc(cx, 2 * (size_t)len * sizeof(jsval));
>+    vec = (jsval *) JS_malloc(cx, (2 * (size_t)len + 1) * sizeof(jsval));

> static JSBool
> array_unshift(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *obj;
>-    jsval *argv, *localroot;
>+    jsval *argv;
>     jsuint length, last;
>-    JSBool hole;
>+    JSBool hole, ok;
>+    JSTempValueRooter tvr;
> 
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!js_GetLengthProperty(cx, obj, &length))
>         return JS_FALSE;
>     if (argc > 0) {
>         /* Slide up the array to make room for argc at the bottom. */
>         argv = JS_ARGV(cx, vp);
>         if (length > 0) {
>             last = length;
>-            localroot = argv + argc;
>+            ok = JS_TRUE;
>+            JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>             do {
>                 --last;
>                 if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                    !GetArrayElement(cx, obj, last, &hole, localroot) ||
>+                    !GetArrayElement(cx, obj, last, &hole, &tvr.u.value) ||
>                     !SetOrDeleteArrayElement(cx, obj, last + argc, hole,
>-                                             *localroot)) {
>-                    return JS_FALSE;
>+                                             tvr.u.value)) {
>+                    ok = JS_FALSE;
>+                    break;

Nit: using ok = call1 && call2..; if (!ok) ... reads better.

Comment 13

[Igor Bukanov]

(In reply to comment #9)
> Thanks Igor.  Agreed on your first two comments, but I don't know about the
> last one.  I think the initialization is necessary regardless.

I was thinking of using localroots[tvr.count++] to root elements as necessary like in localroots[tvr.count++] = JSVAL_NULL before using localroots[2] as lvalue for JS_GetProperty call. But it would make unclear that this roots localroots[2]. So initializing the localroots as you did in the patch wins from code clarity and code size points of view.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: diff v3 v4

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: v4

Thanks for catching the array bounds check.

Now that I'm posting this, I think the constant there should be
  ((size_t)-1 / sizeof(jsval) - 1) / 2

rather than
  (size_t)-1 / (2 * sizeof(jsval)) - 1

...which makes very little difference, but I'll change that for check-in if you concur.

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> Now that I'm posting this, I think the constant there should be
>   ((size_t)-1 / sizeof(jsval) - 1) / 2
> 
> rather than
>   (size_t)-1 / (2 * sizeof(jsval)) - 1

The code allocates an array with 2 * len + 1 elements each of size sizeof(jsval). The overflow happens when:

(2 * len + 1) * sizeof(jsval) > MAX_SIZE_T  or
(2 * len + 1) > MAX_SIZE_T / sizeof(jsval) (integer division here) or
2 * len > MAX_SIZE_T / sizeof(jsval) - 1 or
len > (MAX_SIZE_T / sizeof(jsval) - 1) / 2 or 
the first condition.

The second condition (one from the patch) is
len > MAX_SIZE_T / (2 * sizeof(jsval)) - 1 or
len > (MAX_SIZE_T / sizeof(jsval) - 1) / 2 - 1 (since MAX_SIZE_T = 2^n - 1) or
stronger then necessary by one.

So the patch should be updated ideally with comments to show how to deduce the condition.

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: diff v4 v5

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: v5

Added explanation.

(I tried writing a less verbose comment, but it sounded unnecessarily cryptic, so I just went ahead and included the derivation--it's only 4 more lines.)

Comment 19

[Igor Bukanov]

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

>Index: js/src/jsarray.c
> static JSBool
> array_reverse(JSContext *cx, uintN argc, jsval *vp)
> {
>+    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>     half = len / 2;
>     for (i = 0; i < half; i++) {
>         if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-            !GetArrayElement(cx, obj, i, &hole, tmproot) ||
>-            !GetArrayElement(cx, obj, len - i - 1, &hole2, tmproot2) ||
>-            !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, *tmproot) ||
>-            !SetOrDeleteArrayElement(cx, obj, i, hole2, *tmproot2)) {
>+            !GetArrayElement(cx, obj, i, &hole, &tvr.u.value) ||
>+            !GetArrayElement(cx, obj, len - i - 1, &hole2, vp) ||
>+            !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, tvr.u.value) ||
>+            !SetOrDeleteArrayElement(cx, obj, i, hole2, *vp)) {
>             return JS_FALSE;

I missed this during the prev review, but this must pop tvr on exit.

> static JSBool
> array_shift(JSContext *cx, uintN argc, jsval *vp)
> {
...
>+        ok = JS_TRUE;
>+        JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>         for (i = 0; i != length; i++) {
>             if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-                !GetArrayElement(cx, obj, i + 1, &hole, &vp[2]) ||
>-                !SetOrDeleteArrayElement(cx, obj, i, hole, vp[2])) {
>-                return JS_FALSE;
>+                !GetArrayElement(cx, obj, i + 1, &hole, &tvr.u.value) ||
>+                !SetOrDeleteArrayElement(cx, obj, i, hole, tvr.u.value)) {
>+                ok = JS_FALSE;
>+                break;

Nit: use here
ok = JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) &&
     GetArrayElement(cx, obj, i + 1, &hole, &tvr.u.value) &&
...
if (!ok)
    break;

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: diff v5 v6

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: v6

This fixes the bug and the nit.

Comment 22

[Igor Bukanov]

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

>     for (i = 0; i < half; i++) {
>         if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
>-            !GetArrayElement(cx, obj, i, &hole, tmproot) ||
>-            !GetArrayElement(cx, obj, len - i - 1, &hole2, tmproot2) ||
>-            !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, *tmproot) ||
>-            !SetOrDeleteArrayElement(cx, obj, i, hole2, *tmproot2)) {
>-            return JS_FALSE;
>+            !GetArrayElement(cx, obj, i, &hole, &tvr.u.value) ||
>+            !GetArrayElement(cx, obj, len - i - 1, &hole2, vp) ||
>+            !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, tvr.u.value) ||
>+            !SetOrDeleteArrayElement(cx, obj, i, hole2, *vp)) {
>+            ok = JS_FALSE;
>+            break;
>         }

The last nit: use that ok = a&&b&&c&&d; pattern here as well. r+ with that fixed.

Comment 23

[Jason Orendorff [:jorendorff]]

Attachment: diff v6 v7

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: v7

Comment 25

[Igor Bukanov]

Comment on attachment 284531 [details] [diff] [review]
v7

In addition to making invocation of fast natives faster the patch would allow to preserve fast native call API compatibility between 1.9 and Mozilla 2.0.

Comment 26

[timeless]

mozilla/js/src/js.c 	3.168
mozilla/js/src/jsapi.h 	3.162
mozilla/js/src/jsarray.c 	3.126
mozilla/js/src/jsbool.c 	3.31
mozilla/js/src/jsdate.c 	3.91
mozilla/js/src/jsexn.c 	3.91
mozilla/js/src/jsfun.c 	3.224
mozilla/js/src/jsinterp.c 	3.377
mozilla/js/src/jsiter.c 	3.77
mozilla/js/src/jsmath.c 	3.31
mozilla/js/src/jsnum.c 	3.90
mozilla/js/src/jsobj.c 	3.389
mozilla/js/src/jsregexp.c 	3.164
mozilla/js/src/jsscript.c 	3.158
mozilla/js/src/jsstr.c 	3.170
mozilla/js/src/jsxml.c 	3.170