Remove emitter special casing for __proto__, bug 717249. r=waldo
☠☠ backed out by 408ab9247ef1 ☠ ☠
authorBrian Hackett <bhackett1024@gmail.com>
Fri, 20 Jan 2012 07:14:55 -0800
changeset 86226 5cdf9574bedecfbd8ce311a64cb6de2cd199868d
parent 86225 82ec65fda578ebe3410a5605583ff5c81c5c992f
child 86227 0ed3beaf82473b5bc051244c140f86f3a9346e7a
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswaldo
bugs717249
milestone12.0a1
Remove emitter special casing for __proto__, bug 717249. r=waldo
js/src/frontend/BytecodeEmitter.cpp
js/src/jit-test/tests/basic/bug717249.js
js/src/jsarray.h
js/src/jsobj.cpp
js/src/jsobjinlines.h
js/src/methodjit/PolyIC.cpp
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -1942,59 +1942,25 @@ EmitElemOpBase(JSContext *cx, BytecodeEm
         return false;
     CheckTypeSet(cx, bce, op);
     if (op == JSOP_CALLELEM)
         return Emit1(cx, bce, JSOP_SWAP) >= 0;
     return true;
 }
 
 static bool
-EmitSpecialPropOp(JSContext *cx, ParseNode *pn, JSOp op, BytecodeEmitter *bce)
-{
-    /*
-     * Special case for obj.__proto__ to deoptimize away from fast paths in the
-     * interpreter and trace recorder, which skip dense array instances by
-     * going up to Array.prototype before looking up the property name.
-     */
-    jsatomid index;
-    if (!bce->makeAtomIndex(pn->pn_atom, &index))
-        return false;
-    if (!EmitIndexOp(cx, JSOP_QNAMEPART, index, bce))
-        return false;
-
-    if (op == JSOP_CALLELEM && Emit1(cx, bce, JSOP_DUP) < 0)
-        return false;
-
-    if (!EmitElemOpBase(cx, bce, op))
-        return false;
-
-    if (op == JSOP_CALLELEM && Emit1(cx, bce, JSOP_SWAP) < 0)
-        return false;
-
-    return true;
-}
-
-static bool
 EmitPropOp(JSContext *cx, ParseNode *pn, JSOp op, BytecodeEmitter *bce,
            JSBool callContext, JSOp *psuffix = NULL)
 {
     ParseNode *pn2, *pndot, *pnup, *pndown;
     ptrdiff_t top;
 
     JS_ASSERT(pn->isArity(PN_NAME));
     pn2 = pn->maybeExpr();
 
-    /* Special case deoptimization for __proto__. */
-    if ((op == JSOP_GETPROP || op == JSOP_CALLPROP) &&
-        pn->pn_atom == cx->runtime->atomState.protoAtom) {
-        if (pn2 && !EmitTree(cx, bce, pn2))
-            return false;
-        return EmitSpecialPropOp(cx, pn, callContext ? JSOP_CALLELEM : JSOP_GETELEM, bce);
-    }
-
     if (callContext) {
         JS_ASSERT(pn->isKind(PNK_DOT));
         JS_ASSERT(op == JSOP_GETPROP);
         op = JSOP_CALLPROP;
     } else if (op == JSOP_GETPROP && pn->isKind(PNK_DOT)) {
         if (pn2->isKind(PNK_NAME)) {
             if (!BindNameToSlot(cx, bce, pn2))
                 return false;
@@ -2026,23 +1992,18 @@ EmitPropOp(JSContext *cx, ParseNode *pn,
         if (!EmitTree(cx, bce, pndown))
             return false;
 
         do {
             /* Walk back up the list, emitting annotated name ops. */
             if (NewSrcNote2(cx, bce, SRC_PCBASE, bce->offset() - pndown->pn_offset) < 0)
                 return false;
 
-            /* Special case deoptimization on __proto__, as above. */
-            if (pndot->isArity(PN_NAME) && pndot->pn_atom == cx->runtime->atomState.protoAtom) {
-                if (!EmitSpecialPropOp(cx, pndot, JSOP_GETELEM, bce))
-                    return false;
-            } else if (!EmitAtomOp(cx, pndot, pndot->getOp(), bce)) {
+            if (!EmitAtomOp(cx, pndot, pndot->getOp(), bce))
                 return false;
-            }
 
             /* Reverse the pn_expr link again. */
             pnup = pndot->pn_expr;
             pndot->pn_expr = pndown;
             pndown = pndot;
         } while ((pndot = pnup) != NULL);
     } else {
         if (!EmitTree(cx, bce, pn2))
@@ -3773,29 +3734,23 @@ EmitAssignment(JSContext *cx, BytecodeEm
             } else if (lhs->isOp(JSOP_SETGNAME)) {
                 if (!BindGlobal(cx, bce, lhs, lhs->pn_atom))
                     return false;
                 EmitAtomOp(cx, lhs, JSOP_GETGNAME, bce);
             } else {
                 EMIT_UINT16_IMM_OP(lhs->isOp(JSOP_SETARG) ? JSOP_GETARG : JSOP_GETLOCAL, atomIndex);
             }
             break;
-          case PNK_DOT:
+          case PNK_DOT: {
             if (Emit1(cx, bce, JSOP_DUP) < 0)
                 return false;
-            if (lhs->pn_atom == cx->runtime->atomState.protoAtom) {
-                if (!EmitIndexOp(cx, JSOP_QNAMEPART, atomIndex, bce))
-                    return false;
-                if (!EmitElemOpBase(cx, bce, JSOP_GETELEM))
-                    return false;
-            } else {
-                bool isLength = (lhs->pn_atom == cx->runtime->atomState.lengthAtom);
-                EMIT_INDEX_OP(isLength ? JSOP_LENGTH : JSOP_GETPROP, atomIndex);
-            }
+            bool isLength = (lhs->pn_atom == cx->runtime->atomState.lengthAtom);
+            EMIT_INDEX_OP(isLength ? JSOP_LENGTH : JSOP_GETPROP, atomIndex);
             break;
+          }
           case PNK_LB:
           case PNK_LP:
 #if JS_HAS_XML_SUPPORT
           case PNK_XMLUNARY:
 #endif
             if (Emit1(cx, bce, JSOP_DUP2) < 0)
                 return false;
             if (!EmitElemOpBase(cx, bce, JSOP_GETELEM))
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug717249.js
@@ -0,0 +1,2 @@
+// |jit-test| error: TypeError
+[].__proto__();
--- a/js/src/jsarray.h
+++ b/js/src/jsarray.h
@@ -69,38 +69,16 @@ js_IdIsIndex(jsid id, jsuint *indexp)
     }
 
     if (JS_UNLIKELY(!JSID_IS_STRING(id)))
         return JS_FALSE;
 
     return js::StringIsArrayIndex(JSID_TO_ATOM(id), indexp);
 }
 
-/*
- * Dense arrays are not native -- aobj->isNative() for a dense array aobj
- * results in false, meaning aobj->map does not point to a js::Shape.
- *
- * But Array methods are called via aobj.sort(), e.g., and the interpreter and
- * the trace recorder must consult the property cache in order to perform well.
- * The cache works only for native objects.
- *
- * Therefore the interpreter (js_Interpret in JSOP_GETPROP and JSOP_CALLPROP)
- * and js_GetPropertyHelper use this inline function to skip up one link in the
- * prototype chain when obj is a dense array, in order to find a native object
- * (to wit, Array.prototype) in which to probe for cached methods.
- *
- * Note that setting aobj.__proto__ for a dense array aobj turns aobj into a
- * slow array, avoiding the neede to skip.
- *
- * Callers of js_GetProtoIfDenseArray must take care to use the original object
- * (obj) for the |this| value of a getter, setter, or method call (bug 476447).
- */
-inline JSObject *
-js_GetProtoIfDenseArray(JSObject *obj);
-
 extern JSObject *
 js_InitArrayClass(JSContext *cx, JSObject *obj);
 
 extern bool
 js_InitContextBusyArrayTable(JSContext *cx);
 
 namespace js {
 
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -5354,26 +5354,25 @@ js_NativeSet(JSContext *cx, JSObject *ob
 
     return true;
 }
 
 static JS_ALWAYS_INLINE JSBool
 js_GetPropertyHelperInline(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id,
                            uint32_t getHow, Value *vp)
 {
-    JSObject *aobj, *obj2;
+    JSObject *obj2;
     JSProperty *prop;
     const Shape *shape;
 
     /* Convert string indices to integers if appropriate. */
     id = js_CheckForStringIndex(id);
 
-    aobj = js_GetProtoIfDenseArray(obj);
     /* This call site is hot -- use the always-inlined variant of LookupPropertyWithFlags(). */
-    if (!LookupPropertyWithFlagsInline(cx, aobj, id, cx->resolveFlags, &obj2, &prop))
+    if (!LookupPropertyWithFlagsInline(cx, obj, id, cx->resolveFlags, &obj2, &prop))
         return false;
 
     if (!prop) {
         vp->setUndefined();
 
         if (!CallJSPropertyOp(cx, obj->getClass()->getProperty, obj, id, vp))
             return JS_FALSE;
 
@@ -5437,17 +5436,17 @@ js_GetPropertyHelperInline(JSContext *cx
         return obj2->isProxy()
                ? Proxy::get(cx, obj2, receiver, id, vp)
                : obj2->getGeneric(cx, id, vp);
     }
 
     shape = (Shape *) prop;
 
     if (getHow & JSGET_CACHE_RESULT)
-        JS_PROPERTY_CACHE(cx).fill(cx, aobj, 0, obj2, shape);
+        JS_PROPERTY_CACHE(cx).fill(cx, obj, 0, obj2, shape);
 
     /* This call site is hot -- use the always-inlined variant of js_NativeGet(). */
     if (!js_NativeGetInline(cx, receiver, obj, obj2, shape, getHow, vp))
         return JS_FALSE;
 
     return JS_TRUE;
 }
 
--- a/js/src/jsobjinlines.h
+++ b/js/src/jsobjinlines.h
@@ -1990,22 +1990,16 @@ DefineConstructorAndPrototype(JSContext 
 extern JSObject *
 js_InitClass(JSContext *cx, js::HandleObject obj, JSObject *parent_proto,
              js::Class *clasp, JSNative constructor, uintN nargs,
              JSPropertySpec *ps, JSFunctionSpec *fs,
              JSPropertySpec *static_ps, JSFunctionSpec *static_fs,
              JSObject **ctorp = NULL,
              js::gc::AllocKind ctorKind = JSFunction::FinalizeKind);
 
-inline JSObject *
-js_GetProtoIfDenseArray(JSObject *obj)
-{
-    return obj->isDenseArray() ? obj->getProto() : obj;
-}
-
 /*
  * js_PurgeScopeChain does nothing if obj is not itself a prototype or parent
  * scope, else it reshapes the scope and prototype chains it links. It calls
  * js_PurgeScopeChainHelper, which asserts that obj is flagged as a delegate
  * (i.e., obj has ever been on a prototype or parent chain).
  */
 extern bool
 js_PurgeScopeChainHelper(JSContext *cx, JSObject *obj, jsid id);
--- a/js/src/methodjit/PolyIC.cpp
+++ b/js/src/methodjit/PolyIC.cpp
@@ -756,17 +756,29 @@ struct GetPropHelper {
             return ic.disable(cx, "non-native");
         if (!IsCacheableProtoChain(obj, holder))
             return ic.disable(cx, "non-native holder");
         shape = (const Shape *)prop;
         return Lookup_Cacheable;
     }
 
     LookupStatus lookup() {
-        JSObject *aobj = js_GetProtoIfDenseArray(obj);
+        /*
+         * Skip to the prototype of dense arrays, which are non-native objects
+         * and otherwise uncacheable. This is not valid to do for indexed
+         * properties (which will not be a PropertyName), nor for __proto__,
+         * which needs to be filtered here.
+         */
+        JSObject *aobj = obj;
+        if (obj->isDenseArray()) {
+            if (name == cx->runtime->atomState.protoAtom)
+                return ic.disable(cx, "__proto__");
+            aobj = obj->getProto();
+        }
+
         if (!aobj->isNative())
             return ic.disable(f, "non-native");
 
         RecompilationMonitor monitor(cx);
         if (!aobj->lookupProperty(cx, name, &holder, &prop))
             return ic.error(cx);
         if (monitor.recompiled())
             return Lookup_Uncacheable;
@@ -2599,18 +2611,18 @@ GetElementIC::attachTypedArray(VMFrame &
 #endif /* JS_METHODJIT_TYPED_ARRAY */
 
 LookupStatus
 GetElementIC::update(VMFrame &f, JSObject *obj, const Value &v, jsid id, Value *vp)
 {
     /*
      * Only treat this as a GETPROP for non-numeric string identifiers. The
      * GETPROP IC assumes the id has already gone through filtering for string
-     * indexes in the emitter, i.e. js_GetProtoIfDenseArray is only valid to
-     * use when looking up non-integer identifiers.
+     * indexes in the emitter, i.e. skipping to the prototype of dense arrays
+     * is only valid to do when looking up non-integer identifiers.
      */
     if (v.isString() && js_CheckForStringIndex(id) == id)
         return attachGetProp(f, obj, v, JSID_TO_ATOM(id)->asPropertyName(), vp);
 
     if (obj->isArguments())
         return attachArguments(f, obj, v, id, vp);
 
 #if defined JS_METHODJIT_TYPED_ARRAY