Skip to content
Permalink
Browse files

JSArray::shiftCountWithArrayStorage is wrong when an array has holes

https://bugs.webkit.org/show_bug.cgi?id=190262
<rdar://problem/44986241>

Reviewed by Mark Lam.

JSTests:

* stress/array-prototype-concat-of-long-spliced-arrays.js:
(test):
* stress/slice-array-storage-with-holes.js: Added.
(main):

Source/JavaScriptCore:

We would take the fast path for shiftCountWithArrayStorage when the array
hasHoles(). However, the code for this was wrong. It'd incorrectly update
ArrayStorage::m_numValuesInVector. Since the hasHoles() for ArrayStorage
path is never taken in JetStream 2, this patch just removes that from
the fast path. Instead, we just fallback to the slow path when hasHoles().
If we find evidence that this matters for real use cases, we can
figure out a way to make the fast path work.

* runtime/JSArray.cpp:
(JSC::JSArray::shiftCountWithArrayStorage):



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237129 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information...
sbarati@apple.com
sbarati@apple.com committed Oct 15, 2018
1 parent 21687be commit 51a62eb53815863a1bd2dd946d12f383e8695db0
@@ -1,3 +1,16 @@
2018-10-15 Saam Barati <sbarati@apple.com>

JSArray::shiftCountWithArrayStorage is wrong when an array has holes
https://bugs.webkit.org/show_bug.cgi?id=190262
<rdar://problem/44986241>

Reviewed by Mark Lam.

* stress/array-prototype-concat-of-long-spliced-arrays.js:
(test):
* stress/slice-array-storage-with-holes.js: Added.
(main):

2018-10-15 Commit Queue <commit-queue@webkit.org>

Unreviewed, rolling out r237054.
@@ -8,7 +8,7 @@ function test() {
var exception;
try {
var a = [];
a.length = 0xffffff00;
a.length = 0x1fff00;

var b = a.splice(0, 0x100000); // Undecided array

@@ -0,0 +1,11 @@
function main() {
let arr = [1];

arr.length = 0x100000;
arr.splice(0, 0x11);

arr.length = 0xfffffff0;
arr.splice(0xfffffff0, 0, 1);
}

main();
@@ -1,3 +1,22 @@
2018-10-15 Saam Barati <sbarati@apple.com>

JSArray::shiftCountWithArrayStorage is wrong when an array has holes
https://bugs.webkit.org/show_bug.cgi?id=190262
<rdar://problem/44986241>

Reviewed by Mark Lam.

We would take the fast path for shiftCountWithArrayStorage when the array
hasHoles(). However, the code for this was wrong. It'd incorrectly update
ArrayStorage::m_numValuesInVector. Since the hasHoles() for ArrayStorage
path is never taken in JetStream 2, this patch just removes that from
the fast path. Instead, we just fallback to the slow path when hasHoles().
If we find evidence that this matters for real use cases, we can
figure out a way to make the fast path work.

* runtime/JSArray.cpp:
(JSC::JSArray::shiftCountWithArrayStorage):

2018-10-15 Commit Queue <commit-queue@webkit.org>

Unreviewed, rolling out r237054.
@@ -803,7 +803,7 @@ bool JSArray::shiftCountWithArrayStorage(VM& vm, unsigned startIndex, unsigned c

// If the array contains holes or is otherwise in an abnormal state,
// use the generic algorithm in ArrayPrototype.
if ((storage->hasHoles() && this->structure(vm)->holesMustForwardToPrototype(vm, this))
if (storage->hasHoles()
|| hasSparseMap()
|| shouldUseSlowPut(indexingType())) {
return false;
@@ -844,22 +844,9 @@ bool JSArray::shiftCountWithArrayStorage(VM& vm, unsigned startIndex, unsigned c
// after the shift region, so we move the elements before to the right.
if (numElementsBeforeShiftRegion) {
RELEASE_ASSERT(count + startIndex <= vectorLength);
if (storage->hasHoles()) {
for (unsigned i = startIndex; i-- > 0;) {
unsigned destinationIndex = count + i;
JSValue source = storage->m_vector[i].get();
JSValue dest = storage->m_vector[destinationIndex].get();
// Any time we overwrite a hole we know we overcounted the number of values we removed
// when we subtracted count from m_numValuesInVector above.
if (!dest && destinationIndex >= firstIndexAfterShiftRegion)
storage->m_numValuesInVector++;
storage->m_vector[count + i].setWithoutWriteBarrier(source);
}
} else {
memmove(storage->m_vector + count,
storage->m_vector,
sizeof(JSValue) * startIndex);
}
memmove(storage->m_vector + count,
storage->m_vector,
sizeof(JSValue) * startIndex);
}
// Adjust the Butterfly and the index bias. We only need to do this here because we're changing
// the start of the Butterfly, which needs to point at the first indexed property in the used
@@ -875,22 +862,10 @@ bool JSArray::shiftCountWithArrayStorage(VM& vm, unsigned startIndex, unsigned c
} else {
// The number of elements before the shift region is greater than or equal to the number
// of elements after the shift region, so we move the elements after the shift region to the left.
if (storage->hasHoles()) {
for (unsigned i = 0; i < numElementsAfterShiftRegion; ++i) {
unsigned destinationIndex = startIndex + i;
JSValue source = storage->m_vector[firstIndexAfterShiftRegion + i].get();
JSValue dest = storage->m_vector[destinationIndex].get();
// Any time we overwrite a hole we know we overcounted the number of values we removed
// when we subtracted count from m_numValuesInVector above.
if (!dest && destinationIndex < firstIndexAfterShiftRegion)
storage->m_numValuesInVector++;
storage->m_vector[startIndex + i].setWithoutWriteBarrier(source);
}
} else {
memmove(storage->m_vector + startIndex,
storage->m_vector + firstIndexAfterShiftRegion,
sizeof(JSValue) * numElementsAfterShiftRegion);
}
memmove(storage->m_vector + startIndex,
storage->m_vector + firstIndexAfterShiftRegion,
sizeof(JSValue) * numElementsAfterShiftRegion);

// Clear the slots of the elements we just moved.
unsigned startOfEmptyVectorTail = usedVectorLength - count;
for (unsigned i = startOfEmptyVectorTail; i < usedVectorLength; ++i)

0 comments on commit 51a62eb

Please sign in to comment.
You can’t perform that action at this time.