Bug 1514682 - Split up AddSlot IC logic r=jandem
authorTed Campbell <tcampbell@mozilla.com>
Sat, 16 Feb 2019 02:18:06 +0000
changeset 459896 e74ee65c18a6a836ed52088b1542e30a8ebf0955
parent 459895 4fcdd89bf518a85fde2279b0fc86fb86b0d9fbff
child 459897 1fc71d46c09a20908e6d016af6312ac1682d101f
push id78455
push usertcampbell@mozilla.com
push dateTue, 19 Feb 2019 13:38:52 +0000
treeherderautoland@e74ee65c18a6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1514682
milestone67.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
Bug 1514682 - Split up AddSlot IC logic r=jandem Differential Revision: https://phabricator.services.mozilla.com/D18631
js/src/jit-test/tests/cacheir/bug1514682.js
js/src/jit/BaselineIC.cpp
js/src/jit/CacheIR.cpp
js/src/jit/CacheIR.h
js/src/jit/IonIC.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/cacheir/bug1514682.js
@@ -0,0 +1,15 @@
+function f(o, n) {
+    if (n) {
+        o[n] = true;
+    } else {
+        o.x = true;
+    }
+}
+
+// Warm up object so HadElementsAccess check will trip next
+var o = {};
+for (var i = 0; i < 43; ++i) {
+    o["x" + i] = true;
+}
+
+f(o, "y");
--- a/js/src/jit/BaselineIC.cpp
+++ b/js/src/jit/BaselineIC.cpp
@@ -2227,26 +2227,27 @@ static bool DoSetElemFallback(JSContext*
     MOZ_ASSERT(!oldShape);
     if (UnboxedExpandoObject* expando =
             obj->as<UnboxedPlainObject>().maybeExpando()) {
       oldShape = expando->lastProperty();
     }
   }
 
   bool isTemporarilyUnoptimizable = false;
