[array] Move Array.p.unshift fall-back to Torque

This CL implements a generic baseline version of Array.p.unshift
in Torque, enabling us to remove the JS fall-back.

The elements-accessor fast-path is still used, but the check whether
to use it is also moved to Torque.

Support for sparse JSArrays is removed.

Drive-by change: Small refactoring in builtins-array that will
get extended to other array builtins in a follow-up CL.

R=cbruni@chromium.org, jgruber@chromium.org

Bug: v8:7624
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: I7b23ce15e7b922eb333f61a408050dedec77c95a
Reviewed-on: https://chromium-review.googlesource.com/1189902
Commit-Queue: Simon Zünd <szuend@google.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55670}

BUILD.gn

@@ -886,6 +886,7 @@ torque_files = [
   "src/builtins/array-lastindexof.tq",
   "src/builtins/array-reverse.tq",
   "src/builtins/array-splice.tq",
+  "src/builtins/array-unshift.tq",
   "src/builtins/typed-array.tq",
   "src/builtins/data-view.tq",
   "test/torque/test-torque.tq",

src/bootstrapper.cc

@@ -1745,8 +1745,8 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
                           Builtins::kArrayPrototypeReverse, 0, false);
     SimpleInstallFunction(isolate_, proto, "shift",
                           Builtins::kArrayPrototypeShift, 0, false);
