Skip to content

Commit

Permalink
[Perf] Reduce o_set overhead
Browse files Browse the repository at this point in the history 
Summary:
ObjectData's o_set family of functions all took a bool flag "forInit". The flag
was used to bypass access checks, and was only used when initializing an
ObjectData from hphpi, and from o_setArray. This meant that we were always
passing in, and checking a mostly unused parameter. In addition, Object's o_set
family didnt have the flag. code-gen didnt differentiate the two cases, and so
in some cases was passing the class name as the "forInit" flag. Bizarrely, this
mostly worked, because the class-name would be converted to true, and access
would be allowed (which is usually the right thing to do in correct programs).

With this diff, ObjectData's o_set family matches the Object o_set family,
solving the correctness problem, and also reducing the call overhead by dropping
a parameter.

Test Plan: fast_tests slow_tests

Reviewers: qigao, myang

Reviewed By: myang

CC: ps, mwilliams, myang, qigao

Differential Revision: 346544
  • Loading branch information
mwilliams authored and macvicar committed Nov 29, 2011
1 parent 7447c5f commit f05b80f
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 47 deletions.
2 changes: 1 addition & 1 deletion src/runtime/base/builtin_functions.cpp
View file
Expand Up @@ -1426,7 +1426,7 @@ bool AutoloadHandler::invokeHandler(CStrRef className,
cur = next.toObject();
next = cur->o_get(s_previous, false, s_exception);
}
cur->o_set(s_previous, autoloadException, false, s_exception);
cur->o_set(s_previous, autoloadException, s_exception);
autoloadException = ex;
}
}
Expand Down
60 changes: 35 additions & 25 deletions src/runtime/base/object_data.cpp
View file
Expand Up @@ -937,22 +937,28 @@ inline ALWAYS_INLINE Variant ObjectData::o_setImpl(CStrRef propName, T v,
return variant(v);
}

