Title: [237129] trunk
- Revision
- 237129
- Author
- sbar...@apple.com
- Date
- 2018-10-15 12:14:42 -0700 (Mon, 15 Oct 2018)
Log Message
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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (237128 => 237129)
--- trunk/JSTests/ChangeLog 2018-10-15 18:42:44 UTC (rev 237128)
+++ trunk/JSTests/ChangeLog 2018-10-15 19:14:42 UTC (rev 237129)
@@ -1,3 +1,16 @@
+2018-10-15 Saam Barati <sbar...@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-qu...@webkit.org>
Unreviewed, rolling out r237054.
Modified: trunk/JSTests/stress/array-prototype-concat-of-long-spliced-arrays.js (237128 => 237129)
--- trunk/JSTests/stress/array-prototype-concat-of-long-spliced-arrays.js 2018-10-15 18:42:44 UTC (rev 237128)
+++ trunk/JSTests/stress/array-prototype-concat-of-long-spliced-arrays.js 2018-10-15 19:14:42 UTC (rev 237129)
@@ -8,7 +8,7 @@
var exception;
try {
var a = [];
- a.length = 0xffffff00;
+ a.length = 0x1fff00;
var b = a.splice(0, 0x100000); // Undecided array
Added: trunk/JSTests/stress/slice-array-storage-with-holes.js (0 => 237129)
--- trunk/JSTests/stress/slice-array-storage-with-holes.js (rev 0)
+++ trunk/JSTests/stress/slice-array-storage-with-holes.js 2018-10-15 19:14:42 UTC (rev 237129)
@@ -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();
Modified: trunk/Source/_javascript_Core/ChangeLog (237128 => 237129)
--- trunk/Source/_javascript_Core/ChangeLog 2018-10-15 18:42:44 UTC (rev 237128)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-10-15 19:14:42 UTC (rev 237129)
@@ -1,3 +1,22 @@
+2018-10-15 Saam Barati <sbar...@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-qu...@webkit.org>
Unreviewed, rolling out r237054.
Modified: trunk/Source/_javascript_Core/runtime/JSArray.cpp (237128 => 237129)
--- trunk/Source/_javascript_Core/runtime/JSArray.cpp 2018-10-15 18:42:44 UTC (rev 237128)
+++ trunk/Source/_javascript_Core/runtime/JSArray.cpp 2018-10-15 19:14:42 UTC (rev 237129)
@@ -803,7 +803,7 @@
// 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 @@
// 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 @@
} 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)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
