Fix for bug 852092 (Improve DOM list ICs) - part 2, don't generate stubs if there's shadowing and enable the Ion IC for slot reads. r=jandem.
authorPeter Van der Beken <peterv@propagandism.org>
Thu, 28 Mar 2013 10:23:04 +0100
changeset 126560 7538d3ff1e1458d1e630fae17f55ecfc5f6eddbe
parent 126559 202716eedf730b17e24b3b47bf0d4d10aeb212f3
child 126561 6b3122df808da3ffeef8ef2ddcab9d1913ea64f3
push id24488
push userryanvm@gmail.com
push dateFri, 29 Mar 2013 00:54:52 +0000
treeherdermozilla-central@8aeabe064932 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs852092
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Fix for bug 852092 (Improve DOM list ICs) - part 2, don't generate stubs if there's shadowing and enable the Ion IC for slot reads. r=jandem.
js/src/ion/IonCaches.cpp
js/src/methodjit/PolyIC.cpp
--- a/js/src/ion/IonCaches.cpp
+++ b/js/src/ion/IonCaches.cpp
@@ -485,75 +485,74 @@ EmitLoadSlot(MacroAssembler &masm, JSObj
 
         Address addr(scratchReg, holder->dynamicSlotIndex(shape->slot()) * sizeof(Value));
         masm.loadTypedOrValue(addr, output);
     }
 }
 
 static void
 GenerateListBaseChecks(JSContext *cx, MacroAssembler &masm, JSObject *obj,
-                       PropertyName *name, Register object, Label &stubFailure)
+                       PropertyName *name, Register object, Label *stubFailure)
 {
     MOZ_ASSERT(IsCacheableListBase(obj));
 
     // Guard the following:
     //      1. The object is a ListBase.
     //      2. The object does not have expando properties, or has an expando
     //          which is known to not have the desired property.
     Address handlerAddr(object, JSObject::getFixedSlotOffset(JSSLOT_PROXY_HANDLER));
     Address expandoAddr(object, JSObject::getFixedSlotOffset(GetListBaseExpandoSlot()));
 
     // Check that object is a ListBase.
-    masm.branchPrivatePtr(Assembler::NotEqual, handlerAddr, ImmWord(GetProxyHandler(obj)), &stubFailure);
+    masm.branchPrivatePtr(Assembler::NotEqual, handlerAddr, ImmWord(GetProxyHandler(obj)), stubFailure);
 
     // For the remaining code, we need to reserve some registers to load a value.
     // This is ugly, but unvaoidable.
     RegisterSet listBaseRegSet(RegisterSet::All());
     listBaseRegSet.take(AnyRegister(object));
     ValueOperand tempVal = listBaseRegSet.takeValueOperand();
     masm.pushValue(tempVal);
 
     Label failListBaseCheck;
     Label listBaseOk;
 
-    Value expandoVal = obj->getFixedSlot(GetListBaseExpandoSlot());
-    JSObject *expando = expandoVal.isObject() ? &(expandoVal.toObject()) : NULL;
-    JS_ASSERT_IF(expando, expando->isNative() && expando->getProto() == NULL);
-
     masm.loadValue(expandoAddr, tempVal);
 
     // If the incoming object does not have an expando object then we're sure we're not
     // shadowing.
     masm.branchTestUndefined(Assembler::Equal, tempVal, &listBaseOk);
 
-    if (expando && !expando->nativeContains(cx, name)) {
+    Value expandoVal = obj->getFixedSlot(GetListBaseExpandoSlot());
+    if (expandoVal.isObject()) {
+        JS_ASSERT(!expandoVal.toObject().nativeContains(cx, name));
+
         // Reference object has an expando object that doesn't define the name. Check that
         // the incoming object has an expando object with the same shape.
         masm.branchTestObject(Assembler::NotEqual, tempVal, &failListBaseCheck);
         masm.extractObject(tempVal, tempVal.scratchReg());
         masm.branchPtr(Assembler::Equal,
                        Address(tempVal.scratchReg(), JSObject::offsetOfShape()),
-                       ImmGCPtr(expando->lastProperty()),
+                       ImmGCPtr(expandoVal.toObject().lastProperty()),
                        &listBaseOk);
     }
 
     // Failure case: restore the tempVal registers and jump to failures.
     masm.bind(&failListBaseCheck);
     masm.popValue(tempVal);
-    masm.jump(&stubFailure);
+    masm.jump(stubFailure);
 
     // Success case: restore the tempval and proceed.
     masm.bind(&listBaseOk);
     masm.popValue(tempVal);
 }
 
 static void
 GenerateReadSlot(JSContext *cx, MacroAssembler &masm, IonCache::StubAttacher &attacher,
-                 JSObject *obj, JSObject *holder, Shape *shape, Register object,
-                 TypedOrValueRegister output, Label *failures = NULL)
+                 JSObject *obj, PropertyName *name, JSObject *holder, Shape *shape,
+                 Register object, TypedOrValueRegister output, Label *failures = NULL)
 {
     // If there's a single jump to |failures|, we can patch the shape guard
     // jump directly. Otherwise, jump to the end of the stub, so there's a
     // common point to patch.
     bool multipleFailureJumps = (obj != holder) || (failures != NULL && failures->used());
 
     // If we have multiple failure jumps but didn't get a label from the
     // outside, make one ourselves.
@@ -562,16 +561,23 @@ GenerateReadSlot(JSContext *cx, MacroAss
         failures = &failures_;
 
     // Guard on the shape of the object.
     attacher.branchNextStubOrLabel(masm, Assembler::NotEqual,
                                    Address(object, JSObject::offsetOfShape()),
                                    ImmGCPtr(obj->lastProperty()),
                                    failures);
 
+    bool isCacheableListBase = IsCacheableListBase(obj);
+    Label listBaseFailures;
+    if (isCacheableListBase) {
+        JS_ASSERT(multipleFailureJumps);
+        GenerateListBaseChecks(cx, masm, obj, name, object, &listBaseFailures);
+    }
+
     // If we need a scratch register, use either an output register or the
     // object register. After this point, we cannot jump directly to
     // |failures| since we may still have to pop the object register.
     bool restoreScratch = false;
     Register scratchReg = Register::FromCode(0); // Quell compiler warning.
 
     if (obj != holder || !holder->isFixedSlot(shape->slot())) {
         if (output.hasValue()) {
@@ -650,16 +656,18 @@ GenerateReadSlot(JSContext *cx, MacroAss
         masm.pop(scratchReg);
 
     attacher.jumpRejoin(masm);
 
     if (multipleFailureJumps) {
         masm.bind(&prototypeFailures);
         if (restoreScratch)
             masm.pop(scratchReg);
+        if (isCacheableListBase)
+            masm.bind(&listBaseFailures);
         masm.bind(failures);
     }
 
     attacher.jumpNextStub(masm);
 
     if (restoreScratch)
         masm.pop(scratchReg);
 }
@@ -671,17 +679,17 @@ GenerateCallGetter(JSContext *cx, MacroA
                    void *returnAddr, jsbytecode *pc)
 {
     // Initial shape check.
     Label stubFailure;
     masm.branchPtr(Assembler::NotEqual, Address(object, JSObject::offsetOfShape()),
                    ImmGCPtr(obj->lastProperty()), &stubFailure);
 
     if (IsCacheableListBase(obj))
-        GenerateListBaseChecks(cx, masm, obj, name, object, stubFailure);
+        GenerateListBaseChecks(cx, masm, obj, name, object, &stubFailure);
 
     JS_ASSERT(output.hasValue());
     Register scratchReg = output.valueReg().scratchReg();
 
     // Note: this may clobber the object register if it's used as scratch.
     if (obj != holder)
         GeneratePrototypeGuards(cx, masm, obj, holder, object, scratchReg, &stubFailure);
 
@@ -847,17 +855,17 @@ GenerateCallGetter(JSContext *cx, MacroA
 
 bool
 GetPropertyIC::attachReadSlot(JSContext *cx, IonScript *ion, JSObject *obj, JSObject *holder,
                               HandleShape shape)
 {
     MacroAssembler masm(cx);
 
     RepatchStubAppender attacher(rejoinLabel(), fallbackLabel_, &lastJump_);
-    GenerateReadSlot(cx, masm, attacher, obj, holder, shape, object(), output());
+    GenerateReadSlot(cx, masm, attacher, obj, name(), holder, shape, object(), output());
 
     const char *attachKind = "non idempotent reading";
     if (idempotent())
         attachKind = "idempotent reading";
     return linkAndAttachStub(cx, masm, attacher, ion, attachKind);
 }
 
 bool
@@ -979,19 +987,29 @@ TryAttachNativeGetPropStub(JSContext *cx
                            GetPropertyIC &cache, HandleObject obj,
                            HandlePropertyName name,
                            const SafepointIndex *safepointIndex,
                            void *returnAddr, bool *isCacheable)
 {
     JS_ASSERT(!*isCacheable);
 
     RootedObject checkObj(cx, obj);
-    bool isListBase = IsCacheableListBase(obj);
-    if (isListBase)
+    if (IsCacheableListBase(obj)) {
+        Value expandoVal = obj->getFixedSlot(GetListBaseExpandoSlot());
+
+        // Expando objects just hold any extra properties the object has been given by a script,
+        // and have no prototype or anything else that will complicate property lookups on them.
+        JS_ASSERT_IF(expandoVal.isObject(),
+                     expandoVal.toObject().isNative() && !expandoVal.toObject().getProto());
+
+        if (expandoVal.isObject() && expandoVal.toObject().nativeContains(cx, name))
+            return true;
+
         checkObj = obj->getTaggedProto().toObjectOrNull();
+    }
 
     if (!checkObj || !checkObj->isNative())
         return true;
 
     // If the cache is idempotent, watch out for resolve hooks or non-native
     // objects on the proto chain. We check this before calling lookupProperty,
     // to make sure no effectful lookup hooks or resolve hooks are called.
     if (cache.idempotent() && !checkObj->hasIdempotentProtoChain())
@@ -1006,23 +1024,23 @@ TryAttachNativeGetPropStub(JSContext *cx
     // or a getter call.
     bool readSlot = false;
     bool callGetter = false;
 
     RootedScript script(cx);
     jsbytecode *pc;
     cache.getScriptedLocation(&script, &pc);
 
-    if (IsCacheableGetPropReadSlot(obj, holder, shape) ||
-        IsCacheableNoProperty(obj, holder, shape, pc, cache.output()))
+    if (IsCacheableGetPropReadSlot(checkObj, holder, shape) ||
+        IsCacheableNoProperty(checkObj, holder, shape, pc, cache.output()))
     {
         // With Proxies, we cannot garantee any property access as the proxy can
         // mask any property from the prototype chain.
-        if (!obj->isProxy())
-            readSlot = true;
+        JS_ASSERT(!checkObj->isProxy());
+        readSlot = true;
     } else if (IsCacheableGetPropCallNative(checkObj, holder, shape) ||
                IsCacheableGetPropCallPropertyOp(checkObj, holder, shape))
     {
         // Don't enable getter call if cache is idempotent, since
         // they can be effectful.
         if (!cache.idempotent() && cache.allowGetters())
             callGetter = true;
     }
@@ -1641,17 +1659,17 @@ GetElementIC::attachGetProp(JSContext *c
     Label failures;
     MacroAssembler masm(cx);
 
     // Guard on the index value.
     ValueOperand val = index().reg().valueReg();
     masm.branchTestValue(Assembler::NotEqual, val, idval, &failures);
 
     RepatchStubAppender attacher(rejoinLabel(), fallbackLabel_, &lastJump_);
-    GenerateReadSlot(cx, masm, attacher, obj, holder, shape, object(), output(),
+    GenerateReadSlot(cx, masm, attacher, obj, name, holder, shape, object(), output(),
                      &failures);
 
     return linkAndAttachStub(cx, masm, attacher, ion, "property");
 }
 
 bool
 GetElementIC::attachDenseElement(JSContext *cx, IonScript *ion, JSObject *obj, const Value &idval)
 {
--- a/js/src/methodjit/PolyIC.cpp
+++ b/js/src/methodjit/PolyIC.cpp
@@ -672,18 +672,30 @@ struct GetPropHelper {
         if (!IsCacheableProtoChain(obj, holder))
             return ic.disable(cx, "non-native holder");
         shape = prop;
         return Lookup_Cacheable;
     }
 
     LookupStatus lookup() {
         RootedObject aobj(cx, obj);
-        if (IsCacheableListBase(obj))
+        if (IsCacheableListBase(obj)) {
+            Value expandoValue = obj->getFixedSlot(GetListBaseExpandoSlot());
+
+            // Expando objects just hold any extra properties the object has been given by a
+            // script, and have no prototype or anything else that will complicate property
+            // lookups on them.
+            JS_ASSERT_IF(expandoValue.isObject(),
+                         expandoValue.toObject().isNative() && !expandoValue.toObject().getProto());
+
+            if (expandoValue.isObject() && expandoValue.toObject().nativeContains(cx, name))
+                return Lookup_Uncacheable;
+
             aobj = obj->getTaggedProto().toObjectOrNull();
+        }
 
         if (!aobj->isNative())
             return ic.disable(f, "non-native");
 
         RecompilationMonitor monitor(cx);
         if (!JSObject::lookupProperty(cx, aobj, name, &holder, &prop))
             return ic.error(cx);
         if (monitor.recompiled())
@@ -1288,37 +1300,36 @@ class GetPropCompiler : public PICStubCo
             Address handler(pic.objReg, JSObject::getFixedSlotOffset(JSSLOT_PROXY_HANDLER));
             Jump handlerGuard = masm.testPrivate(Assembler::NotEqual, handler, GetProxyHandler(obj));
             if (!shapeMismatches.append(handlerGuard))
                 return error();
 
             Address expandoAddress(pic.objReg, JSObject::getFixedSlotOffset(GetListBaseExpandoSlot()));
 
             Value expandoValue = obj->getFixedSlot(GetListBaseExpandoSlot());
-            JSObject *expando = expandoValue.isObject() ? &expandoValue.toObject() : NULL;
-
-            // Expando objects just hold any extra properties the object has
-            // been given by a script, and have no prototype or anything else
-            // that will complicate property lookups on them.
-            JS_ASSERT_IF(expando, expando->isNative() && expando->getProto() == NULL);
-
-            if (expando && !expando->nativeContains(cx, name)) {
+            if (expandoValue.isObject()) {
+                JS_ASSERT(!expandoValue.toObject().nativeContains(cx, name));
+
+                // Reference object has an expando object that doesn't define the name. Check that
+                // the incoming object has an expando object with the same shape.
                 Jump expandoGuard = masm.testObject(Assembler::NotEqual, expandoAddress);
                 if (!shapeMismatches.append(expandoGuard))
                     return error();
 
                 masm.loadPayload(expandoAddress, pic.shapeReg);
                 pic.shapeRegHasBaseShape = false;
 
                 Jump shapeGuard = masm.branchPtr(Assembler::NotEqual,
                                                  Address(pic.shapeReg, JSObject::offsetOfShape()),
-                                                 ImmPtr(expando->lastProperty()));
+                                                 ImmPtr(expandoValue.toObject().lastProperty()));
                 if (!shapeMismatches.append(shapeGuard))
                     return error();
             } else {
+                // If the incoming object does not have an expando object then
+                // we're sure we're not shadowing.
                 Jump expandoGuard = masm.testUndefined(Assembler::NotEqual, expandoAddress);
                 if (!shapeMismatches.append(expandoGuard))
                     return error();
             }
         }
 
         RegisterID holderReg = pic.objReg;
         if (obj != holder) {