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 +