From c2c12b9c522dbb0f1bfb4fee582bc8f83d3ec613 Mon Sep 17 00:00:00 2001 From: David Detlefs Date: Fri, 17 Jan 2025 13:54:02 -0800 Subject: [PATCH] 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 | 14 ++++++++ include/hermes/VM/PropertyCache.h | 14 ++++++-- include/hermes/VM/sh_mirror.h | 1 + lib/VM/CodeBlock.cpp | 3 ++ lib/VM/Interpreter.cpp | 57 +++++++++++++++++++------------ lib/VM/JSObject.cpp | 18 ++++++++++ lib/VM/StaticHUnit.cpp | 6 +++- test/hermes/proto-cache.js | 45 ++++++++++++++++++++++++ 8 files changed, 133 insertions(+), 25 deletions(-) create mode 100644 test/hermes/proto-cache.js diff --git a/include/hermes/VM/JSObject.h b/include/hermes/VM/JSObject.h index f782f2af02b..9e9b049add6 100644 --- a/include/hermes/VM/JSObject.h +++ b/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 &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); diff --git a/include/hermes/VM/PropertyCache.h b/include/hermes/VM/PropertyCache.h index 797bacbd324..035a7ad6178 100644 --- a/include/hermes/VM/PropertyCache.h +++ b/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 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 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}; }; diff --git a/include/hermes/VM/sh_mirror.h b/include/hermes/VM/sh_mirror.h index b5b3421c3e5..6c5496cbbb1 100644 --- a/include/hermes/VM/sh_mirror.h +++ b/include/hermes/VM/sh_mirror.h @@ -31,6 +31,7 @@ typedef struct SHWritePropertyCacheEntry { typedef struct SHReadPropertyCacheEntry { SHCompressedPointerRawType clazz; + SHCompressedPointerRawType negMatchClazz; uint32_t slot; } SHReadPropertyCacheEntry; diff --git a/lib/VM/CodeBlock.cpp b/lib/VM/CodeBlock.cpp index 1682cddc9ba..8852d91b3ad 100644 --- a/lib/VM/CodeBlock.cpp +++ b/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_)) { diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp index a7868733cbf..26d11e96a88 100644 --- a/lib/VM/Interpreter.cpp +++ b/lib/VM/Interpreter.cpp @@ -2295,6 +2295,29 @@ CallResult 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 &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 fastPathResult = @@ -2330,28 +2353,6 @@ CallResult 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 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; diff --git a/lib/VM/JSObject.cpp b/lib/VM/JSObject.cpp index f05813ab94c..c1ec6d642f3 100644 --- a/lib/VM/JSObject.cpp +++ b/lib/VM/JSObject.cpp @@ -1095,6 +1095,24 @@ CallResult> 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)); diff --git a/lib/VM/StaticHUnit.cpp b/lib/VM/StaticHUnit.cpp index 9f39dd0f56d..c387cc9a8ea 100644 --- a/lib/VM/StaticHUnit.cpp +++ b/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(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( diff --git a/test/hermes/proto-cache.js b/test/hermes/proto-cache.js new file mode 100644 index 00000000000..baf8c519ec4 --- /dev/null +++ b/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 +