Faster caching of prototype accesses.

Summary:
We have a kind of "prototype caching" for properties found on an
object's prototype chain, but it is only used after a dictionary
lookup ensures that the property is not an own property of the object,
which decreases it's effectiveness as an optimization.

This diff changes prototype caching.  A PropertyCache entry gains a
new field: the "negMatchClazz".  When we find a property in the
(immediate) prototype (and therefore not in the object), we set the
"clazz" field of the PropertyCache to the HC of the prototype object,
and the "slot" field to the slot index of the property in the
prototype.  This is not sufficient, however: between one access and
the next, the property might get added to the object, and then it
would then be incorrect to retrieve it from the prototype.  To guard
against this, we record the HC of the object in "negMatchClazz" --
when the object had this HC, the property was not in the object.  Both
HC's must match to get a prototype cache hit.  But when they do, we
avoid doing a dictionary lookup in the object.

Follow-ons planned:

Implement this optimization for shermes, and for
the JIT.  The structure of the optimization is such that things still
work correctly, but they may be slightly slower.

Currently, the prototype cache path checks that the current object is
not lazy, a proxy, or a host object.  The latter two, at least, would
be unnecessary if we ensured that proxy and host objects had an HC
distinct from that of an empty object.  Hopefully, something similar
would apply for lazy objects.

Reviewed By: neildhar

Differential Revision: D65297624

fbshipit-source-id: a7dce0e17944d11b75950306eb1666894e43665b

include/hermes/VM/JSObject.h

@@ -409,6 +409,13 @@ class JSObject : public GCCell {
     return flags_.proxyObject;
   }
 
