Skip to content
Browse files

[Perf] Reduce o_set overhead

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...
1 parent 7447c5f commit f05b80f2232738357cd9cea5d8ad9dc913062791 @markw65 markw65 committed with scottmac Oct 18, 2011
View
2 src/runtime/base/builtin_functions.cpp
@@ -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;
}
}
View
60 src/runtime/base/object_data.cpp
@@ -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>
@@ -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) {
@@ -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);
}
}
}
View
29 src/runtime/base/object_data.h
@@ -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) {
@@ -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,
View
8 src/runtime/base/type_object.cpp
@@ -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,
@@ -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) {
View
8 src/runtime/base/type_variant.cpp
@@ -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,
@@ -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) {
@@ -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) {
@@ -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 */) {
View
2 src/runtime/eval/ast/class_statement.cpp
@@ -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);
}
}
}

0 comments on commit f05b80f

Please sign in to comment.
Something went wrong with that request. Please try again.