-    SimpleInstallFunction(isolate_, proto, "unshift", Builtins::kArrayUnshift,
-                          1, false);
+    SimpleInstallFunction(isolate_, proto, "unshift",
+                          Builtins::kArrayPrototypeUnshift, 1, false);
     SimpleInstallFunction(isolate_, proto, "slice",
                           Builtins::kArrayPrototypeSlice, 2, false);
     SimpleInstallFunction(isolate_, proto, "sort",

src/builtins/array-reverse.tq

@@ -143,29 +143,16 @@ module array {
     return object;
   }
 
-  macro EnsureWriteableFastElements(array: JSArray) {
-    const elements: FixedArrayBase = array.elements;
-    if (elements.map != kCOWMap) return;
-
-    // There are no COW *_DOUBLE_ELEMENTS arrays, so we are allowed to always
-    // extract FixedArrays and don't have to worry about FixedDoubleArrays.
-    assert(IsFastSmiOrTaggedElementsKind(array.map.elements_kind));
-
-    const length: Smi = array.length_fast;
-    array.elements =
-        ExtractFixedArray(unsafe_cast<FixedArray>(elements), 0, length, length);
-  }
-
   macro TryFastPackedArrayReverse(receiver: Object) labels Slow {
     const array: JSArray = cast<JSArray>(receiver) otherwise Slow;
-    EnsureWriteableFastElements(array);
-    assert(array.elements.map != kCOWMap);
 
     const kind: ElementsKind = array.map.elements_kind;
     if (kind == PACKED_SMI_ELEMENTS) {
+      EnsureWriteableFastElements(array);
       FastPackedArrayReverse<FastPackedSmiElements, Smi>(
           array.elements, array.length_fast);
     } else if (kind == PACKED_ELEMENTS) {
+      EnsureWriteableFastElements(array);
       FastPackedArrayReverse<FastPackedObjectElements, Object>(
           array.elements, array.length_fast);
     } else if (kind == PACKED_DOUBLE_ELEMENTS) {

src/builtins/array-unshift.tq

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+module array {
+  extern builtin ArrayUnshift(Context, JSFunction, Object, int32);
+
+  macro TryFastArrayUnshift(
+      context: Context, receiver: Object, arguments: constexpr Arguments): never
+  labels Slow {
+    EnsureFastJSArray(context, receiver) otherwise Slow;
+    const array: JSArray = unsafe_cast<JSArray>(receiver);
+    EnsureWriteableFastElements(array);
+
+    const map: Map = array.map;
+    if (!IsExtensibleMap(map)) goto Slow;
+    EnsureArrayLengthWritable(map) otherwise Slow;
+
+    tail ArrayUnshift(
+        context, LoadTargetFromFrame(), Undefined,
+        convert<int32>(arguments.length));
+  }
+
+  macro GenericArrayUnshift(
+      context: Context, receiver: Object,
+      arguments: constexpr Arguments): Number {
+    // 1. Let O be ? ToObject(this value).
+    const object: JSReceiver = ToObject_Inline(context, receiver);
+
+    // 2. Let len be ? ToLength(? Get(O, "length")).
+    const length: Number = GetLengthProperty(context, object);
+
+    // 3. Let argCount be the number of actual arguments.
+    const arg_count: Smi = convert<Smi>(arguments.length);
+
+    // 4. If argCount > 0, then.
+    if (arg_count > 0) {
+      // a. If len + argCount > 2**53 - 1, throw a TypeError exception.
+      if (length + arg_count > kMaxSafeInteger) {
+        ThrowTypeError(context, kInvalidArrayLength);
+      }
+
+      // b. Let k be len.
+      let k: Number = length;
+
+      // c. Repeat, while k > 0.
+      while (k > 0) {
+        // i. Let from be ! ToString(k - 1).
+        const from: Number = k - 1;
+
+        // ii. Let to be ! ToString(k + argCount - 1).
+        const to: Number = k + arg_count - 1;
+
+        // iii. Let fromPresent be ? HasProperty(O, from).
+        const from_present: Boolean = HasProperty(context, object, from);
+
+        // iv. If fromPresent is true, then
+        if (from_present == True) {
+          // 1. Let fromValue be ? Get(O, from).
+          const from_value: Object = GetProperty(context, object, from);
+
+          // 2. Perform ? Set(O, to, fromValue, true).
+          SetProperty(context, object, to, from_value);
+        } else {
+          // 1. Perform ? DeletePropertyOrThrow(O, to).
+          DeleteProperty(context, object, to, kStrict);
+        }
+
+        // vi. Decrease k by 1.
+        --k;
+      }
+
+      // d. Let j be 0.
+      let j: Smi = 0;
+
+      // e. Let items be a List whose elements are, in left to right order,
+      //    the arguments that were passed to this function invocation.
+      // f. Repeat, while items is not empty
+      while (j < arg_count) {
+        // ii .Perform ? Set(O, ! ToString(j), E, true).
+        SetProperty(context, object, j, arguments[convert<intptr>(j)]);
+
+        // iii. Increase j by 1.
+        ++j;
+      }
+    }
+
+    // 5. Perform ? Set(O, "length", len + argCount, true).
+    const new_length: Number = length + arg_count;
+    SetProperty(context, object, kLengthString, new_length);
+
+    // 6. Return length + arg_count.
+    return new_length;
+  }
+
+  // https://tc39.github.io/ecma262/#sec-array.prototype.unshift
+  javascript builtin ArrayPrototypeUnshift(
+      context: Context, receiver: Object, ...arguments): Object {
+    try {
+      TryFastArrayUnshift(context, receiver, arguments) otherwise Baseline;
+    }
+    label Baseline {
+      return GenericArrayUnshift(context, receiver, arguments);
+    }
+  }
+}

src/builtins/array.tq

@@ -24,7 +24,23 @@ module array {
       }
   }
 
-  macro IsArray(o: Object): bool {
+  macro EnsureWriteableFastElements(array: JSArray) {
+    assert(IsFastElementsKind(array.map.elements_kind));
+
+    const elements: FixedArrayBase = array.elements;
+    if (elements.map != kCOWMap) return;
+
+    // There are no COW *_DOUBLE_ELEMENTS arrays, so we are allowed to always
+    // extract FixedArrays and don't have to worry about FixedDoubleArrays.
+    assert(IsFastSmiOrTaggedElementsKind(array.map.elements_kind));
+
+    const length: Smi = array.length_fast;
+    array.elements = ExtractFixedArray(
+        elements, 0, length, length, kFixedArrays);
+    assert(array.elements.map != kCOWMap);
+  }
+
+  macro IsJSArray(o: Object): bool {
     try {
       let array: JSArray = cast<JSArray>(o) otherwise NotArray;
       return true;

src/builtins/base.tq

@@ -201,6 +201,7 @@ extern macro ThrowTypeError(
 extern macro ArraySpeciesCreate(Context, Object, Number): JSReceiver;
 extern macro InternalArrayCreate(Context, Number): JSArray;
 extern macro EnsureArrayPushable(Map): ElementsKind labels Bailout;
+extern macro EnsureArrayLengthWritable(Map) labels Bailout;
 
 extern builtin ToObject(Context, Object): JSReceiver;
 extern macro ToObject_Inline(Context, Object): JSReceiver;
@@ -761,6 +762,8 @@ extern macro Call(
     Context, Callable, Object, Object, Object, Object, Object, Object): Object;
 
 extern macro ExtractFixedArray(FixedArrayBase, Smi, Smi, Smi): FixedArrayBase;
+extern macro ExtractFixedArray(FixedArrayBase, Smi, Smi, Smi,
+    constexpr ExtractFixedArrayFlags): FixedArrayBase;
 
 extern builtin ExtractFastJSArray(Context, JSArray, Smi, Smi): JSArray;
 
@@ -810,6 +813,7 @@ extern macro IsExtensibleMap(Map): bool;
 extern macro IsCustomElementsReceiverInstanceType(int32): bool;
 extern macro IsFastJSArray(Object, Context): bool;
 extern macro Typeof(Object): Object;
+extern macro LoadTargetFromFrame(): JSFunction;
 
 // Return true iff number is NaN.
 macro NumberIsNaN(number: Number): bool {

src/builtins/builtins-array.cc

@@ -51,36 +51,20 @@ inline bool HasOnlySimpleElements(Isolate* isolate, JSReceiver* receiver) {
   return true;
 }
 
-// Returns |false| if not applicable.
-// TODO(szuend): Refactor this function because it is getting hard to
-//               understand what each call-site actually checks.
-V8_WARN_UNUSED_RESULT
-inline bool EnsureJSArrayWithWritableFastElements(Isolate* isolate,
-                                                  Handle<Object> receiver,
-                                                  BuiltinArguments* args,
-                                                  int first_arg_index,
-                                                  int num_arguments) {
-  if (!receiver->IsJSArray()) return false;
-  Handle<JSArray> array = Handle<JSArray>::cast(receiver);
-  ElementsKind origin_kind = array->GetElementsKind();
-  if (IsDictionaryElementsKind(origin_kind)) return false;
-  if (!array->map()->is_extensible()) return false;
-  if (args == nullptr) return true;
-
-  // If there may be elements accessors in the prototype chain, the fast path
-  // cannot be used if there arguments to add to the array.
-  if (!IsJSArrayFastElementMovingAllowed(isolate, *array)) return false;
+// This method may transition the elements kind of the JSArray once, to make
+// sure that all elements provided as arguments in the specified range can be
+// added without further elements kinds transitions.
+void MatchArrayElementsKindToArguments(Isolate* isolate, Handle<JSArray> array,
+                                       BuiltinArguments* args,
+                                       int first_arg_index, int num_arguments) {
+  int args_length = args->length();
+  if (first_arg_index >= args_length) return;
 
-  // Adding elements to the array prototype would break code that makes sure
-  // it has no elements. Handle that elsewhere.
-  if (isolate->IsAnyInitialArrayPrototype(array)) return false;
+  ElementsKind origin_kind = array->GetElementsKind();
 
-  // Need to ensure that the arguments passed in args can be contained in
-  // the array.
-  int args_length = args->length();
-  if (first_arg_index >= args_length) return true;
+  // We do not need to transition for PACKED/HOLEY_ELEMENTS.
+  if (IsObjectElementsKind(origin_kind)) return;
 
-  if (IsObjectElementsKind(origin_kind)) return true;
   ElementsKind target_kind = origin_kind;
   {
     DisallowHeapAllocation no_gc;
@@ -103,20 +87,37 @@ inline bool EnsureJSArrayWithWritableFastElements(Isolate* isolate,
     HandleScope scope(isolate);
     JSObject::TransitionElementsKind(array, target_kind);
   }
-  return true;
 }
 
-V8_WARN_UNUSED_RESULT static Object* CallJsIntrinsic(
-    Isolate* isolate, Handle<JSFunction> function, BuiltinArguments args) {
-  HandleScope handleScope(isolate);
-  int argc = args.length() - 1;
-  ScopedVector<Handle<Object>> argv(argc);
-  for (int i = 0; i < argc; ++i) {
-    argv[i] = args.at(i + 1);
-  }
-  RETURN_RESULT_OR_FAILURE(
-      isolate,
-      Execution::Call(isolate, function, args.receiver(), argc, argv.start()));
+// Returns |false| if not applicable.
+// TODO(szuend): Refactor this function because it is getting hard to
+//               understand what each call-site actually checks.
+V8_WARN_UNUSED_RESULT
+inline bool EnsureJSArrayWithWritableFastElements(Isolate* isolate,
+                                                  Handle<Object> receiver,
+                                                  BuiltinArguments* args,
+                                                  int first_arg_index,
+                                                  int num_arguments) {
+  if (!receiver->IsJSArray()) return false;
+  Handle<JSArray> array = Handle<JSArray>::cast(receiver);
+  ElementsKind origin_kind = array->GetElementsKind();
+  if (IsDictionaryElementsKind(origin_kind)) return false;
+  if (!array->map()->is_extensible()) return false;
+  if (args == nullptr) return true;
+
+  // If there may be elements accessors in the prototype chain, the fast path
+  // cannot be used if there arguments to add to the array.
+  if (!IsJSArrayFastElementMovingAllowed(isolate, *array)) return false;
+
+  // Adding elements to the array prototype would break code that makes sure
+  // it has no elements. Handle that elsewhere.
+  if (isolate->IsAnyInitialArrayPrototype(array)) return false;
+
+  // Need to ensure that the arguments passed in args can be contained in
+  // the array.
+  MatchArrayElementsKindToArguments(isolate, array, args, first_arg_index,
+                                    num_arguments);
+  return true;
 }
 
 // If |index| is Undefined, returns init_if_undefined.
@@ -588,21 +589,24 @@ BUILTIN(ArrayShift) {
 
 BUILTIN(ArrayUnshift) {
   HandleScope scope(isolate);
-  Handle<Object> receiver = args.receiver();
-  if (!EnsureJSArrayWithWritableFastElements(isolate, receiver, &args, 1,
-                                             args.length() - 1)) {
-    return CallJsIntrinsic(isolate, isolate->array_unshift(), args);
-  }
-  Handle<JSArray> array = Handle<JSArray>::cast(receiver);
+  DCHECK(args.receiver()->IsJSArray());
+  Handle<JSArray> array = Handle<JSArray>::cast(args.receiver());
+
+  // These are checked in the Torque builtin.
+  DCHECK(array->map()->is_extensible());
+  DCHECK(!IsDictionaryElementsKind(array->GetElementsKind()));
+  DCHECK(IsJSArrayFastElementMovingAllowed(isolate, *array));
+  DCHECK(!isolate->IsAnyInitialArrayPrototype(array));
+
+  MatchArrayElementsKindToArguments(isolate, array, &args, 1,
+                                    args.length() - 1);
+
   int to_add = args.length() - 1;
   if (to_add == 0) return array->length();
 
   // Currently fixed arrays cannot grow too big, so we should never hit this.
   DCHECK_LE(to_add, Smi::kMaxValue - Smi::ToInt(array->length()));
-
-  if (JSArray::HasReadOnlyLength(array)) {
-    return CallJsIntrinsic(isolate, isolate->array_unshift(), args);
-  }
+  DCHECK(!JSArray::HasReadOnlyLength(array));
 
   ElementsAccessor* accessor = array->GetElementsAccessor();
   int new_length = accessor->Unshift(array, &args, to_add);

src/code-stub-assembler.h

@@ -1585,11 +1585,12 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
           ExtractFixedArrayFlag::kAllFixedArrays,
       ParameterMode parameter_mode = INTPTR_PARAMETERS);
 
-  TNode<FixedArrayBase> ExtractFixedArray(TNode<FixedArrayBase> source,
-                                          TNode<Smi> first, TNode<Smi> count,
-                                          TNode<Smi> capacity) {
-    return ExtractFixedArray(source, first, count, capacity,
-                             ExtractFixedArrayFlag::kAllFixedArrays,
+  TNode<FixedArrayBase> ExtractFixedArray(
+      TNode<FixedArrayBase> source, TNode<Smi> first, TNode<Smi> count,
+      TNode<Smi> capacity,
+      ExtractFixedArrayFlags extract_flags =
+          ExtractFixedArrayFlag::kAllFixedArrays) {
+    return ExtractFixedArray(source, first, count, capacity, extract_flags,
                              SMI_PARAMETERS);
   }
 

src/contexts.h

@@ -72,8 +72,6 @@ enum ContextLookupFlags {
   V(ASYNC_GENERATOR_AWAIT_UNCAUGHT, JSFunction, async_generator_await_uncaught)
 
 #define NATIVE_CONTEXT_IMPORTED_FIELDS(V)                                 \
-  V(ARRAY_SHIFT_INDEX, JSFunction, array_shift)                           \
-  V(ARRAY_UNSHIFT_INDEX, JSFunction, array_unshift)                       \
   V(ARRAY_ENTRIES_ITERATOR_INDEX, JSFunction, array_entries_iterator)     \
   V(ARRAY_FOR_EACH_ITERATOR_INDEX, JSFunction, array_for_each_iterator)   \
   V(ARRAY_KEYS_ITERATOR_INDEX, JSFunction, array_keys_iterator)           \

src/debug/debug-evaluate.cc

@@ -812,6 +812,7 @@ DebugInfo::SideEffectState BuiltinGetSideEffectState(Builtins::Name id) {
     case Builtins::kArrayPrototypePush:
     case Builtins::kArrayPrototypeReverse:
     case Builtins::kArrayPrototypeShift:
+    case Builtins::kArrayPrototypeUnshift:
     case Builtins::kArraySplice:
     case Builtins::kArrayUnshift:
     // Map builtins.

src/js/array.js

@@ -379,32 +379,6 @@ DEFINE_METHOD(
 );
 
 
-function ArrayUnshiftFallback(arg1) {  // length == 1
-  var array = TO_OBJECT(this);
-  var len = TO_LENGTH(array.length);
-  var num_arguments = arguments.length;
-
-  const new_len = len + num_arguments;
-  if (num_arguments > 0) {
-    if (new_len >= 2**53) throw %make_type_error(kInvalidArrayLength);
-
-    if (len > 0 && UseSparseVariant(array, len, IS_ARRAY(array), len) &&
-        !%object_is_sealed(array)) {
-      SparseMove(array, 0, 0, len, num_arguments);
-    } else {
-      SimpleMove(array, 0, 0, len, num_arguments);
-    }
-
-    for (var i = 0; i < num_arguments; i++) {
-      array[i] = arguments[i];
-    }
-  }
-
-  array.length = new_len;
-  return new_len;
-}
-
-
 // Oh the humanity... don't remove the following function because js2c for some
 // reason gets symbol minifiation wrong if it's not there. Instead of spending
 // the time fixing js2c (which will go away when all of the internal .js runtime
@@ -649,7 +623,6 @@ utils.Export(function(to) {
   "array_keys_iterator", ArrayKeys,
   "array_values_iterator", ArrayValues,
   // Fallback implementations of Array builtins.
-  "array_unshift", ArrayUnshiftFallback,
 ]);
 
 });

test/mjsunit/array-functions-prototype-misc.js

@@ -40,7 +40,7 @@ function PseudoArray() {
 };
 
 for (var use_real_arrays = 0; use_real_arrays <= 1; use_real_arrays++) {
-  var poses = [0, 140, 20000, VERYLARGE];
+  var poses = [0, 140, 20000];
   var the_prototype;
   var new_function;
   var push_function;

test/mjsunit/array-unshift.js

@@ -190,15 +190,12 @@
 (function() {
   for (var i = 0; i < 7; i++) {
     try {
-      new Array(Math.pow(2, 32) - 3).unshift(1, 2, 3, 4, 5);
-      throw 'Should have thrown RangeError';
+      let obj = { length: 2 ** 53 - 3};
+      Array.prototype.unshift.call(obj, 1, 2, 3, 4, 5);
+      throw 'Should have thrown TypeError';
     } catch (e) {
-      assertTrue(e instanceof RangeError);
+      assertTrue(e instanceof TypeError);
     }
-
-    // Check smi boundary
-    var bigNum = (1 << 30) - 3;
-    assertEquals(bigNum + 7, new Array(bigNum).unshift(1, 2, 3, 4, 5, 6, 7));
   }
 })();
 

test/mjsunit/regress/regress-splice-large-index.js

-// Copyright 2013 the V8 project authors. All rights reserved.
-// Redistribution and use in source and binary forms, with or without
-// modification, are permitted provided that the following conditions are
-// met:
-//
-//     * Redistributions of source code must retain the above copyright
-//       notice, this list of conditions and the following disclaimer.
-//     * Redistributions in binary form must reproduce the above
-//       copyright notice, this list of conditions and the following
-//       disclaimer in the documentation and/or other materials provided
-//       with the distribution.
-//     * Neither the name of Google Inc. nor the names of its
-//       contributors may be used to endorse or promote products derived
-//       from this software without specific prior written permission.
-//
-// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-var a = [];
-a[0xfffffffe] = 10;
-assertThrows("a.unshift(1);", RangeError);
-assertEquals(0xffffffff, a.length);
-assertEquals(10, a[0xffffffff]);
-assertEquals(0xffffffff, a.length);
-assertEquals(undefined, a[0xfffffffe]);

test/mozilla/mozilla.status

@@ -158,6 +158,11 @@
   'js1_5/Regress/regress-462292': [SKIP],
   'js1_5/decompilation/regress-443071-01': [SKIP],
 
+  # This test checks that 'unshift' doesn't cause a OOM. Since the range error
+  # will get thrown at the end of the operation, this test runs for a long time.
+  # https://crbug.com/v8/8120
+  'ecma_3/Array/regress-322135-04': [SKIP],
+
   ##################### SLOW TESTS #####################
 
   # Compiles a long chain of && or || operations, can time out under slower

test/test262/test262.status

@@ -652,7 +652,6 @@
   'annexB/language/function-code/block-decl-func-skip-arguments': [FAIL],
 
   # https://bugs.chromium.org/p/v8/issues/detail?id=6538
-  'built-ins/Array/prototype/unshift/throws-if-integer-limit-exceeded': [SKIP],
 
   # https://bugs.chromium.org/p/v8/issues/detail?id=6541
   'language/export/escaped-as-export-specifier': [FAIL],