From 460d29b318f3e992ed054c08c512d7fc64f99efa Mon Sep 17 00:00:00 2001 From: win7-7 Date: Wed, 7 Jan 2026 19:57:41 +0200 Subject: [PATCH] 1344173 - Fix performance cliff involving OBJECT_FLAG_ITERATED and array natives also includes 1323782. 1344173 - Fix performance cliff involving OBJECT_FLAG_ITERATED and array natives. 1323782: Check iterated-flag before proceeding to fast path in Array.prototype.unshift. --- js/src/jsarray.cpp | 103 +++++++++++++++++++--------------- js/src/jscompartment.h | 1 + js/src/jscompartmentinlines.h | 18 ++++++ js/src/jsiter.cpp | 11 +++- 4 files changed, 86 insertions(+), 47 deletions(-) diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp index 1b3f5385c3..7d2819f51f 100644 --- a/js/src/jsarray.cpp +++ b/js/src/jsarray.cpp @@ -526,6 +526,36 @@ struct ReverseIndexComparator } }; +static bool +MaybeInIteration(HandleObject obj, JSContext* cx) +{ + /* + * Don't optimize if the array might be in the midst of iteration. We + * rely on this to be able to safely move dense array elements around with + * just a memmove (see NativeObject::moveDenseArrayElements), without worrying + * about updating any in-progress enumerators for properties implicitly + * deleted if a hole is moved from one location to another location not yet + * visited. See bug 690622. + * + * Note that it's fine to optimize if |obj| is on the prototype of another + * object: SuppressDeletedProperty only suppresses properties deleted from + * the iterated object itself. + */ + + if (MOZ_LIKELY(!cx->compartment()->objectMaybeInIteration(obj))) + return false; + + ObjectGroup* group = JSObject::getGroup(cx, obj); + if (MOZ_UNLIKELY(!group)) { + cx->recoverFromOutOfMemory(); + return true; + } + + if (MOZ_UNLIKELY(group->hasAllFlags(OBJECT_FLAG_ITERATED))) + return true; + + return false; +} bool js::CanonicalizeArrayLengthValue(JSContext* cx, HandleValue v, uint32_t* newLen) { @@ -626,10 +656,7 @@ js::ArraySetLength(JSContext* cx, Handle arr, HandleId id, // for..in iteration over the array. Keys deleted before being reached // during the iteration must not be visited, and suppressing them here // would be too costly. - ObjectGroup* arrGroup = JSObject::getGroup(cx, arr); - if (MOZ_UNLIKELY(!arrGroup)) - return false; - if (!arr->isIndexed() && !MOZ_UNLIKELY(arrGroup->hasAllFlags(OBJECT_FLAG_ITERATED))) { + if (!arr->isIndexed() && !MaybeInIteration(arr, cx)) { if (!arr->maybeCopyElementsForWrite(cx)) return false; @@ -2221,11 +2248,7 @@ ArrayShiftDenseKernel(JSContext* cx, HandleObject obj, MutableHandleValue rval) if (ObjectMayHaveExtraIndexedProperties(obj)) return DenseElementResult::Incomplete; - RootedObjectGroup group(cx, JSObject::getGroup(cx, obj)); - if (MOZ_UNLIKELY(!group)) - return DenseElementResult::Failure; - - if (MOZ_UNLIKELY(group->hasAllFlags(OBJECT_FLAG_ITERATED))) + if (MaybeInIteration(obj, cx)) return DenseElementResult::Incomplete; size_t initlen = GetBoxedOrUnboxedInitializedLength(obj); @@ -2331,22 +2354,28 @@ js::array_unshift(JSContext* cx, unsigned argc, Value* vp) const unsigned argCount = args.length(); double newlen = length; - if (argCount > 0) { - /* Slide up the array to make room for all args at the bottom. */ - if (length > 0) { - // Only include a fast path for boxed arrays. Unboxed arrays can'nt - // be optimized here because unshifting temporarily places holes at - // the start of the array. - bool optimized = false; - do { - if (!obj->is()) - break; - if (ObjectMayHaveExtraIndexedProperties(obj)) - break; - ArrayObject* aobj = &obj->as(); - if (!aobj->lengthIsWritable()) - break; - DenseElementResult result = aobj->ensureDenseElements(cx, length, argCount); + if (args.length() > 0) { + // Only include a fast path for native objects. Unboxed arrays can't + // be optimized here because unshifting temporarily places holes at + // the start of the array. + // TODO: Implement unboxed array optimization similar to the one in + // array_splice_impl(), unshift() is a special version of splice(): + // arr.unshift(...values) ~= arr.splice(0, 0, ...values). + bool optimized = false; + do { + if (!obj->isNative()) + break; + if (ObjectMayHaveExtraIndexedProperties(obj)) + break; + if (MaybeInIteration(obj, cx)) + break; + NativeObject* nobj = &obj->as(); + if (nobj->denseElementsAreFrozen()) + break; + if (nobj->is() && !nobj->as().lengthIsWritable()) + break; + if (!nobj->tryUnshiftDenseElements(args.length())) { + DenseElementResult result = nobj->ensureDenseElements(cx, length, args.length()); if (result != DenseElementResult::Success) { if (result == DenseElementResult::Failure) return false; @@ -2402,6 +2431,7 @@ js::array_unshift(JSContext* cx, unsigned argc, Value* vp) * etc. along the prototype chain, or of enumerators requiring notification of * modifications. */ + static inline bool CanOptimizeForDenseStorage(HandleObject arr, uint32_t startingIndex, uint32_t count, JSContext* cx) { @@ -2417,27 +2447,8 @@ CanOptimizeForDenseStorage(HandleObject arr, uint32_t startingIndex, uint32_t co if (arr->is() && arr->as().denseElementsAreFrozen()) return false; - /* - * Don't optimize if the array might be in the midst of iteration. We - * rely on this to be able to safely move dense array elements around with - * just a memmove (see NativeObject::moveDenseArrayElements), without worrying - * about updating any in-progress enumerators for properties implicitly - * deleted if a hole is moved from one location to another location not yet - * visited. See bug 690622. - */ - ObjectGroup* arrGroup = JSObject::getGroup(cx, arr); - if (!arrGroup) { - cx->recoverFromOutOfMemory(); - return false; - } - if (MOZ_UNLIKELY(arrGroup->hasAllFlags(OBJECT_FLAG_ITERATED))) - return false; - - /* - * Another potential wrinkle: what if the enumeration is happening on an - * object which merely has |arr| on its prototype chain? - */ - if (arr->isDelegate()) + /* Also pick the slow path if the object is being iterated over. */ + if (MaybeInIteration(arr, cx)) return false; /* diff --git a/js/src/jscompartment.h b/js/src/jscompartment.h index a02b39301d..977069f1e7 100644 --- a/js/src/jscompartment.h +++ b/js/src/jscompartment.h @@ -852,6 +852,7 @@ struct JSCompartment compartmentStats_ = newStats; } + MOZ_ALWAYS_INLINE bool objectMaybeInIteration(JSObject* obj); // These flags help us to discover if a compartment that shouldn't be alive // manages to outlive a GC. bool scheduledForDestruction; diff --git a/js/src/jscompartmentinlines.h b/js/src/jscompartmentinlines.h index c092889e28..7d236c3ed3 100644 --- a/js/src/jscompartmentinlines.h +++ b/js/src/jscompartmentinlines.h @@ -7,6 +7,7 @@ #define jscompartmentinlines_h #include "jscompartment.h" +#include "jsiter.h" #include "gc/Barrier.h" @@ -130,4 +131,21 @@ JSCompartment::wrap(JSContext* cx, JS::MutableHandleValue vp) return true; } +MOZ_ALWAYS_INLINE bool +JSCompartment::objectMaybeInIteration(JSObject* obj) +{ + MOZ_ASSERT(obj->compartment() == this); + + // If the list is empty we're not iterating any objects. + js::NativeIterator* next = enumerators->next(); + if (enumerators == next) + return false; + + // If the list contains a single object, check if it's |obj|. + if (next->next() == enumerators) + return next->obj == obj; + + return true; +} + #endif /* jscompartmentinlines_h */ diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp index 740009d9fe..017b600f95 100644 --- a/js/src/jsiter.cpp +++ b/js/src/jsiter.cpp @@ -1500,6 +1500,9 @@ public: bool js::SuppressDeletedProperty(JSContext* cx, HandleObject obj, jsid id) { + if (MOZ_LIKELY(!cx->compartment()->objectMaybeInIteration(obj))) + return true; + if (JSID_IS_SYMBOL(id)) return true; @@ -1512,10 +1515,16 @@ js::SuppressDeletedProperty(JSContext* cx, HandleObject obj, jsid id) bool js::SuppressDeletedElement(JSContext* cx, HandleObject obj, uint32_t index) { + if (MOZ_LIKELY(!cx->compartment()->objectMaybeInIteration(obj))) + return true; RootedId id(cx); if (!IndexToId(cx, index, &id)) return false; - return SuppressDeletedProperty(cx, obj, id); + + Rooted str(cx, IdToString(cx, id)); + if (!str) + return false; + return SuppressDeletedPropertyHelper(cx, obj, SingleStringPredicate(str)); } bool