Adding ElementsAccessor::Shift

- Use the new ElementsAccessor methods
- improve test coverage

BUG=

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

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

src/builtins.cc

@@ -195,14 +195,6 @@ inline bool ClampedToInteger(Object* object, int* out) {
 }
 
 
-void MoveDoubleElements(FixedDoubleArray* dst, int dst_index,
-                        FixedDoubleArray* src, int src_index, int len) {
-  if (len == 0) return;
-  MemMove(dst->data_start() + dst_index, src->data_start() + src_index,
-          len * kDoubleSize);
-}
-
-
 inline bool GetSloppyArgumentsLength(Isolate* isolate, Handle<JSObject> object,
                                      int* out) {
   Map* arguments_map =
@@ -432,30 +424,7 @@ BUILTIN(ArrayShift) {
     return CallJsIntrinsic(isolate, isolate->array_shift(), args);
   }
 
-  // Get first element
-  Handle<Object> first;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, first,
-                                     Object::GetElement(isolate, array, 0));
-
-  if (heap->CanMoveObjectStart(*elms_obj)) {
-    array->set_elements(heap->LeftTrimFixedArray(*elms_obj, 1));
-  } else {
-    // Shift the elements.
-    if (elms_obj->IsFixedArray()) {
-      Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
-      DisallowHeapAllocation no_gc;
-      heap->MoveElements(*elms, 0, 1, len - 1);
-      elms->set(len - 1, heap->the_hole_value());
-    } else {
-      Handle<FixedDoubleArray> elms = Handle<FixedDoubleArray>::cast(elms_obj);
-      MoveDoubleElements(*elms, 0, *elms, 1, len - 1);
-      elms->set_the_hole(len - 1);
-    }
-  }
-
-  // Set the length.
-  array->set_length(Smi::FromInt(len - 1));
-
+  Handle<Object> first = array->GetElementsAccessor()->Shift(array, elms_obj);
   return *first;
 }
 
