Array Builtin Refactoring: Creating API methods on ElementsAccessor

BUG=

Review URL: https://codereview.chromium.org/1260283002

Cr-Commit-Position: refs/heads/master@{#29958}

src/builtins.cc

@@ -262,7 +262,7 @@ static inline MaybeHandle<FixedArrayBase> EnsureJSArrayWithWritableFastElements(
   ElementsKind target_kind = origin_kind;
   {
     DisallowHeapAllocation no_gc;
-    int arg_count = args->length() - first_added_arg;
+    int arg_count = args_length - first_added_arg;
     Object** arguments = args->arguments() - first_added_arg - (arg_count - 1);
     for (int i = 0; i < arg_count; i++) {
       Object* arg = arguments[i];
@@ -321,104 +321,22 @@ BUILTIN(ArrayPush) {
   if (!maybe_elms_obj.ToHandle(&elms_obj)) {
     return CallJsBuiltin(isolate, "$arrayPush", args);
   }
-
+  // Fast Elements Path
+  int push_size = args.length() - 1;
   Handle<JSArray> array = Handle<JSArray>::cast(receiver);
   int len = Smi::cast(array->length())->value();
-  int to_add = args.length() - 1;
-  if (to_add > 0 && JSArray::WouldChangeReadOnlyLength(array, len + to_add)) {
+  if (push_size == 0) {
+    return Smi::FromInt(len);
+  }
+  if (push_size > 0 &&
+      JSArray::WouldChangeReadOnlyLength(array, len + push_size)) {
     return CallJsBuiltin(isolate, "$arrayPush", args);
   }
   DCHECK(!array->map()->is_observed());
-
-  ElementsKind kind = array->GetElementsKind();
-
-  if (IsFastSmiOrObjectElementsKind(kind)) {
-    Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
-    if (to_add == 0) {
-      return Smi::FromInt(len);
-    }
-    // Currently fixed arrays cannot grow too big, so
-    // we should never hit this case.
-    DCHECK(to_add <= (Smi::kMaxValue - len));
-
-    int new_length = len + to_add;
-
-    if (new_length > elms->length()) {
-      // New backing storage is needed.
-      int capacity = new_length + (new_length >> 1) + 16;
-      Handle<FixedArray> new_elms =
-          isolate->factory()->NewUninitializedFixedArray(capacity);
-
-      ElementsAccessor* accessor = array->GetElementsAccessor();
-      accessor->CopyElements(
-          elms_obj, 0, kind, new_elms, 0,
-          ElementsAccessor::kCopyToEndAndInitializeToHole);
-
-      elms = new_elms;
-    }
-
-    // Add the provided values.
-    DisallowHeapAllocation no_gc;
-    WriteBarrierMode mode = elms->GetWriteBarrierMode(no_gc);
-    for (int index = 0; index < to_add; index++) {
-      elms->set(index + len, args[index + 1], mode);
-    }
-
-    if (*elms != array->elements()) {
-      array->set_elements(*elms);
-    }
-
-    // Set the length.
-    array->set_length(Smi::FromInt(new_length));
-    return Smi::FromInt(new_length);
-  } else {
-    int elms_len = elms_obj->length();
-    if (to_add == 0) {
-      return Smi::FromInt(len);
-    }
-    // Currently fixed arrays cannot grow too big, so
-    // we should never hit this case.
-    DCHECK(to_add <= (Smi::kMaxValue - len));
-
-    int new_length = len + to_add;
-
-    Handle<FixedDoubleArray> new_elms;
-
-    if (new_length > elms_len) {
-      // New backing storage is needed.
-      int capacity = new_length + (new_length >> 1) + 16;
-      // Create new backing store; since capacity > 0, we can
-      // safely cast to FixedDoubleArray.
-      new_elms = Handle<FixedDoubleArray>::cast(
-          isolate->factory()->NewFixedDoubleArray(capacity));
-
-      ElementsAccessor* accessor = array->GetElementsAccessor();
-      accessor->CopyElements(
-          elms_obj, 0, kind, new_elms, 0,
-          ElementsAccessor::kCopyToEndAndInitializeToHole);
-
-    } else {
-      // to_add is > 0 and new_length <= elms_len, so elms_obj cannot be the
-      // empty_fixed_array.
-      new_elms = Handle<FixedDoubleArray>::cast(elms_obj);
-    }
-
-    // Add the provided values.
-    DisallowHeapAllocation no_gc;
-    int index;
-    for (index = 0; index < to_add; index++) {
-      Object* arg = args[index + 1];
-      new_elms->set(index + len, arg->Number());
-    }
-
-    if (*new_elms != array->elements()) {
-      array->set_elements(*new_elms);
-    }
-
-    // Set the length.
-    array->set_length(Smi::FromInt(new_length));
-    return Smi::FromInt(new_length);
-  }
+  ElementsAccessor* accessor = array->GetElementsAccessor();
+  int new_length = accessor->Push(array, elms_obj, &args[1], push_size,
+                                  ElementsAccessor::kDirectionReverse);
+  return Smi::FromInt(new_length);
 }
 
 
@@ -503,7 +421,6 @@ BUILTIN(ArrayShift) {
 
 BUILTIN(ArrayUnshift) {
   HandleScope scope(isolate);
-  Heap* heap = isolate->heap();
   Handle<Object> receiver = args.receiver();
   MaybeHandle<FixedArrayBase> maybe_elms_obj =
       EnsureJSArrayWithWritableFastElements(isolate, receiver, &args, 1);
@@ -545,6 +462,7 @@ BUILTIN(ArrayUnshift) {
     array->set_elements(*elms);
   } else {
     DisallowHeapAllocation no_gc;
+    Heap* heap = isolate->heap();
     heap->MoveElements(*elms, to_add, 0, len);
   }
 

src/elements.cc

@@ -245,6 +245,7 @@ static void CopyDoubleToObjectElements(FixedArrayBase* from_base,
       }
     }
   }
+
   DCHECK((copy_size + static_cast<int>(to_start)) <= to_base->length() &&
          (copy_size + static_cast<int>(from_start)) <= from_base->length());
   if (copy_size == 0) return;
@@ -571,6 +572,20 @@ class ElementsAccessorBase : public ElementsAccessor {
     UNREACHABLE();
   }
 
+  virtual uint32_t Push(Handle<JSArray> receiver,
+                        Handle<FixedArrayBase> backing_store, Object** objects,
+                        uint32_t push_size, int direction) {
+    return ElementsAccessorSubclass::PushImpl(receiver, backing_store, objects,
+                                              push_size, direction);
+  }
+
+  static uint32_t PushImpl(Handle<JSArray> receiver,
+                           Handle<FixedArrayBase> elms_obj, Object** objects,
+                           uint32_t push_size, int direction) {
+    UNREACHABLE();
+    return 0;
+  }
+
   virtual void SetLength(Handle<JSArray> array, uint32_t length) final {
     ElementsAccessorSubclass::SetLengthImpl(array, length,
                                             handle(array->elements()));
@@ -1140,6 +1155,53 @@ class FastElementsAccessor
     }
 #endif
   }
+
+  static uint32_t PushImpl(Handle<JSArray> receiver,
+                           Handle<FixedArrayBase> backing_store,
+                           Object** objects, uint32_t push_size,
+                           int direction) {
+    uint32_t len = Smi::cast(receiver->length())->value();
+    if (push_size == 0) {
+      return len;
+    }
+    uint32_t elms_len = backing_store->length();
+    // Currently fixed arrays cannot grow too big, so
+    // we should never hit this case.
+    DCHECK(push_size <= static_cast<uint32_t>(Smi::kMaxValue - len));
+    uint32_t new_length = len + push_size;
+    Handle<FixedArrayBase> new_elms;
+
+    if (new_length > elms_len) {
+      // New backing storage is needed.
+      uint32_t capacity = new_length + (new_length >> 1) + 16;
+      new_elms = FastElementsAccessorSubclass::ConvertElementsWithCapacity(
+          receiver, backing_store, KindTraits::Kind, capacity);
+    } else {
+      // push_size is > 0 and new_length <= elms_len, so backing_store cannot be
+      // the
+      // empty_fixed_array.
+      new_elms = backing_store;
+    }
+
+    // Add the provided values.
+    DisallowHeapAllocation no_gc;
+    DCHECK(direction == ElementsAccessor::kDirectionForward ||
+           direction == ElementsAccessor::kDirectionReverse);
+    STATIC_ASSERT(ElementsAccessor::kDirectionForward == 1);
+    STATIC_ASSERT(ElementsAccessor::kDirectionReverse == -1);
+    for (uint32_t index = 0; index < push_size; index++) {
+      int offset = direction * index;
+      Object* object = objects[offset];
+      FastElementsAccessorSubclass::SetImpl(*new_elms, index + len, object);
+    }
+    if (!new_elms.is_identical_to(backing_store)) {
+      receiver->set_elements(*new_elms);
+    }
+    DCHECK(*new_elms == receiver->elements());
+    // Set the length.
+    receiver->set_length(Smi::FromInt(new_length));
+    return new_length;
+  }
 };
 
 

src/elements.h

@@ -60,6 +60,9 @@ class ElementsAccessor {
   // destination array with the hole.
   static const int kCopyToEndAndInitializeToHole = -2;
 
+  static const int kDirectionForward = 1;
+  static const int kDirectionReverse = -1;
+
   // Copy elements from one backing store to another. Typically, callers specify
   // the source JSObject or JSArray in source_holder. If the holder's backing
   // store is available, it can be passed in source and source_holder is
@@ -112,14 +115,22 @@ class ElementsAccessor {
 
   virtual void Set(FixedArrayBase* backing_store, uint32_t entry,
                    Object* value) = 0;
+
   virtual void Reconfigure(Handle<JSObject> object,
                            Handle<FixedArrayBase> backing_store, uint32_t entry,
                            Handle<Object> value,
                            PropertyAttributes attributes) = 0;
+
   virtual void Add(Handle<JSObject> object, uint32_t index,
                    Handle<Object> value, PropertyAttributes attributes,
                    uint32_t new_capacity) = 0;
 
+  // TODO(cbruni): Consider passing Arguments* instead of Object** depending on
+  // the requirements of future callers.
+  virtual uint32_t Push(Handle<JSArray> receiver,
+                        Handle<FixedArrayBase> backing_store, Object** objects,
+                        uint32_t start, int direction) = 0;
+
  protected:
   friend class LookupIterator;
 

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

@@ -312,3 +312,75 @@ Array.prototype[1] = undefined;
 
 // Test http://code.google.com/p/chromium/issues/detail?id=21860
 Array.prototype.push.apply([], [1].splice(0, -(-1 % 5)));
+
+
+// Check that the Array functions work also properly on non-Arrays
+var receiver;
+
+receiver = 'a string';
+assertThrows(function(){
+  Array.prototype.push.call(receiver);
+});
+
+receiver = 0;
+assertEquals(undefined, receiver.length);
+assertEquals(0, Array.prototype.push.call(receiver));
+assertEquals(1, Array.prototype.push.call(receiver, 'first'));
+assertEquals(undefined, receiver.length);
+
+receiver = {};
+assertEquals(undefined, receiver.length);
+assertEquals(0, Array.prototype.push.call(receiver));
+assertEquals(0, Array.prototype.push.call(receiver));
+assertEquals(0, receiver.length);
+assertEquals(1, Array.prototype.push.call(receiver, 'first'));
+assertEquals(1, receiver.length);
+assertEquals('first', receiver[0]);
+assertEquals(2, Array.prototype.push.call(receiver, 'second'));
+assertEquals(2, receiver.length);
+assertEquals('first', receiver[0]);
+assertEquals('second', receiver[1]);
+
+receiver = {'length': 10};
+assertEquals(10, Array.prototype.push.call(receiver));
+assertEquals(10, receiver.length);
+assertEquals(11, Array.prototype.push.call(receiver, 'first'));
+assertEquals(11, receiver.length);
+assertEquals('first', receiver[10]);
+assertEquals(13, Array.prototype.push.call(receiver, 'second', 'third'));
+assertEquals(13, receiver.length);
+assertEquals('first', receiver[10]);
+assertEquals('second', receiver[11]);
+assertEquals('third', receiver[12]);
+
+receiver = {
+  get length() { return 10; },
+  set length(l) {}
+};
+assertEquals(10, Array.prototype.push.call(receiver));
+assertEquals(10, receiver.length);
+assertEquals(11, Array.prototype.push.call(receiver, 'first'));
+assertEquals(10, receiver.length);
+assertEquals('first', receiver[10]);
+assertEquals(12, Array.prototype.push.call(receiver, 'second', 'third'));
+assertEquals(10, receiver.length);
+assertEquals('second', receiver[10]);
+assertEquals('third', receiver[11]);
+
+// readonly length
+receiver = {
+  get length() { return 10; },
+};
+assertThrows(function(){
+  Array.prototype.push.call(receiver);
+});
+
+receiver = {
+  set length(l) {}
+};
+assertEquals(0, Array.prototype.push.call(receiver));
+assertEquals(undefined, receiver.length);
+assertEquals(1, Array.prototype.push.call(receiver, 'first'));
+assertEquals(undefined, receiver.length);
+assertEquals(2, Array.prototype.push.call(receiver, 'third', 'second'));
+assertEquals(undefined, receiver.length);