Skip to content

Commit efffcbc

Browse files
sosukesuzukisosukesuzuki
committed
[JSC] Optimize Array#toReversed for an array contains hole
https://bugs.webkit.org/show_bug.cgi?id=294097 Reviewed by Yusuke Suzuki. Previously, the fast path for `toReversed` would fall back to the slow path whenever `this` was a holey array. This patch adopts the same strategy as `tryCloneArrayFromFast`, allowing the fast path to be retained whenever possible even when `this` contains holes. TipOfTree Patched array-prototype-toReversed-hole-string 30.8089+-0.6040 ^ 7.3049+-0.2939 ^ definitely 4.2175x faster array-prototype-toReversed-hole-int32 32.4921+-0.6975 ^ 7.6792+-0.3627 ^ definitely 4.2312x faster array-prototype-toReversed-hole-object 30.7505+-0.4679 ^ 7.3179+-0.3822 ^ definitely 4.2021x faster Canonical link: https://commits.webkit.org/296030@main
1 parent 7181420 commit efffcbc

7 files changed

Lines changed: 134 additions & 21 deletions

‎JSTests/microbenchmarks/array-prototype-toReversed-hole-int32.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
const arr = [];
2+
for (let i = 0; i < 1024; i++) {
3+
arr.push(i);
4+
}
5+
delete arr[512];
6+
7+
function test() {
8+
arr.toReversed();
9+
}
10+
noInline(test);
11+
12+
for (let i = 0; i < 1e4; i++) {
13+
test();
14+
}

‎JSTests/microbenchmarks/array-prototype-toReversed-hole-object.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
const arr = [];
2+
for (let i = 0; i < 1024; i++) {
3+
arr.push({ i });
4+
}
5+
delete arr[512];
6+
7+
function test() {
8+
arr.toReversed();
9+
}
10+
noInline(test);
11+
12+
for (let i = 0; i < 1e4; i++) {
13+
test();
14+
}

‎JSTests/microbenchmarks/array-prototype-toReversed-hole-string.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
const characters = "abcdefghijklmnopqrstuvwxyz";
2+
const arr = [];
3+
for (let i = 0; i < 1024; i++) {
4+
arr.push(characters[i % 26]);
5+
}
6+
delete arr[512];
7+
8+
function test() {
9+
arr.toReversed();
10+
}
11+
noInline(test);
12+
13+
for (let i = 0; i < 1e4; i++) {
14+
test();
15+
}

‎JSTests/stress/array-prototype-toReversed-hole-contiguous.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
function sameArray(a, b) {
2+
if (a.length !== b.length)
3+
throw new Error(`Expected array length ${b.length} but got ${a.length}`);
4+
for (let i = 0; i < a.length; i++) {
5+
if (a[i] !== b[i])
6+
throw new Error(`Expected ${b[i]} but got ${a[i]} (${i})`);
7+
}
8+
}
9+
const arr = [];
10+
for (let i = 0; i < 10; i++) {
11+
arr.push({ i });
12+
}
13+
delete arr[5];
14+
15+
const result = arr.toReversed();
16+
sameArray(result, [arr[9], arr[8], arr[7], arr[6], undefined, arr[4], arr[3], arr[2], arr[1], arr[0]]);

‎JSTests/stress/array-prototype-toReversed-hole-double.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
function sameArray(a, b) {
2+
if (a.length !== b.length)
3+
throw new Error(`Expected array length ${b.length} but got ${a.length}`);
4+
for (let i = 0; i < a.length; i++) {
5+
if (a[i] !== b[i])
6+
throw new Error(`Expected ${b[i]} but got ${a[i]} (${i})`);
7+
}
8+
}
9+
10+
11+
const arr = [];
12+
for (let i = 0; i < 10; i++) {
13+
arr.push(i + 0.1);
14+
}
15+
delete arr[5];
16+
const result = arr.toReversed();
17+
sameArray(result, [9.1, 8.1, 7.1, 6.1, undefined, 4.1, 3.1, 2.1, 1.1, 0.1]);

‎JSTests/stress/array-prototype-toReversed-hole-int32.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
function sameArray(a, b) {
2+
if (a.length !== b.length)
3+
throw new Error(`Expected array length ${b.length} but got ${a.length}`);
4+
for (let i = 0; i < a.length; i++) {
5+
if (a[i] !== b[i])
6+
throw new Error(`Expected ${b[i]} but got ${a[i]} (${i})`);
7+
}
8+
}
9+
10+
11+
const arr = [];
12+
for (let i = 0; i < 10; i++) {
13+
arr.push(i);
14+
}
15+
delete arr[5];
16+
const result = arr.toReversed();
17+
sameArray(result, [9, 8, 7, 6, undefined, 4, 3, 2, 1, 0]);

‎Source/JavaScriptCore/runtime/JSArray.cpp‎

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -534,26 +534,32 @@ JSArray* JSArray::fastToReversed(JSGlobalObject* globalObject, uint64_t length)
534534

