Bug 466905 - Fix JSOP_NEWARRAY to be not-buggy and use it when possible. NOT REVIEWED YET
authorJeff Walden <jwalden@mit.edu>
Sat, 10 Jan 2009 12:15:08 -0800
changeset 23708 6475993319c4799269e365623f994f9e7b1bf5ed
parent 23707 411b2f1e19de674725fe6e2d4f9b01882e1dcc98
child 23709 71e7b627ed3109dae8002039ea22df7633b6f1af
push id4690
push userrsayre@mozilla.com
push dateThu, 15 Jan 2009 07:42:55 +0000
treeherdermozilla-central@ddfa483fea2a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs466905
milestone1.9.2a1pre
Bug 466905 - Fix JSOP_NEWARRAY to be not-buggy and use it when possible. NOT REVIEWED YET
js/src/jsemit.cpp
js/src/jsopcode.cpp
js/src/jsopcode.tbl
js/src/jstracer.cpp
js/src/jstracer.h
--- a/js/src/jsemit.cpp
+++ b/js/src/jsemit.cpp
@@ -5989,17 +5989,17 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
          *
          * but use a stack slot for t and avoid dup'ing and popping it using
          * the JSOP_NEWINIT and JSOP_INITELEM bytecodes.
          *
          * If no sharp variable is defined and the initialiser is not for an
          * array comprehension, use JSOP_NEWARRAY.
          */
         pn2 = pn->pn_head;
-        op = JSOP_NEWINIT;      // FIXME: 260106 patch disabled for now
+        op = JSOP_NEWARRAY;
 
 #if JS_HAS_SHARP_VARS
         if (pn2 && pn2->pn_type == TOK_DEFSHARP)
             op = JSOP_NEWINIT;
 #endif
 #if JS_HAS_GENERATORS
         if (pn->pn_type == TOK_ARRAYCOMP)
             op = JSOP_NEWINIT;
--- a/js/src/jsopcode.cpp
+++ b/js/src/jsopcode.cpp
@@ -1,9 +1,9 @@
-/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
  * vim: set sw=4 ts=8 et tw=99:
  *
  * ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
@@ -3463,53 +3463,49 @@ Decompile(SprintStack *ss, jsbytecode *p
 
               case JSOP_NEW:
               case JSOP_CALL:
               case JSOP_EVAL:
               case JSOP_APPLY:
 #if JS_HAS_LVALUE_RETURN
               case JSOP_SETCALL:
 #endif
-                /* Turn off most parens (all if there's only one argument). */
                 argc = GET_ARGC(pc);
-                op = (argc == 1) ? JSOP_NOP : JSOP_SETNAME;
                 argv = (char **)
                     JS_malloc(cx, (size_t)(argc + 1) * sizeof *argv);
                 if (!argv)
                     return NULL;
 
+                op = JSOP_SETNAME;
                 ok = JS_TRUE;
-                for (i = argc; i > 0; i--) {
+                for (i = argc; i > 0; i--)
                     argv[i] = JS_strdup(cx, POP_STR());
-                    if (!argv[i])
-                        ok = JS_FALSE;
-                }
 
                 /* Skip the JSOP_PUSHOBJ-created empty string. */
                 LOCAL_ASSERT(ss->top >= 2);
                 (void) PopOff(ss, op);
 
                 /*
                  * Special case: new (x(y)(z)) must be parenthesized like so.
                  * Same for new (x(y).z) -- contrast with new x(y).z.
                  * See PROPAGATE_CALLNESS.
                  */
-                op = (JSOp) ss->opcodes[ss->top-1];
+                op = (JSOp) ss->opcodes[ss->top - 1];
                 lval = PopStr(ss,
                               (saveop == JSOP_NEW &&
                                (op == JSOP_CALL || 
                                 op == JSOP_EVAL ||
                                 op == JSOP_APPLY ||
                                 (js_CodeSpec[op].format & JOF_CALLOP)))
                               ? JSOP_NAME
                               : saveop);
                 op = saveop;
 
                 argv[0] = JS_strdup(cx, lval);
-                if (!argv[i])
+                if (!argv[0])
                     ok = JS_FALSE;
 
                 lval = "(", rval = ")";
                 if (op == JSOP_NEW) {
                     if (argc == 0)
                         lval = rval = "";
                     todo = Sprint(&ss->sprinter, "%s %s%s",
                                   js_new_str, argv[0], lval);
@@ -3526,20 +3522,18 @@ Decompile(SprintStack *ss, jsbytecode *p
                                argv[i], (i < argc) ? ", " : "") < 0) {
                         ok = JS_FALSE;
                         break;
                     }
                 }
                 if (Sprint(&ss->sprinter, rval) < 0)
                     ok = JS_FALSE;
 
