From 41a7492086c69371dd643960cef9619eb3b9379f Mon Sep 17 00:00:00 2001 From: Cameron Kaiser Date: Mon, 9 Jul 2018 03:38:14 -0700 Subject: [PATCH] closes #509: M1364346 parts 2+3 (unshift) --- .../tests/basic/shifted-elements4-unshift.js | 11 ++ .../jit-test/tests/basic/shifted-elements7.js | 47 +++++++ js/src/jsarray.cpp | 20 +-- js/src/jsobj.cpp | 2 +- js/src/vm/NativeObject-inl.h | 11 +- js/src/vm/NativeObject.cpp | 118 +++++++++++++++--- js/src/vm/NativeObject.h | 33 +++-- 7 files changed, 208 insertions(+), 34 deletions(-) create mode 100644 js/src/jit-test/tests/basic/shifted-elements4-unshift.js create mode 100644 js/src/jit-test/tests/basic/shifted-elements7.js diff --git a/js/src/jit-test/tests/basic/shifted-elements4-unshift.js b/js/src/jit-test/tests/basic/shifted-elements4-unshift.js new file mode 100644 index 000000000..60a520624 --- /dev/null +++ b/js/src/jit-test/tests/basic/shifted-elements4-unshift.js @@ -0,0 +1,11 @@ +function f() { + var arr = []; + for (var i = 0; i < 2; i++) { + for (var j = 0; j < 90000; j++) + arr.unshift(j); + for (var j = 0; j < 90000; j++) + assertEq(arr.pop(), j); + assertEq(arr.length, 0); + } +} +f(); diff --git a/js/src/jit-test/tests/basic/shifted-elements7.js b/js/src/jit-test/tests/basic/shifted-elements7.js new file mode 100644 index 000000000..3adcb61be --- /dev/null +++ b/js/src/jit-test/tests/basic/shifted-elements7.js @@ -0,0 +1,47 @@ +function test1() { + var a = []; + for (var i = 0; i < 100; i++) + a.unshift("foo" + i); + for (var i = 99; i >= 0; i--) { + assertEq(a.shift(), "foo" + i); + a.unshift("foo" + (i - 1)); + } + assertEq(a.length, 100); +} +test1(); + +function sum(arr) { + var res = 0; + for (var i = 0; i < arr.length; i++) + res += arr[i]; + return res; +} +function test2() { + var a = []; + for (var i = 0; i < 200; i++) + a.push(i); + for (var i = 0; i < 100; i++) + a.shift(); + for (var i = 0; i < 200; i++) + a.unshift(i); + assertEq(a.length, 300); + assertEq(sum(a), 34850); +} +test2(); + +function test3() { + var a = []; + for (var i = 0; i < 200; i++) + a.push(i); + var toAdd = []; + var step = 1; + for (var i = 0; i < 2500; i += step) { + for (var j = 0; j < step; j++) + toAdd.unshift(i + j); + a.unshift(...toAdd); + step = Math.max((i / 16)|0, 1); + } + assertEq(a.length, 41463); + assertEq(sum(a), 26657756); +} +test3(); diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp index f70e9456f..a488ad777 100644 --- a/js/src/jsarray.cpp +++ b/js/src/jsarray.cpp @@ -769,7 +769,7 @@ js::ArraySetLength(JSContext* cx, Handle arr, HandleId id, if (attrs & JSPROP_READONLY) { if (header->numShiftedElements() > 0) { - arr->unshiftElements(); + arr->moveShiftedElements(); header = arr->getElementsHeader(); } @@ -2255,7 +2255,7 @@ js::array_unshift(JSContext* cx, unsigned argc, Value* vp) if (args.length() > 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 + // Only include a fast path for boxed arrays. Unboxed arrays can't // be optimized here because unshifting temporarily places holes at // the start of the array. bool optimized = false; @@ -2267,14 +2267,16 @@ js::array_unshift(JSContext* cx, unsigned argc, Value* vp) ArrayObject* aobj = &obj->as(); if (!aobj->lengthIsWritable()) break; - DenseElementResult result = aobj->ensureDenseElements(cx, length, args.length()); - if (result != DenseElementResult::Success) { - if (result == DenseElementResult::Failure) - return false; - MOZ_ASSERT(result == DenseElementResult::Incomplete); - break; + if (MOZ_UNLIKELY(length > UINT32_MAX) || !aobj->tryUnshiftDenseElements(args.length())) { + DenseElementResult result = aobj->ensureDenseElements(cx, length, args.length()); + if (result != DenseElementResult::Success) { + if (result == DenseElementResult::Failure) + return false; + MOZ_ASSERT(result == DenseElementResult::Incomplete); + break; + } + aobj->moveDenseElements(args.length(), 0, length); } - aobj->moveDenseElements(args.length(), 0, length); for (uint32_t i = 0; i < args.length(); i++) aobj->setDenseElement(i, MagicValue(JS_ELEMENTS_HOLE)); optimized = true; diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp index 5cee1ccdc..a70114711 100644 --- a/js/src/jsobj.cpp +++ b/js/src/jsobj.cpp @@ -537,7 +537,7 @@ js::SetIntegrityLevel(JSContext* cx, HandleObject obj, IntegrityLevel level) if (!obj->as().maybeCopyElementsForWrite(cx)) return false; if (nobj->getElementsHeader()->numShiftedElements() > 0) - nobj->unshiftElements(); + nobj->moveShiftedElements(); obj->as().getElementsHeader()->setNonwritableArrayLength(); } } else { diff --git a/js/src/vm/NativeObject-inl.h b/js/src/vm/NativeObject-inl.h index ef26da6d4..2043e64ec 100644 --- a/js/src/vm/NativeObject-inl.h +++ b/js/src/vm/NativeObject-inl.h @@ -300,11 +300,19 @@ NativeObject::tryShiftDenseElements(uint32_t count) return false; } + shiftDenseElementsUnchecked(count); + return true; +} + +inline void +NativeObject::shiftDenseElementsUnchecked(uint32_t count) +{ + ObjectElements* header = getElementsHeader(); MOZ_ASSERT(count > 0); MOZ_ASSERT(count < header->initializedLength); if (MOZ_UNLIKELY(header->numShiftedElements() + count > ObjectElements::MaxShiftedElements)) { - unshiftElements(); + moveShiftedElements(); header = getElementsHeader(); } @@ -314,7 +322,6 @@ NativeObject::tryShiftDenseElements(uint32_t count) elements_ += count; ObjectElements* newHeader = getElementsHeader(); memmove(newHeader, header, sizeof(ObjectElements)); - return true; } /* Make an object with pregenerated shape from a NEWOBJECT bytecode. */ diff --git a/js/src/vm/NativeObject.cpp b/js/src/vm/NativeObject.cpp index d814fae5b..eb4c967e1 100644 --- a/js/src/vm/NativeObject.cpp +++ b/js/src/vm/NativeObject.cpp @@ -760,7 +760,7 @@ NativeObject::goodElementsAllocationAmount(ExclusiveContext* cx, uint32_t reqCap } void -NativeObject::unshiftElements() +NativeObject::moveShiftedElements() { ObjectElements* header = getElementsHeader(); uint32_t numShifted = header->numShiftedElements(); @@ -776,7 +776,7 @@ NativeObject::unshiftElements() elements_ = newHeader->elements(); // To move the elements, temporarily update initializedLength to include - // both shifted and unshifted elements. + // the shifted elements. newHeader->initializedLength += numShifted; // Move the elements. Initialize to |undefined| to ensure pre-barriers @@ -792,14 +792,95 @@ NativeObject::unshiftElements() } void -NativeObject::maybeUnshiftElements() +NativeObject::maybeMoveShiftedElements() { ObjectElements* header = getElementsHeader(); MOZ_ASSERT(header->numShiftedElements() > 0); - // Unshift if less than a third of the allocated space is in use. + // Move the elements if less than a third of the allocated space is in use. if (header->capacity < header->numAllocatedElements() / 3) - unshiftElements(); + moveShiftedElements(); +} + +bool +NativeObject::tryUnshiftDenseElements(uint32_t count) +{ + MOZ_ASSERT(count > 0); + + ObjectElements* header = getElementsHeader(); + uint32_t numShifted = header->numShiftedElements(); + + if (count > numShifted) { + // We need more elements than are easily available. Try to make space + // for more elements than we need (and shift the remaining ones) so + // that unshifting more elements later will be fast. + + // Don't bother reserving elements if the number of elements is small. + // Note that there's no technical reason for using this particular + // limit. + if (header->initializedLength <= 10 || + header->isCopyOnWrite() || + // header->isFrozen() || // NYI + header->hasNonwritableArrayLength() || + MOZ_UNLIKELY(count > ObjectElements::MaxShiftedElements)) + { + return false; + } + + MOZ_ASSERT(header->capacity >= header->initializedLength); + uint32_t unusedCapacity = header->capacity - header->initializedLength; + + // Determine toShift, the number of extra elements we want to make + // available. + uint32_t toShift = count - numShifted; + MOZ_ASSERT(toShift <= ObjectElements::MaxShiftedElements, + "count <= MaxShiftedElements so toShift <= MaxShiftedElements"); + + // Give up if we need to allocate more elements. + if (toShift > unusedCapacity) + return false; + + // Move more elements than we need, so that other unshift calls will be + // fast. We just have to make sure we don't exceed unusedCapacity. + toShift = Min(toShift + unusedCapacity / 2, unusedCapacity); + + // Ensure |numShifted + toShift| does not exceed MaxShiftedElements. + if (numShifted + toShift > ObjectElements::MaxShiftedElements) + toShift = ObjectElements::MaxShiftedElements - numShifted; + + MOZ_ASSERT(count <= numShifted + toShift); + MOZ_ASSERT(numShifted + toShift <= ObjectElements::MaxShiftedElements); + MOZ_ASSERT(toShift <= unusedCapacity); + + // Now move/unshift the elements. + uint32_t initLen = header->initializedLength; + setDenseInitializedLength(initLen + toShift); + for (uint32_t i = 0; i < toShift; i++) + initDenseElement(initLen + i, UndefinedValue()); + moveDenseElements(toShift, 0, initLen); + + // Shift the elements we just prepended. + shiftDenseElementsUnchecked(toShift); + + // We can now fall-through to the fast path below. + header = getElementsHeader(); + MOZ_ASSERT(header->numShiftedElements() == numShifted + toShift); + + numShifted = header->numShiftedElements(); + MOZ_ASSERT(count <= numShifted); + } + + elements_ -= count; + ObjectElements* newHeader = getElementsHeader(); + memmove(newHeader, header, sizeof(ObjectElements)); + + newHeader->unshiftShiftedElements(count); + + // Initialize to |undefined| to ensure pre-barriers don't see garbage. + for (uint32_t i = 0; i < count; i++) + initDenseElement(i, UndefinedValue()); + + return true; } bool @@ -810,22 +891,31 @@ NativeObject::growElements(ExclusiveContext* cx, uint32_t reqCapacity) if (denseElementsAreCopyOnWrite()) MOZ_CRASH(); - // If there are shifted elements, consider unshifting them first. If we - // don't unshift here, the code below will include the shifted elements in - // the resize. + // If there are shifted elements, consider moving them first. If we don't + // move them here, the code below will include the shifted elements in the + // resize. uint32_t numShifted = getElementsHeader()->numShiftedElements(); if (numShifted > 0) { - maybeUnshiftElements(); + // If the number of elements is small, it's cheaper to just move them as + // it may avoid a malloc/realloc. Note that there's no technical reason + // for using this particular value, but it works well in real-world use + // cases. + static const size_t MaxElementsToMoveEagerly = 20; + + if (getElementsHeader()->initializedLength <= MaxElementsToMoveEagerly) + moveShiftedElements(); + else + maybeMoveShiftedElements(); if (getDenseCapacity() >= reqCapacity) return true; numShifted = getElementsHeader()->numShiftedElements(); - // Ensure |reqCapacity + numShifted| below won't overflow by forcing an - // unshift in that case. + // If |reqCapacity + numShifted| overflows, we just move all shifted + // elements to avoid the problem. CheckedInt checkedReqCapacity(reqCapacity); checkedReqCapacity += numShifted; if (MOZ_UNLIKELY(!checkedReqCapacity.isValid())) { - unshiftElements(); + moveShiftedElements(); numShifted = 0; } } @@ -895,10 +985,10 @@ NativeObject::shrinkElements(ExclusiveContext* cx, uint32_t reqCapacity) if (!hasDynamicElements()) return; - // If we have shifted elements, consider unshifting them. + // If we have shifted elements, consider moving them. uint32_t numShifted = getElementsHeader()->numShiftedElements(); if (numShifted > 0) { - maybeUnshiftElements(); + maybeMoveShiftedElements(); numShifted = getElementsHeader()->numShiftedElements(); } diff --git a/js/src/vm/NativeObject.h b/js/src/vm/NativeObject.h index dfeb09df0..feb5c10e5 100644 --- a/js/src/vm/NativeObject.h +++ b/js/src/vm/NativeObject.h @@ -169,7 +169,7 @@ ArraySetLength(JSContext* cx, Handle obj, HandleId id, * to the next element and moving the ObjectElements header in memory (so it's * stored where the shifted Value used to be). * - * Shifted elements can be unshifted when we grow the array, when the array is + * Shifted elements can be moved when we grow the array, when the array is * frozen (for simplicity, shifted elements are not supported on objects that * are frozen, have copy-on-write elements, or on arrays with non-writable * length). @@ -205,12 +205,13 @@ class ObjectElements }; // The flags word stores both the flags and the number of shifted elements. - // Allow shifting 2047 elements before unshifting. - static const size_t NumShiftedElementsBits = 11; + // Allow shifting 1023 elements before actually moving the elements. + // (Reduced from 2047 due to memory concerns on our 32-bit Power Macs.) + static const size_t NumShiftedElementsBits = 10; static const size_t MaxShiftedElements = (1 << NumShiftedElementsBits) - 1; static const size_t NumShiftedElementsShift = 32 - NumShiftedElementsBits; static const size_t FlagsMask = (1 << NumShiftedElementsShift) - 1; - static_assert(MaxShiftedElements == 2047, + static_assert(MaxShiftedElements == 1023, "MaxShiftedElements should match the comment"); private: @@ -280,6 +281,16 @@ class ObjectElements capacity -= count; initializedLength -= count; } + void unshiftShiftedElements(uint32_t count) { + MOZ_ASSERT(count > 0); + MOZ_ASSERT(!(flags & (NONWRITABLE_ARRAY_LENGTH | FROZEN | COPY_ON_WRITE))); + uint32_t numShifted = numShiftedElements(); + MOZ_ASSERT(count <= numShifted); + numShifted -= count; + flags = (numShifted << NumShiftedElementsShift) | (flags & FlagsMask); + capacity += count; + initializedLength += count; + } void clearShiftedElements() { flags &= FlagsMask; MOZ_ASSERT(numShiftedElements() == 0); @@ -917,6 +928,8 @@ class NativeObject : public JSObject elements_[i].HeapSlot::~HeapSlot(); } + inline void shiftDenseElementsUnchecked(uint32_t count); + static bool rollbackProperties(ExclusiveContext* cx, HandleNativeObject obj, uint32_t slotSpan); @@ -1032,11 +1045,15 @@ class NativeObject : public JSObject // Try to shift |count| dense elements, see the "Shifted elements" comment. inline bool tryShiftDenseElements(uint32_t count); - // Unshift all shifted elements so that numShiftedElements is 0. - void unshiftElements(); + // Try to make space for |count| dense elements at the start of the array. + bool tryUnshiftDenseElements(uint32_t count); - // If this object has many shifted elements, unshift them. - void maybeUnshiftElements(); + // Move the elements header and all shifted elements to the start of the + // allocated elements space, so that numShiftedElements is 0 afterwards. + void moveShiftedElements(); + + // If this object has many shifted elements call moveShiftedElements. + void maybeMoveShiftedElements(); static bool goodElementsAllocationAmount(ExclusiveContext* cx, uint32_t reqAllocated, uint32_t length, uint32_t* goodAmount);