From cc0ef7c4565bc1592567f94eeba8882d8115a0c0 Mon Sep 17 00:00:00 2001 From: smith Date: Fri, 29 Mar 2013 12:24:43 -0700 Subject: [PATCH] Change copy-on-write protocol to always return valid pointers Streamline the array access methods by always returning an array pointer instead of a new pointer or null. Callsites compare (new != old) to detect escalation, rather than (new != null). --- hphp/runtime/base/array/array_data.cpp | 11 +- hphp/runtime/base/array/hphp_array.cpp | 291 ++++++++----------- hphp/runtime/base/array/hphp_array.h | 26 +- hphp/runtime/base/shared/shared_map.cpp | 140 +++------ hphp/runtime/base/shared/shared_variant.cpp | 8 +- hphp/runtime/base/shared/shared_variant.h | 4 +- hphp/runtime/base/type_array.cpp | 83 ++---- hphp/runtime/base/type_array.h | 39 +-- hphp/runtime/base/type_variant.cpp | 64 ++-- hphp/runtime/base/type_variant.h | 4 +- hphp/runtime/vm/member_operations.h | 28 +- hphp/runtime/vm/name_value_table_wrapper.cpp | 10 +- 12 files changed, 255 insertions(+), 453 deletions(-) diff --git a/hphp/runtime/base/array/array_data.cpp b/hphp/runtime/base/array/array_data.cpp index 47c7b356e..6a1e61cf0 100644 --- a/hphp/runtime/base/array/array_data.cpp +++ b/hphp/runtime/base/array/array_data.cpp @@ -223,7 +223,7 @@ ArrayData *ArrayData::pop(Variant &value) { return remove(getKey(pos), getCount() > 1); } value = uninit_null(); - return nullptr; + return this; } ArrayData *ArrayData::dequeue(Variant &value) { @@ -233,16 +233,11 @@ ArrayData *ArrayData::dequeue(Variant &value) { ArrayData *ret = remove(getKey(pos), getCount() > 1); // In PHP, array_shift() will cause all numerically key-ed values re-keyed - if (ret) { - ret->renumber(); - } else { - renumber(); - } - + ret->renumber(); return ret; } value = uninit_null(); - return nullptr; + return this; } /////////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/base/array/hphp_array.cpp b/hphp/runtime/base/array/hphp_array.cpp index 1e6f8ef0c..8f16fd7f3 100644 --- a/hphp/runtime/base/array/hphp_array.cpp +++ b/hphp/runtime/base/array/hphp_array.cpp @@ -926,11 +926,11 @@ bool HphpArray::nextInsert(CVarRef data) { return true; } -void HphpArray::nextInsertRef(CVarRef data) { +ArrayData* HphpArray::nextInsertRef(CVarRef data) { if (UNLIKELY(m_nextKI < 0)) { raise_warning("Cannot add element to the array as the next element is " "already occupied"); - return; + return this; } resizeIfNeeded(); int64_t ki = m_nextKI; @@ -941,9 +941,10 @@ void HphpArray::nextInsertRef(CVarRef data) { initElmInt(allocElm(ei), ki, data, true /*byRef*/); // Update next free element. ++m_nextKI; + return this; } -void HphpArray::nextInsertWithRef(CVarRef data) { +ArrayData* HphpArray::nextInsertWithRef(CVarRef data) { resizeIfNeeded(); int64_t ki = m_nextKI; ElmInd* ei = findForInsert(ki); @@ -957,14 +958,15 @@ void HphpArray::nextInsertWithRef(CVarRef data) { e->setIntKey(ki); // Update next free element. ++m_nextKI; + return this; } -void HphpArray::addLvalImpl(int64_t ki, Variant** pDest) { +ArrayData* HphpArray::addLvalImpl(int64_t ki, Variant** pDest) { assert(pDest != nullptr); ElmInd* ei = findForInsert(ki); if (validElmInd(*ei)) { *pDest = &tvAsVariant(&m_data[*ei].data); - return; + return this; } Elm* e = newElm(ei, ki); tvWriteNull(&e->data); @@ -973,9 +975,10 @@ void HphpArray::addLvalImpl(int64_t ki, Variant** pDest) { if (ki >= m_nextKI && m_nextKI >= 0) { m_nextKI = ki + 1; } + return this; } -void HphpArray::addLvalImpl(StringData* key, strhash_t h, Variant** pDest) { +ArrayData* HphpArray::addLvalImpl(StringData* key, strhash_t h, Variant** pDest) { assert(key != nullptr && pDest != nullptr); ElmInd* ei = findForInsert(key, h); if (validElmInd(*ei)) { @@ -983,7 +986,7 @@ void HphpArray::addLvalImpl(StringData* key, strhash_t h, Variant** pDest) { TypedValue* tv; tv = &e->data; *pDest = &tvAsVariant(tv); - return; + return this; } Elm* e = newElm(ei, h); // Initialize element to null and store the address of the element into @@ -993,9 +996,10 @@ void HphpArray::addLvalImpl(StringData* key, strhash_t h, Variant** pDest) { e->setStrKey(key, h); e->key->incRefCount(); *pDest = &(tvAsVariant(&e->data)); + return this; } -inline void HphpArray::addVal(int64_t ki, CVarRef data) { +inline ArrayData* HphpArray::addVal(int64_t ki, CVarRef data) { assert(!exists(ki)); resizeIfNeeded(); ElmInd* ei = findForNewInsert(ki); @@ -1007,9 +1011,10 @@ inline void HphpArray::addVal(int64_t ki, CVarRef data) { if (ki >= m_nextKI && m_nextKI >= 0) { m_nextKI = ki + 1; } + return this; } -inline void HphpArray::addVal(StringData* key, CVarRef data) { +inline ArrayData* HphpArray::addVal(StringData* key, CVarRef data) { assert(!exists(key)); resizeIfNeeded(); strhash_t h = key->hash(); @@ -1022,157 +1027,139 @@ inline void HphpArray::addVal(StringData* key, CVarRef data) { // Set the key after data is written e->setStrKey(key, h); e->key->incRefCount(); + return this; } -inline void HphpArray::addValWithRef(int64_t ki, CVarRef data) { +inline ArrayData* HphpArray::addValWithRef(int64_t ki, CVarRef data) { resizeIfNeeded(); ElmInd* ei = findForInsert(ki); - if (validElmInd(*ei)) { - return; - } - Elm* e = allocElm(ei); - tvWriteNull(&e->data); - tvAsVariant(&e->data).setWithRef(data); - e->setIntKey(ki); - if (ki >= m_nextKI) { - m_nextKI = ki + 1; + if (!validElmInd(*ei)) { + Elm* e = allocElm(ei); + tvWriteNull(&e->data); + tvAsVariant(&e->data).setWithRef(data); + e->setIntKey(ki); + if (ki >= m_nextKI) { + m_nextKI = ki + 1; + } } + return this; } -inline void HphpArray::addValWithRef(StringData* key, CVarRef data) { +inline ArrayData* HphpArray::addValWithRef(StringData* key, CVarRef data) { resizeIfNeeded(); strhash_t h = key->hash(); ElmInd* ei = findForInsert(key, h); - if (validElmInd(*ei)) { - return; + if (!validElmInd(*ei)) { + Elm* e = allocElm(ei); + tvWriteNull(&e->data); + tvAsVariant(&e->data).setWithRef(data); + e->setStrKey(key, h); + e->key->incRefCount(); } - Elm* e = allocElm(ei); - tvWriteNull(&e->data); - tvAsVariant(&e->data).setWithRef(data); - e->setStrKey(key, h); - e->key->incRefCount(); + return this; } inline INLINE_SINGLE_CALLER -void HphpArray::update(int64_t ki, CVarRef data) { +ArrayData* HphpArray::update(int64_t ki, CVarRef data) { ElmInd* ei = findForInsert(ki); if (validElmInd(*ei)) { Elm* e = &m_data[*ei]; tvAsVariant(&e->data).assignValHelper(data); - return; + return this; } newElmInt(ei, ki, data); if (ki >= m_nextKI && m_nextKI >= 0) { m_nextKI = ki + 1; } + return this; } inline INLINE_SINGLE_CALLER -void HphpArray::update(StringData* key, CVarRef data) { +ArrayData* HphpArray::update(StringData* key, CVarRef data) { strhash_t h = key->hash(); ElmInd* ei = findForInsert(key, h); if (validElmInd(*ei)) { Elm* e = &m_data[*ei]; tvAsVariant(&e->data).assignValHelper(data); - return; + return this; } newElmStr(ei, h, key, data); + return this; } -void HphpArray::updateRef(int64_t ki, CVarRef data) { +ArrayData* HphpArray::updateRef(int64_t ki, CVarRef data) { ElmInd* ei = findForInsert(ki); if (validElmInd(*ei)) { Elm* e = &m_data[*ei]; tvAsVariant(&e->data).assignRefHelper(data); - return; + return this; } newElmInt(ei, ki, data, true /*byRef*/); if (ki >= m_nextKI && m_nextKI >= 0) { m_nextKI = ki + 1; } + return this; } -void HphpArray::updateRef(StringData* key, CVarRef data) { +ArrayData* HphpArray::updateRef(StringData* key, CVarRef data) { strhash_t h = key->hash(); ElmInd* ei = findForInsert(key, h); if (validElmInd(*ei)) { Elm* e = &m_data[*ei]; tvAsVariant(&e->data).assignRefHelper(data); - return; + return this; } newElmStr(ei, h, key, data, true /*byRef*/); + return this; } ArrayData* HphpArray::lval(int64_t k, Variant*& ret, bool copy, bool checkExist /* = false */) { - if (!copy) { - addLvalImpl(k, &ret); - return nullptr; - } - if (!checkExist) { - HphpArray* a = copyImpl(); - a->addLvalImpl(k, &ret); - return a; - } - ssize_t /*ElmInd*/ pos = find(k); - if (pos != (ssize_t)ElmIndEmpty) { - Elm* e = &m_data[pos]; - if (tvAsVariant(&e->data).isReferenced() || - tvAsVariant(&e->data).isObject()) { - ret = &tvAsVariant(&e->data); - return nullptr; + if (!copy) return addLvalImpl(k, &ret); + if (checkExist) { + ssize_t /*ElmInd*/ pos = find(k); + if (pos != (ssize_t)ElmIndEmpty) { + Elm* e = &m_data[pos]; + if (tvAsVariant(&e->data).isReferenced() || + tvAsVariant(&e->data).isObject()) { + ret = &tvAsVariant(&e->data); + return this; + } } } - HphpArray* a = copyImpl(); - a->addLvalImpl(k, &ret); - return a; + return copyImpl()->addLvalImpl(k, &ret); } ArrayData* HphpArray::lval(StringData* key, Variant*& ret, bool copy, bool checkExist /* = false */) { strhash_t prehash = key->hash(); - if (!copy) { - addLvalImpl(key, prehash, &ret); - return nullptr; - } - if (!checkExist) { - HphpArray* a = copyImpl(); - a->addLvalImpl(key, prehash, &ret); - return a; - } - ssize_t /*ElmInd*/ pos = find(key, prehash); - if (pos != (ssize_t)ElmIndEmpty) { - Elm* e = &m_data[pos]; - TypedValue* tv = &e->data; - if (tvAsVariant(tv).isReferenced() || - tvAsVariant(tv).isObject()) { - ret = &tvAsVariant(tv); - return nullptr; + if (!copy) return addLvalImpl(key, prehash, &ret); + if (checkExist) { + ssize_t /*ElmInd*/ pos = find(key, prehash); + if (pos != (ssize_t)ElmIndEmpty) { + Elm* e = &m_data[pos]; + TypedValue* tv = &e->data; + if (tvAsVariant(tv).isReferenced() || + tvAsVariant(tv).isObject()) { + ret = &tvAsVariant(tv); + return this; + } } } - HphpArray* a = copyImpl(); - a->addLvalImpl(key, prehash, &ret); - return a; + return copyImpl()->addLvalImpl(key, prehash, &ret); } ArrayData *HphpArray::lvalPtr(StringData* key, Variant*& ret, bool copy, bool create) { strhash_t prehash = key->hash(); - HphpArray* a = 0; - HphpArray* t = this; - if (copy) { - a = t = copyImpl(); - } - if (create) { - t->addLvalImpl(key, prehash, &ret); + HphpArray* a = !copy ? this : copyImpl(); + if (create) return a->addLvalImpl(key, prehash, &ret); + ssize_t /*ElmInd*/ pos = a->find(key, prehash); + if (pos != (ssize_t)ElmIndEmpty) { + Elm* e = &a->m_data[pos]; + ret = &tvAsVariant(&e->data); } else { - ssize_t /*ElmInd*/ pos = t->find(key, prehash); - if (pos != (ssize_t)ElmIndEmpty) { - Elm* e = &t->m_data[pos]; - ret = &tvAsVariant(&e->data); - } else { - ret = nullptr; - } + ret = nullptr; } return a; } @@ -1189,71 +1176,55 @@ ArrayData* HphpArray::lvalNew(Variant*& ret, bool copy) { } ArrayData* HphpArray::set(int64_t k, CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->update(k, v); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->update(k, v); } ArrayData* HphpArray::set(StringData* k, CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->update(k, v); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->update(k, v); } ArrayData* HphpArray::setRef(int64_t k, CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->updateRef(k, v); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->updateRef(k, v); } ArrayData* HphpArray::setRef(StringData* k, CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->updateRef(k, v); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->updateRef(k, v); } ArrayData* HphpArray::add(int64_t k, CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->addVal(k, v); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->addVal(k, v); } ArrayData* HphpArray::add(StringData* k, CVarRef v, bool copy) { assert(!exists(k)); - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->addVal(k, v); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->addVal(k, v); } ArrayData* HphpArray::addLval(int64_t k, Variant*& ret, bool copy) { assert(!exists(k)); - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->addLvalImpl(k, &ret); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->addLvalImpl(k, &ret); } ArrayData* HphpArray::addLval(StringData* k, Variant*& ret, bool copy) { assert(!exists(k)); - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->addLvalImpl(k, k->hash(), &ret); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->addLvalImpl(k, k->hash(), &ret); } //============================================================================= // Delete. -void HphpArray::erase(ElmInd* ei, bool updateNext /* = false */) { +ArrayData* HphpArray::erase(ElmInd* ei, bool updateNext /* = false */) { ElmInd pos = *ei; if (!validElmInd(pos)) { - return; + return this; } Elm* elms = m_data; @@ -1324,20 +1295,17 @@ void HphpArray::erase(ElmInd* ei, bool updateNext /* = false */) { // Compact in order to keep elms from being overly sparse. compact(); } + return this; } ArrayData* HphpArray::remove(int64_t k, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->erase(a->findForInsert(k)); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->erase(a->findForInsert(k)); } ArrayData* HphpArray::remove(const StringData* key, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->erase(a->findForInsert(key, key->hash())); - return t; + HphpArray* a = !copy ? this : copyImpl(); + return a->erase(a->findForInsert(key, key->hash())); } ArrayData* HphpArray::copy() const { @@ -1384,16 +1352,15 @@ TypedValue* HphpArray::nvGet(const StringData* k) const { } ArrayData* HphpArray::nvNew(TypedValue*& ret, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); + HphpArray* a = !copy ? this : copyImpl(); if (UNLIKELY(!a->nextInsert(uninit_null()))) { ret = nullptr; - return t; + return a; } assert(a->m_lastE != ElmIndEmpty); ssize_t lastE = (ssize_t)a->m_lastE; ret = &a->m_data[lastE].data; - return t; + return a; } TypedValue* HphpArray::nvGetValueRef(ssize_t pos) { @@ -1487,10 +1454,9 @@ NEVER_INLINE HphpArray* HphpArray::copyImpl() const { } ArrayData* HphpArray::append(CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); + HphpArray *a = !copy ? this : copyImpl(); a->nextInsert(v); - return t; + return a; } /* @@ -1502,7 +1468,7 @@ ArrayData* genericAddNewElemC(ArrayData* a, TypedValue value) { assert(a->getCount() <= 1); ArrayData* UNUSED r = a->append(tvAsCVarRef(&value), false); tvRefcountedDecRef(value); - assert(!r); + assert(r == a); return a; } @@ -1534,26 +1500,17 @@ ArrayData* HphpArray::AddNewElemC(ArrayData* a, TypedValue value) { } ArrayData* HphpArray::appendRef(CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->nextInsertRef(v); - return t; + HphpArray *a = !copy ? this : copyImpl(); + return a->nextInsertRef(v); } ArrayData *HphpArray::appendWithRef(CVarRef v, bool copy) { - HphpArray *a = this, *t = 0; - if (copy) a = t = copyImpl(); - a->nextInsertWithRef(v); - return t; + HphpArray *a = !copy ? this : copyImpl(); + return a->nextInsertWithRef(v); } ArrayData* HphpArray::append(const ArrayData* elems, ArrayOp op, bool copy) { - HphpArray* a = this; - HphpArray* result = nullptr; - if (copy) { - result = a = copyImpl(); - } - + HphpArray* a = !copy ? this : copyImpl(); if (op == Plus) { for (ArrayIter it(elems); !it.end(); it.next()) { Variant key = it.first(); @@ -1579,15 +1536,11 @@ ArrayData* HphpArray::append(const ArrayData* elems, ArrayOp op, bool copy) { } } } - return result; + return a; } ArrayData* HphpArray::pop(Variant& value) { - HphpArray* a = this; - HphpArray* result = nullptr; - if (getCount() > 1) { - result = a = copyImpl(); - } + HphpArray* a = getCount() <= 1 ? this : copyImpl(); Elm* elms = a->m_data; ElmInd pos = a->HphpArray::iter_end(); if (validElmInd(pos)) { @@ -1604,15 +1557,11 @@ ArrayData* HphpArray::pop(Variant& value) { // To conform to PHP behavior, the pop operation resets the array's // internal iterator. a->m_pos = a->nextElm(elms, ElmIndEmpty); - return result; + return a; } ArrayData* HphpArray::dequeue(Variant& value) { - HphpArray* a = this; - HphpArray* result = nullptr; - if (getCount() > 1) { - result = a = copyImpl(); - } + HphpArray* a = getCount() <= 1 ? this : copyImpl(); // To conform to PHP behavior, we invalidate all strong iterators when an // element is removed from the beginning of the array. a->freeStrongIterators(); @@ -1631,15 +1580,11 @@ ArrayData* HphpArray::dequeue(Variant& value) { // To conform to PHP behavior, the dequeue operation resets the array's // internal iterator a->m_pos = ssize_t(a->nextElm(elms, ElmIndEmpty)); - return result; + return a; } ArrayData* HphpArray::prepend(CVarRef v, bool copy) { - HphpArray* a = this; - HphpArray* result = nullptr; - if (copy) { - result = a = copyImpl(); - } + HphpArray* a = getCount() <= 1 ? this : copyImpl(); // To conform to PHP behavior, we invalidate all strong iterators when an // element is added to the beginning of the array. a->freeStrongIterators(); @@ -1669,7 +1614,7 @@ ArrayData* HphpArray::prepend(CVarRef v, bool copy) { // To conform to PHP behavior, the prepend operation resets the array's // internal iterator a->m_pos = ssize_t(a->nextElm(elms, ElmIndEmpty)); - return result; + return a; } void HphpArray::renumber() { @@ -1770,7 +1715,7 @@ array_setm(RefData* ref, ArrayData* ad, Key key, TypedValue* value) { if (DecRefValue) tvRefcountedDecRef(value); if (!ref) return arrayRefShuffle(ad, retval, nullptr); arrayRefShuffle(ad, retval, ref->tv()); - return nullptr; + return retval; } /** @@ -1936,7 +1881,7 @@ ArrayData* array_add(ArrayData* a1, ArrayData* a2) { if (a1 != a2) { ArrayData *escalated = a1->append(a2, ArrayData::Plus, a1->getCount() > 1); - if (escalated) { + if (escalated != a1) { escalated->incRefCount(); decRefArr(a2); decRefArr(a1); diff --git a/hphp/runtime/base/array/hphp_array.h b/hphp/runtime/base/array/hphp_array.h index b4f524cec..60951ab31 100644 --- a/hphp/runtime/base/array/hphp_array.h +++ b/hphp/runtime/base/array/hphp_array.h @@ -412,21 +412,21 @@ private: } bool nextInsert(CVarRef data); - void nextInsertRef(CVarRef data); - void nextInsertWithRef(CVarRef data); - void addLvalImpl(int64_t ki, Variant** pDest); - void addLvalImpl(StringData* key, strhash_t h, Variant** pDest); - void addVal(int64_t ki, CVarRef data); - void addVal(StringData* key, CVarRef data); - void addValWithRef(int64_t ki, CVarRef data); - void addValWithRef(StringData* key, CVarRef data); + ArrayData* nextInsertRef(CVarRef data); + ArrayData* nextInsertWithRef(CVarRef data); + ArrayData* addLvalImpl(int64_t ki, Variant** pDest); + ArrayData* addLvalImpl(StringData* key, strhash_t h, Variant** pDest); + ArrayData* addVal(int64_t ki, CVarRef data); + ArrayData* addVal(StringData* key, CVarRef data); + ArrayData* addValWithRef(int64_t ki, CVarRef data); + ArrayData* addValWithRef(StringData* key, CVarRef data); - void update(int64_t ki, CVarRef data); - void update(StringData* key, CVarRef data); - void updateRef(int64_t ki, CVarRef data); - void updateRef(StringData* key, CVarRef data); + ArrayData* update(int64_t ki, CVarRef data); + ArrayData* update(StringData* key, CVarRef data); + ArrayData* updateRef(int64_t ki, CVarRef data); + ArrayData* updateRef(StringData* key, CVarRef data); - void erase(ElmInd* ei, bool updateNext = false); + ArrayData* erase(ElmInd* ei, bool updateNext = false); HphpArray* copyImpl(HphpArray* target) const; HphpArray* copyImpl() const; diff --git a/hphp/runtime/base/shared/shared_map.cpp b/hphp/runtime/base/shared/shared_map.cpp index 19fa17776..250172f0b 100644 --- a/hphp/runtime/base/shared/shared_map.cpp +++ b/hphp/runtime/base/shared/shared_map.cpp @@ -89,155 +89,90 @@ CVarRef SharedMap::get(int64_t k, bool error /* = false */) const { return getValueRef(index); } +/* if a2 is modified copy of a1 (i.e. != a1), then release a1 and return a2 */ +inline ArrayData* releaseIfCopied(ArrayData* a1, ArrayData* a2) { + if (a1 != a2) a1->release(); + return a2; +} + ArrayData *SharedMap::lval(int64_t k, Variant *&ret, bool copy, bool checkExist /* = false */) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->lval(k, ret, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->lval(k, ret, false)); } ArrayData *SharedMap::lval(StringData* k, Variant *&ret, bool copy, bool checkExist /* = false */) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->lval(k, ret, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->lval(k, ret, false)); } ArrayData *SharedMap::lvalNew(Variant *&ret, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->lvalNew(ret, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->lvalNew(ret, false)); } ArrayData *SharedMap::set(int64_t k, CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->set(k, v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->set(k, v, false)); } ArrayData *SharedMap::set(StringData* k, CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->set(k, v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->set(k, v, false)); } ArrayData *SharedMap::setRef(int64_t k, CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->setRef(k, v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->setRef(k, v, false)); } ArrayData *SharedMap::setRef(StringData* k, CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->setRef(k, v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->setRef(k, v, false)); } ArrayData *SharedMap::remove(int64_t k, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->remove(k, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->remove(k, false)); } ArrayData *SharedMap::remove(const StringData* k, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->remove(k, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->remove(k, false)); } ArrayData *SharedMap::copy() const { - return escalate(); + return SharedMap::escalate(); } ArrayData *SharedMap::append(CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->append(v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->append(v, false)); } ArrayData *SharedMap::appendRef(CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->appendRef(v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->appendRef(v, false)); } ArrayData *SharedMap::appendWithRef(CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->appendWithRef(v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->appendWithRef(v, false)); } ArrayData *SharedMap::append(const ArrayData *elems, ArrayOp op, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->append(elems, op, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->append(elems, op, false)); } ArrayData *SharedMap::prepend(CVarRef v, bool copy) { - ArrayData *escalated = escalate(); - ArrayData *ee = escalated->prepend(v, false); - if (ee) { - escalated->release(); - return ee; - } - return escalated; + ArrayData *escalated = SharedMap::escalate(); + return releaseIfCopied(escalated, escalated->prepend(v, false)); } ArrayData *SharedMap::escalate() const { - ArrayData *ret = nullptr; - m_arr->loadElems(ret, *this); + ArrayData *ret = m_arr->loadElems(*this); assert(!ret->isStatic()); return ret; } @@ -280,8 +215,7 @@ TypedValue* SharedMap::nvGetCell(const StringData* key) const { } ArrayData* SharedMap::escalateForSort() { - ArrayData *ret = nullptr; - m_arr->loadElems(ret, *this, true /* mapInit */); + ArrayData *ret = m_arr->loadElems(*this, true /* mapInit */); assert(!ret->isStatic()); return ret; } diff --git a/hphp/runtime/base/shared/shared_variant.cpp b/hphp/runtime/base/shared/shared_variant.cpp index e45fb78ac..cbde8b2d3 100644 --- a/hphp/runtime/base/shared/shared_variant.cpp +++ b/hphp/runtime/base/shared/shared_variant.cpp @@ -272,9 +272,8 @@ SharedVariant* SharedVariant::getValue(ssize_t pos) const { return m_data.map->getValIndex(pos); } -void SharedVariant::loadElems(ArrayData *&elems, - const SharedMap &sharedMap, - bool mapInit /* = false */) { +ArrayData* SharedVariant::loadElems(const SharedMap &sharedMap, + bool mapInit /* = false */) { assert(is(KindOfArray)); uint count = arrSize(); bool isVector = getIsVector(); @@ -291,8 +290,9 @@ void SharedVariant::loadElems(ArrayData *&elems, true); } } - elems = ai.create(); + ArrayData* elems = ai.create(); if (elems->isStatic()) elems = elems->copy(); + return elems; } int SharedVariant::countReachable() const { diff --git a/hphp/runtime/base/shared/shared_variant.h b/hphp/runtime/base/shared/shared_variant.h index cf46713cc..5e595af11 100644 --- a/hphp/runtime/base/shared/shared_variant.h +++ b/hphp/runtime/base/shared/shared_variant.h @@ -113,8 +113,8 @@ public: int getIndex(int64_t key); int getIndex(const StringData* key); - void loadElems(ArrayData *&elems, const SharedMap &sharedMap, - bool mapInit = false); + ArrayData* loadElems(const SharedMap &sharedMap, + bool mapInit = false); Variant getKey(ssize_t pos) const; diff --git a/hphp/runtime/base/type_array.cpp b/hphp/runtime/base/type_array.cpp index 1704d02f5..94021c3ae 100644 --- a/hphp/runtime/base/type_array.cpp +++ b/hphp/runtime/base/type_array.cpp @@ -37,9 +37,7 @@ const Array null_array = Array(); void Array::setEvalScalar() const { Array* thisPtr = const_cast(this); - if (!m_px) { - *thisPtr = ArrayData::Create(); - } + if (!m_px) *thisPtr = ArrayData::Create(); if (!m_px->isStatic()) { ArrayData *ad = ArrayData::GetScalarArray(m_px); *thisPtr = ad; @@ -295,9 +293,7 @@ Array &Array::mergeImpl(ArrayData *data, ArrayData::ArrayOp op) { ArrayBase::operator=(data); } else if (m_px != data || op == ArrayData::Merge) { ArrayData *escalated = m_px->append(data, op, m_px->getCount() > 1); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); } } else if (op == ArrayData::Merge) { m_px->renumber(); @@ -540,9 +536,7 @@ Variant Array::rvalAt(CVarRef key, ACCESSPARAMS_IMPL) const { Variant *Array::lvalPtr(CStrRef key, bool forWrite, bool create) { if (create) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); return &lvalAt(key, AccessFlags::Key); } Variant *ret = nullptr; @@ -550,23 +544,17 @@ Variant *Array::lvalPtr(CStrRef key, bool forWrite, bool create) { ArrayData *escalated = m_px->lvalPtr(key, ret, forWrite && m_px->getCount() > 1, false); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); } return ret; } Variant &Array::lvalAt() { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); Variant *ret = nullptr; ArrayData *arr = m_px; ArrayData *escalated = arr->lvalNew(ret, arr->getCount() > 1); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != arr) ArrayBase::operator=(escalated); assert(ret); return *ret; } @@ -596,11 +584,8 @@ CVarRef Array::setImpl(const T &key, CVarRef v) { ArrayData *data = ArrayData::Create(key, v); ArrayBase::operator=(data); } else { - ArrayData *escalated = - m_px->set(key, v, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + ArrayData *escalated = m_px->set(key, v, (m_px->getCount() > 1)); + if (escalated != m_px) ArrayBase::operator=(escalated); } return v; } @@ -613,11 +598,8 @@ CVarRef Array::setRefImpl(const T &key, CVarRef v) { ArrayBase::operator=(data); } else { escalate(); - ArrayData *escalated = - m_px->setRef(key, v, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + ArrayData *escalated = m_px->setRef(key, v, (m_px->getCount() > 1)); + if (escalated != m_px) ArrayBase::operator=(escalated); } return v; } @@ -630,9 +612,7 @@ CVarRef Array::addImpl(const T &key, CVarRef v) { ArrayBase::operator=(data); } else { ArrayData *escalated = m_px->add(key, v, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); } return v; } @@ -849,9 +829,7 @@ CVarRef Array::append(CVarRef v) { ArrayBase::operator=(ArrayData::Create(v)); } else { ArrayData *escalated = m_px->append(v, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); } return v; } @@ -861,33 +839,23 @@ CVarRef Array::appendRef(CVarRef v) { ArrayBase::operator=(ArrayData::CreateRef(v)); } else { ArrayData *escalated = m_px->appendRef(v, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); } return v; } CVarRef Array::appendWithRef(CVarRef v) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); ArrayData *escalated = m_px->appendWithRef(v, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); return v; } Variant Array::appendOpEqual(int op, CVarRef v) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); Variant *cv = nullptr; ArrayData *escalated = m_px->lvalNew(cv, m_px->getCount() > 1); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); assert(cv); switch (op) { case T_CONCAT_EQUAL: return concat_assign((*cv), v); @@ -910,9 +878,7 @@ Variant Array::pop() { if (m_px) { Variant ret; ArrayData *newarr = m_px->pop(ret); - if (newarr) { - ArrayBase::operator=(newarr); - } + if (newarr != m_px) ArrayBase::operator=(newarr); return ret; } return null_variant; @@ -922,24 +888,17 @@ Variant Array::dequeue() { if (m_px) { Variant ret; ArrayData *newarr = m_px->dequeue(ret); - if (newarr) { - ArrayBase::operator=(newarr); - } + if (newarr != m_px) ArrayBase::operator=(newarr); return ret; } return null_variant; } void Array::prepend(CVarRef v) { - if (!m_px) { - operator=(Create()); - } + if (!m_px) operator=(Create()); assert(m_px); - ArrayData *newarr = m_px->prepend(v, (m_px->getCount() > 1)); - if (newarr) { - ArrayBase::operator=(newarr); - } + if (newarr != m_px) ArrayBase::operator=(newarr); } /////////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/base/type_array.h b/hphp/runtime/base/type_array.h index aae046a8d..58e1e2d75 100644 --- a/hphp/runtime/base/type_array.h +++ b/hphp/runtime/base/type_array.h @@ -288,27 +288,19 @@ class Array : protected ArrayBase { const Variant operator[](CVarRef key) const; Variant &lval(int64_t key) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); Variant *ret = nullptr; ArrayData *escalated = m_px->lval(key, ret, m_px->getCount() > 1); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); assert(ret); return *ret; } Variant &lval(CStrRef key) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); Variant *ret = nullptr; ArrayData *escalated = m_px->lval(key, ret, m_px->getCount() > 1); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); assert(ret); return *ret; } @@ -408,14 +400,10 @@ class Array : protected ArrayBase { // defined in type_variant.h template Variant &addLvalImpl(const T &key) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); Variant *ret = nullptr; ArrayData *escalated = m_px->addLval(key, ret, m_px->getCount() > 1); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); assert(ret); return *ret; } @@ -470,11 +458,8 @@ class Array : protected ArrayBase { template void removeImpl(const T &key) { if (m_px) { - ArrayData *escalated = - m_px->remove(key, (m_px->getCount() > 1)); - if (escalated) { - ArrayBase::operator=(escalated); - } + ArrayData *escalated = m_px->remove(key, (m_px->getCount() > 1)); + if (escalated != m_px) ArrayBase::operator=(escalated); } } void remove(bool key) { @@ -556,16 +541,12 @@ class Array : protected ArrayBase { template Variant &lvalAtImpl(const T &key, ACCESSPARAMS_DECL) { - if (!m_px) { - ArrayBase::operator=(ArrayData::Create()); - } + if (!m_px) ArrayBase::operator=(ArrayData::Create()); Variant *ret = nullptr; ArrayData *escalated = m_px->lval(key, ret, m_px->getCount() > 1, flags & AccessFlags::CheckExist); - if (escalated) { - ArrayBase::operator=(escalated); - } + if (escalated != m_px) ArrayBase::operator=(escalated); assert(ret); return *ret; } diff --git a/hphp/runtime/base/type_variant.cpp b/hphp/runtime/base/type_variant.cpp index 5da4cddcf..c1dbaec1b 100644 --- a/hphp/runtime/base/type_variant.cpp +++ b/hphp/runtime/base/type_variant.cpp @@ -471,10 +471,9 @@ Variant Variant::pop() { } Variant ret; - ArrayData *newarr = getArrayData()->pop(ret); - if (newarr) { - set(newarr); - } + ArrayData* a = getArrayData(); + ArrayData* newarr = a->pop(ret); + if (newarr != a) set(newarr); return ret; } @@ -488,10 +487,9 @@ Variant Variant::dequeue() { } Variant ret; - ArrayData *newarr = getArrayData()->dequeue(ret); - if (newarr) { - set(newarr); - } + ArrayData* a = getArrayData(); + ArrayData* newarr = a->dequeue(ret); + if (newarr != a) set(newarr); return ret; } @@ -500,16 +498,11 @@ void Variant::prepend(CVarRef v) { m_data.pref->var()->prepend(v); return; } - if (isNull()) { - set(Array::Create()); - } - + if (isNull()) set(Array::Create()); if (is(KindOfArray)) { ArrayData *arr = getArrayData(); ArrayData *newarr = arr->prepend(v, (arr->getCount() > 1)); - if (newarr) { - set(newarr); - } + if (newarr != arr) set(newarr); } else { throw_bad_type_exception("expecting an array"); } @@ -518,8 +511,7 @@ void Variant::prepend(CVarRef v) { Variant Variant::array_iter_reset() { if (is(KindOfArray)) { ArrayData *arr = getArrayData(); - if (arr->getCount() > 1 && !arr->isHead() - && !arr->noCopyOnWrite()) { + if (arr->getCount() > 1 && !arr->isHead() && !arr->noCopyOnWrite()) { arr = arr->copy(); set(arr); assert(arr == getArrayData()); @@ -533,8 +525,7 @@ Variant Variant::array_iter_reset() { Variant Variant::array_iter_prev() { if (is(KindOfArray)) { ArrayData *arr = getArrayData(); - if (arr->getCount() > 1 && !arr->isInvalid() - && !arr->noCopyOnWrite()) { + if (arr->getCount() > 1 && !arr->isInvalid() && !arr->noCopyOnWrite()) { arr = arr->copy(); set(arr); assert(arr == getArrayData()); @@ -571,8 +562,7 @@ Variant Variant::array_iter_current_ref() { Variant Variant::array_iter_next() { if (is(KindOfArray)) { ArrayData *arr = getArrayData(); - if (arr->getCount() > 1 && !arr->isInvalid() - && !arr->noCopyOnWrite()) { + if (arr->getCount() > 1 && !arr->isInvalid() && !arr->noCopyOnWrite()) { arr = arr->copy(); set(arr); assert(arr == getArrayData()); @@ -586,8 +576,7 @@ Variant Variant::array_iter_next() { Variant Variant::array_iter_end() { if (is(KindOfArray)) { ArrayData *arr = getArrayData(); - if (arr->getCount() > 1 && !arr->isTail() - && !arr->noCopyOnWrite()) { + if (arr->getCount() > 1 && !arr->isTail() && !arr->noCopyOnWrite()) { arr = arr->copy(); set(arr); assert(arr == getArrayData()); @@ -609,8 +598,7 @@ Variant Variant::array_iter_key() const { Variant Variant::array_iter_each() { if (is(KindOfArray)) { ArrayData *arr = getArrayData(); - if (arr->getCount() > 1 && !arr->isInvalid() - && !arr->noCopyOnWrite()) { + if (arr->getCount() > 1 && !arr->isInvalid() && !arr->noCopyOnWrite()) { arr = arr->copy(); set(arr); assert(arr == getArrayData()); @@ -718,7 +706,7 @@ Variant &Variant::operator+=(CVarRef var) { } ArrayData *escalated = arr1->append(arr2, ArrayData::Plus, arr1->getCount() > 1); - if (escalated) set(escalated); + if (escalated != arr1) set(escalated); return *this; } else if (na) { throw BadArrayMergeException(); @@ -2276,10 +2264,10 @@ head: } else { if (blackHole) ret = &lvalBlackHole(); else ret = tmp; - escalated = 0; + escalated = arr; } } - if (escalated) { + if (escalated != arr) { self->set(escalated); } assert(ret); @@ -2414,7 +2402,7 @@ Variant &Variant::lvalAt() { Variant *ret = nullptr; ArrayData *arr = m_data.parr; ArrayData *escalated = arr->lvalNew(ret, arr->getCount() > 1); - if (escalated) { + if (escalated != arr) { set(escalated); } assert(ret); @@ -2510,7 +2498,7 @@ inline ALWAYS_INLINE CVarRef Variant::SetImpl(Variant *self, T key, if (!LvalHelper::CheckKey(k)) return lvalBlackHole(); escalated = self->m_data.parr->set(k, v, self->needCopyForSet(v)); } - if (escalated) { + if (escalated != self->m_data.parr) { self->set(escalated); } return v; @@ -2599,7 +2587,7 @@ CVarRef Variant::append(CVarRef v) { case KindOfArray: { ArrayData *escalated = m_data.parr->append(v, needCopyForSet(v)); - if (escalated) { + if (escalated != m_data.parr) { set(escalated); } } @@ -2644,7 +2632,7 @@ inline ALWAYS_INLINE CVarRef Variant::SetRefImpl(Variant *self, T key, if (!LvalHelper::CheckKey(k)) return lvalBlackHole(); escalated = self->m_data.parr->setRef(k, v, self->needCopyForSetRef(v)); } - if (escalated) { + if (escalated != self->m_data.parr) { self->set(escalated); } return v; @@ -2728,7 +2716,7 @@ CVarRef Variant::appendRef(CVarRef v) { case KindOfArray: { ArrayData *escalated = m_data.parr->appendRef(v, needCopyForSetRef(v)); - if (escalated) { + if (escalated != m_data.parr) { set(escalated); } } @@ -2768,7 +2756,7 @@ void Variant::removeImpl(double key) { ArrayData *arr = getArrayData(); if (arr) { ArrayData *escalated = arr->remove(ToKey(key), (arr->getCount() > 1)); - if (escalated) { + if (escalated != arr) { set(escalated); } } @@ -2793,7 +2781,7 @@ void Variant::removeImpl(int64_t key) { ArrayData *arr = getArrayData(); if (arr) { ArrayData *escalated = arr->remove(key, (arr->getCount() > 1)); - if (escalated) { + if (escalated != arr) { set(escalated); } } @@ -2818,7 +2806,7 @@ void Variant::removeImpl(bool key) { ArrayData *arr = getArrayData(); if (arr) { ArrayData *escalated = arr->remove(ToKey(key), (arr->getCount() > 1)); - if (escalated) { + if (escalated != arr) { set(escalated); } } @@ -2850,7 +2838,7 @@ void Variant::removeImpl(CVarRef key, bool isString /* false */) { if (k.isNull()) return; escalated = arr->remove(k, (arr->getCount() > 1)); } - if (escalated) { + if (escalated != arr) { set(escalated); } } @@ -2880,7 +2868,7 @@ void Variant::removeImpl(CStrRef key, bool isString /* false */) { } else { escalated = arr->remove(key.toKey(), (arr->getCount() > 1)); } - if (escalated) { + if (escalated != arr) { set(escalated); } } diff --git a/hphp/runtime/base/type_variant.h b/hphp/runtime/base/type_variant.h index ff823e686..6d13735cd 100644 --- a/hphp/runtime/base/type_variant.h +++ b/hphp/runtime/base/type_variant.h @@ -851,9 +851,7 @@ class Variant : private VariantBase { Variant *ret = nullptr; ArrayData *arr = m_data.parr; ArrayData *escalated = arr->lval(key, ret, arr->getCount() > 1); - if (escalated) { - set(escalated); - } + if (escalated != arr) set(escalated); assert(ret); return *ret; } diff --git a/hphp/runtime/vm/member_operations.h b/hphp/runtime/vm/member_operations.h index 0b14f644a..0e54ed712 100644 --- a/hphp/runtime/vm/member_operations.h +++ b/hphp/runtime/vm/member_operations.h @@ -544,7 +544,7 @@ template<> struct ShuffleReturn { template inline typename ShuffleReturn::return_type arrayRefShuffle(ArrayData* oldData, ArrayData* newData, TypedValue* base) { - if (newData == nullptr) { + if (newData == oldData) { return ShuffleReturn::do_return(oldData); } @@ -587,7 +587,7 @@ template inline void SetElemArray(TypedValue* base, TypedValue* key, Cell* value) { ArrayData* a = base->m_data.parr; - ArrayData* newData = nullptr; + ArrayData* newData = a; bool copy = (a->getCount() > 1) || (value->m_type == KindOfArray && value->m_data.parr == a); @@ -612,6 +612,7 @@ inline void SetElemArray(TypedValue* base, TypedValue* key, arrayRefShuffle(a, newData, base); } + // SetElem() leaves the result in 'value', rather than returning it as in // SetOpElem(), because doing so avoids a dup operation that SetOpElem() can't // get around. @@ -781,11 +782,11 @@ inline void SetNewElem(TypedValue* base, Cell* value) { ArrayData* a = base->m_data.parr; bool copy = (a->getCount() > 1) || (value->m_type == KindOfArray && value->m_data.parr == a); - a = a->append(tvCellAsCVarRef(value), copy); - if (a) { - a->incRefCount(); + ArrayData* a2 = a->append(tvCellAsCVarRef(value), copy); + if (a2 != a) { + a2->incRefCount(); base->m_data.parr->decRefCount(); - base->m_data.parr = a; + base->m_data.parr = a2; } break; } @@ -1196,26 +1197,27 @@ inline ArrayData* UnsetElemArrayRawKey(ArrayData* a, StringData* key, template inline void UnsetElemArray(TypedValue* base, TypedValue* key) { ArrayData* a = base->m_data.parr; + ArrayData* a2; bool copy = a->getCount() > 1; if (keyType == AnyKey) { if (IS_STRING_TYPE(key->m_type)) { - a = UnsetElemArrayRawKey(a, key->m_data.pstr, copy); + a2 = UnsetElemArrayRawKey(a, key->m_data.pstr, copy); } else if (key->m_type == KindOfInt64) { - a = UnsetElemArrayRawKey(a, key->m_data.num, copy); + a2 = UnsetElemArrayRawKey(a, key->m_data.num, copy); } else { VarNR varKey = tvAsCVarRef(key).toKey(); if (varKey.isNull()) { return; } - a = a->remove(varKey, copy); + a2 = a->remove(varKey, copy); } } else { - a = UnsetElemArrayRawKey(a, keyAsRaw(key), copy); + a2 = UnsetElemArrayRawKey(a, keyAsRaw(key), copy); } - if (a) { - a->incRefCount(); + if (a2 != a) { + a2->incRefCount(); base->m_data.parr->decRefCount(); - base->m_data.parr = a; + base->m_data.parr = a2; } } diff --git a/hphp/runtime/vm/name_value_table_wrapper.cpp b/hphp/runtime/vm/name_value_table_wrapper.cpp index d0289c56a..6791951a6 100644 --- a/hphp/runtime/vm/name_value_table_wrapper.cpp +++ b/hphp/runtime/vm/name_value_table_wrapper.cpp @@ -116,12 +116,12 @@ ArrayData* NameValueTableWrapper::lval(StringData* k, Variant*& ret, tv = m_tab->set(k, &nulVal); } ret = &tvAsVariant(tv); - return 0; + return this; } ArrayData* NameValueTableWrapper::lvalNew(Variant*& ret, bool copy) { ret = &Variant::lvalBlackHole(); - return 0; + return this; } ArrayData* NameValueTableWrapper::set(int64_t k, CVarRef v, bool copy) { @@ -131,7 +131,7 @@ ArrayData* NameValueTableWrapper::set(int64_t k, CVarRef v, bool copy) { ArrayData* NameValueTableWrapper::set(StringData* k, CVarRef v, bool copy) { tvAsVariant(m_tab->lookupAdd(k)).assignVal(v); - return 0; + return this; } ArrayData* NameValueTableWrapper::setRef(int64_t k, CVarRef v, bool copy) { @@ -140,7 +140,7 @@ ArrayData* NameValueTableWrapper::setRef(int64_t k, CVarRef v, bool copy) { ArrayData* NameValueTableWrapper::setRef(StringData* k, CVarRef v, bool copy) { tvAsVariant(m_tab->lookupAdd(k)).assignRef(v); - return 0; + return this; } ArrayData* NameValueTableWrapper::remove(int64_t k, bool copy) { @@ -149,7 +149,7 @@ ArrayData* NameValueTableWrapper::remove(int64_t k, bool copy) { ArrayData* NameValueTableWrapper::remove(const StringData* k, bool copy) { m_tab->unset(k); - return 0; + return this; } /*