@@ -472,6 +441,9 @@ BUILTIN(ArrayUnshift) {
   Handle<JSArray> array = Handle<JSArray>::cast(receiver);
   DCHECK(!array->map()->is_observed());
   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 case.
   DCHECK(to_add <= (Smi::kMaxValue - Smi::cast(array->length())->value()));
@@ -553,6 +525,12 @@ BUILTIN(ArraySlice) {
   uint32_t actual_end =
       (relative_end < 0) ? Max(len + relative_end, 0) : Min(relative_end, len);
 
+  if (actual_end <= actual_start) {
+    Handle<JSArray> result_array =
+        isolate->factory()->NewJSArray(GetInitialFastElementsKind(), 0, 0);
+    return *result_array;
+  }
+
   ElementsAccessor* accessor = object->GetElementsAccessor();
   if (is_sloppy_arguments &&
       !accessor->IsPacked(object, elms_obj, actual_start, actual_end)) {

src/elements.cc

@@ -670,13 +670,64 @@ class ElementsAccessorBase : public ElementsAccessor {
     return Handle<Object>();
   }
 
+  virtual Handle<Object> Shift(Handle<JSArray> receiver,
+                               Handle<FixedArrayBase> backing_store) final {
+    return ElementsAccessorSubclass::ShiftImpl(receiver, backing_store);
+  }
+
+  static Handle<Object> ShiftImpl(Handle<JSArray> receiver,
+                                  Handle<FixedArrayBase> backing_store) {
+    UNREACHABLE();
+    return Handle<Object>();
+  }
+
   virtual void SetLength(Handle<JSArray> array, uint32_t length) final {
     ElementsAccessorSubclass::SetLengthImpl(array, length,
                                             handle(array->elements()));
   }
 
   static void SetLengthImpl(Handle<JSArray> array, uint32_t length,
-                            Handle<FixedArrayBase> backing_store);
+                            Handle<FixedArrayBase> backing_store) {
+    DCHECK(!array->SetLengthWouldNormalize(length));
+    DCHECK(IsFastElementsKind(array->GetElementsKind()));
+    uint32_t old_length = 0;
+    CHECK(array->length()->ToArrayIndex(&old_length));
+
+    if (old_length < length) {
+      ElementsKind kind = array->GetElementsKind();
+      if (!IsFastHoleyElementsKind(kind)) {
+        kind = GetHoleyElementsKind(kind);
+        JSObject::TransitionElementsKind(array, kind);
+      }
+    }
+
+    // Check whether the backing store should be shrunk.
+    uint32_t capacity = backing_store->length();
+    if (length == 0) {
+      array->initialize_elements();
+    } else if (length <= capacity) {
+      if (array->HasFastSmiOrObjectElements()) {
+        backing_store = JSObject::EnsureWritableFastElements(array);
+      }
+      if (2 * length <= capacity) {
+        // If more than half the elements won't be used, trim the array.
+        array->GetHeap()->RightTrimFixedArray<Heap::CONCURRENT_TO_SWEEPER>(
+            *backing_store, capacity - length);
+      } else {
+        // Otherwise, fill the unused tail with holes.
+        for (uint32_t i = length; i < old_length; i++) {
+          BackingStore::cast(*backing_store)->set_the_hole(i);
+        }
+      }
+    } else {
+      // Check whether the backing store should be expanded.
+      capacity = Max(length, JSObject::NewElementsCapacity(capacity));
+      ElementsAccessorSubclass::GrowCapacityAndConvertImpl(array, capacity);
+    }
+
+    array->set_length(Smi::FromInt(length));
+    JSObject::ValidateElements(array);
+  }
 
   static Handle<FixedArrayBase> ConvertElementsWithCapacity(
       Handle<JSObject> object, Handle<FixedArrayBase> old_elements,
@@ -1261,13 +1312,40 @@ class FastElementsAccessor
 
   static Handle<Object> PopImpl(Handle<JSArray> receiver,
                                 Handle<FixedArrayBase> backing_store) {
-    uint32_t new_length =
-        static_cast<uint32_t>(Smi::cast(receiver->length())->value()) - 1;
+    uint32_t len =
+        static_cast<uint32_t>(Smi::cast(receiver->length())->value());
+    DCHECK(len > 0);
+    uint32_t new_length = len - 1;
     Handle<Object> result =
         FastElementsAccessorSubclass::GetImpl(backing_store, new_length);
     FastElementsAccessorSubclass::SetLengthImpl(receiver, new_length,
                                                 backing_store);
 
+    if (IsHoleyElementsKind(KindTraits::Kind) && result->IsTheHole()) {
+      return receiver->GetIsolate()->factory()->undefined_value();
+    }
+    return result;
+  }
+
+  static Handle<Object> ShiftImpl(Handle<JSArray> receiver,
+                                  Handle<FixedArrayBase> backing_store) {
+    uint32_t len =
+        static_cast<uint32_t>(Smi::cast(receiver->length())->value());
+    Isolate* isolate = receiver->GetIsolate();
+    DCHECK(len > 0);
+    int new_length = len - 1;
+    Handle<Object> result =
+        FastElementsAccessorSubclass::GetImpl(backing_store, 0);
+    Heap* heap = isolate->heap();
+    if (heap->CanMoveObjectStart(*backing_store)) {
+      receiver->set_elements(heap->LeftTrimFixedArray(*backing_store, 1));
+    } else {
+      FastElementsAccessorSubclass::MoveElements(heap, backing_store, 0, 1,
+                                                 new_length, 0, 0);
+    }
+    FastElementsAccessorSubclass::SetLengthImpl(receiver, new_length,
+                                                backing_store);
+
     if (IsHoleyElementsKind(KindTraits::Kind) && result->IsTheHole()) {
       result = receiver->GetIsolate()->factory()->undefined_value();
     }
@@ -1278,9 +1356,7 @@ class FastElementsAccessor
                            Handle<FixedArrayBase> backing_store,
                            Arguments* args, uint32_t push_size) {
     uint32_t len = Smi::cast(receiver->length())->value();
-    if (push_size == 0) {
-      return len;
-    }
+    DCHECK(push_size > 0);
     uint32_t elms_len = backing_store->length();
     // Currently fixed arrays cannot grow too big, so
     // we should never hit this case.
@@ -1314,9 +1390,7 @@ class FastElementsAccessor
                               Handle<FixedArrayBase> backing_store,
                               Arguments* args, uint32_t unshift_size) {
     uint32_t len = Smi::cast(receiver->length())->value();
-    if (unshift_size == 0) {
-      return len;
-    }
+    DCHECK(unshift_size > 0);
     uint32_t elms_len = backing_store->length();
     // Currently fixed arrays cannot grow too big, so
     // we should never hit this case.
@@ -1362,10 +1436,8 @@ class FastElementsAccessor
   static Handle<JSArray> SliceImpl(Handle<JSObject> receiver,
                                    Handle<FixedArrayBase> backing_store,
                                    uint32_t start, uint32_t end) {
+    DCHECK(start < end);
     Isolate* isolate = receiver->GetIsolate();
-    if (end <= start) {
-      return isolate->factory()->NewJSArray(KindTraits::Kind, 0, 0);
-    }
     int result_len = end - start;
     Handle<JSArray> result_array = isolate->factory()->NewJSArray(
         KindTraits::Kind, result_len, result_len);
@@ -2122,50 +2194,6 @@ class FastSloppyArgumentsElementsAccessor
 };
 
 
-template <typename ElementsAccessorSubclass, typename ElementsKindTraits>
-void ElementsAccessorBase<ElementsAccessorSubclass, ElementsKindTraits>::
-    SetLengthImpl(Handle<JSArray> array, uint32_t length,
-                  Handle<FixedArrayBase> backing_store) {
-  DCHECK(!array->SetLengthWouldNormalize(length));
-  DCHECK(IsFastElementsKind(array->GetElementsKind()));
-  uint32_t old_length = 0;
-  CHECK(array->length()->ToArrayIndex(&old_length));
-
-  if (old_length < length) {
-    ElementsKind kind = array->GetElementsKind();
-    if (!IsFastHoleyElementsKind(kind)) {
-      kind = GetHoleyElementsKind(kind);
-      JSObject::TransitionElementsKind(array, kind);
-    }
-  }
-
-  // Check whether the backing store should be shrunk.
-  uint32_t capacity = backing_store->length();
-  if (length == 0) {
-    array->initialize_elements();
-  } else if (length <= capacity) {
-    if (array->HasFastSmiOrObjectElements()) {
-      backing_store = JSObject::EnsureWritableFastElements(array);
-    }
-    if (2 * length <= capacity) {
-      // If more than half the elements won't be used, trim the array.
-      array->GetHeap()->RightTrimFixedArray<Heap::CONCURRENT_TO_SWEEPER>(
-          *backing_store, capacity - length);
-    } else {
-      // Otherwise, fill the unused tail with holes.
-      for (uint32_t i = length; i < old_length; i++) {
-        BackingStore::cast(*backing_store)->set_the_hole(i);
-      }
-    }
-  } else {
-    // Check whether the backing store should be expanded.
-    capacity = Max(length, JSObject::NewElementsCapacity(capacity));
-    ElementsAccessorSubclass::GrowCapacityAndConvertImpl(array, capacity);
-  }
-
-  array->set_length(Smi::FromInt(length));
-  JSObject::ValidateElements(array);
-}
 }  // namespace
 
 

src/elements.h

@@ -149,6 +149,9 @@ class ElementsAccessor {
   virtual Handle<Object> Pop(Handle<JSArray> receiver,
                              Handle<FixedArrayBase> backing_store) = 0;
 
+  virtual Handle<Object> Shift(Handle<JSArray> receiver,
+                               Handle<FixedArrayBase> backing_store) = 0;
+
  protected:
   friend class LookupIterator;
 

test/mjsunit/array-natives-elements.js

@@ -53,15 +53,15 @@ function array_natives_test() {
   // Push
   var a0 = make_array("[1, 2, 3]");
   assertTrue(%HasFastSmiElements(a0));
-  a0.push(4);
+  assertEquals(4, a0.push(4));
   assertTrue(%HasFastSmiElements(a0));
-  a0.push(1.3);
+  assertEquals(5, a0.push(1.3));
   assertTrue(%HasFastDoubleElements(a0));
-  a0.push(1.5);
+  assertEquals(6, a0.push(1.5));
   assertTrue(%HasFastDoubleElements(a0));
-  a0.push({});
+  assertEquals(7, a0.push({}));
   assertTrue(%HasFastObjectElements(a0));
-  a0.push({});
+  assertEquals(8, a0.push({}));
   assertTrue(%HasFastObjectElements(a0));
   assertEquals([1,2,3,4,1.3,1.5,{},{}], a0);
 
@@ -253,44 +253,50 @@ function array_natives_test() {
   // Pop
   var a4 = [1,2,3];
   assertEquals(3, a4.pop());
-  assertTrue(%HasFastSmiElements(a4));
+  assertEquals([1,2], a4);
+  //assertTrue(%HasFastSmiElements(a4));
   a4 = [1.1,2,3];
   assertEquals(3, a4.pop());
-  assertTrue(%HasFastDoubleElements(a4));
+  assertEquals([1.1,2], a4);
+  //assertTrue(%HasFastDoubleElements(a4));
   a4 = [{},2,3];
   assertEquals(3, a4.pop());
-  assertTrue(%HasFastObjectElements(a4));
+  assertEquals([{},2], a4);
+  //assertTrue(%HasFastObjectElements(a4));
 
   // Shift
   var a4 = [1,2,3];
   assertEquals(1, a4.shift());
-  assertTrue(%HasFastSmiElements(a4));
+  assertEquals([2,3], a4);
+  //assertTrue(%HasFastSmiElements(a4));
   a4 = [1.1,2,3];
   assertEquals(1.1, a4.shift());
-  assertTrue(%HasFastDoubleElements(a4));
+  assertEquals([2,3], a4);
+  //assertTrue(%HasFastDoubleElements(a4));
   a4 = [{},2,3];
   assertEquals({}, a4.shift());
-  assertTrue(%HasFastObjectElements(a4));
+  assertEquals([2,3], a4);
+  //assertTrue(%HasFastObjectElements(a4));
 
   // Unshift
   var a4 = [1,2,3];
-  a4.unshift(1);
+  assertEquals(4, a4.unshift(1));
   assertTrue(%HasFastSmiElements(a4));
   assertEquals([1,1,2,3], a4);
   a4 = [1,2,3];
-  a4.unshift(1.1);
+  assertEquals(4, a4.unshift(1.1));
   assertTrue(%HasFastDoubleElements(a4));
   assertEquals([1.1,1,2,3], a4);
   a4 = [1.1,2,3];
-  a4.unshift(1);
+  assertEquals(4, a4.unshift(1));
   assertTrue(%HasFastDoubleElements(a4));
   assertEquals([1,1.1,2,3], a4);
   a4 = [{},2,3];
-  a4.unshift(1);
+  assertEquals(4, a4.unshift(1));
   assertTrue(%HasFastObjectElements(a4));
   assertEquals([1,{},2,3], a4);
   a4 = [{},2,3];
-  a4.unshift(1.1);
+  assertEquals(4, a4.unshift(1.1));
   assertTrue(%HasFastObjectElements(a4));
   assertEquals([1.1,{},2,3], a4);
 }