Skip to content
Permalink
Browse files
Speed-up property type-hint enforcement for APCObject
Summary:
When re-creating an object from APCObject, it is required to validate the
object's property values against their type-hints. This is because some of the
type-hints may change meaning between requests, and therefore the object may
have valid values in one request and not in another.

However, if the values conservatively satisfy their type-hints in all contexts
(regardless of what type-aliases are defined), then we can skip the checks. Do
this conservative check when creating the APCObject and set a bit if it
passes. That bit will cause us to skip the type-hint checks when re-creating the
object.

Reviewed By: markw65

Differential Revision: D13689713

fbshipit-source-id: 01cc91021c0b3efb6dd67cd0a8b4865c32844f60
  • Loading branch information
ricklavoie authored and hhvm-bot committed Jan 18, 2019
1 parent a6374b2 commit 3c24a9686dda5af888a8da0cb65e53d75745e2ee
@@ -56,6 +56,7 @@ APCObject::APCObject(ClassOrName cls, uint32_t propCount)
, m_persistent{0}
, m_no_wakeup{0}
, m_fast_init{0}
, m_no_verify_prop_types{0}
{}

APCHandle::Pair APCObject::Construct(ObjectData* objectData) {
@@ -89,6 +90,7 @@ APCHandle::Pair APCObject::Construct(ObjectData* objectData) {
auto const objPropVec = objectData->propVec();
const TypedValueAux* propInit = nullptr;

auto propsDontNeedCheck = RuntimeOption::EvalCheckPropTypeHints > 0;
for (unsigned i = 0; i < numRealProps; ++i) {
auto const attrs = propInfo[i].attrs;
assertx((attrs & AttrStatic) == 0);
@@ -130,12 +132,22 @@ APCHandle::Pair APCObject::Construct(ObjectData* objectData) {
}
}

// If the property value satisfies its type-hint in all contexts, we don't
// need to do any validation when we re-create the object.
if (propsDontNeedCheck) {
propsDontNeedCheck = propInfo[i].typeConstraint.alwaysPasses(objProp);
}

auto val = APCHandle::Create(const_variant_ref{objProp}, false,
APCHandleLevel::Inner, true);
size += val.size;
apcPropVec[i] = val.handle;
}

if (RuntimeOption::EvalCheckPropTypeHints <= 0 || propsDontNeedCheck) {
apcObj->m_no_verify_prop_types = 1;
}

if (UNLIKELY(hasDynProps)) {
auto val = APCHandle::Create(VarNR{objectData->dynPropArray()}, false,
APCHandleLevel::Inner, true);
@@ -287,8 +299,10 @@ Object APCObject::createObject() const {
}
}

// Make sure the unserialized values don't violate any type-hints.
obj->verifyPropTypeHints();
// Make sure the unserialized values don't violate any type-hints if they
// require validation.
if (!m_no_verify_prop_types) obj->verifyPropTypeHints();
assertx(obj->assertPropTypeHints());

if (UNLIKELY(numProps < m_propCount)) {
auto dynProps = apcProp[numProps];
@@ -114,6 +114,7 @@ struct APCObject {
uint8_t m_persistent:1;
uint8_t m_no_wakeup:1;
uint8_t m_fast_init:1;
uint8_t m_no_verify_prop_types:1;
};

//////////////////////////////////////////////////////////////////////
@@ -204,6 +204,15 @@ inline void ObjectData::verifyPropTypeHints() const {
verifyPropTypeHints(m_cls->declProperties().size());
}

inline bool ObjectData::assertPropTypeHints() const {
auto const props = propVec();
auto const end = m_cls->declProperties().size();
for (size_t idx = 0; idx < end; ++idx) {
if (!assertTypeHint(&props[idx], idx)) return false;
}
return true;
}

inline Class* ObjectData::getVMClass() const {
assertx(kindIsValid());
return m_cls;
@@ -123,11 +123,15 @@ void unsetTypeHint(const Class::Prop* prop) {
);
}

}

//////////////////////////////////////////////////////////////////////

// Check that the given property's type matches its type-hint.
bool assertTypeHint(const Class* cls, tv_rval prop, Slot propIdx) {
bool ObjectData::assertTypeHint(tv_rval prop, Slot propIdx) const {
assertx(tvIsPlausible(*prop));
assertx(propIdx < cls->numDeclProperties());
auto const& propDecl = cls->declProperties()[propIdx];
assertx(propIdx < m_cls->numDeclProperties());
auto const& propDecl = m_cls->declProperties()[propIdx];

// AttrLateInitSoft properties can be potentially anything due to default
// values, so don't assume anything.
@@ -157,8 +161,6 @@ bool assertTypeHint(const Class* cls, tv_rval prop, Slot propIdx) {
return propDecl.typeConstraint.assertCheck(prop);
}

}

//////////////////////////////////////////////////////////////////////

NEVER_INLINE bool ObjectData::destructImpl() {
@@ -603,7 +605,7 @@ void ObjectData::o_getArray(Array& props,
IteratePropToArrayOrderNoInc(
this,
[&](Slot slot, const Class::Prop& prop, tv_rval val) {
assertx(assertTypeHint(cls, val, slot));
assertx(assertTypeHint(val, slot));
if (UNLIKELY(val.type() == KindOfUninit)) {
if (!ignoreLateInit) {
if (prop.attrs & AttrLateInitSoft) {
@@ -967,7 +969,7 @@ ObjectData* ObjectData::clone() {
auto const clonePropVec = clone->propVecForConstruct();
for (auto i = Slot{0}; i < nProps; i++) {
tvDupWithRef(propVec()[i], clonePropVec[i]);
assertx(assertTypeHint(m_cls, &clonePropVec[i], i));
assertx(assertTypeHint(&clonePropVec[i], i));
}

if (UNLIKELY(getAttribute(HasDynPropArr))) {
@@ -1351,7 +1353,7 @@ ObjectData::PropLookup ObjectData::getPropImpl(
// We found a visible property, but it might not be accessible. No need to
// check if there is a dynamic property with this name.
auto const prop = const_cast<TypedValue*>(&propVec()[propIdx]);
assertx(assertTypeHint(m_cls, prop, propIdx));
assertx(assertTypeHint(prop, propIdx));

auto const& declProp = m_cls->declProperties()[propIdx];
if (!ignoreLateInit && lookup.accessible) {
@@ -433,6 +433,8 @@ struct ObjectData : Countable, type_scan::MarkCollectable<ObjectData> {
void verifyPropTypeHints() const;
void verifyPropTypeHints(size_t end) const;

bool assertPropTypeHints() const;

// Accessors for declared properties at statically known offsets. In the lval
// case, the property must be statically known to be mutable. If the caller
// modifies the lval, they are responsible for validating the value with any
@@ -569,6 +571,8 @@ struct ObjectData : Countable, type_scan::MarkCollectable<ObjectData> {

bool slowDestroyCheck() const;

bool assertTypeHint(tv_rval, Slot) const;

// offset: 0 8 12 16 20 32
// 64bit: header cls id [subclass] [props...]
// lowptr: header cls id [subclass][props...]
@@ -0,0 +1,51 @@
<?hh
// Copyright 2004-present Facebook. All Rights Reserved.

function get_count() {
$count = apc_fetch('count');
if ($count === false) {
$count = 0;
apc_store('count', $count);
}
return $count;
}

if ((get_count() % 2) == 0) {
require 'apc2.inc';
} else {
require 'apc3.inc';
}

require 'apc1.inc';

function store() {
echo "store()\n";
$a = new A();
$a->x = true;
apc_store('some-key', $a);
apc_fetch('some-key');

$b = new B();
$b->x = true;
apc_store('some-key2', $b);
apc_fetch('some-key2');
}

function get() {
echo "get()\n";
apc_fetch('some-key');
apc_fetch('some-key2');
}

function run() {
$count = get_count();
echo "Count: $count\n";
if ($count == 0) {
store();
} else {
get();
}
apc_store('count', $count+1);
}

run();
@@ -0,0 +1,30 @@
Count: 0
store()
Count: 1
get()

Warning: Property 'A::x' declared as type MaybeBool, bool assigned in %s/apc.php on line 36
Count: 2
get()
Count: 3
get()

Warning: Property 'A::x' declared as type MaybeBool, bool assigned in %s/apc.php on line 36
Count: 4
get()
Count: 5
get()

Warning: Property 'A::x' declared as type MaybeBool, bool assigned in %s/apc.php on line 36
Count: 6
get()
Count: 7
get()

Warning: Property 'A::x' declared as type MaybeBool, bool assigned in %s/apc.php on line 36
Count: 8
get()
Count: 9
get()

Warning: Property 'A::x' declared as type MaybeBool, bool assigned in %s/apc.php on line 36
@@ -0,0 +1 @@
-vCheckPropTypeHints=1
@@ -0,0 +1,3 @@
-vEval.CheckPropTypeHints=1
-vServer.APC.AllowObject=true
--count=10
@@ -0,0 +1,9 @@
<?hh

class A {
public MaybeBool $x;
}

class B {
public bool $x;
}
@@ -0,0 +1,3 @@
<?hh

type MaybeBool = bool;
@@ -0,0 +1,3 @@
<?hh

type MaybeBool = string;

0 comments on commit 3c24a96

Please sign in to comment.