Bug 1518753 part 5 - Stop using JSProtoKey for initial shapes. r=tcampbell
authorJan de Mooij <jdemooij@mozilla.com>
Sat, 12 Jan 2019 10:50:04 +0000
changeset 513616 cfa1c48c717048f00eb4811b5719cd716eb1e5b3
parent 513615 1d49da4facd722bc4da5ef9d7a0bff92c6e7d940
child 513617 02251eb9e2c15b86723ebcd838a569dfdd320c4a
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell
bugs1518753, 1299107
milestone66.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 1518753 part 5 - Stop using JSProtoKey for initial shapes. r=tcampbell I added this optimization in bug 1299107 to share more shapes across compartments. Unfortunately this doesn't play well with same-compartment realms (ICs can misbehave) because it relies on compartments being isolated from each other. I think we should remove this optimization: * Fixing the IC issue is impossible without deoptimizing everything. * I added it mainly for chrome globals. The shared-JSM-global work has eliminated the need for this there. * Same-compartment realms win memory back by eliminating CCWs etc. * It's quite a lot of complicated code. Differential Revision: https://phabricator.services.mozilla.com/D16170
js/src/jit-test/tests/basic/globals-shared-shapes.js
js/src/jit-test/tests/realms/bug1518753.js
js/src/vm/Shape.cpp
js/src/vm/Shape.h
--- a/js/src/jit-test/tests/basic/globals-shared-shapes.js
+++ b/js/src/jit-test/tests/basic/globals-shared-shapes.js
@@ -1,30 +1,13 @@
-// Test that objects in different compartments can have the same shape.
+// Test that objects in different compartments can have the same shape if they
+// have a null proto.
 
 var g1 = newGlobal();
 var g2 = newGlobal({sameZoneAs: g1});
 
 g1.eval("x1 = {foo: 1}");
 g2.eval("x2 = {foo: 2}");
-assertEq(unwrappedObjectsHaveSameShape(g1.x1, g2.x2), true);
-
-g1.eval("x1 = [1]");
-g2.eval("x2 = [2]");
-assertEq(unwrappedObjectsHaveSameShape(g1.x1, g2.x2), true);
-
-g1.eval("x1 = function f(){}");
-g2.eval("x2 = function f(){}");
-assertEq(unwrappedObjectsHaveSameShape(g1.x1, g2.x2), true);
+assertEq(unwrappedObjectsHaveSameShape(g1.x1, g2.x2), false);
 
-g1.eval("x1 = /abc/;");
-g2.eval("x2 = /def/");
+g1.eval("x1 = Object.create(null); x1.foo = 1;");
+g2.eval("x2 = Object.create(null); x2.foo = 2;");
 assertEq(unwrappedObjectsHaveSameShape(g1.x1, g2.x2), true);
-
-// Now the same, but we change Array.prototype.__proto__.
-// The arrays should no longer get the same Shape, as their
-// proto chain is different.
-g1 = newGlobal();
-g2 = newGlobal({sameZoneAs: g1});
-g1.eval("x1 = [1];");
-g2.eval("Array.prototype.__proto__ = Math;");
-g2.eval("x2 = [2];");
-assertEq(unwrappedObjectsHaveSameShape(g1.x1, g2.x2), false);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/realms/bug1518753.js
@@ -0,0 +1,13 @@
+var g = newGlobal({sameCompartmentAs: this});
+
+var o1 = Array(1, 2);
+var o2 = g.Array(1, 2);
+Array.prototype.x = 10;
+
+function test(o, v) {
+    for (var i = 0; i < 15; i++) {
+        assertEq(o.x, v);
+    }
+}
+test(o1, 10);
+test(o2, undefined);
--- a/js/src/vm/Shape.cpp
+++ b/js/src/vm/Shape.cpp
@@ -1588,41 +1588,40 @@ void BaseShape::finalize(FreeOp* fop) {
     fop->delete_(table_);
     table_ = nullptr;
   }
 }
 
 inline InitialShapeEntry::InitialShapeEntry() : shape(nullptr), proto() {}
 
 inline InitialShapeEntry::InitialShapeEntry(Shape* shape,
-                                            const Lookup::ShapeProto& proto)
+                                            const TaggedProto& proto)
     : shape(shape), proto(proto) {}
 
 #ifdef JSGC_HASH_TABLE_CHECKS
 
 void Zone::checkInitialShapesTableAfterMovingGC() {
   /*
    * Assert that the postbarriers have worked and that nothing is left in
    * initialShapes that points into the nursery, and that the hash table
    * entries are discoverable.
    */
   for (auto r = initialShapes().all(); !r.empty(); r.popFront()) {
     InitialShapeEntry entry = r.front();
-    JSProtoKey protoKey = entry.proto.key();
-    TaggedProto proto = entry.proto.proto().unbarrieredGet();
+    TaggedProto proto = entry.proto.unbarrieredGet();
     Shape* shape = entry.shape.unbarrieredGet();
 
     CheckGCThingAfterMovingGC(shape);
     if (proto.isObject()) {
       CheckGCThingAfterMovingGC(proto.toObject());
     }
 
     using Lookup = InitialShapeEntry::Lookup;
-    Lookup lookup(shape->getObjectClass(), Lookup::ShapeProto(protoKey, proto),
-                  shape->numFixedSlots(), shape->getObjectFlags());
+    Lookup lookup(shape->getObjectClass(), proto, shape->numFixedSlots(),
+                  shape->getObjectFlags());
     InitialShapeSet::Ptr ptr = initialShapes().lookup(lookup);
     MOZ_RELEASE_ASSERT(ptr.found() && &*ptr == &r.front());
   }
 }
 
 #endif  // JSGC_HASH_TABLE_CHECKS
 
 Shape* EmptyShape::new_(JSContext* cx, Handle<UnownedBaseShape*> base,
@@ -2048,135 +2047,49 @@ void Shape::dumpSubtree(int level, js::G
         kid->dumpSubtree(level, out);
       }
     }
   }
 }
 
 #endif
 
