From 458d587c25d2bf1a882b308b2d3fa08a74d67c24 Mon Sep 17 00:00:00 2001 From: Owen Yamauchi Date: Mon, 8 Oct 2012 15:35:27 -0700 Subject: [PATCH] Tweak mergeImpl loop for hoistable classes This loop seems to be a significant source of LLC load misses. To some extent this is unavoidable, since its working set (all the classes in the universe) is way too big for cache. However, we can avoid doing a bunch of the same work repeatedly. The series of pointer chases (*pre->namedEntity()->clsList()) was the major culprit, and if all the Unit's classes are unique and defined without failure, we "cache" the results of those pointer chases and identify them with a marked low-order bit. We already do something very similar for the non-hoistable classes. Also, comment a few subtle things that I realized as I worked, including a presumably deliberate (but safe) race condition. --- src/runtime/vm/unit.cpp | 74 ++++++++++++++++++++++++++++++++++--------------- src/runtime/vm/unit.h | 4 ++- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/src/runtime/vm/unit.cpp b/src/runtime/vm/unit.cpp index 4f1a18f22dd..c1295b14756 100644 --- a/src/runtime/vm/unit.cpp +++ b/src/runtime/vm/unit.cpp @@ -341,7 +341,7 @@ bool Unit::compileTimeFatal(const StringData*& msg, int& line) const { return true; } -Class* Unit::defClass(PreClass* preClass, +Class* Unit::defClass(const PreClass* preClass, bool failIsFatal /* = true */) { Class* const* clsList = preClass->namedEntity()->clsList(); Class* top = *clsList; @@ -423,7 +423,9 @@ Class* Unit::defClass(PreClass* preClass, ec->m_pc = preClass->unit()->at(preClass->getOffset()); ec->pushLocalsAndIterators(tmp.m_func); } - ClassPtr newClass(Class::newClass(preClass, parent)); + // The only reason the newClass param is not const is to increment its + // SmartPtr refcount, which is only modifying a mutable member anyway + ClassPtr newClass(Class::newClass(const_cast(preClass), parent)); if (needsFrame) { ec->m_stack.top() = (Cell*)(ec->m_fp+1); ec->m_fp = fp; @@ -681,12 +683,13 @@ void Unit::mergeImpl(void* tcbase) { // iterate over all the potentially hoistable classes // with no fatals on failure if (ix < end) { - if (LIKELY((m_mergeState & UnitMergeStateUniqueDefinedClasses) != 0)) { - do { - PreClass* pre = (PreClass*)mergeableObj(ix++); - Class* cls = *pre->namedEntity()->clsList(); - ASSERT(cls && !cls->m_nextClass); - ASSERT(cls->preClass() == pre); + do { + // The first time this unit is merged, if the classes turn out to be all + // unique and defined, we replace the PreClass*'s with the corresponding + // Class*'s, with the low-order bit marked. + PreClass* pre = (PreClass*)mergeableObj(ix); + if (LIKELY(uintptr_t(pre) & 1)) { + Class* cls = (Class*)(uintptr_t(pre) & ~1); if (Class* parent = cls->parent()) { if (UNLIKELY(!getDataRef(tcbase, parent->m_cachedOffset))) { redoHoistable = true; @@ -695,13 +698,12 @@ void Unit::mergeImpl(void* tcbase) { } getDataRef(tcbase, cls->m_cachedOffset) = cls; if (debugger) phpDefClassHook(cls); - } while (ix < end); - } else { - do { - PreClass* pre = (PreClass*)mergeableObj(ix++); - if (UNLIKELY(!defClass(pre, false))) redoHoistable = true; - } while (ix < end); - } + } else { + if (UNLIKELY(!defClass(pre, false))) { + redoHoistable = true; + } + } + } while (++ix < end); if (UNLIKELY(redoHoistable)) { // if this unit isnt mergeOnly, we're done if (!isMergeOnly()) return; @@ -724,9 +726,14 @@ void Unit::mergeImpl(void* tcbase) { if (end == (int)m_mergeablesSize) { ix = m_firstHoistablePreClass; do { - PreClass* pre = (PreClass*)mergeableObj(ix++); - defClass(pre, true); - } while (ix < end); + void* obj = mergeableObj(ix); + if (UNLIKELY(uintptr_t(obj) & 1)) { + Class* cls = (Class*)(uintptr_t(obj) & ~1); + defClass(cls->preClass(), true); + } else { + defClass((PreClass*)obj, true); + } + } while (++ix < end); return; } } @@ -819,22 +826,45 @@ void Unit::mergeImpl(void* tcbase) { * All the classes are known to be unique, and we just got * here, so all were successfully defined. We can now go * back and convert all UnitMergeKindClass entries to - * UnitMergeKindUniqueDefinedClass. + * UnitMergeKindUniqueDefinedClass, and all hoistable + * classes to their Class*'s instead of PreClass*'s. * * This is a pure optimization: whether readers see the * old value or the new does not affect correctness. * Also, its idempotent - even if multiple threads do - * this update simultaneously, they all make exactly the - * same change. + * this update simultaneously (which they can -- there is + * a race here, since the check-and-write of m_mergeState + * is not atomic), they all make exactly the same change, + * and can deal with reading pointers that have already + * been marked. */ m_mergeState |= UnitMergeStateUniqueDefinedClasses; - end = ix; + + ix = m_firstHoistablePreClass; + end = m_firstMergeablePreClass; + for (; ix < end; ++ix) { + obj = mergeableObj(ix); + // The mark check is necessary, since the pointer may have already + // been marked, even though this code is "only executed once". See + // the note about races above. + if ((uintptr_t(obj) & 1) == 0) { + PreClass* pre = (PreClass*)obj; + Class* cls = *pre->namedEntity()->clsList(); + ASSERT(cls && !cls->m_nextClass); + ASSERT(cls->preClass() == pre); + mergeableObj(ix) = (void*)(uintptr_t(cls) | 1); + } + } + ix = m_firstMergeablePreClass; + end = m_mergeablesSize; do { obj = mergeableObj(ix); k = UnitMergeKind(uintptr_t(obj) & 7); switch (k) { case UnitMergeKindClass: { + // obj's low-order bits are UnitMergeKindClass, but fortunately, + // UnitMergeKindClass == 0. PreClass* pre = (PreClass*)obj; Class* cls = *pre->namedEntity()->clsList(); ASSERT(cls && !cls->m_nextClass); diff --git a/src/runtime/vm/unit.h b/src/runtime/vm/unit.h index 0e8728f39eb..13e307c6571 100644 --- a/src/runtime/vm/unit.h +++ b/src/runtime/vm/unit.h @@ -39,6 +39,7 @@ enum UnitOrigin { }; enum UnitMergeKind { + // UnitMergeKindClass is required to be 0 for correctness. UnitMergeKindClass = 0, UnitMergeKindUniqueDefinedClass = 1, UnitMergeKindDefine = 2, @@ -47,6 +48,7 @@ enum UnitMergeKind { UnitMergeKindReqSrc = 5, // " UnitMergeKindReqDoc = 6, // " UnitMergeKindDone = 7, + // We cannot add more kinds here; this has to fit in 3 bits. }; enum UnitMergeState { @@ -420,7 +422,7 @@ struct Unit { static Func *lookupFunc(const StringData *funcName); - static Class* defClass(HPHP::VM::PreClass* preClass, + static Class* defClass(const HPHP::VM::PreClass* preClass, bool failIsFatal = true); static Class *lookupClass(const NamedEntity *ne) { -- 2.11.4.GIT