From 5ecb4407d5439a1a8a26fb7a646a1613379c7396 Mon Sep 17 00:00:00 2001 From: Shaunak Kishore Date: Tue, 17 Mar 2020 21:35:18 -0700 Subject: [PATCH] unrestrict-layout part 5: Eliminate set in place Summary: ...and here's the follow-up to delete SetInPlace. Again, the only significant user is ArrayInit, which always knows that the array is a mixed array. This diff is a bit more involved than the previous one because we were calling the generic setInPlace in ArrayInit in a bunch of places that I had to switch to the MixedArray one. But that's also a good sign: if this change has any perf effect, it should be a small win, because we're de-virtualizing those calls. Reviewed By: ricklavoie Differential Revision: D20476851 fbshipit-source-id: 61798dc1534c061c143ba3f61c3538b7d95a8398 --- hphp/runtime/base/apc-local-array.cpp | 4 +- hphp/runtime/base/apc-local-array.h | 2 - hphp/runtime/base/array-data-defs.h | 41 ------- hphp/runtime/base/array-data.cpp | 12 +- hphp/runtime/base/array-data.h | 15 --- hphp/runtime/base/array-init.h | 142 ++++++++-------------- hphp/runtime/base/empty-array.h | 2 - hphp/runtime/base/mixed-array.cpp | 2 + hphp/runtime/base/mixed-array.h | 19 ++- hphp/runtime/base/packed-array.cpp | 21 ---- hphp/runtime/base/packed-array.h | 5 - hphp/runtime/base/record-array.cpp | 9 -- hphp/runtime/base/record-array.h | 2 - hphp/runtime/base/set-array.h | 2 - hphp/runtime/ext/vsdebug/set_variable_command.cpp | 4 +- hphp/runtime/vm/globals-array.cpp | 6 +- hphp/runtime/vm/globals-array.h | 6 +- 17 files changed, 78 insertions(+), 216 deletions(-) diff --git a/hphp/runtime/base/apc-local-array.cpp b/hphp/runtime/base/apc-local-array.cpp index 23f6c30e9e5..2a8f58efa4a 100644 --- a/hphp/runtime/base/apc-local-array.cpp +++ b/hphp/runtime/base/apc-local-array.cpp @@ -190,7 +190,7 @@ arr_lval APCLocalArray::LvalForceNew(ArrayData* ad, bool /*copy*/) { ArrayData* APCLocalArray::SetInt(ArrayData* ad, int64_t k, TypedValue v) { EscalateHelper helper{ad}; - return helper.release(helper.escalated->setInPlace(k, v)); + return helper.release(helper.escalated->set(k, v)); } ArrayData* APCLocalArray::SetIntMove(ArrayData* ad, int64_t k, TypedValue v) { @@ -202,7 +202,7 @@ ArrayData* APCLocalArray::SetIntMove(ArrayData* ad, int64_t k, TypedValue v) { ArrayData* APCLocalArray::SetStr(ArrayData* ad, StringData* k, TypedValue v) { EscalateHelper helper{ad}; - return helper.release(helper.escalated->setInPlace(k, v)); + return helper.release(helper.escalated->set(k, v)); } ArrayData* APCLocalArray::SetStrMove(ArrayData* ad, StringData* k, TypedValue v) { diff --git a/hphp/runtime/base/apc-local-array.h b/hphp/runtime/base/apc-local-array.h index 97650e6e76a..e25361d05e3 100644 --- a/hphp/runtime/base/apc-local-array.h +++ b/hphp/runtime/base/apc-local-array.h @@ -55,10 +55,8 @@ struct APCLocalArray final : ArrayData, static arr_lval LvalForceNew(ArrayData*, bool copy); static ArrayData* SetInt(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetIntMove(ArrayData*, int64_t k, TypedValue v); - static constexpr auto SetIntInPlace = &SetInt; static ArrayData* SetStr(ArrayData*, StringData* k, TypedValue v); static ArrayData* SetStrMove(ArrayData*, StringData* k, TypedValue v); - static constexpr auto SetStrInPlace = &SetStr; static ArrayData *RemoveInt(ArrayData* ad, int64_t k); static ArrayData *RemoveStr(ArrayData* ad, const StringData* k); static ArrayData* Copy(const ArrayData*); diff --git a/hphp/runtime/base/array-data-defs.h b/hphp/runtime/base/array-data-defs.h index 985acffc68e..6fbc0ad4501 100644 --- a/hphp/runtime/base/array-data-defs.h +++ b/hphp/runtime/base/array-data-defs.h @@ -167,12 +167,6 @@ inline ArrayData* ArrayData::set(int64_t k, TypedValue v) { return g_array_funcs.setInt[kind()](this, k, v); } -inline ArrayData* ArrayData::setInPlace(int64_t k, TypedValue v) { - assertx(tvIsPlausible(v)); - assertx(notCyclic(v)); - return g_array_funcs.setIntInPlace[kind()](this, k, v); -} - inline ArrayData* ArrayData::setMove(int64_t k, TypedValue v) { assertx(tvIsPlausible(v)); assertx(cowCheck() || notCyclic(v)); @@ -185,12 +179,6 @@ inline ArrayData* ArrayData::set(StringData* k, TypedValue v) { return g_array_funcs.setStr[kind()](this, k, v); } -inline ArrayData* ArrayData::setInPlace(StringData* k, TypedValue v) { - assertx(tvIsPlausible(v)); - assertx(notCyclic(v)); - return g_array_funcs.setStrInPlace[kind()](this, k, v); -} - inline ArrayData* ArrayData::setMove(StringData* k, TypedValue v) { assertx(tvIsPlausible(v)); assertx(cowCheck() || notCyclic(v)); @@ -209,12 +197,6 @@ inline ArrayData* ArrayData::set(StringData* k, const Variant& v) { return g_array_funcs.setStr[kind()](this, k, c); } -inline ArrayData* ArrayData::setInPlace(StringData* k, const Variant& v) { - auto c = *v.asTypedValue(); - assertx(notCyclic(c)); - return g_array_funcs.setStrInPlace[kind()](this, k, c); -} - inline ArrayData* ArrayData::remove(int64_t k) { return g_array_funcs.removeInt[kind()](this, k); } @@ -394,15 +376,6 @@ inline ArrayData* ArrayData::set(TypedValue k, TypedValue v) { : set(detail::getStringKey(k), v); } -inline ArrayData* ArrayData::setInPlace(TypedValue k, TypedValue v) { - assertx(tvIsPlausible(k)); - assertx(tvIsPlausible(v)); - assertx(IsValidKey(k)); - - return detail::isIntKey(k) ? setInPlace(detail::getIntKey(k), v) - : setInPlace(detail::getStringKey(k), v); -} - inline ArrayData* ArrayData::remove(TypedValue k) { assertx(IsValidKey(k)); return detail::isIntKey(k) ? remove(detail::getIntKey(k)) @@ -453,28 +426,14 @@ inline ArrayData* ArrayData::set(const String& k, TypedValue v) { return set(k.get(), v); } -inline ArrayData* ArrayData::setInPlace(const String& k, TypedValue v) { - assertx(tvIsPlausible(v)); - assertx(IsValidKey(k)); - return setInPlace(k.get(), v); -} - inline ArrayData* ArrayData::set(const String& k, const Variant& v) { return set(k, *v.asTypedValue()); } -inline ArrayData* ArrayData::setInPlace(const String& k, const Variant& v) { - return setInPlace(k, *v.asTypedValue()); -} - inline ArrayData* ArrayData::set(const Variant& k, const Variant& v) { return set(*k.asTypedValue(), *v.asTypedValue()); } -inline ArrayData* ArrayData::setInPlace(const Variant& k, const Variant& v) { - return setInPlace(*k.asTypedValue(), *v.asTypedValue()); -} - inline ArrayData* ArrayData::remove(const String& k) { assertx(IsValidKey(k)); return remove(k.get()); diff --git a/hphp/runtime/base/array-data.cpp b/hphp/runtime/base/array-data.cpp index f0661be0eb1..833e6773825 100644 --- a/hphp/runtime/base/array-data.cpp +++ b/hphp/runtime/base/array-data.cpp @@ -378,22 +378,22 @@ const ArrayFunctions g_array_funcs = { /* * ArrayData* SetInt(ArrayData*, int64_t key, TypedValue v) * - * Set a value in the array for an integer key. SetInt() and SetIntMove() - * have copy/grow semantics; SetIntInPlace() may only escalate or grow. + * Set a value in the array for an integer key, with copies / escalation. + * SetIntMove is equivalent to SetInt, followed by a dec-ref of the value, + * followed by a dec-ref of the old array (if it was copied or escalated). */ DISPATCH(SetInt) DISPATCH(SetIntMove) - DISPATCH(SetIntInPlace) /* * ArrayData* SetStr(ArrayData*, StringData*, TypedValue v) * - * Set a value in the array for an string key. SetStr() and SetStrMove() - * have copy/grow semantics; SetStrInPlace() may only escalate or grow. + * Set a value in the array for a string key, with copies / escalation. + * SetStrMove is equivalent to SetStr, followed by a dec-ref of the value, + * followed by a dec-ref of the old array (if it was copied or escalated). */ DISPATCH(SetStr) DISPATCH(SetStrMove) - DISPATCH(SetStrInPlace) /* * size_t Vsize(const ArrayData*) diff --git a/hphp/runtime/base/array-data.h b/hphp/runtime/base/array-data.h index 9f49e2a1527..758ac808c15 100644 --- a/hphp/runtime/base/array-data.h +++ b/hphp/runtime/base/array-data.h @@ -478,10 +478,6 @@ public: * Set the element at key `k' to `v'. set() methods make a copy first if * cowCheck() returns true. If `v' is a ref, its inner value is used. * - * setInPlace() methods attempt to modify the array in place without - * calling cowCheck(), but may still return a new array for escalation - * or reallocating to grow. - * * Semantically, setMove() methods 1) do a set, 2) dec-ref the value, and * 3) if the operation required copy/escalation, dec-ref the old array. This * sequence is needed for member ops and can be implemented more efficiently @@ -494,10 +490,6 @@ public: ArrayData* set(StringData* k, TypedValue v); ArrayData* set(TypedValue k, TypedValue v); ArrayData* set(const String& k, TypedValue v); - ArrayData* setInPlace(int64_t k, TypedValue v); - ArrayData* setInPlace(StringData* k, TypedValue v); - ArrayData* setInPlace(TypedValue k, TypedValue v); - ArrayData* setInPlace(const String& k, TypedValue v); ArrayData* setMove(int64_t k, TypedValue v); ArrayData* setMove(StringData* k, TypedValue v); @@ -505,14 +497,9 @@ public: ArrayData* set(StringData* k, const Variant& v); ArrayData* set(const String& k, const Variant& v); ArrayData* set(const Variant& k, const Variant& v); - ArrayData* setInPlace(StringData* k, const Variant& v); - ArrayData* setInPlace(const String& k, const Variant& v); - ArrayData* setInPlace(const Variant& k, const Variant& v); ArrayData* set(const StringData*, TypedValue) = delete; ArrayData* set(const StringData*, const Variant&) = delete; - ArrayData* setInPlace(const StringData*, TypedValue) = delete; - ArrayData* setInPlace(const StringData*, const Variant&) = delete; /* * Remove the value at key `k', making a copy first if necessary. Returns @@ -898,10 +885,8 @@ struct ArrayFunctions { TypedValue (*nvGetKey[NK])(const ArrayData*, ssize_t pos); ArrayData* (*setInt[NK])(ArrayData*, int64_t k, TypedValue v); ArrayData* (*setIntMove[NK])(ArrayData*, int64_t k, TypedValue v); - ArrayData* (*setIntInPlace[NK])(ArrayData*, int64_t k, TypedValue v); ArrayData* (*setStr[NK])(ArrayData*, StringData* k, TypedValue v); ArrayData* (*setStrMove[NK])(ArrayData*, StringData* k, TypedValue v); - ArrayData* (*setStrInPlace[NK])(ArrayData*, StringData* k, TypedValue v); size_t (*vsize[NK])(const ArrayData*); tv_rval (*nvGetPos[NK])(const ArrayData*, ssize_t pos); bool (*isVectorData[NK])(const ArrayData*); diff --git a/hphp/runtime/base/array-init.h b/hphp/runtime/base/array-init.h index 776578ef2b3..b6beb066c01 100644 --- a/hphp/runtime/base/array-init.h +++ b/hphp/runtime/base/array-init.h @@ -37,6 +37,30 @@ namespace HPHP { /////////////////////////////////////////////////////////////////////////////// +// SetInPlace helpers that assume that the input array has a mixed layout. +// These helpers work for both mixed PHP arrays and dicts. +namespace arr_init { +inline ArrayData* SetInPlace(ArrayData* ad, int64_t k, TypedValue v) { + return MixedArray::SetIntInPlace(ad, k, tvToInit(v)); +} +inline ArrayData* SetInPlace(ArrayData* ad, StringData* k, TypedValue v) { + return MixedArray::SetStrInPlace(ad, k, tvToInit(v)); +} +inline ArrayData* SetInPlace(ArrayData* ad, const String& k, TypedValue v) { + return MixedArray::SetStrInPlace(ad, k.get(), tvToInit(v)); +} +inline ArrayData* SetInPlace(ArrayData* ad, TypedValue k, TypedValue v) { + if (isIntType(k.m_type)) { + return MixedArray::SetIntInPlace(ad, k.m_data.num, tvToInit(v)); + } else if (isStringType(k.m_type)) { + return MixedArray::SetStrInPlace(ad, k.m_data.pstr, tvToInit(v)); + } + return ad->set(k, v); +} +} + +/////////////////////////////////////////////////////////////////////////////// + /* * Flag indicating whether an array allocation should be pre-checked for OOM. */ @@ -227,24 +251,16 @@ struct MixedPHPArrayInitBase : ArrayInitBase { * Call set() on the underlying ArrayData. */ MixedPHPArrayInitBase& set(int64_t name, TypedValue tv) { - this->performOp([&]{ - return MixedArray::SetIntInPlace(this->m_arr, name, tvToInit(tv)); - }); + this->performOp([&]{ return arr_init::SetInPlace(this->m_arr, name, tv); }); return *this; } MixedPHPArrayInitBase& set(const String& name, TypedValue tv) { - this->performOp([&]{ - return MixedArray::SetStrInPlace( - this->m_arr, name.get(), tvToInit(tv) - ); - }); + this->performOp([&]{ return arr_init::SetInPlace(this->m_arr, name, tv); }); return *this; } template MixedPHPArrayInitBase& set(const T& name, TypedValue tv) { - this->performOp([&]{ - return this->m_arr->setInPlace(name, tvToInit(tv)); - }); + this->performOp([&]{ return arr_init::SetInPlace(this->m_arr, name, tv); }); return *this; } @@ -265,9 +281,7 @@ struct MixedPHPArrayInitBase : ArrayInitBase { * Same as set(), but for the deleted double `const Variant&' overload. */ MixedPHPArrayInitBase& setValidKey(TypedValue name, TypedValue v) { - this->performOp([&]{ - return this->m_arr->setInPlace(tvToInit(name), tvToInit(v)); - }); + set(tvToInit(name), v); return *this; } MixedPHPArrayInitBase& setValidKey(const Variant& name, const Variant& v) { @@ -285,11 +299,7 @@ struct MixedPHPArrayInitBase : ArrayInitBase { template MixedPHPArrayInitBase& setUnknownKey(const Variant& name, const Variant& v) { auto const k = name.toKey(this->m_arr).tv(); - if (LIKELY(!isNullType(k.m_type))) { - this->performOp([&]{ - return this->m_arr->setInPlace(k, v.asInitTVTmp()); - }); - } + if (LIKELY(!isNullType(k.m_type))) set(k, *v.asTypedValue()); return *this; } @@ -308,16 +318,10 @@ struct MixedPHPArrayInitBase : ArrayInitBase { bool keyConverted = false) { if (keyConverted) { this->performOp([&]{ - return MixedArray::AddStr( - this->m_arr, name.get(), tvToInit(tv), false - ); + return MixedArray::AddStr(this->m_arr, name.get(), tvToInit(tv), false); }); } else if (!name.isNull()) { - this->performOp([&]{ - return this->m_arr->setInPlace( - VarNR::MakeKey(name).tv(), tvToInit(tv) - ); - }); + set(VarNR::MakeKey(name).tv(), tv); } return *this; } @@ -325,16 +329,10 @@ struct MixedPHPArrayInitBase : ArrayInitBase { MixedPHPArrayInitBase& add(const Variant& name, TypedValue tv, bool keyConverted = false) { if (keyConverted) { - this->performOp([&]{ - return this->m_arr->setInPlace(name.asInitTVTmp(), tvToInit(tv)); - }); + set(name.asInitTVTmp(), tv); } else { auto const k = name.toKey(this->m_arr).tv(); - if (!isNullType(k.m_type)) { - this->performOp([&]{ - return this->m_arr->setInPlace(k, tvToInit(tv)); - }); - } + if (!isNullType(k.m_type)) set(k, tv); } return *this; } @@ -343,16 +341,10 @@ struct MixedPHPArrayInitBase : ArrayInitBase { MixedPHPArrayInitBase& add(const T& name, TypedValue tv, bool keyConverted = false) { if (keyConverted) { - this->performOp([&]{ - return this->m_arr->setInPlace(name, tvToInit(tv)); - }); + set(name, tv); } else { auto const k = Variant(name).toKey(this->m_arr).tv(); - if (!isNullType(k.m_type)) { - this->performOp([&]{ - return this->m_arr->setInPlace(k, tvToInit(tv)); - }); - } + if (!isNullType(k.m_type)) set(k, tv); } return *this; } @@ -393,9 +385,7 @@ struct DictInit : ArrayInitBase { ///////////////////////////////////////////////////////////////////////////// DictInit& append(TypedValue tv) { - performOp([&]{ - return MixedArray::AppendDict(m_arr, tvToInit(tv)); - }); + performOp([&]{ return MixedArray::AppendDict(m_arr, tvToInit(tv)); }); return *this; } DictInit& append(const Variant& v) { @@ -403,21 +393,15 @@ struct DictInit : ArrayInitBase { } DictInit& set(int64_t name, TypedValue tv) { - performOp([&]{ - return MixedArray::SetIntInPlaceDict(m_arr, name, tvToInit(tv)); - }); + performOp([&]{ return arr_init::SetInPlace(m_arr, name, tv); }); return *this; } DictInit& set(StringData* name, TypedValue tv) { - performOp([&]{ - return MixedArray::SetStrInPlaceDict(m_arr, name, tvToInit(tv)); - }); + performOp([&]{ return arr_init::SetInPlace(m_arr, name, tv); }); return *this; } DictInit& set(const String& name, TypedValue tv) { - performOp([&]{ - return MixedArray::SetStrInPlaceDict(m_arr, name.get(), tvToInit(tv)); - }); + performOp([&]{ return arr_init::SetInPlace(m_arr, name, tv); }); return *this; } @@ -439,11 +423,7 @@ struct DictInit : ArrayInitBase { DictInit& setValidKey(TypedValue name, TypedValue v) { performOp([&]{ assertx(isIntType(name.m_type) || isStringType(name.m_type)); - - return isIntType(name.m_type) - ? MixedArray::SetIntInPlaceDict(m_arr, name.m_data.num, tvToInit(v)) - : MixedArray::SetStrInPlaceDict(m_arr, name.m_data.pstr, - tvToInit(v)); + return arr_init::SetInPlace(m_arr, name, v); }); return *this; } @@ -653,20 +633,16 @@ struct DArrayInit { */ DArrayInit& add(int64_t name, TypedValue tv, bool /*keyConverted*/ = false) { - performOp([&]{ return m_arr->setInPlace(name, tvToInit(tv)); }); + set(name, tv); return *this; } DArrayInit& add(const String& name, TypedValue tv, bool keyConverted = false) { if (keyConverted) { - performOp([&]{ return m_arr->setInPlace(name, tvToInit(tv)); }); + set(name, tv); } else if (!name.isNull()) { - performOp( - [&]{ - return m_arr->setInPlace(VarNR::MakeKey(name).tv(), tvToInit(tv)); - } - ); + set(VarNR::MakeKey(name).tv(), tv); } return *this; } @@ -674,14 +650,10 @@ struct DArrayInit { DArrayInit& add(const Variant& name, TypedValue tv, bool keyConverted = false) { if (keyConverted) { - performOp( - [&]{ return m_arr->setInPlace(name.asInitTVTmp(), tvToInit(tv)); } - ); + set(name.asInitTVTmp(), tv); } else { auto const k = name.toKey(m_arr).tv(); - if (!isNullType(k.m_type)) { - performOp([&]{ return m_arr->setInPlace(k, tvToInit(tv)); }); - } + if (!isNullType(k.m_type)) set(k, tv); } return *this; } @@ -690,12 +662,10 @@ struct DArrayInit { DArrayInit& add(const T& name, TypedValue tv, bool keyConverted = false) { if (keyConverted) { - performOp([&]{ return m_arr->setInPlace(name, tvToInit(tv)); }); + set(name, tv); } else { auto const k = Variant(name).toKey(m_arr).tv(); - if (!isNullType(k.m_type)) { - performOp([&]{ return m_arr->setInPlace(k, tvToInit(tv)); }); - } + if (!isNullType(k.m_type)) set(k, tv); } return *this; } @@ -716,16 +686,16 @@ struct DArrayInit { * Call set() on the underlying ArrayData. */ DArrayInit& set(int64_t name, TypedValue tv) { - performOp([&]{ return m_arr->setInPlace(name, tvToInit(tv)); }); + performOp([&]{ return arr_init::SetInPlace(m_arr, name, tv); }); return *this; } DArrayInit& set(const String& name, TypedValue tv) { - performOp([&]{ return m_arr->setInPlace(name, tvToInit(tv)); }); + performOp([&]{ return arr_init::SetInPlace(m_arr, name, tv); }); return *this; } template DArrayInit& set(const T& name, TypedValue tv) { - performOp([&]{ return m_arr->setInPlace(name, tvToInit(tv)); }); + performOp([&]{ return arr_init::SetInPlace(m_arr, name, tv); }); return *this; } @@ -742,9 +712,7 @@ struct DArrayInit { #undef IMPL_SET DArrayInit& setValidKey(TypedValue name, TypedValue v) { - performOp( - [&]{ return m_arr->setInPlace(tvToInit(name), tvToInit(v)); } - ); + set(tvToInit(name), v); return *this; } DArrayInit& setValidKey(const Variant& name, const Variant& v) { @@ -754,9 +722,7 @@ struct DArrayInit { template DArrayInit& setUnknownKey(const Variant& name, const Variant& v) { auto const k = name.toKey(m_arr).tv(); - if (LIKELY(!isNullType(k.m_type))) { - performOp([&]{ return m_arr->setInPlace(k, v.asInitTVTmp()); }); - } + if (LIKELY(!isNullType(k.m_type))) set(k, v.asInitTVTmp()); return *this; } @@ -835,9 +801,7 @@ struct KeysetInit : ArrayInitBase { return *this; } KeysetInit& add(TypedValue tv) { - performOp([&]{ - return SetArray::Append(m_arr, tvToInit(tv)); - }); + performOp([&]{ return SetArray::Append(m_arr, tvToInit(tv)); }); return *this; } KeysetInit& add(const Variant& v) { diff --git a/hphp/runtime/base/empty-array.h b/hphp/runtime/base/empty-array.h index 9499b119234..394a5f4dbe6 100644 --- a/hphp/runtime/base/empty-array.h +++ b/hphp/runtime/base/empty-array.h @@ -70,10 +70,8 @@ struct EmptyArray final : type_scan::MarkCollectable { static TypedValue NvGetKey(const ArrayData*, ssize_t pos); static ArrayData* SetInt(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetIntMove(ArrayData*, int64_t k, TypedValue v); - static constexpr auto SetIntInPlace = &SetInt; static ArrayData* SetStr(ArrayData*, StringData* k, TypedValue v); static ArrayData* SetStrMove(ArrayData*, StringData* k, TypedValue v); - static constexpr auto SetStrInPlace = &SetStr; static ArrayData* RemoveInt(ArrayData* ad, int64_t); static ArrayData* RemoveStr(ArrayData* ad, const StringData*); static size_t Vsize(const ArrayData*); diff --git a/hphp/runtime/base/mixed-array.cpp b/hphp/runtime/base/mixed-array.cpp index 1f1dae453c3..6c7031e8bdb 100644 --- a/hphp/runtime/base/mixed-array.cpp +++ b/hphp/runtime/base/mixed-array.cpp @@ -1198,6 +1198,7 @@ ArrayData* MixedArray::SetIntMove(ArrayData* ad, int64_t k, TypedValue v) { } ArrayData* MixedArray::SetIntInPlace(ArrayData* ad, int64_t k, TypedValue v) { + assertx(!ad->cowCheck()); assertx(ad->notCyclic(v)); return asMixed(ad)->prepareForInsert(false/*copy*/)->update(k, v); } @@ -1216,6 +1217,7 @@ ArrayData* MixedArray::SetStrMove(ArrayData* ad, StringData* k, TypedValue v) { } ArrayData* MixedArray::SetStrInPlace(ArrayData* ad, StringData* k, TypedValue v) { + assertx(!ad->cowCheck()); assertx(ad->notCyclic(v)); return asMixed(ad)->prepareForInsert(false/*copy*/)->update(k, v); } diff --git a/hphp/runtime/base/mixed-array.h b/hphp/runtime/base/mixed-array.h index 8b3e0346745..8f8e405a9fe 100644 --- a/hphp/runtime/base/mixed-array.h +++ b/hphp/runtime/base/mixed-array.h @@ -354,15 +354,15 @@ public: static bool ExistsInt(const ArrayData*, int64_t k); static bool ExistsStr(const ArrayData*, const StringData* k); static arr_lval LvalInt(ArrayData* ad, int64_t k, bool copy); + static arr_lval LvalSilentInt(ArrayData*, int64_t, bool); static arr_lval LvalStr(ArrayData* ad, StringData* k, bool copy); + static arr_lval LvalSilentStr(ArrayData*, StringData*, bool); static arr_lval LvalForceNew(ArrayData*, bool copy); static arr_lval LvalForce(ArrayData* ad, const Variant& k, bool copy); static ArrayData* SetInt(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetIntMove(ArrayData*, int64_t k, TypedValue v); - static ArrayData* SetIntInPlace(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetStr(ArrayData*, StringData* k, TypedValue v); static ArrayData* SetStrMove(ArrayData*, StringData* k, TypedValue v); - static ArrayData* SetStrInPlace(ArrayData*, StringData* k, TypedValue v); static ArrayData* AddInt(ArrayData*, int64_t k, TypedValue v, bool copy); static ArrayData* AddStr(ArrayData*, StringData* k, TypedValue v, bool copy); static ArrayData* RemoveInt(ArrayData*, int64_t k); @@ -427,10 +427,8 @@ public: static constexpr auto NvGetKeyDict = &NvGetKey; static constexpr auto SetIntDict = &SetInt; static constexpr auto SetIntMoveDict = &SetIntMove; - static constexpr auto SetIntInPlaceDict = &SetIntInPlace; static constexpr auto SetStrDict = &SetStr; static constexpr auto SetStrMoveDict = &SetStrMove; - static constexpr auto SetStrInPlaceDict = &SetStrInPlace; static constexpr auto AddIntDict = &AddInt; static constexpr auto AddStrDict = &AddStr; static constexpr auto VsizeDict = &Vsize; @@ -439,7 +437,9 @@ public: static constexpr auto ExistsIntDict = &ExistsInt; static constexpr auto ExistsStrDict = &ExistsStr; static constexpr auto LvalIntDict = &LvalInt; + static constexpr auto LvalSilentIntDict = &LvalSilentInt; static constexpr auto LvalStrDict = &LvalStr; + static constexpr auto LvalSilentStrDict = &LvalSilentStr; static constexpr auto LvalForceNewDict = &LvalForceNew; static constexpr auto RemoveIntDict = &RemoveInt; static constexpr auto RemoveStrDict = &RemoveStr; @@ -475,13 +475,10 @@ public: ////////////////////////////////////////////////////////////////////// - // Like Lval[Int,Str], but silently does nothing if the element does not - // exist. Not part of the ArrayData interface, but used for member operations. - static arr_lval LvalSilentInt(ArrayData*, int64_t, bool); - static arr_lval LvalSilentStr(ArrayData*, StringData*, bool); - - static constexpr auto LvalSilentIntDict = &LvalSilentInt; - static constexpr auto LvalSilentStrDict = &LvalSilentStr; + // Helpers used by ArrayInit to update MixedArrays without refcount ops. + // These helpers work for both mixed PHP arrays and dicts. + static ArrayData* SetIntInPlace(ArrayData*, int64_t k, TypedValue v); + static ArrayData* SetStrInPlace(ArrayData*, StringData* k, TypedValue v); ////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/base/packed-array.cpp b/hphp/runtime/base/packed-array.cpp index 7ecb7b0d474..2167df3b9d8 100644 --- a/hphp/runtime/base/packed-array.cpp +++ b/hphp/runtime/base/packed-array.cpp @@ -852,14 +852,6 @@ ArrayData* PackedArray::SetIntMove(ArrayData* adIn, int64_t k, TypedValue v) { return result; } -ArrayData* PackedArray::SetIntInPlace(ArrayData* adIn, int64_t k, TypedValue v) { - return MutableOpInt(adIn, k, false, - [&] (ArrayData* ad) { setElem(LvalUncheckedInt(ad, k), v); return ad; }, - [&] { return AppendInPlace(adIn, v); }, - [&] (MixedArray* mixed) { return mixed->addVal(k, v); } - ); -} - ArrayData* PackedArray::SetIntVec(ArrayData* adIn, int64_t k, TypedValue v) { assertx(adIn->cowCheck() || adIn->notCyclic(v)); return MutableOpIntVec(adIn, k, adIn->cowCheck(), @@ -880,13 +872,6 @@ ArrayData* PackedArray::SetIntMoveVec(ArrayData* adIn, int64_t k, TypedValue v) ); } -ArrayData* PackedArray::SetIntInPlaceVec(ArrayData* adIn, int64_t k, TypedValue v) { - assertx(adIn->notCyclic(v)); - return MutableOpIntVec(adIn, k, false, - [&] (ArrayData* ad) { setElem(LvalUncheckedInt(ad, k), v); return ad; } - ); -} - ArrayData* PackedArray::SetStr(ArrayData* adIn, StringData* k, TypedValue v) { return MutableOpStr(adIn, k, adIn->cowCheck(), [&] (MixedArray* mixed) { return mixed->addVal(k, v); } @@ -901,12 +886,6 @@ ArrayData* PackedArray::SetStrMove(ArrayData* adIn, StringData* k, TypedValue v) return result; } -ArrayData* PackedArray::SetStrInPlace(ArrayData* adIn, StringData* k, TypedValue v) { - return MutableOpStr(adIn, k, false/*copy*/, - [&] (MixedArray* mixed) { return mixed->addVal(k, v); } - ); -} - ArrayData* PackedArray::SetStrVec(ArrayData* adIn, StringData* k, TypedValue v) { assertx(checkInvariants(adIn)); assertx(adIn->isVecArrayKind()); diff --git a/hphp/runtime/base/packed-array.h b/hphp/runtime/base/packed-array.h index 454e4a6ec61..3293a1a2ab1 100644 --- a/hphp/runtime/base/packed-array.h +++ b/hphp/runtime/base/packed-array.h @@ -85,10 +85,8 @@ struct PackedArray final : type_scan::MarkCollectable { static TypedValue NvGetKey(const ArrayData*, ssize_t pos); static ArrayData* SetInt(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetIntMove(ArrayData*, int64_t k, TypedValue v); - static ArrayData* SetIntInPlace(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetStr(ArrayData*, StringData* k, TypedValue v); static ArrayData* SetStrMove(ArrayData*, StringData* k, TypedValue v); - static ArrayData* SetStrInPlace(ArrayData*, StringData* k, TypedValue v); static size_t Vsize(const ArrayData*); static tv_rval RvalPos(const ArrayData* ad, ssize_t pos); static bool IsVectorData(const ArrayData*) { @@ -138,10 +136,8 @@ struct PackedArray final : type_scan::MarkCollectable { static tv_rval NvTryGetStrVec(const ArrayData*, const StringData*); static ArrayData* SetIntVec(ArrayData*, int64_t, TypedValue); static ArrayData* SetIntMoveVec(ArrayData*, int64_t, TypedValue); - static ArrayData* SetIntInPlaceVec(ArrayData*, int64_t, TypedValue); static ArrayData* SetStrVec(ArrayData*, StringData*, TypedValue); static constexpr auto SetStrMoveVec = &SetStrVec; - static constexpr auto SetStrInPlaceVec = &SetStrVec; static ArrayData* RemoveIntVec(ArrayData*, int64_t); static arr_lval LvalIntVec(ArrayData*, int64_t, bool); static arr_lval LvalStrVec(ArrayData*, StringData*, bool); @@ -166,7 +162,6 @@ struct PackedArray final : type_scan::MarkCollectable { static constexpr auto ExistsStrVec = &ExistsStr; static constexpr auto LvalForceNewVec = &LvalForceNew; static constexpr auto RemoveStrVec = &RemoveStr; - static constexpr auto RemoveStrInPlaceVec = &RemoveStr; static constexpr auto IterBeginVec = &IterBegin; static constexpr auto IterLastVec = &IterLast; static constexpr auto IterEndVec = &IterEnd; diff --git a/hphp/runtime/base/record-array.cpp b/hphp/runtime/base/record-array.cpp index 9db09ab6759..67d4d0f2cd2 100644 --- a/hphp/runtime/base/record-array.cpp +++ b/hphp/runtime/base/record-array.cpp @@ -165,15 +165,6 @@ tv_rval RecordArray::NvGetStr(const ArrayData* base, const StringData* key) { return MixedArray::NvGetStr(extra, key); } -ArrayData* RecordArray::SetStrInPlace(ArrayData* base, - StringData* key, TypedValue val) { - assertx(base->notCyclic(val)); - auto const ra = asRecordArray(base); - auto const idx = ra->checkFieldForWrite(key, val); - ra->updateField(key, val, idx); - return ra; -} - ArrayData* RecordArray::SetStr(ArrayData* base, StringData* key, TypedValue val) { assertx(base->cowCheck() || base->notCyclic(val)); auto ra = asRecordArray(base); diff --git a/hphp/runtime/base/record-array.h b/hphp/runtime/base/record-array.h index e5ef6e380d3..4239a1c39b8 100644 --- a/hphp/runtime/base/record-array.h +++ b/hphp/runtime/base/record-array.h @@ -61,10 +61,8 @@ struct RecordArray : ArrayData, static TypedValue NvGetKey(const ArrayData*, ssize_t pos); static ArrayData* SetInt(ArrayData*, int64_t key, TypedValue v); static ArrayData* SetIntMove(ArrayData*, int64_t key, TypedValue v); - static constexpr auto SetIntInPlace = &SetInt; static ArrayData* SetStr(ArrayData*, StringData*, TypedValue v); static ArrayData* SetStrMove(ArrayData*, StringData*, TypedValue v); - static ArrayData* SetStrInPlace(ArrayData*, StringData*, TypedValue v); static size_t Vsize(const ArrayData*); static tv_rval RvalPos(const ArrayData*, ssize_t pos); static bool IsVectorData(const ArrayData*); diff --git a/hphp/runtime/base/set-array.h b/hphp/runtime/base/set-array.h index 0720120b7f8..48596928cce 100644 --- a/hphp/runtime/base/set-array.h +++ b/hphp/runtime/base/set-array.h @@ -423,10 +423,8 @@ public: static arr_lval LvalForceNew(ArrayData*, bool); static ArrayData* SetInt(ArrayData*, int64_t, TypedValue); static constexpr auto SetIntMove = &SetInt; - static constexpr auto SetIntInPlace = &SetInt; static ArrayData* SetStr(ArrayData*, StringData*, TypedValue); static constexpr auto SetStrMove = &SetStr; - static constexpr auto SetStrInPlace = &SetStr; static ArrayData* RemoveInt(ArrayData*, int64_t); static ArrayData* RemoveStr(ArrayData*, const StringData*); static ArrayData* Copy(const ArrayData*); diff --git a/hphp/runtime/ext/vsdebug/set_variable_command.cpp b/hphp/runtime/ext/vsdebug/set_variable_command.cpp index 63d640be307..25278ff9786 100644 --- a/hphp/runtime/ext/vsdebug/set_variable_command.cpp +++ b/hphp/runtime/ext/vsdebug/set_variable_command.cpp @@ -263,10 +263,10 @@ bool SetVariableCommand::setArrayVariable( auto keyVariant = iter.first(); if (keyVariant.isString()) { HPHP::String key = keyVariant.toString(); - arr->setInPlace(key, tvToInit(*arrayValue)); + arr.set(key, tvToInit(*arrayValue)); } else if (keyVariant.isInteger()) { int64_t key = keyVariant.toInt64(); - arr->setInPlace(key, tvToInit(*arrayValue)); + arr.set(key, tvToInit(*arrayValue)); } else { throw DebuggerCommandException("Unsupported array key type."); } diff --git a/hphp/runtime/vm/globals-array.cpp b/hphp/runtime/vm/globals-array.cpp index 07f806cb348..332c94e51b3 100644 --- a/hphp/runtime/vm/globals-array.cpp +++ b/hphp/runtime/vm/globals-array.cpp @@ -182,8 +182,8 @@ ArrayData* GlobalsArray::SetIntMove(ArrayData* ad, int64_t k, TypedValue v) { return SetStrMove(ad, String(k).get(), v); } -ArrayData* GlobalsArray::SetIntInPlace(ArrayData* ad, int64_t k, TypedValue v) { - return SetStrInPlace(ad, String(k).get(), v); +ArrayData* GlobalsArray::SetInt(ArrayData* ad, int64_t k, TypedValue v) { + return SetStr(ad, String(k).get(), v); } ArrayData* GlobalsArray::SetStrMove(ArrayData* ad, StringData* k, TypedValue v) { @@ -191,7 +191,7 @@ ArrayData* GlobalsArray::SetStrMove(ArrayData* ad, StringData* k, TypedValue v) return ad; } -ArrayData* GlobalsArray::SetStrInPlace(ArrayData* ad, StringData* k, TypedValue v) { +ArrayData* GlobalsArray::SetStr(ArrayData* ad, StringData* k, TypedValue v) { tvSet(v, *asGlobals(ad)->m_tab->lookupAdd(k)); return ad; } diff --git a/hphp/runtime/vm/globals-array.h b/hphp/runtime/vm/globals-array.h index eec72365e52..af5908b7883 100644 --- a/hphp/runtime/vm/globals-array.h +++ b/hphp/runtime/vm/globals-array.h @@ -87,12 +87,10 @@ public: static arr_lval LvalSilentStr(ArrayData*, StringData* k, bool copy); static arr_lval LvalForceNew(ArrayData*, bool copy); + static ArrayData* SetInt(ArrayData*, int64_t k, TypedValue v); static ArrayData* SetIntMove(ArrayData*, int64_t k, TypedValue v); - static ArrayData* SetIntInPlace(ArrayData*, int64_t k, TypedValue v); - static constexpr auto SetInt = &SetIntInPlace; + static ArrayData* SetStr(ArrayData*, StringData* k, TypedValue v); static ArrayData* SetStrMove(ArrayData*, StringData* k, TypedValue v); - static ArrayData* SetStrInPlace(ArrayData*, StringData* k, TypedValue v); - static constexpr auto SetStr = &SetStrInPlace; static ArrayData* RemoveInt(ArrayData*, int64_t k); static ArrayData* RemoveStr(ArrayData*, const StringData* k); static ArrayData* Append(ArrayData*, TypedValue v); -- 2.11.4.GIT