Skip to content

Commit

Permalink
Add mayHaveAccessor field to ClassFlags
Browse files Browse the repository at this point in the history
Summary:
Add a new field to the struct `ClassFlags`, which represents if there
is *potentially* a property accessor in the `HiddenClass`. This field
is pessimistic and can sometimes give false positives. This is due to
the fact that the field may only ever be set, but never cleared. So if
you define a getter property on an object, and then delete that
property, the field will remain set. This field will open up the door
to some potential optimizations now that we have this knowledge. It is
also used to fix a correctness bug in the next diff.

Reviewed By: neildhar

Differential Revision: D41751159

fbshipit-source-id: 5ce88c9d84c989302525df7f87a172c5510b7c51
  • Loading branch information
fbmal7 authored and facebook-github-bot committed Jan 10, 2023

Verified

This commit was signed with the committer’s verified signature.
psliwka Piotr Śliwka
1 parent e42cdb4 commit 79c94de
Showing 2 changed files with 104 additions and 0 deletions.
13 changes: 13 additions & 0 deletions include/hermes/VM/HiddenClass.h
Original file line number Diff line number Diff line change
@@ -55,6 +55,12 @@ struct ClassFlags {
/// searched for - they don't exist.
uint8_t hasIndexLikeProperties : 1;

/// There may be a accessor property somewhere in the entire chain of leading
/// up to this HiddenClass. Set when a property is an accessor, and can never
/// be unset. That means this is a pessimistic flag: if a getter/setter
/// property is set and then deleted, this will still be set to true.
uint8_t mayHaveAccessor : 1;

ClassFlags() {
::memset(this, 0, sizeof(*this));
}
@@ -314,6 +320,10 @@ class HiddenClass final : public GCCell {
return flags_.hasIndexLikeProperties;
}

bool getMayHaveAccessor() const {
return flags_.mayHaveAccessor;
}

/// \return The for-in cache if one has been set, otherwise nullptr.
BigStorage *getForInCache(Runtime &runtime) const {
return forInCache_.get(runtime);
@@ -654,6 +664,9 @@ inline ClassFlags HiddenClass::computeFlags(
PropertyFlags pf,
bool addedIndexLike) {
flags.hasIndexLikeProperties |= addedIndexLike;
// Carry over the the existing mayHaveAccessor flag. Once an accessor property
// has been set, all subsequent classes must have this property marked.
flags.mayHaveAccessor |= pf.accessor;
return flags;
}

91 changes: 91 additions & 0 deletions unittests/VMRuntime/HiddenClassTest.cpp
Original file line number Diff line number Diff line change
@@ -249,6 +249,97 @@ TEST_F(HiddenClassTest, SmokeTest) {
}
}

TEST_F(HiddenClassTest, AccessorsTest) {
GCScope gcScope{runtime, "HiddenClassTest.SmokeTest", 48};
runtime.collect("test");
auto aSym = *runtime.getIdentifierTable().getSymbolHandle(
runtime, createUTF16Ref(u"a"));
auto bSym = *runtime.getIdentifierTable().getSymbolHandle(
runtime, createUTF16Ref(u"b"));
auto cSym = *runtime.getIdentifierTable().getSymbolHandle(
runtime, createUTF16Ref(u"c"));
auto dSym = *runtime.getIdentifierTable().getSymbolHandle(
runtime, createUTF16Ref(u"d"));
auto defaultFlags = PropertyFlags::defaultNewNamedPropertyFlags();
auto accessorFlags = PropertyFlags::defaultNewNamedPropertyFlags();
accessorFlags.accessor = true;

// We will simulate and verify the following property operations, starting
// from the same root.
/// x.a, x.b, x.c accessor, x.d
/// y.a accessor, y.b, delete y.a, y.c

MutableHandle<HiddenClass> x{runtime};
MutableHandle<HiddenClass> y{runtime};

auto rootCls = runtime.makeHandle<HiddenClass>(
runtime.ignoreAllocationFailure(HiddenClass::createRoot(runtime)));

ASSERT_FALSE(rootCls->getMayHaveAccessor());

// x = {}
x = *rootCls;
{
// x.a
auto addRes = HiddenClass::addProperty(x, runtime, *aSym, defaultFlags);
x = *addRes->first;
ASSERT_FALSE(x->getMayHaveAccessor());
}
{
// x.b
auto addRes = HiddenClass::addProperty(x, runtime, *bSym, defaultFlags);
x = *addRes->first;
ASSERT_FALSE(x->getMayHaveAccessor());
}
{
// x.c accessor
auto addRes = HiddenClass::addProperty(x, runtime, *cSym, accessorFlags);
x = *addRes->first;
ASSERT_TRUE(x->getMayHaveAccessor());
}
{
// x.d
auto addRes = HiddenClass::addProperty(x, runtime, *dSym, defaultFlags);
x = *addRes->first;
// Since there may be an accessor in the chain, we must still return
// true.
ASSERT_TRUE(x->getMayHaveAccessor());
}

// y = {}
y = *rootCls;
{
// y.a
auto addRes = HiddenClass::addProperty(y, runtime, *aSym, accessorFlags);
y = *addRes->first;
ASSERT_TRUE(y->getMayHaveAccessor());
}
{
// y.b
auto addRes = HiddenClass::addProperty(y, runtime, *bSym, defaultFlags);
y = *addRes->first;
ASSERT_TRUE(y->getMayHaveAccessor());
}
{
// delete y.a
NamedPropertyDescriptor desc;
auto found =
HiddenClass::findProperty(y, runtime, *aSym, accessorFlags, desc);
ASSERT_TRUE(found);
ASSERT_EQ(0u, desc.slot);
y = HiddenClass::deleteProperty(y, runtime, *found);
ASSERT_TRUE(y->getMayHaveAccessor());
}
{
// y.d
auto addRes = HiddenClass::addProperty(y, runtime, *cSym, defaultFlags);
y = *addRes->first;
// Since there may still be an accessor in the chain, we must still return
// true.
ASSERT_TRUE(y->getMayHaveAccessor());
}
}

TEST_F(HiddenClassTest, UpdatePropertyFlagsWithoutTransitionsTest) {
GCScope gcScope{
runtime, "HiddenClassTest.UpdatePropertyFlagsWithoutTransitionsTest", 48};

0 comments on commit 79c94de

Please sign in to comment.