Variant ObjectData::o_set(CStrRef propName, CVarRef v,
bool forInit /* = false */,
CStrRef context /* = null_string */) {
return o_setImpl<CVarRef>(propName, v, forInit, context);
Variant ObjectData::o_set(CStrRef propName, CVarRef v) {
return o_setImpl<CVarRef>(propName, v, false, null_string);
}

Variant ObjectData::o_setRef(CStrRef propName, CVarRef v,
bool forInit /* = false */,
CStrRef context /* = null_string */) {
return o_setImpl<RefResult>(propName, ref(v), forInit, context);
Variant ObjectData::o_set(CStrRef propName, RefResult v) {
return o_setRef(propName, variant(v), null_string);
}

Variant ObjectData::o_set(CStrRef propName, RefResult v,
bool forInit /* = false */,
CStrRef context /* = null_string */) {
return o_setRef(propName, variant(v), forInit, context);
Variant ObjectData::o_setRef(CStrRef propName, CVarRef v) {
return o_setImpl<RefResult>(propName, ref(v), false, null_string);
}

Variant ObjectData::o_set(CStrRef propName, CVarRef v, CStrRef context) {
return o_setImpl<CVarRef>(propName, v, false, context);
}

Variant ObjectData::o_set(CStrRef propName, RefResult v, CStrRef context) {
return o_setRef(propName, variant(v), context);
}

Variant ObjectData::o_setRef(CStrRef propName, CVarRef v, CStrRef context) {
return o_setImpl<RefResult>(propName, ref(v), false, context);
}

template<typename T>
Expand Down Expand Up @@ -984,24 +990,28 @@ inline ALWAYS_INLINE Variant ObjectData::o_setPublicImpl(CStrRef propName,
return variant(v);
}

Variant ObjectData::o_setPublic(CStrRef propName, CVarRef v,
bool forInit /* = false */) {
return o_setPublicImpl<CVarRef>(propName, v, forInit);
Variant ObjectData::o_setPublic(CStrRef propName, CVarRef v) {
return o_setPublicImpl<CVarRef>(propName, v, false);
}

Variant ObjectData::o_setPublic(CStrRef propName, RefResult v) {
return o_setPublicRef(propName, variant(v));
}

Variant ObjectData::o_setPublicRef(CStrRef propName, CVarRef v) {
return o_setPublicImpl<CVarStrongBind>(propName, strongBind(v), false);
}

Variant ObjectData::o_setPublic(CStrRef propName, RefResult v,
bool forInit /* = false */) {
return o_setPublicRef(propName, variant(v), forInit);
Variant ObjectData::o_setPublicWithRef(CStrRef propName, CVarRef v) {
return o_setPublicImpl<CVarWithRefBind>(propName, withRefBind(v), false);
}

Variant ObjectData::o_setPublicRef(CStrRef propName, CVarRef v,
bool forInit /* = false */) {
return o_setPublicImpl<CVarStrongBind>(propName, strongBind(v), forInit);
Variant ObjectData::o_i_set(CStrRef propName, CVarRef v) {
return o_setPublicImpl<CVarRef>(propName, v, true);
}

Variant ObjectData::o_setPublicWithRef(CStrRef propName, CVarRef v,
bool forInit /* = false */) {
return o_setPublicImpl<CVarWithRefBind>(propName, withRefBind(v), forInit);
Variant ObjectData::o_i_setPublicWithRef(CStrRef propName, CVarRef v) {
return o_setPublicImpl<CVarWithRefBind>(propName, withRefBind(v), true);
}

void ObjectData::o_setArray(CArrRef properties) {
Expand All @@ -1010,7 +1020,7 @@ void ObjectData::o_setArray(CArrRef properties) {
if (key.empty() || key.charAt(0) != '\0') {
// non-private property
CVarRef secondRef = iter.secondRef();
o_setPublicWithRef(key, secondRef, true);
o_i_setPublicWithRef(key, secondRef);
}
}
}
Expand Down
29 changes: 17 additions & 12 deletions src/runtime/base/object_data.h
View file
Expand Up @@ -191,16 +191,22 @@ class ObjectData : public CountableNF {
CStrRef context = null_string);
Variant o_getPublic(CStrRef s, bool error = true);
Variant o_getUnchecked(CStrRef s, CStrRef context = null_string);
Variant o_set(CStrRef s, CVarRef v, bool forInit = false,
CStrRef context = null_string);
Variant o_set(CStrRef s, RefResult v, bool forInit = false,
CStrRef context = null_string);
Variant o_setRef(CStrRef s, CVarRef v, bool forInit = false,
CStrRef context = null_string);
Variant o_setPublic(CStrRef s, CVarRef v, bool forInit = false);
Variant o_setPublic(CStrRef s, RefResult v, bool forInit = false);
Variant o_setPublicRef(CStrRef s, CVarRef v, bool forInit = false);
Variant o_setPublicWithRef(CStrRef s, CVarRef v, bool forInit = false);

Variant o_i_set(CStrRef s, CVarRef v);
Variant o_i_setPublicWithRef(CStrRef s, CVarRef v);

Variant o_set(CStrRef s, CVarRef v);
Variant o_set(CStrRef s, RefResult v);
Variant o_setRef(CStrRef s, CVarRef v);

Variant o_set(CStrRef s, CVarRef v, CStrRef context);
Variant o_set(CStrRef s, RefResult v, CStrRef context);
Variant o_setRef(CStrRef s, CVarRef v, CStrRef context);
Variant o_setPublic(CStrRef s, CVarRef v);
Variant o_setPublic(CStrRef s, RefResult v);
Variant o_setPublicRef(CStrRef s, CVarRef v);
Variant o_setPublicWithRef(CStrRef s, CVarRef v);

Variant &o_lval(CStrRef s, CVarRef tmpForGet, CStrRef context = null_string);
Variant &o_unsetLval(CStrRef s, CVarRef tmpForGet,
CStrRef context = null_string) {
Expand Down Expand Up @@ -317,8 +323,7 @@ class ObjectData : public CountableNF {
inline Variant o_setImpl(CStrRef propName, T v,
bool forInit, CStrRef context);
template <typename T>
inline Variant o_setPublicImpl(CStrRef propName, T v,
bool forInit);
inline Variant o_setPublicImpl(CStrRef propName, T v, bool forInit);

static Variant *RealPropPublicHelper(CStrRef propName, int64 hash, int flags,
const ObjectData *obj,
Expand Down
8 changes: 4 additions & 4 deletions src/runtime/base/type_object.cpp
View file
Expand Up @@ -111,15 +111,15 @@ Variant Object::o_set(CStrRef propName, CVarRef val,
if (!m_px) {
operator=(SystemLib::AllocStdClassObject());
}
return m_px->o_set(propName, val, false, context);
return m_px->o_set(propName, val, context);
}

Variant Object::o_setRef(CStrRef propName, CVarRef val,
CStrRef context /* = null_string */) {
if (!m_px) {
operator=(SystemLib::AllocStdClassObject());
}
return m_px->o_setRef(propName, val, false, context);
return m_px->o_setRef(propName, val, context);
}

Variant Object::o_set(CStrRef propName, RefResult val,
Expand All @@ -131,14 +131,14 @@ Variant Object::o_setPublic(CStrRef propName, CVarRef val) {
if (!m_px) {
operator=(SystemLib::AllocStdClassObject());
}
return m_px->o_setPublic(propName, val, false);
return m_px->o_setPublic(propName, val);
}

Variant Object::o_setPublicRef(CStrRef propName, CVarRef val) {
if (!m_px) {
operator=(SystemLib::AllocStdClassObject());
}
return m_px->o_setPublicRef(propName, val, false);
return m_px->o_setPublicRef(propName, val);
}

Variant Object::o_setPublic(CStrRef propName, RefResult val) {
Expand Down
8 changes: 4 additions & 4 deletions src/runtime/base/type_variant.cpp
View file
Expand Up @@ -2710,7 +2710,7 @@ Variant Variant::o_set(CStrRef propName, CVarRef val,
raise_warning("Attempt to assign property of non-object");
return val;
}
return m_data.pobj->o_set(propName, val, false, context);
return m_data.pobj->o_set(propName, val, context);
}

Variant Variant::o_setRef(CStrRef propName, CVarRef val,
Expand All @@ -2729,7 +2729,7 @@ Variant Variant::o_setRef(CStrRef propName, CVarRef val,
raise_warning("Attempt to assign property of non-object");
return val;
}
return m_data.pobj->o_setRef(propName, val, false, context);
return m_data.pobj->o_setRef(propName, val, context);
}

Variant Variant::o_setPublic(CStrRef propName, CVarRef val) {
Expand All @@ -2747,7 +2747,7 @@ Variant Variant::o_setPublic(CStrRef propName, CVarRef val) {
raise_warning("Attempt to assign property of non-object");
return val;
}
return m_data.pobj->o_setPublic(propName, val, false);
return m_data.pobj->o_setPublic(propName, val);
}

Variant Variant::o_setPublicRef(CStrRef propName, CVarRef val) {
Expand All @@ -2765,7 +2765,7 @@ Variant Variant::o_setPublicRef(CStrRef propName, CVarRef val) {
raise_warning("Attempt to assign property of non-object");
return val;
}
return m_data.pobj->o_setPublicRef(propName, val, false);
return m_data.pobj->o_setPublicRef(propName, val);
}

Variant Variant::o_invoke(CStrRef s, CArrRef params, int64 hash /* = -1 */) {
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/eval/ast/class_statement.cpp
View file
Expand Up @@ -50,7 +50,7 @@ void ClassVariable::set(VariableEnvironment &env, EvalObjectData *self) const {
if (m_modifiers & ClassStatement::Private) {
self->o_setPrivate(m_cls->name(), m_name, val);
} else if (!self->o_exists(m_name)) {
self->o_set(m_name, val, true);
self->o_i_set(m_name, val);
}
}
}
Expand Down

0 comments on commit f05b80f

Please sign in to comment.