Optimize Array.reverse for HOLEY arrays

Array.reverse was optimized only for PACKED_* arrays. This CL adds fast paths for HOLEY_* arrays as well.

Bug: chromium:1305342
Change-Id: I83c5ffa6e823478992c2caabd9a88d405b35e464
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4062673
Commit-Queue: Choongwoo Han <choongwoo.han@microsoft.com>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84580}

src/builtins/array-reverse.tq

@@ -3,65 +3,50 @@
 // found in the LICENSE file.
 
 namespace array {
-macro LoadElement<ElementsAccessor : type extends ElementsKind, T: type>(
+macro LoadElement<Elements : type extends FixedArrayBase, T: type>(
     elements: FixedArrayBase, index: Smi): T;
 
-LoadElement<array::FastPackedSmiElements, Smi>(implicit context: Context)(
+LoadElement<FixedArray, Object>(implicit context: Context)(
-    elements: FixedArrayBase, index: Smi): Smi {
+    elements: FixedArrayBase, index: Smi): Object {
   const elements: FixedArray = UnsafeCast<FixedArray>(elements);
-  return UnsafeCast<Smi>(elements.objects[index]);
+  return elements.objects[index];
 }
 
-LoadElement<array::FastPackedObjectElements, JSAny>(implicit context: Context)(
+LoadElement<FixedDoubleArray, float64_or_hole>(implicit context: Context)(
-    elements: FixedArrayBase, index: Smi): JSAny {
+    elements: FixedArrayBase, index: Smi): float64_or_hole {
-  const elements: FixedArray = UnsafeCast<FixedArray>(elements);
-  return UnsafeCast<JSAny>(elements.objects[index]);
-}
-
-LoadElement<array::FastPackedDoubleElements, float64>(
-    implicit context: Context)(elements: FixedArrayBase, index: Smi): float64 {
   const elements: FixedDoubleArray = UnsafeCast<FixedDoubleArray>(elements);
-  // This macro is only used for PACKED_DOUBLE, loading the hole should
+  return elements.floats[index];
-  // be impossible.
-  return elements.floats[index].Value() otherwise unreachable;
 }
 
-macro StoreElement<ElementsAccessor : type extends ElementsKind, T: type>(
+macro StoreElement<Elements : type extends FixedArrayBase, T: type>(
     implicit context: Context)(
     elements: FixedArrayBase, index: Smi, value: T): void;
 
-StoreElement<array::FastPackedSmiElements, Smi>(implicit context: Context)(
+StoreElement<FixedArray, Object>(implicit context: Context)(
-    elements: FixedArrayBase, index: Smi, value: Smi): void {
+    elements: FixedArrayBase, index: Smi, value: Object): void {
-  const elems: FixedArray = UnsafeCast<FixedArray>(elements);
-  StoreFixedArrayElement(elems, index, value);
-}
-
-StoreElement<array::FastPackedObjectElements, JSAny>(implicit context: Context)(
-    elements: FixedArrayBase, index: Smi, value: JSAny): void {
   const elements: FixedArray = UnsafeCast<FixedArray>(elements);
   elements.objects[index] = value;
 }
 
-StoreElement<array::FastPackedDoubleElements, float64>(
+StoreElement<FixedDoubleArray, float64_or_hole>(implicit context: Context)(
-    implicit context: Context)(
+    elements: FixedArrayBase, index: Smi, value: float64_or_hole): void {
-    elements: FixedArrayBase, index: Smi, value: float64): void {
   const elems: FixedDoubleArray = UnsafeCast<FixedDoubleArray>(elements);
-  StoreFixedDoubleArrayElement(elems, index, value);
+  elems.floats[index] = value;
 }
 
 // Fast-path for all PACKED_* elements kinds. These do not need to check
 // whether a property is present, so we can simply swap them using fast
 // FixedArray loads/stores.
-macro FastPackedArrayReverse<Accessor: type, T: type>(
+macro FastArrayReverse<Elements : type extends FixedArrayBase, T: type>(
     implicit context: Context)(elements: FixedArrayBase, length: Smi): void {
   let lower: Smi = 0;
   let upper: Smi = length - 1;
 
   while (lower < upper) {
-    const lowerValue: T = LoadElement<Accessor, T>(elements, lower);
+    const lowerValue: T = LoadElement<Elements, T>(elements, lower);
-    const upperValue: T = LoadElement<Accessor, T>(elements, upper);
+    const upperValue: T = LoadElement<Elements, T>(elements, upper);
-    StoreElement<Accessor>(elements, lower, upperValue);
+    StoreElement<Elements>(elements, lower, upperValue);
-    StoreElement<Accessor>(elements, upper, lowerValue);
+    StoreElement<Elements>(elements, upper, lowerValue);
     ++lower;
     --upper;
   }
@@ -144,19 +129,27 @@ macro TryFastPackedArrayReverse(implicit context: Context)(receiver: JSAny):
   const array: FastJSArray = Cast<FastJSArray>(receiver) otherwise Slow;
 
   const kind: ElementsKind = array.map.elements_kind;
-  if (kind == ElementsKind::PACKED_SMI_ELEMENTS) {
+  if (kind == ElementsKind::PACKED_SMI_ELEMENTS ||
+      kind == ElementsKind::PACKED_ELEMENTS) {
     array::EnsureWriteableFastElements(array);
-    FastPackedArrayReverse<array::FastPackedSmiElements, Smi>(
+    FastArrayReverse<FixedArray, Object>(array.elements, array.length);
-        array.elements, array.length);
-  } else if (kind == ElementsKind::PACKED_ELEMENTS) {
-    array::EnsureWriteableFastElements(array);
-    FastPackedArrayReverse<array::FastPackedObjectElements, JSAny>(
-        array.elements, array.length);
   } else if (kind == ElementsKind::PACKED_DOUBLE_ELEMENTS) {
-    FastPackedArrayReverse<array::FastPackedDoubleElements, float64>(
+    FastArrayReverse<FixedDoubleArray, float64_or_hole>(
         array.elements, array.length);
   } else {
-    goto Slow;
+    if (!IsPrototypeInitialArrayPrototype(array.map)) goto Slow;
+    if (IsNoElementsProtectorCellInvalid()) goto Slow;
+
+    if (kind == ElementsKind::HOLEY_SMI_ELEMENTS ||
+        kind == ElementsKind::HOLEY_ELEMENTS) {
+      array::EnsureWriteableFastElements(array);
+      FastArrayReverse<FixedArray, Object>(array.elements, array.length);
+    } else if (kind == ElementsKind::HOLEY_DOUBLE_ELEMENTS) {
+      FastArrayReverse<FixedDoubleArray, float64_or_hole>(
+          array.elements, array.length);
+    } else {
+      goto Slow;
+    }
   }
 }
 

src/builtins/array-to-reversed.tq

@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 namespace array {
-macro FastPackedArrayToReversed<Accessor: type, T: type>(
+macro FastArrayToReversed<Elements : type extends FixedArrayBase, T: type>(
     implicit context: Context)(
     kind: constexpr ElementsKind, elements: FixedArrayBase,
     length: Smi): JSArray {
@@ -21,10 +21,10 @@ macro FastPackedArrayToReversed<Accessor: type, T: type>(
     const from = length - k - 1;
 
     // c. Let fromValue be ? Get(O, from).
-    const fromValue: T = LoadElement<Accessor, T>(elements, from);
+    const fromValue: T = LoadElement<Elements, T>(elements, from);
 
     // d. Perform ! CreateDataPropertyOrThrow(A, Pk, fromValue).
-    StoreElement<Accessor>(copy, k, fromValue);
+    StoreElement<Elements>(copy, k, fromValue);
 
     // e. Set k to k + 1.
     ++k;
@@ -35,7 +35,7 @@ macro FastPackedArrayToReversed<Accessor: type, T: type>(
   return NewJSArray(map, copy);
 }
 
-macro TryFastPackedArrayToReversed(implicit context: Context)(receiver: JSAny):
+macro TryFastArrayToReversed(implicit context: Context)(receiver: JSAny):
     JSArray labels Slow {
   const array: FastJSArray = Cast<FastJSArray>(receiver) otherwise Slow;
 
@@ -43,19 +43,31 @@ macro TryFastPackedArrayToReversed(implicit context: Context)(receiver: JSAny):
 
   const kind: ElementsKind = array.map.elements_kind;
   if (kind == ElementsKind::PACKED_SMI_ELEMENTS) {
-    return FastPackedArrayToReversed<array::FastPackedSmiElements, Smi>(
+    return FastArrayToReversed<FixedArray, Object>(
         ElementsKind::PACKED_SMI_ELEMENTS, array.elements, array.length);
-  }
+  } else if (kind == ElementsKind::PACKED_ELEMENTS) {
-  if (kind == ElementsKind::PACKED_ELEMENTS) {
+    return FastArrayToReversed<FixedArray, Object>(
-    return FastPackedArrayToReversed<array::FastPackedObjectElements, JSAny>(
         ElementsKind::PACKED_ELEMENTS, array.elements, array.length);
-  }
+  } else if (kind == ElementsKind::PACKED_DOUBLE_ELEMENTS) {
-  if (kind == ElementsKind::PACKED_DOUBLE_ELEMENTS) {
+    return FastArrayToReversed<FixedDoubleArray, float64_or_hole>(
-    return FastPackedArrayToReversed<array::FastPackedDoubleElements, float64>(
         ElementsKind::PACKED_DOUBLE_ELEMENTS, array.elements, array.length);
-  }
+  } else {
+    if (!IsPrototypeInitialArrayPrototype(array.map)) goto Slow;
+    if (IsNoElementsProtectorCellInvalid()) goto Slow;
 
-  goto Slow;
+    if (kind == ElementsKind::HOLEY_SMI_ELEMENTS) {
+      return FastArrayToReversed<FixedArray, Object>(
+          ElementsKind::HOLEY_SMI_ELEMENTS, array.elements, array.length);
+    } else if (kind == ElementsKind::HOLEY_ELEMENTS) {
+      return FastArrayToReversed<FixedArray, Object>(
+          ElementsKind::HOLEY_ELEMENTS, array.elements, array.length);
+    } else if (kind == ElementsKind::HOLEY_DOUBLE_ELEMENTS) {
+      return FastArrayToReversed<FixedDoubleArray, float64_or_hole>(
+          ElementsKind::HOLEY_DOUBLE_ELEMENTS, array.elements, array.length);
+    }
+
+    goto Slow;
+  }
 }
 
 transitioning builtin GenericArrayToReversed(implicit context: Context)(
@@ -96,7 +108,7 @@ transitioning builtin GenericArrayToReversed(implicit context: Context)(
 transitioning javascript builtin ArrayPrototypeToReversed(
     js-implicit context: NativeContext, receiver: JSAny)(...arguments): JSAny {
   try {
-    return TryFastPackedArrayToReversed(receiver) otherwise Slow;
+    return TryFastArrayToReversed(receiver) otherwise Slow;
   } label Slow {
     return GenericArrayToReversed(receiver);
   }

test/mjsunit/array-reverse.js

@@ -10,6 +10,8 @@ assertArrayEquals(["str4", "str3", "str2"], ["str2", "str3", "str4"].reverse());
 assertArrayEquals([4,3,,1], [1,,3,4].reverse());
 assertArrayEquals([4,,2,1], [1,2,,4].reverse());
 assertArrayEquals([5,,3,,1], [1,,3,,5].reverse());
+assertArrayEquals([0.5,,0.3,,0.1], [0.1,,0.3,,0.5].reverse());
+assertArrayEquals(["5",,"3",,"1"], ["1",,"3",,"5"].reverse());
 
 function TestReverseWithObject() {
   let obj = { length: 5 };

test/mjsunit/harmony/array-to-reversed.js

@@ -31,6 +31,30 @@ assertEquals("toReversed", Array.prototype.toReversed.name);
   assertFalse(a === r);
 })();
 
+(function TestSmiHoley() {
+  let a = [1,,3,4];
+  let r = a.toReversed();
+  assertEquals([1,,3,4], a);
+  assertEquals([4,3,,1], r);
+  assertFalse(a === r);
+})();
+
+(function TestDoubleHoley() {
+  let a = [1.1,,3.3,4.4];
+  let r = a.toReversed();
+  assertEquals([1.1,,3.3,4.4], a);
+  assertEquals([4.4,3.3,,1.1], r);
+  assertFalse(a === r);
+})();
+
+(function TestHoley() {
+  let a = [true,false,,1,42.42];
+  let r = a.toReversed();
+  assertEquals([true,false,,1,42.42], a);
+  assertEquals([42.42,1,,false,true], r);
+  assertFalse(a === r);
+})();
+
 (function TestGeneric() {
   let a = { length: 4,
             get "0"() { return "hello"; },