535535
VM& vm = globalObject->vm();
536536

537-
auto type = indexingType();
538-
switch (type) {
537+
auto sourceType = indexingType();
538+
switch (sourceType) {
539539
case ArrayWithInt32:
540540
case ArrayWithContiguous:
541541
case ArrayWithDouble: {
542-
if (length > this->butterfly()->vectorLength())
542+
if (length > this->butterfly()->vectorLength()) [[unlikely]]
543543
return nullptr;
544-
Structure* resultStructure = globalObject->arrayStructureForIndexingTypeDuringAllocation(type);
545-
IndexingType indexingType = resultStructure->indexingType();
546-
if (hasAnyArrayStorage(indexingType)) [[unlikely]]
544+
if (holesMustForwardToPrototype()) [[unlikely]]
547545
return nullptr;
548-
ASSERT(!globalObject->isHavingABadTime());
549546

550-
auto srcData = this->butterfly()->contiguous().data();
547+
IndexingType resultType = sourceType;
548+
if (sourceType == ArrayWithDouble) {
549+
auto* buffer = this->butterfly()->contiguousDouble().data();
550+
if (containsHole(buffer, length)) [[unlikely]]
551+
resultType = ArrayWithContiguous;
552+
} else if (sourceType == ArrayWithInt32) {
553+
auto* buffer = this->butterfly()->contiguousInt32().data();
554+
if (containsHole(buffer, length)) [[unlikely]]
555+
resultType = ArrayWithContiguous;
556+
}
551557

552-
if (hasDouble(indexingType)) {
553-
if (containsHole(this->butterfly()->contiguousDouble().data(), static_cast<uint32_t>(length)))
554-
return nullptr;
555-
} else if (containsHole(srcData, static_cast<uint32_t>(length)))
558+
Structure* resultStructure = globalObject->arrayStructureForIndexingTypeDuringAllocation(resultType);
559+
IndexingType indexingType = resultStructure->indexingType();
560+
if (hasAnyArrayStorage(indexingType)) [[unlikely]]
556561
return nullptr;
562+
ASSERT(!globalObject->isHavingABadTime());
557563

558564
auto vectorLength = Butterfly::optimalContiguousVectorLength(resultStructure, length);
559565
void* memory = vm.auxiliarySpace().allocate(
@@ -566,16 +572,30 @@ JSArray* JSArray::fastToReversed(JSGlobalObject* globalObject, uint64_t length)
566572
butterfly->setVectorLength(vectorLength);
567573
butterfly->setPublicLength(length);
568574

569-
auto resultData = butterfly->contiguous().data();
570-
memcpy(resultData, srcData, sizeof(JSValue) * length);
571-
Butterfly::clearOptimalVectorLengthGap(type, butterfly, vectorLength, length);
572-
573575
if (hasDouble(indexingType)) {
574-
auto data = butterfly->contiguousDouble().data();
575-
std::reverse(data, data + length);
576-
} else
577-
std::reverse(resultData, resultData + length);
578-
576+
ASSERT(!containsHole(this->butterfly()->contiguousDouble().data(), length));
577+
auto* sourceBuffer = this->butterfly()->contiguousDouble().data();
578+
auto* resultBuffer = butterfly->contiguousDouble().data();
579+
copyArrayElements<ArrayFillMode::Empty, NeedsGCSafeOps::No>(resultBuffer, 0, sourceBuffer, 0, length, sourceType);
580+
std::reverse(resultBuffer, resultBuffer + length);
581+
} else if (hasInt32(indexingType)) {
582+
ASSERT(!containsHole(this->butterfly()->contiguous().data(), length));
583+
auto* sourceBuffer = this->butterfly()->contiguous().data();
584+
auto* resultBuffer = butterfly->contiguous().data();
585+
copyArrayElements<ArrayFillMode::Empty, NeedsGCSafeOps::No>(resultBuffer, 0, sourceBuffer, 0, length, sourceType);
586+
std::reverse(resultBuffer, resultBuffer + length);
587+
} else {
588+
auto* resultBuffer = butterfly->contiguous().data();
589+
if (sourceType == ArrayWithDouble) {
590+
auto* sourceBuffer = this->butterfly()->contiguousDouble().data();
591+
copyArrayElements<ArrayFillMode::Undefined, NeedsGCSafeOps::No>(resultBuffer, 0, sourceBuffer, 0, length, sourceType);
592+
} else {
593+
auto* sourceBuffer = this->butterfly()->contiguous().data();
594+
copyArrayElements<ArrayFillMode::Undefined, NeedsGCSafeOps::No>(resultBuffer, 0, sourceBuffer, 0, length, sourceType);
595+
}
596+
std::reverse(resultBuffer, resultBuffer + length);
597+
}
598+
Butterfly::clearOptimalVectorLengthGap(resultType, butterfly, vectorLength, length);
579599
return createWithButterfly(vm, nullptr, resultStructure, butterfly);
580600
}
581601
case ArrayWithArrayStorage: {

0 commit comments

Comments
 (0)