+  bool canAddSlot = false;
   bool attached = false;
 
   if (stub->state().maybeTransition()) {
     stub->discardStubs(cx);
   }
 
   if (stub->state().canAttachStub()) {
     SetPropIRGenerator gen(cx, script, pc, CacheKind::SetElem,
                            stub->state().mode(), &isTemporarilyUnoptimizable,
-                           objv, index, rhs);
+                           &canAddSlot, objv, index, rhs);
     if (gen.tryAttachStub()) {
       ICStub* newStub = AttachBaselineCacheIRStub(
           cx, gen.writerRef(), gen.cacheKind(),
           BaselineCacheIRStubKind::Updated, frame->script(), stub, &attached);
       if (newStub) {
         JitSpew(JitSpew_BaselineIC, "  Attached SetElem CacheIR stub");
 
         SetUpdateStubData(newStub->toCacheIR_Updated(), gen.typeCheckInfo());
@@ -2306,18 +2307,18 @@ static bool DoSetElemFallback(JSContext*
   // to transition.
   if (stub->state().maybeTransition()) {
     stub->discardStubs(cx);
   }
 
   if (stub->state().canAttachStub()) {
     SetPropIRGenerator gen(cx, script, pc, CacheKind::SetElem,
                            stub->state().mode(), &isTemporarilyUnoptimizable,
-                           objv, index, rhs);
-    if (gen.tryAttachAddSlotStub(oldGroup, oldShape)) {
+                           &canAddSlot, objv, index, rhs);
+    if (canAddSlot && gen.tryAttachAddSlotStub(oldGroup, oldShape)) {
       ICStub* newStub = AttachBaselineCacheIRStub(
           cx, gen.writerRef(), gen.cacheKind(),
           BaselineCacheIRStubKind::Updated, frame->script(), stub, &attached);
       if (newStub) {
         JitSpew(JitSpew_BaselineIC, "  Attached SetElem CacheIR stub");
 
         SetUpdateStubData(newStub->toCacheIR_Updated(), gen.typeCheckInfo());
 
@@ -3013,27 +3014,28 @@ static bool DoSetPropFallback(JSContext*
     }
   }
 
   // There are some reasons we can fail to attach a stub that are temporary.
   // We want to avoid calling noteUnoptimizableAccess() if the reason we
   // failed to attach a stub is one of those temporary reasons, since we might
   // end up attaching a stub for the exact same access later.
   bool isTemporarilyUnoptimizable = false;
+  bool canAddSlot = false;
 
   bool attached = false;
   if (stub->state().maybeTransition()) {
     stub->discardStubs(cx);
   }
 
   if (stub->state().canAttachStub()) {
     RootedValue idVal(cx, StringValue(name));
     SetPropIRGenerator gen(cx, script, pc, CacheKind::SetProp,
                            stub->state().mode(), &isTemporarilyUnoptimizable,
-                           lhs, idVal, rhs);
+                           &canAddSlot, lhs, idVal, rhs);
     if (gen.tryAttachStub()) {
       ICStub* newStub = AttachBaselineCacheIRStub(
           cx, gen.writerRef(), gen.cacheKind(),
           BaselineCacheIRStubKind::Updated, frame->script(), stub, &attached);
       if (newStub) {
         JitSpew(JitSpew_BaselineIC, "  Attached SetProp CacheIR stub");
 
         SetUpdateStubData(newStub->toCacheIR_Updated(), gen.typeCheckInfo());
@@ -3091,18 +3093,18 @@ static bool DoSetPropFallback(JSContext*
   if (stub->state().maybeTransition()) {
     stub->discardStubs(cx);
   }
 
   if (stub->state().canAttachStub()) {
     RootedValue idVal(cx, StringValue(name));
     SetPropIRGenerator gen(cx, script, pc, CacheKind::SetProp,
                            stub->state().mode(), &isTemporarilyUnoptimizable,
-                           lhs, idVal, rhs);
-    if (gen.tryAttachAddSlotStub(oldGroup, oldShape)) {
+                           &canAddSlot, lhs, idVal, rhs);
+    if (canAddSlot && gen.tryAttachAddSlotStub(oldGroup, oldShape)) {
       ICStub* newStub = AttachBaselineCacheIRStub(
           cx, gen.writerRef(), gen.cacheKind(),
           BaselineCacheIRStubKind::Updated, frame->script(), stub, &attached);
       if (newStub) {
         JitSpew(JitSpew_BaselineIC, "  Attached SetProp CacheIR stub");
 
         SetUpdateStubData(newStub->toCacheIR_Updated(), gen.typeCheckInfo());
 
--- a/js/src/jit/CacheIR.cpp
+++ b/js/src/jit/CacheIR.cpp
@@ -3390,24 +3390,25 @@ bool IRGenerator::maybeGuardInt32Index(c
     return true;
   }
 
   return false;
 }
 
 SetPropIRGenerator::SetPropIRGenerator(
     JSContext* cx, HandleScript script, jsbytecode* pc, CacheKind cacheKind,
-    ICState::Mode mode, bool* isTemporarilyUnoptimizable, HandleValue lhsVal,
-    HandleValue idVal, HandleValue rhsVal, bool needsTypeBarrier,
-    bool maybeHasExtraIndexedProps)
+    ICState::Mode mode, bool* isTemporarilyUnoptimizable, bool* canAddSlot,
+    HandleValue lhsVal, HandleValue idVal, HandleValue rhsVal,
+    bool needsTypeBarrier, bool maybeHasExtraIndexedProps)
     : IRGenerator(cx, script, pc, cacheKind, mode),
       lhsVal_(lhsVal),
       idVal_(idVal),
       rhsVal_(rhsVal),
       isTemporarilyUnoptimizable_(isTemporarilyUnoptimizable),
+      canAddSlot_(canAddSlot),
       typeCheckInfo_(cx, needsTypeBarrier),
       preliminaryObjectAction_(PreliminaryObjectAction::None),
       attachedTypedArrayOOBStub_(false),
       maybeHasExtraIndexedProps_(maybeHasExtraIndexedProps) {}
 
 bool SetPropIRGenerator::tryAttachStub() {
   AutoAssertNoPendingException aanpe(cx_);
 
@@ -3460,16 +3461,19 @@ bool SetPropIRGenerator::tryAttachStub()
         }
         if (tryAttachWindowProxy(obj, objId, id, rhsValId)) {
           return true;
         }
         if (tryAttachProxy(obj, objId, id, rhsValId)) {
           return true;
         }
       }
+      if (canAttachAddSlotStub(obj, id)) {
+        *canAddSlot_ = true;
+      }
       return false;
     }
 
     if (IsPropertySetOp(JSOp(*pc_))) {
       if (tryAttachProxyElement(obj, objId, rhsValId)) {
         return true;
       }
     }
@@ -4509,16 +4513,89 @@ bool SetPropIRGenerator::tryAttachWindow
   typeCheckInfo_.set(windowObj->group(), id);
 
   EmitStoreSlotAndReturn(writer, windowObjId, windowObj, propShape, rhsId);
 
   trackAttached("WindowProxySlot");
   return true;
 }
 
+bool SetPropIRGenerator::canAttachAddSlotStub(HandleObject obj, HandleId id) {
+  // Special-case JSFunction resolve hook to allow redefining the 'prototype'
+  // property without triggering lazy expansion of property and object
+  // allocation.
+  if (obj->is<JSFunction>() && JSID_IS_ATOM(id, cx_->names().prototype)) {
+    MOZ_ASSERT(ClassMayResolveId(cx_->names(), obj->getClass(), id, obj));
+
+    // We check group->maybeInterpretedFunction() here and guard on the
+    // group. The group is unique for a particular function so this ensures
+    // we don't add the default prototype property to functions that don't
+    // have it.
+    JSFunction* fun = &obj->as<JSFunction>();
+    if (!obj->group()->maybeInterpretedFunction() ||
+        !fun->needsPrototypeProperty()) {
+      return false;
+    }
+
+    // If property exists this isn't an "add"
+    if (fun->lookupPure(id)) {
+      return false;
+    }
+  } else {
+    // Normal Case: If property exists this isn't an "add"
+    PropertyResult prop;
+    if (!LookupOwnPropertyPure(cx_, obj, id, &prop)) {
+      return false;
+    }
+    if (prop) {
+      return false;
+    }
+  }
+
+  // Object must be extensible.
+  if (!obj->nonProxyIsExtensible()) {
+    return false;
+  }
+
+  // Also watch out for addProperty hooks. Ignore the Array addProperty hook,
+  // because it doesn't do anything for non-index properties.
+  DebugOnly<uint32_t> index;
+  MOZ_ASSERT_IF(obj->is<ArrayObject>(), !IdIsIndex(id, &index));
+  if (!obj->is<ArrayObject>() && obj->getClass()->getAddProperty()) {
+    return false;
+  }
+
+  // Walk up the object prototype chain and ensure that all prototypes are
+  // native, and that all prototypes have no setter defined on the property.
+  for (JSObject* proto = obj->staticPrototype(); proto;
+       proto = proto->staticPrototype()) {
+    if (!proto->isNative()) {
+      return false;
+    }
+
+    // If prototype defines this property in a non-plain way, don't optimize.
+    Shape* protoShape = proto->as<NativeObject>().lookup(cx_, id);
+    if (protoShape && !protoShape->isDataDescriptor()) {
+      return false;
+    }
+
+    // Otherwise, if there's no such property, watch out for a resolve hook
+    // that would need to be invoked and thus prevent inlining of property
+    // addition. Allow the JSFunction resolve hook as it only defines plain
+    // data properties and we don't need to invoke it for objects on the
+    // proto chain.
+    if (ClassMayResolveId(cx_->names(), proto->getClass(), id, proto) &&
+        !proto->is<JSFunction>()) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 bool SetPropIRGenerator::tryAttachAddSlotStub(HandleObjectGroup oldGroup,
                                               HandleShape oldShape) {
   ValOperandId objValId(writer.setInputOperandId(0));
   ValOperandId rhsValId;
   if (cacheKind_ == CacheKind::SetProp) {
     rhsValId = ValOperandId(writer.setInputOperandId(1));
   } else {
     MOZ_ASSERT(cacheKind_ == CacheKind::SetElem);
@@ -4536,21 +4613,20 @@ bool SetPropIRGenerator::tryAttachAddSlo
 
   if (!lhsVal_.isObject() || !nameOrSymbol) {
     return false;
   }
 
   RootedObject obj(cx_, &lhsVal_.toObject());
 
   PropertyResult prop;
-  JSObject* holder;
-  if (!LookupPropertyPure(cx_, obj, id, &holder, &prop)) {
-    return false;
-  }
-  if (obj != holder) {
+  if (!LookupOwnPropertyPure(cx_, obj, id, &prop)) {
+    return false;
+  }
+  if (!prop) {
     return false;
   }
 
   Shape* propShape = nullptr;
   NativeObject* holderOrExpando = nullptr;
 
   if (obj->isNative()) {
     propShape = prop.shape();
@@ -4569,92 +4645,40 @@ bool SetPropIRGenerator::tryAttachAddSlo
       return false;
     }
     holderOrExpando = expando;
   }
 
   MOZ_ASSERT(propShape);
 
   // The property must be the last added property of the object.
-  if (holderOrExpando->lastProperty() != propShape) {
-    return false;
-  }
-
-  // Object must be extensible, oldShape must be immediate parent of
-  // current shape.
-  if (!obj->nonProxyIsExtensible() || propShape->previous() != oldShape) {
+  MOZ_RELEASE_ASSERT(holderOrExpando->lastProperty() == propShape);
+
+  // Old shape should be parent of new shape. Object flag updates may make this
+  // false even for simple data properties. It may be possible to support these
+  // transitions in the future, but ignore now for simplicity.
+  if (propShape->previous() != oldShape) {
     return false;
   }
 
   // Basic shape checks.
   if (propShape->inDictionary() || !propShape->isDataProperty() ||
       !propShape->writable()) {
     return false;
   }
 
-  // Watch out for resolve hooks.
-  if (ClassMayResolveId(cx_->names(), obj->getClass(), id, obj)) {
-    // The JSFunction resolve hook defines a (non-configurable and
-    // non-enumerable) |prototype| property on certain functions. Scripts
-    // often assign a custom |prototype| object and we want to optimize
-    // this |prototype| set and eliminate the default object allocation.
-    //
-    // We check group->maybeInterpretedFunction() here and guard on the
-    // group. The group is unique for a particular function so this ensures
-    // we don't add the default prototype property to functions that don't
-    // have it.
-    if (!obj->is<JSFunction>() || !JSID_IS_ATOM(id, cx_->names().prototype) ||
-        !oldGroup->maybeInterpretedFunction() ||
-        !obj->as<JSFunction>().needsPrototypeProperty()) {
-      return false;
-    }
-    MOZ_ASSERT(!propShape->configurable());
-    MOZ_ASSERT(!propShape->enumerable());
-  }
-
-  // Also watch out for addProperty hooks. Ignore the Array addProperty hook,
-  // because it doesn't do anything for non-index properties.
-  DebugOnly<uint32_t> index;
-  MOZ_ASSERT_IF(obj->is<ArrayObject>(), !IdIsIndex(id, &index));
-  if (!obj->is<ArrayObject>() && obj->getClass()->getAddProperty()) {
-    return false;
-  }
-
-  // Walk up the object prototype chain and ensure that all prototypes are
-  // native, and that all prototypes have no setter defined on the property.
-  for (JSObject* proto = obj->staticPrototype(); proto;
-       proto = proto->staticPrototype()) {
-    if (!proto->isNative()) {
-      return false;
-    }
-
-    // If prototype defines this property in a non-plain way, don't optimize.
-    Shape* protoShape = proto->as<NativeObject>().lookup(cx_, id);
-    if (protoShape && !protoShape->hasDefaultSetter()) {
-      return false;
-    }
-
-    // Otherwise, if there's no such property, watch out for a resolve hook
-    // that would need to be invoked and thus prevent inlining of property
-    // addition. Allow the JSFunction resolve hook as it only defines plain
-    // data properties and we don't need to invoke it for objects on the
-    // proto chain.
-    if (ClassMayResolveId(cx_->names(), proto->getClass(), id, proto) &&
-        !proto->is<JSFunction>()) {
-      return false;
-    }
-  }
-
   ObjOperandId objId = writer.guardIsObject(objValId);
   maybeEmitIdGuard(id);
 
   // In addition to guarding for type barrier, we need this group guard (or
-  // shape guard below) to ensure class is unchanged.
+  // shape guard below) to ensure class is unchanged. This group guard may also
+  // implay maybeInterpretedFunction() for the special-case of function
+  // prototype property set.
   MOZ_ASSERT(!oldGroup->hasUncacheableClass() || obj->is<ShapedObject>());
-  writer.guardGroupForTypeBarrier(objId, oldGroup);
+  writer.guardGroup(objId, oldGroup);
 
   // If we are adding a property to an object for which the new script
   // properties analysis hasn't been performed yet, make sure the stub fails
   // after we run the analysis as a group change may be required here. The
   // group change is not required for correctness but improves type
   // information elsewhere.
   AutoSweepObjectGroup sweep(oldGroup);
   if (oldGroup->newScript(sweep) && !oldGroup->newScript(sweep)->analyzed()) {
--- a/js/src/jit/CacheIR.h
+++ b/js/src/jit/CacheIR.h
@@ -701,25 +701,23 @@ class MOZ_RAII CacheIRWriter : public JS
   void guardNoAllocationMetadataBuilder() {
     writeOp(CacheOp::GuardNoAllocationMetadataBuilder);
   }
   void guardObjectGroupNotPretenured(ObjectGroup* group) {
     writeOp(CacheOp::GuardObjectGroupNotPretenured);
     addStubField(uintptr_t(group), StubField::Type::ObjectGroup);
   }
 
- private:
+ public:
   // Use (or create) a specialization below to clarify what constaint the
   // group guard is implying.
   void guardGroup(ObjOperandId obj, ObjectGroup* group) {
     writeOpWithOperandId(CacheOp::GuardGroup, obj);
     addStubField(uintptr_t(group), StubField::Type::ObjectGroup);
   }
-
- public:
   void guardGroupForProto(ObjOperandId obj, ObjectGroup* group) {
     MOZ_ASSERT(!group->hasUncacheableProto());
     guardGroup(obj, group);
   }
   void guardGroupForTypeBarrier(ObjOperandId obj, ObjectGroup* group) {
     // Typesets will always be a super-set of any typesets previously seen
     // for this group. If the type/group of a value being stored to a
     // property in this group is not known, a TypeUpdate IC chain should be
@@ -1798,16 +1796,17 @@ class MOZ_RAII PropertyTypeCheckInfo {
 };
 
 // SetPropIRGenerator generates CacheIR for a SetProp IC.
 class MOZ_RAII SetPropIRGenerator : public IRGenerator {
   HandleValue lhsVal_;
   HandleValue idVal_;
   HandleValue rhsVal_;
   bool* isTemporarilyUnoptimizable_;
+  bool* canAddSlot_;
   PropertyTypeCheckInfo typeCheckInfo_;
 
   enum class PreliminaryObjectAction { None, Unlink, NotePreliminary };
   PreliminaryObjectAction preliminaryObjectAction_;
   bool attachedTypedArrayOOBStub_;
 
   bool maybeHasExtraIndexedProps_;
 
@@ -1867,21 +1866,23 @@ class MOZ_RAII SetPropIRGenerator : publ
                                 HandleId id, ValOperandId rhsId);
   bool tryAttachProxy(HandleObject obj, ObjOperandId objId, HandleId id,
                       ValOperandId rhsId);
   bool tryAttachProxyElement(HandleObject obj, ObjOperandId objId,
                              ValOperandId rhsId);
   bool tryAttachMegamorphicSetElement(HandleObject obj, ObjOperandId objId,
                                       ValOperandId rhsId);
 
+  bool canAttachAddSlotStub(HandleObject obj, HandleId id);
+
  public:
   SetPropIRGenerator(JSContext* cx, HandleScript script, jsbytecode* pc,
                      CacheKind cacheKind, ICState::Mode mode,
-                     bool* isTemporarilyUnoptimizable, HandleValue lhsVal,
-                     HandleValue idVal, HandleValue rhsVal,
+                     bool* isTemporarilyUnoptimizable, bool* canAddSlot,
+                     HandleValue lhsVal, HandleValue idVal, HandleValue rhsVal,
                      bool needsTypeBarrier = true,
                      bool maybeHasExtraIndexedProps = true);
 
   bool tryAttachStub();
   bool tryAttachAddSlotStub(HandleObjectGroup oldGroup, HandleShape oldShape);
   void trackAttached(const char* name);
 
   bool shouldUnlinkPreliminaryObjectStubs() const {
--- a/js/src/jit/IonIC.cpp
+++ b/js/src/jit/IonIC.cpp
@@ -248,16 +248,17 @@ void IonIC::trace(JSTracer* trc) {
                                            HandleObject obj, HandleValue idVal,
                                            HandleValue rhs) {
   RootedShape oldShape(cx);
   RootedObjectGroup oldGroup(cx);
   IonScript* ionScript = outerScript->ionScript();
 
   bool attached = false;
   bool isTemporarilyUnoptimizable = false;
+  bool canAddSlot = false;
 
   if (ic->state().maybeTransition()) {
     ic->discardStubs(cx->zone());
   }
 
   if (ic->state().canAttachStub()) {
     oldShape = obj->maybeShape();
     oldGroup = JSObject::getGroup(cx, obj);
@@ -271,18 +272,19 @@ void IonIC::trace(JSTracer* trc) {
         oldShape = expando->lastProperty();
       }
     }
 
     RootedValue objv(cx, ObjectValue(*obj));
     RootedScript script(cx, ic->script());
     jsbytecode* pc = ic->pc();
     SetPropIRGenerator gen(cx, script, pc, ic->kind(), ic->state().mode(),
-                           &isTemporarilyUnoptimizable, objv, idVal, rhs,
-                           ic->needsTypeBarrier(), ic->guardHoles());
+                           &isTemporarilyUnoptimizable, &canAddSlot, objv,
+                           idVal, rhs, ic->needsTypeBarrier(),
+                           ic->guardHoles());
     if (gen.tryAttachStub()) {
       ic->attachCacheIRStub(cx, gen.writerRef(), gen.cacheKind(), ionScript,
                             &attached, gen.typeCheckInfo());
     }
   }
 
   jsbytecode* pc = ic->pc();
   if (ic->kind() == CacheKind::SetElem) {
@@ -335,19 +337,20 @@ void IonIC::trace(JSTracer* trc) {
     ic->discardStubs(cx->zone());
   }
 
   if (ic->state().canAttachStub()) {
     RootedValue objv(cx, ObjectValue(*obj));
     RootedScript script(cx, ic->script());
     jsbytecode* pc = ic->pc();
     SetPropIRGenerator gen(cx, script, pc, ic->kind(), ic->state().mode(),
-                           &isTemporarilyUnoptimizable, objv, idVal, rhs,
-                           ic->needsTypeBarrier(), ic->guardHoles());
-    if (gen.tryAttachAddSlotStub(oldGroup, oldShape)) {
+                           &isTemporarilyUnoptimizable, &canAddSlot, objv,
+                           idVal, rhs, ic->needsTypeBarrier(),
+                           ic->guardHoles());
+    if (canAddSlot && gen.tryAttachAddSlotStub(oldGroup, oldShape)) {
       ic->attachCacheIRStub(cx, gen.writerRef(), gen.cacheKind(), ionScript,
                             &attached, gen.typeCheckInfo());
     } else {
       gen.trackAttached(nullptr);
     }
 
     if (!attached && !isTemporarilyUnoptimizable) {
       ic->state().trackNotAttached();