-                for (i = 0; i <= argc; i++) {
-                    if (argv[i])
-                        JS_free(cx, argv[i]);
-                }
+                for (i = 0; i <= argc; i++)
+                    JS_free(cx, argv[i]);
                 JS_free(cx, argv);
                 if (!ok)
                     return NULL;
 #if JS_HAS_LVALUE_RETURN
                 if (op == JSOP_SETCALL) {
                     if (!PushOff(ss, todo, op))
                         return NULL;
                     todo = Sprint(&ss->sprinter, "");
@@ -4282,63 +4276,58 @@ Decompile(SprintStack *ss, jsbytecode *p
                 todo = -2;
                 break;
 
               case JSOP_HOLE:
                 todo = SprintPut(&ss->sprinter, "", 0);
                 break;
 
               case JSOP_NEWARRAY:
-              {
-                ptrdiff_t off;
-                char *base, *from, *to;
-
-                /*
-                 * All operands are stacked and ready for in-place formatting.
-                 * We know that PAREN_SLOP is 3 here, and take advantage of it
-                 * to avoid strdup'ing.
-                 */
                 argc = GET_UINT24(pc);
                 LOCAL_ASSERT(ss->top >= (uintN) argc);
+                if (argc == 0) {
+                    todo = SprintCString(&ss->sprinter, "[]");
+                    break;
+                }
+
+                argv = (char **) JS_malloc(cx, size_t(argc) * sizeof *argv);
+                if (!argv)
+                    return NULL;
+
+                op = JSOP_SETNAME;
+                ok = JS_TRUE;
+                i = argc;
+                while (i > 0)
+                    argv[--i] = JS_strdup(cx, POP_STR());
+
+                todo = SprintCString(&ss->sprinter, "[");
+                if (todo < 0)
+                    break;
+
+                for (i = 0; i < argc; i++) {
+                    if (!argv[i] ||
+                        Sprint(&ss->sprinter, ss_format,
+                               argv[i], (i < argc - 1) ? ", " : "") < 0) {
+                        ok = JS_FALSE;
+                        break;
+                    }
+                }
+
+                for (i = 0; i < argc; i++)
+                    JS_free(cx, argv[i]);
+                JS_free(cx, argv);
+                if (!ok)
+                    return NULL;
+
                 sn = js_GetSrcNote(jp->script, pc);
-                if (argc == 0) {
-                    todo = Sprint(&ss->sprinter, "[%s]",
-                                  (sn && SN_TYPE(sn) == SRC_CONTINUE)
-                                  ? ", "
-                                  : "");
-                } else {
-                    ss->top -= argc;
-                    off = GetOff(ss, ss->top);
-                    LOCAL_ASSERT(off >= PAREN_SLOP);
-                    base = OFF2STR(&ss->sprinter, off);
-                    to = base + 1;
-                    i = 0;
-                    for (;;) {
-                        /* Move to the next string that had been stacked. */
-                        from = OFF2STR(&ss->sprinter, off);
-                        todo = strlen(from);
-                        memmove(to, from, todo);
-                        to += todo;
-                        if (++i == argc &&
-                            !(sn && SN_TYPE(sn) == SRC_CONTINUE)) {
-                            break;
-                        }
-                        *to++ = ',';
-                        *to++ = ' ';
-                        off = GetOff(ss, ss->top + i);
-                    }
-                    LOCAL_ASSERT(to - base < ss->sprinter.offset - PAREN_SLOP);
-                    *base = '[';
-                    *to++ = ']';
-                    *to = '\0';
-                    ss->sprinter.offset = STR2OFF(&ss->sprinter, to);
-                    todo = STR2OFF(&ss->sprinter, base);
-                }
+                if (sn && SN_TYPE(sn) == SRC_CONTINUE && SprintCString(&ss->sprinter, ", ") < 0)
+                    return NULL;
+                if (SprintCString(&ss->sprinter, "]") < 0)
+                    return NULL;
                 break;
-              }
 
               case JSOP_NEWINIT:
               {
                 i = GET_INT8(pc);
                 LOCAL_ASSERT(i == JSProto_Array || i == JSProto_Object);
 
                 todo = ss->sprinter.offset;
 #if JS_HAS_SHARP_VARS
--- a/js/src/jsopcode.tbl
+++ b/js/src/jsopcode.tbl
@@ -554,13 +554,22 @@ OPDEF(JSOP_INT8,          227, "int8",  
 OPDEF(JSOP_INT32,         228, "int32",        NULL,  5,  0,  1, 16,  JOF_INT32)
 
 /*
  * Get the value of the 'length' property from a stacked object.
  */
 OPDEF(JSOP_LENGTH,        229, "length",       NULL,  1,  1,  1, 18,  JOF_BYTE|JOF_PROP)
 
 /*
- * JSOP_NEWARRAY optimizes array literal evaluation using the interpreter stack.
- * JSOP_HOLE pushes a JSVAL_HOLE (used with JSOP_NEWINIT and JSOP_NEWARRAY).
+ * Construct a new dense array whose contents are the values provided on the
+ * stack, consuming those values and replacing them with the newly-constructed
+ * array.  The topmost value is the last value in the new array, and the
+ * bottommost value is the first value in the array; the array length is a
+ * 24-bit immediate operand to the instruction.
  */
 OPDEF(JSOP_NEWARRAY,      230, "newarray",     NULL,  4, -1,  1, 19,  JOF_UINT24)
+
+/*
+ * Push a JSVAL_HOLE value onto the stack, representing an omitted property in
+ * an array literal (e.g. property 0 in the array [, 1]).  This opcode is used
+ * with the JSOP_NEWARRAY and JSOP_NEWINIT opcodes.
+ */
 OPDEF(JSOP_HOLE,          231, "hole",         NULL,  1,  0,  1,  0,  JOF_BYTE)
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -4560,18 +4560,17 @@ TraceRecorder::incProp(jsint incr, bool 
 
     if (slot == SPROP_INVALID_SLOT)
         ABORT_TRACE("incProp on invalid slot");
 
     jsval& v = STOBJ_GET_SLOT(obj, slot);
     if (!inc(v, v_ins, incr, pre))
         return false;
 
-    if (!box_jsval(v, v_ins))
-        return false;
+    box_jsval(v, v_ins);
 
     LIns* dslots_ins = NULL;
     stobj_set_slot(obj_ins, slot, dslots_ins, v_ins);
     return true;
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::incElem(jsint incr, bool pre)
@@ -4582,18 +4581,17 @@ TraceRecorder::incElem(jsint incr, bool 
     LIns* v_ins;
     LIns* addr_ins;
     if (!elem(l, r, vp, v_ins, addr_ins))
         return false;
     if (!addr_ins) // if we read a hole, abort
         return false;
     if (!inc(*vp, v_ins, incr, pre))
         return false;
-    if (!box_jsval(*vp, v_ins))
-        return false;
+    box_jsval(*vp, v_ins);
     lir->insStorei(v_ins, addr_ins, 0);
     return true;
 }
 
 static bool
 evalCmp(LOpcode op, double result)
 {
     bool cond;
@@ -5277,37 +5275,37 @@ TraceRecorder::native_get(LIns* obj_ins,
     else
         v_ins = INS_CONST(JSVAL_TO_BOOLEAN(JSVAL_VOID));
     return true;
 }
 
 // So box_jsval can emit no LIR_or at all to tag an object jsval.
 JS_STATIC_ASSERT(JSVAL_OBJECT == 0);
 
-JS_REQUIRES_STACK bool
+JS_REQUIRES_STACK void
 TraceRecorder::box_jsval(jsval v, LIns*& v_ins)
 {
     if (isNumber(v)) {
         LIns* args[] = { v_ins, cx_ins };
         v_ins = lir->insCall(&js_BoxDouble_ci, args);
         guard(false, lir->ins2(LIR_eq, v_ins, INS_CONST(JSVAL_ERROR_COOKIE)),
               OOM_EXIT);
-        return true;
+        return;
     }
     switch (JSVAL_TAG(v)) {
       case JSVAL_BOOLEAN:
         v_ins = lir->ins2i(LIR_pior, lir->ins2i(LIR_pilsh, v_ins, JSVAL_TAGBITS), JSVAL_BOOLEAN);
-        return true;
+        return;
       case JSVAL_OBJECT:
-        return true;
-      case JSVAL_STRING:
+        return;
+      default:
+        JS_ASSERT(JSVAL_TAG(v) == JSVAL_STRING);
         v_ins = lir->ins2(LIR_pior, v_ins, INS_CONST(JSVAL_STRING));
-        return true;
-    }
-    return false;
+        return;
+    }
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::unbox_jsval(jsval v, LIns*& v_ins)
 {
     if (isNumber(v)) {
         // JSVAL_IS_NUMBER(v)
         guard(false,
@@ -5584,18 +5582,17 @@ TraceRecorder::record_JSOP_PUSH()
     return true;
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::record_JSOP_POPV()
 {
     jsval& rval = stackval(-1);
     LIns *rval_ins = get(&rval);
-    if (!box_jsval(rval, rval_ins))
-        return false;
+    box_jsval(rval, rval_ins);
 
     // Store it in cx->fp->rval. NB: Tricky dependencies. cx->fp is the right
     // frame because POPV appears only in global and eval code and we don't
     // trace JSOP_EVAL or leaving the frame where tracing started.
     LIns *fp_ins = lir->insLoad(LIR_ldp, cx_ins, offsetof(JSContext, fp));
     lir->insStorei(rval_ins, fp_ins, offsetof(JSStackFrame, rval));
     return true;
 }
@@ -6022,18 +6019,17 @@ TraceRecorder::newArray(JSObject *ctor, 
         LIns *args[] = { INS_CONST(argc), proto_ins, cx_ins };
         arr_ins = lir->insCall(&js_NewUninitializedArray_ci, args);
         guard(false, lir->ins_eq0(arr_ins), OOM_EXIT);
 
         // arr->dslots[i] = box_jsval(vp[i]);  for i in 0..argc
         LIns *dslots_ins = NULL;
         for (uint32 i = 0; i < argc; i++) {
             LIns *elt_ins = get(argv + i);
-            if (!box_jsval(argv[i], elt_ins))
-                return false;
+            box_jsval(argv[i], elt_ins);
             stobj_set_dslot(arr_ins, i, dslots_ins, elt_ins, "set_array_elt");
         }
     }
     set(rval, arr_ins);
     return true;
 }
 
 JS_REQUIRES_STACK bool
@@ -6159,18 +6155,17 @@ TraceRecorder::functionCall(bool constru
                     goto next_specialization;
             } else if (argtype == 'r') {
                 if (!VALUE_IS_REGEXP(cx, arg))
                     goto next_specialization;
             } else if (argtype == 'f') {
                 if (!VALUE_IS_FUNCTION(cx, arg))
                     goto next_specialization;
             } else if (argtype == 'v') {
-                if (!box_jsval(arg, *argp))
-                    return false;
+                box_jsval(arg, *argp);
             } else {
                 goto next_specialization;
             }
             argp--;
         }
 
         /*
          * If we got this far, and we have a charCodeAt, check that charCodeAt
@@ -6452,18 +6447,17 @@ TraceRecorder::record_SetPropHit(JSPropC
         LIns* args[] = { INS_CONSTPTR(sprop), obj_ins, cx_ins };
         LIns* ok_ins = lir->insCall(&js_AddProperty_ci, args);
         guard(false, lir->ins_eq0(ok_ins), OOM_EXIT);
     }
 
     LIns* dslots_ins = NULL;
     LIns* v_ins = get(&r);
     LIns* boxed_ins = v_ins;
-    if (!box_jsval(r, boxed_ins))
-        return false;
+    box_jsval(r, boxed_ins);
     if (!native_set(obj_ins, sprop, dslots_ins, boxed_ins))
         return false;
 
     if (*pc != JSOP_INITPROP && pc[JSOP_SETPROP_LENGTH] != JSOP_POP)
         set(&l, v_ins);
     return true;
 }
 
@@ -6584,18 +6578,17 @@ TraceRecorder::record_JSOP_SETELEM()
 
     JSObject* obj = JSVAL_TO_OBJECT(lval);
     LIns* obj_ins = get(&lval);
     LIns* idx_ins = get(&idx);
     LIns* v_ins = get(&v);
     jsid id;
 
     LIns* boxed_v_ins = v_ins;
-    if (!box_jsval(v, boxed_v_ins))
-        ABORT_TRACE("boxing JSOP_SETELEM value");
+    box_jsval(v, boxed_v_ins);
 
     if (JSVAL_IS_STRING(idx)) {
         if (!js_ValueToStringId(cx, idx, &id))
             return false;
         // Store the interned string to the stack to save the interpreter from redoing this work.
         idx = ID_TO_VALUE(id);
         if (!guardElemOp(obj, obj_ins, id, offsetof(JSObjectOps, setProperty), NULL))
             return false;
@@ -7294,28 +7287,16 @@ TraceRecorder::record_JSOP_NEWINIT()
     return true;
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::record_JSOP_ENDINIT()
 {
     jsval& v = stackval(-1);
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(v));
-    JSObject* obj = JSVAL_TO_OBJECT(v);
-    if (OBJ_IS_DENSE_ARRAY(cx, obj)) {
-        // Until we get JSOP_NEWARRAY working, we do our optimizing here...
-        if (obj->fslots[JSSLOT_ARRAY_LENGTH] == 1 &&
-            obj->dslots && JSVAL_IS_STRING(obj->dslots[0])) {
-            LIns* v_ins = get(&v);
-            JS_ASSERT(v_ins->isCall() && v_ins->callInfo() == &js_FastNewArray_ci);
-            LIns* args[] = { stack(1), callArgN(v_ins, 1), cx_ins };
-            v_ins = lir->insCall(&js_Array_1str_ci, args);
-            set(&v, v_ins);
-        }
-    }
     return true;
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::record_JSOP_INITPROP()
 {
     // All the action is in record_SetPropHit.
     return true;
@@ -8594,17 +8575,43 @@ TraceRecorder::record_JSOP_LENGTH()
     LIns* v_ins = lir->ins1(LIR_i2f, stobj_get_fslot(get(&l), JSSLOT_ARRAY_LENGTH));
     set(&l, v_ins);
     return true;
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::record_JSOP_NEWARRAY()
 {
-    return false;
+    JSObject* proto;
+    const CallInfo* ci = &js_NewUninitializedArray_ci;
+    if (!js_GetClassPrototype(cx, globalObj, INT_TO_JSID(JSProto_Array), &proto))
+        return false;
+
+    uint32 len = GET_UINT24(cx->fp->regs->pc);
+    LIns* args[] = { lir->insImm(len), INS_CONSTPTR(proto), cx_ins };
+    LIns* v_ins = lir->insCall(ci, args);
+    guard(false, lir->ins_eq0(v_ins), OOM_EXIT);
+
+    // De-optimize when we might have setters on Array.prototype.
+    LIns* rt_ins = lir->insLoad(LIR_ldp, cx_ins, offsetof(JSContext, runtime));
+    guard(true,
+          lir->ins_eq0(lir->insLoad(LIR_ldp, rt_ins,
+                                    offsetof(JSRuntime, anyArrayProtoHasElement))),
+          MISMATCH_EXIT);
+
+    LIns* dslots_ins = NULL;
+    for (uint32 i = 0; i < len; i++) {
+        jsval& v = stackval(-len + i);
+        LIns* elt_ins = get(&v);
+        box_jsval(v, elt_ins);
+        stobj_set_dslot(v_ins, i, dslots_ins, elt_ins, "set_array_elt");
+    }
+
+    stack(-len, v_ins);
+    return true;
 }
 
 JS_REQUIRES_STACK bool
 TraceRecorder::record_JSOP_HOLE()
 {
     stack(0, INS_CONST(JSVAL_TO_BOOLEAN(JSVAL_HOLE)));
     return true;
 }
--- a/js/src/jstracer.h
+++ b/js/src/jstracer.h
@@ -409,17 +409,17 @@ class TraceRecorder : public avmplus::GC
     JS_REQUIRES_STACK bool prop(JSObject* obj, nanojit::LIns* obj_ins, uint32& slot,
                                 nanojit::LIns*& v_ins);
     JS_REQUIRES_STACK bool elem(jsval& oval, jsval& idx, jsval*& vp, nanojit::LIns*& v_ins,
                                 nanojit::LIns*& addr_ins);
     JS_REQUIRES_STACK bool getProp(JSObject* obj, nanojit::LIns* obj_ins);
     JS_REQUIRES_STACK bool getProp(jsval& v);
     JS_REQUIRES_STACK bool getThis(nanojit::LIns*& this_ins);
 
-    JS_REQUIRES_STACK bool box_jsval(jsval v, nanojit::LIns*& v_ins);
+    JS_REQUIRES_STACK void box_jsval(jsval v, nanojit::LIns*& v_ins);
     JS_REQUIRES_STACK bool unbox_jsval(jsval v, nanojit::LIns*& v_ins);
     JS_REQUIRES_STACK bool guardClass(JSObject* obj, nanojit::LIns* obj_ins, JSClass* clasp,
                                       ExitType exitType = MISMATCH_EXIT);
     JS_REQUIRES_STACK bool guardDenseArray(JSObject* obj, nanojit::LIns* obj_ins,
                                            ExitType exitType = MISMATCH_EXIT);
     JS_REQUIRES_STACK bool guardDenseArrayIndex(JSObject* obj, jsint idx, nanojit::LIns* obj_ins,
                                                 nanojit::LIns* dslots_ins, nanojit::LIns* idx_ins,
                                                 ExitType exitType);