+  /// Returns whether the object has any of properties set in \p flags.
+  /// \p flags should have only the flags set, not the objectID.
+  bool hasFlagIn(SHObjectFlags flags) const {
+    assert(flags.objectID == 0);
+    return flags_.bits & flags.bits;
+  }
+
   /// \return true if this object has fast indexed storage, meaning no property
   ///   checks need to be made when reading an indexed value.
   bool hasFastIndexProperties() const {
@@ -422,6 +429,13 @@ class JSObject : public GCCell {
     return parent_.get(runtime);
   }
 
+  /// \return the `__proto__` internal property, which may be nullptr.
+  const GCPointer<JSObject> &getParentGCPtr() const {
+    assert(
+        !flags_.proxyObject && "getParent cannot be used with proxy objects");
+    return parent_;
+  }
+
   /// \return the hidden class of this object.
   HiddenClass *getClass(PointerBase &base) const {
     return clazz_.getNonNull(base);

include/hermes/VM/PropertyCache.h

@@ -30,10 +30,20 @@ struct WritePropertyCacheEntry {
 
 /// A cache entry for property reads.
 struct ReadPropertyCacheEntry {
-  /// Cached class.
+  /// Cached class: either for the object being read from,
+  /// or it's prototype.
   WeakRoot<HiddenClass> clazz{nullptr};
 
-  /// Cached property index.
+  /// If \p clazz is a HiddenClass for the prototype, this is
+  /// non-null and is the HiddenClass for the child object.  We call this
+  /// a "negative match": the property was not found on an object
+  /// with this HiddenClass, so if the object used in a subsequent
+  /// read has the same HiddenClass, it also won't have the property.
+  WeakRoot<HiddenClass> negMatchClazz{nullptr};
+
+  /// Cached property index: in the object if \p clazz is the object's
+  /// HiddenClass, or in the object's prototype if \p clazz is the
+  /// prototype's HiddenClass.
   SlotIndex slot{0};
 };
 

include/hermes/VM/sh_mirror.h

@@ -31,6 +31,7 @@ typedef struct SHWritePropertyCacheEntry {
 
 typedef struct SHReadPropertyCacheEntry {
   SHCompressedPointerRawType clazz;
+  SHCompressedPointerRawType negMatchClazz;
   uint32_t slot;
 } SHReadPropertyCacheEntry;
 

lib/VM/CodeBlock.cpp

@@ -305,6 +305,9 @@ void CodeBlock::markCachedHiddenClasses(
     if (prop.clazz) {
       acceptor.acceptWeak(prop.clazz);
     }
+    if (prop.negMatchClazz) {
+      acceptor.acceptWeak(prop.negMatchClazz);
+    }
   }
   for (auto &prop : llvh::makeMutableArrayRef(
            writePropertyCache(), writePropertyCacheSize_)) {

lib/VM/Interpreter.cpp

@@ -2295,6 +2295,29 @@ CallResult<HermesValue> Interpreter::interpretFunction(
           ip = nextIP;
           DISPATCH;
         }
+        // TODO(T206393003): if we ensure that lazy, proxy, host objects
+        // have special HC's, different from that of an empty object, and
+        // don't do proto caching for such objects, then we wouldn't have
+        // to do this check.
+        const SHObjectFlags kLazyProxyOrHost{
+            .hostObject = 1, .lazyObject = 1, .proxyObject = 1};
+        if (LLVM_LIKELY(
+                cacheEntry->negMatchClazz == clazzPtr &&
+                !obj->hasFlagIn(kLazyProxyOrHost))) {
+          const GCPointer<JSObject> &parentGCPtr = obj->getParentGCPtr();
+          if (LLVM_LIKELY(parentGCPtr)) {
+            JSObject *parent = parentGCPtr.getNonNull(runtime);
+            if (LLVM_LIKELY(cacheEntry->clazz == parent->getClassGCPtr())) {
+              ++NumGetByIdProtoHits;
+              // We've already checked that this isn't a Proxy.
+              O1REG(GetById) = JSObject::getNamedSlotValueUnsafe(
+                                   parent, runtime, cacheEntry->slot)
+                                   .unboxToHV(runtime);
+              ip = nextIP;
+              DISPATCH;
+            }
+          }
+        }
         auto id = ID(idVal);
         NamedPropertyDescriptor desc;
         OptValue<bool> fastPathResult =
@@ -2330,28 +2353,6 @@ CallResult<HermesValue> Interpreter::interpretFunction(
           DISPATCH;
         }
 
-        // The cache may also be populated via the prototype of the object.
-        // This value is only reliable if the fast path was a definite
-        // not-found.
-        if (fastPathResult.hasValue() && !fastPathResult.getValue() &&
-            LLVM_LIKELY(!obj->isProxyObject())) {
-          JSObject *parent = obj->getParent(runtime);
-          // TODO: This isLazy check is because a lazy object is reported as
-          // having no properties and therefore cannot contain the property.
-          // This check does not belong here, it should be merged into
-          // tryGetOwnNamedDescriptorFast().
-          if (parent && cacheEntry->clazz == parent->getClassGCPtr() &&
-              LLVM_LIKELY(!obj->isLazy())) {
-            ++NumGetByIdProtoHits;
-            // We've already checked that this isn't a Proxy.
-            O1REG(GetById) = JSObject::getNamedSlotValueUnsafe(
-                                 parent, runtime, cacheEntry->slot)
-                                 .unboxToHV(runtime);
-            ip = nextIP;
-            DISPATCH;
-          }
-        }
-
 #ifdef HERMES_SLOW_DEBUG
         // Call to getNamedDescriptorUnsafe is safe because `id` is kept alive
         // by the IdentifierTable.
@@ -2373,6 +2374,18 @@ CallResult<HermesValue> Interpreter::interpretFunction(
         (void)NumGetByIdNotFound;
 #endif
 #ifdef HERMES_SLOW_DEBUG
+        // It's possible that there might be a GC between the time
+        // savedClass is set and the time it is checked (to see if
+        // there was an eviction.  This GC could move the HiddenClass
+        // to which savedClass points, making it an invalid pointer.
+        // We don't dereference this pointer, we just compare it, so
+        // this won't cause a crash.  In this presumably rare case,
+        // the eviction count would be incorrect.  But the
+        // alternative, putting savedClass in a handle so it's a root,
+        // could change GC behavior might alter the program behavior
+        // in some cases: a HiddenClass might be live longer than it
+        // would have been.  We're choosing to go with the first
+        // problem as the lesser of two evils.
         auto *savedClass = cacheIdx != hbc::PROPERTY_CACHING_DISABLED
             ? cacheEntry->clazz.get(runtime, runtime.getHeap())
             : nullptr;

lib/VM/JSObject.cpp

@@ -1095,6 +1095,24 @@ CallResult<PseudoHandle<>> JSObject::getNamedWithReceiver_RJS(
     if (cacheEntry && !propObj->getClass(runtime)->isDictionaryNoCache()) {
       cacheEntry->clazz = propObj->getClassGCPtr();
       cacheEntry->slot = desc.slot;
+      if (selfHandle->getParent(runtime) == propObj &&
+          !selfHandle->getClass(runtime)->isDictionary()) {
+        // Property found on an object in the prototype chain.  The proto
+        // cache only works for the immediate proto of the the object,
+        // so don't cache for deeper prototypes.  We also don't cache
+        // if the object HC is a dictionary; those may gain properties without
+        // changing the HC value, which breaks the "negative caching" of
+        // the object HC.  Note that own-property caching can use
+        // a dictionary HC, as long as it hasn't had any properties deleted (or
+        // property flags changed) -- hence the isDictionaryNoCache test above.
+        // But for the negative caching we do here, we have to exempt all
+        // dictionaries, since adding a property could mean that that a
+        // subsequent execution should get the value from the object rather than
+        // the prototype.
+        cacheEntry->negMatchClazz = selfHandle->getClassGCPtr();
+      } else {
+        cacheEntry->negMatchClazz = CompressedPointer(nullptr);
+      }
     }
     return createPseudoHandle(
         getNamedSlotValueUnsafe(propObj, runtime, desc).unboxToHV(runtime));

lib/VM/StaticHUnit.cpp

@@ -239,12 +239,16 @@ void hermes::vm::sh_unit_mark_long_lived_weak_roots(
            unit->num_read_prop_cache_entries)) {
     if (prop.clazz)
       acceptor.acceptWeak(prop.clazz);
+    if (prop.negMatchClazz) {
+      acceptor.acceptWeak(prop.negMatchClazz);
+    }
   }
   for (auto &prop : llvh::makeMutableArrayRef(
            reinterpret_cast<WritePropertyCacheEntry *>(unit->write_prop_cache),
            unit->num_write_prop_cache_entries)) {
-    if (prop.clazz)
+    if (prop.clazz) {
       acceptor.acceptWeak(prop.clazz);
+    }
   }
 
   for (auto &entry : llvh::makeMutableArrayRef(

test/hermes/proto-cache.js

@@ -0,0 +1,45 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -O -target=HBC %s | %FileCheck --match-full-lines %s
+
+// Exercise the proto cache.
+print(function () {
+  var proto = {x: 7};
+  var o = {};
+  Object.setPrototypeOf(o, proto);
+
+  function access(o) {
+    'noinline'
+    return o.x;
+  }
+
+  // o.x in second call to "access" should get a proto cache hit.
+  return access(o) + access(o);
+}());
+// CHECK: 14
+
+// Show that the proto cache doesn't get improper cache hits
+// if the object changes to have the queried property.
+print(function () {
+  var proto = {x: 7};
+  var o = {};
+  Object.setPrototypeOf(o, proto);
+
+  function access(o) {
+    'noinline'
+    return o.x;
+  }
+
+  // o.x in second call to "access" should get a proto cache hit.
+  var sum = access(o);
+  o.x = 700;
+  sum += access(o);
+  return sum;
+}());
+// CHECK: 707
+