-static bool IsOriginalProto(GlobalObject* global, JSProtoKey key,
-                            NativeObject& proto) {
-  if (global->getPrototype(key) != ObjectValue(proto)) {
-    return false;
-  }
-
-  MOZ_ASSERT(&proto.global() == global);
-
-  if (key == JSProto_Object) {
-    MOZ_ASSERT(proto.staticPrototypeIsImmutable(),
-               "proto should be Object.prototype, whose prototype is "
-               "immutable");
-    MOZ_ASSERT(proto.staticPrototype() == nullptr,
-               "Object.prototype must have null prototype");
-    return true;
-  }
-
-  // Check that other prototypes still have Object.prototype as proto.
-  JSObject* protoProto = proto.staticPrototype();
-  if (!protoProto ||
-      global->getPrototype(JSProto_Object) != ObjectValue(*protoProto)) {
-    return false;
-  }
-
-  MOZ_ASSERT(protoProto->staticPrototypeIsImmutable(),
-             "protoProto should be Object.prototype, whose prototype is "
-             "immutable");
-  MOZ_ASSERT(protoProto->staticPrototype() == nullptr,
-             "Object.prototype must have null prototype");
-  return true;
-}
-
-static JSProtoKey GetInitialShapeProtoKey(TaggedProto proto, JSContext* cx) {
-  if (proto.isObject() && proto.toObject()->isNative()) {
-    GlobalObject* global = cx->global();
-    NativeObject& obj = proto.toObject()->as<NativeObject>();
-
-    if (IsOriginalProto(global, JSProto_Object, obj)) {
-      return JSProto_Object;
-    }
-    if (IsOriginalProto(global, JSProto_Function, obj)) {
-      return JSProto_Function;
-    }
-    if (IsOriginalProto(global, JSProto_Array, obj)) {
-      return JSProto_Array;
-    }
-    if (IsOriginalProto(global, JSProto_RegExp, obj)) {
-      return JSProto_RegExp;
-    }
-  }
-  return JSProto_LIMIT;
-}
-
 /* static */ Shape* EmptyShape::getInitialShape(JSContext* cx,
                                                 const Class* clasp,
                                                 TaggedProto proto,
                                                 size_t nfixed,
                                                 uint32_t objectFlags) {
   MOZ_ASSERT_IF(proto.isObject(),
                 cx->isInsideCurrentCompartment(proto.toObject()));
 
   auto& table = cx->zone()->initialShapes();
 
   using Lookup = InitialShapeEntry::Lookup;
-  auto protoPointer = MakeDependentAddPtr(
-      cx, table, Lookup(clasp, Lookup::ShapeProto(proto), nfixed, objectFlags));
+  auto protoPointer =
+      MakeDependentAddPtr(cx, table, Lookup(clasp, proto, nfixed, objectFlags));
   if (protoPointer) {
     return protoPointer->shape;
   }
 
-  // No entry for this proto. If the proto is one of a few common builtin
-  // prototypes, try to do a lookup based on the JSProtoKey, so we can share
-  // shapes across globals.
   Rooted<TaggedProto> protoRoot(cx, proto);
-  Shape* shape = nullptr;
-  bool insertKey = false;
-  mozilla::Maybe<DependentAddPtr<InitialShapeSet>> keyPointer;
-
-  JSProtoKey key = GetInitialShapeProtoKey(protoRoot, cx);
-  if (key != JSProto_LIMIT) {
-    keyPointer.emplace(MakeDependentAddPtr(
-        cx, table,
-        Lookup(clasp, Lookup::ShapeProto(key), nfixed, objectFlags)));
-    if (keyPointer.ref()) {
-      shape = keyPointer.ref()->shape;
-      MOZ_ASSERT(shape);
-    } else {
-      insertKey = true;
-    }
-  }
-
-  if (!shape) {
-    StackBaseShape base(clasp, objectFlags);
-    Rooted<UnownedBaseShape*> nbase(cx, BaseShape::getUnowned(cx, base));
-    if (!nbase) {
-      return nullptr;
-    }
-
-    shape = EmptyShape::new_(cx, nbase, nfixed);
-    if (!shape) {
-      return nullptr;
-    }
-  }
-
-  Lookup::ShapeProto shapeProto(protoRoot);
-  Lookup lookup(clasp, shapeProto, nfixed, objectFlags);
-  if (!protoPointer.add(cx, table, lookup,
-                        InitialShapeEntry(shape, shapeProto))) {
+  StackBaseShape base(clasp, objectFlags);
+  Rooted<UnownedBaseShape*> nbase(cx, BaseShape::getUnowned(cx, base));
+  if (!nbase) {
     return nullptr;
   }
 
-  // Also add an entry based on the JSProtoKey, if needed.
-  if (insertKey) {
-    Lookup::ShapeProto shapeProto(key);
-    Lookup lookup(clasp, shapeProto, nfixed, objectFlags);
-    if (!keyPointer->add(cx, table, lookup,
-                         InitialShapeEntry(shape, shapeProto))) {
-      return nullptr;
-    }
+  RootedShape shape(cx, EmptyShape::new_(cx, nbase, nfixed));
+  if (!shape) {
+    return nullptr;
+  }
+
+  Lookup lookup(clasp, protoRoot, nfixed, objectFlags);
+  if (!protoPointer.add(cx, table, lookup,
+                        InitialShapeEntry(shape, protoRoot))) {
+    return nullptr;
   }
 
   return shape;
 }
 
 /* static */ Shape* EmptyShape::getInitialShape(JSContext* cx,
                                                 const Class* clasp,
                                                 TaggedProto proto,
@@ -2218,17 +2131,17 @@ void NewObjectCache::invalidateEntriesFo
     PodZero(&entries[entry]);
   }
 }
 
 /* static */ void EmptyShape::insertInitialShape(JSContext* cx,
                                                  HandleShape shape,
                                                  HandleObject proto) {
   using Lookup = InitialShapeEntry::Lookup;
-  Lookup lookup(shape->getObjectClass(), Lookup::ShapeProto(TaggedProto(proto)),
+  Lookup lookup(shape->getObjectClass(), TaggedProto(proto),
                 shape->numFixedSlots(), shape->getObjectFlags());
 
   InitialShapeSet::Ptr p = cx->zone()->initialShapes().lookup(lookup);
   MOZ_ASSERT(p);
 
   InitialShapeEntry& entry = const_cast<InitialShapeEntry&>(*p);
 
   // The metadata callback can end up causing redundant changes of the initial
@@ -2243,32 +2156,16 @@ void NewObjectCache::invalidateEntriesFo
   while (!nshape->isEmptyShape()) {
     nshape = nshape->previous();
   }
   MOZ_ASSERT(nshape == entry.shape);
 #endif
 
   entry.shape = ReadBarrieredShape(shape);
 
-  // For certain prototypes -- namely, those of various builtin classes,
-  // keyed by JSProtoKey |key| -- there are two entries: one for a lookup
-  // via |proto|, and one for a lookup via |key|.  If this is such a
-  // prototype, also update the alternate |key|-keyed shape.
-  JSProtoKey key = GetInitialShapeProtoKey(TaggedProto(proto), cx);
-  if (key != JSProto_LIMIT) {
-    Lookup lookup(shape->getObjectClass(), Lookup::ShapeProto(key),
-                  shape->numFixedSlots(), shape->getObjectFlags());
-    if (InitialShapeSet::Ptr p = cx->zone()->initialShapes().lookup(lookup)) {
-      InitialShapeEntry& entry = const_cast<InitialShapeEntry&>(*p);
-      if (entry.shape != shape) {
-        entry.shape = ReadBarrieredShape(shape);
-      }
-    }
-  }
-
   /*
    * This affects the shape that will be produced by the various NewObject
    * methods, so clear any cache entry referring to the old shape. This is
    * not required for correctness: the NewObject must always check for a
    * nativeEmpty() result and generate the appropriate properties if found.
    * Clearing the cache entry avoids this duplicate regeneration.
    *
    * Clearing is not necessary when this context is running off
@@ -2287,22 +2184,22 @@ void Zone::fixupInitialShapeTable() {
       shape = Forwarded(shape);
       e.mutableFront().shape.set(shape);
     }
     shape->updateBaseShapeAfterMovingGC();
 
     // If the prototype has moved we have to rekey the entry.
     InitialShapeEntry entry = e.front();
     // Use unbarrieredGet() to prevent triggering read barrier while collecting.
-    const TaggedProto& proto = entry.proto.proto().unbarrieredGet();
+    const TaggedProto& proto = entry.proto.unbarrieredGet();
     if (proto.isObject() && IsForwarded(proto.toObject())) {
-      entry.proto.setProto(TaggedProto(Forwarded(proto.toObject())));
+      entry.proto = TaggedProto(Forwarded(proto.toObject()));
       using Lookup = InitialShapeEntry::Lookup;
-      Lookup relookup(shape->getObjectClass(), Lookup::ShapeProto(proto),
-                      shape->numFixedSlots(), shape->getObjectFlags());
+      Lookup relookup(shape->getObjectClass(), proto, shape->numFixedSlots(),
+                      shape->getObjectFlags());
       e.rekeyFront(relookup, entry);
     }
   }
 }
 
 void AutoRooterGetterSetter::Inner::trace(JSTracer* trc) {
   if ((attrs & JSPROP_GETTER) && *pgetter) {
     TraceRoot(trc, (JSObject**)pgetter, "AutoRooterGetterSetter getter");
--- a/js/src/vm/Shape.h
+++ b/js/src/vm/Shape.h
@@ -1249,142 +1249,76 @@ struct EmptyShape : public js::Shape {
    * init method, to ensure that objects of such subclasses compute and cache
    * the initial shape, if it hasn't already been computed.
    */
   template <class ObjectSubclass>
   static inline bool ensureInitialCustomShape(JSContext* cx,
                                               Handle<ObjectSubclass*> obj);
 };
 
-// InitialShapeProto stores either:
-//
-// * A TaggedProto (or ReadBarriered<TaggedProto>).
-//
-// * A JSProtoKey. This is used instead of the TaggedProto if the proto is one
-//   of the global's builtin prototypes. For instance, if the proto is the
-//   initial Object.prototype, we use key_ = JSProto_Object, proto_ = nullptr.
-//
-// Using the JSProtoKey here is an optimization that lets us share more shapes
-// across compartments within a zone.
-template <typename PtrType>
-class InitialShapeProto {
-  template <typename T>
-  friend class InitialShapeProto;
-
-  JSProtoKey key_;
-  PtrType proto_;
-
- public:
-  InitialShapeProto() : key_(JSProto_LIMIT), proto_() {}
-
-  InitialShapeProto(JSProtoKey key, TaggedProto proto)
-      : key_(key), proto_(proto) {}
-
-  template <typename T>
-  explicit InitialShapeProto(const InitialShapeProto<T>& other)
-      : key_(other.key()), proto_(other.proto_) {}
-
-  explicit InitialShapeProto(TaggedProto proto)
-      : key_(JSProto_LIMIT), proto_(proto) {}
-  explicit InitialShapeProto(JSProtoKey key) : key_(key), proto_(nullptr) {
-    MOZ_ASSERT(key < JSProto_LIMIT);
-  }
-
-  JSProtoKey key() const { return key_; }
-  const PtrType& proto() const { return proto_; }
-  void setProto(TaggedProto proto) { proto_ = proto; }
-
-  bool operator==(const InitialShapeProto& other) const {
-    return key_ == other.key_ && proto_ == other.proto_;
-  }
-};
-
-template <>
-struct MovableCellHasher<InitialShapeProto<ReadBarriered<TaggedProto>>> {
-  using Key = InitialShapeProto<ReadBarriered<TaggedProto>>;
-  using Lookup = InitialShapeProto<TaggedProto>;
-
-  static bool hasHash(const Lookup& l) {
-    return MovableCellHasher<TaggedProto>::hasHash(l.proto());
-  }
-  static bool ensureHash(const Lookup& l) {
-    return MovableCellHasher<TaggedProto>::ensureHash(l.proto());
-  }
-  static HashNumber hash(const Lookup& l) {
-    HashNumber hash = MovableCellHasher<TaggedProto>::hash(l.proto());
-    return mozilla::AddToHash(hash, l.key());
-  }
-  static bool match(const Key& k, const Lookup& l) {
-    return k.key() == l.key() && MovableCellHasher<TaggedProto>::match(
-                                     k.proto().unbarrieredGet(), l.proto());
-  }
-};
-
 /*
  * Entries for the per-zone initialShapes set indexing initial shapes for
  * objects in the zone and the associated types.
  */
 struct InitialShapeEntry {
   /*
    * Initial shape to give to the object. This is an empty shape, except for
    * certain classes (e.g. String, RegExp) which may add certain baked-in
    * properties.
    */
   ReadBarriered<Shape*> shape;
 
   /*
    * Matching prototype for the entry. The shape of an object determines its
    * prototype, but the prototype cannot be determined from the shape itself.
    */
-  using ShapeProto = InitialShapeProto<ReadBarriered<TaggedProto>>;
-  ShapeProto proto;
+  ReadBarriered<TaggedProto> proto;
 
   /* State used to determine a match on an initial shape. */
   struct Lookup {
-    using ShapeProto = InitialShapeProto<TaggedProto>;
     const Class* clasp;
-    ShapeProto proto;
+    TaggedProto proto;
     uint32_t nfixed;
     uint32_t baseFlags;
 
-    Lookup(const Class* clasp, ShapeProto proto, uint32_t nfixed,
+    Lookup(const Class* clasp, const TaggedProto& proto, uint32_t nfixed,
            uint32_t baseFlags)
         : clasp(clasp), proto(proto), nfixed(nfixed), baseFlags(baseFlags) {}
 
     explicit Lookup(const InitialShapeEntry& entry)
-        : proto(entry.proto.key(), entry.proto.proto().unbarrieredGet()) {
+        : proto(entry.proto.unbarrieredGet()) {
       const Shape* shape = entry.shape.unbarrieredGet();
       clasp = shape->getObjectClass();
       nfixed = shape->numFixedSlots();
       baseFlags = shape->getObjectFlags();
     }
   };
 
   inline InitialShapeEntry();
-  inline InitialShapeEntry(Shape* shape, const Lookup::ShapeProto& proto);
+  inline InitialShapeEntry(Shape* shape, const TaggedProto& proto);
 
   static HashNumber hash(const Lookup& lookup) {
-    HashNumber hash = MovableCellHasher<ShapeProto>::hash(lookup.proto);
+    HashNumber hash = MovableCellHasher<TaggedProto>::hash(lookup.proto);
     return mozilla::AddToHash(
         hash, mozilla::HashGeneric(lookup.clasp, lookup.nfixed));
   }
   static inline bool match(const InitialShapeEntry& key, const Lookup& lookup) {
     const Shape* shape = key.shape.unbarrieredGet();
     return lookup.clasp == shape->getObjectClass() &&
            lookup.nfixed == shape->numFixedSlots() &&
            lookup.baseFlags == shape->getObjectFlags() &&
-           MovableCellHasher<ShapeProto>::match(key.proto, lookup.proto);
+           key.proto.unbarrieredGet() == lookup.proto;
   }
   static void rekey(InitialShapeEntry& k, const InitialShapeEntry& newKey) {
     k = newKey;
   }
 
   bool needsSweep() {
     Shape* ushape = shape.unbarrieredGet();
-    TaggedProto uproto = proto.proto().unbarrieredGet();
+    TaggedProto uproto = proto.unbarrieredGet();
     JSObject* protoObj = uproto.raw();
     return (
         gc::IsAboutToBeFinalizedUnbarriered(&ushape) ||
         (uproto.isObject() && gc::IsAboutToBeFinalizedUnbarriered(&protoObj)));
   }
 
   bool operator==(const InitialShapeEntry& other) const {
     return shape == other.shape && proto == other.proto;