Revert of Moving ArraySplice Builtin to ElementsAccessor

Revert of Moving ArraySplice Builtin to ElementsAccessor (patchset #8 id:140001 of https://codereview.chromium.org/1293683005/ )

Reason for revert:
Fails layout tests: http://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2032/builds/1450

Original issue's description:
> - remove the Backing-Store specific code from builtins.cc and put it in elements.cc.
> - adding tests to improve coverage of the splice method
>
> BUG=
>
> Committed: https://crrev.com/8533d4b5433d3a9e9fb1015f206997bd6d869fe3
> Cr-Commit-Position: refs/heads/master@{#30269}
>
> Committed: https://crrev.com/07a4a6cb8e2ab940b28a7151a925c796da023524
> Cr-Commit-Position: refs/heads/master@{#30326}

TBR=mvstanton@chromium.org,cbruni@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Committed: https://crrev.com/cfdcc874a99846592f41478112613d5da0c07018
Cr-Commit-Position: refs/heads/master@{#30351}

[Michael Hablich, 2015-08-25 09:51:13]

Created Revert of Moving ArraySplice Builtin to ElementsAccessor

[commit-bot: I haz the power, 2015-08-25 09:51:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315823004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315823004/1

[commit-bot: I haz the power, 2015-08-25 09:51:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-25 09:51:32]

Failed to apply patch for src/builtins.cc:
While running git apply --index -3 -p1;
  error: patch failed: src/builtins.cc:176
  error: repository lacks the necessary blob to fall back on 3-way merge.
  error: src/builtins.cc: patch does not apply

Patch:       src/builtins.cc
Index: src/builtins.cc
diff --git a/src/builtins.cc b/src/builtins.cc
index
8373f73d1e84da61d800af67cef7bcb7cd320ddd..6b0b2229cdfc8d500feb69ef3622ae97325bccdf
100644
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -176,27 +176,6 @@
 }
 
 
-// TODO(cbruni): check if this is a suitable method on Object
-bool ClampedToInteger(Object* object, int* out) {
-  // This is an extended version of ECMA-262 9.4, but additionally
-  // clamps values to [kMinInt, kMaxInt]
-  if (object->IsSmi()) {
-    *out = Smi::cast(object)->value();
-    return true;
-  } else if (object->IsHeapNumber()) {
-    *out = FastD2IChecked(HeapNumber::cast(object)->value());
-    return true;
-  } else if (object->IsUndefined()) {
-    *out = 0;
-    return true;
-  } else if (object->IsBoolean()) {
-    *out = (Oddball::cast(object)->kind() == Oddball::kTrue) ? 1 : 0;
-    return true;
-  }
-  return false;
-}
-
-
 static void MoveDoubleElements(FixedDoubleArray* dst, int dst_index,
                                FixedDoubleArray* src, int src_index, int len) {
   if (len == 0) return;
@@ -642,6 +621,7 @@
 
 BUILTIN(ArraySplice) {
   HandleScope scope(isolate);
+  Heap* heap = isolate->heap();
   Handle<Object> receiver = args.receiver();
   MaybeHandle<FixedArrayBase> maybe_elms_obj =
       EnsureJSArrayWithWritableFastElements(isolate, receiver, &args, 3);
@@ -652,51 +632,209 @@
   Handle<JSArray> array = Handle<JSArray>::cast(receiver);
   DCHECK(!array->map()->is_observed());
 
-  int argument_count = args.length() - 1;
+  int len = Smi::cast(array->length())->value();
+
+  int n_arguments = args.length() - 1;
+
   int relative_start = 0;
-  if (argument_count > 0) {
+  if (n_arguments > 0) {
     DisallowHeapAllocation no_gc;
-    if (!ClampedToInteger(args[1], &relative_start)) {
+    Object* arg1 = args[1];
+    if (arg1->IsSmi()) {
+      relative_start = Smi::cast(arg1)->value();
+    } else if (arg1->IsHeapNumber()) {
+      double start = HeapNumber::cast(arg1)->value();
+      if (start < kMinInt || start > kMaxInt) {
+        AllowHeapAllocation allow_allocation;
+        return CallJsBuiltin(isolate, "$arraySplice", args);
+      }
+      relative_start = std::isnan(start) ? 0 : static_cast<int>(start);
+    } else if (!arg1->IsUndefined()) {
       AllowHeapAllocation allow_allocation;
       return CallJsBuiltin(isolate, "$arraySplice", args);
     }
   }
-  int len = Smi::cast(array->length())->value();
-  // clip relative start to [0, len]
   int actual_start = (relative_start < 0) ? Max(len + relative_start, 0)
                                           : Min(relative_start, len);
 
+  // SpiderMonkey, TraceMonkey and JSC treat the case where no delete count is
+  // given as a request to delete all the elements from the start.
+  // And it differs from the case of undefined delete count.
+  // This does not follow ECMA-262, but we do the same for
+  // compatibility.
   int actual_delete_count;
-  if (argument_count == 1) {
-    // SpiderMonkey, TraceMonkey and JSC treat the case where no delete count
is
-    // given as a request to delete all the elements from the start.
-    // And it differs from the case of undefined delete count.
-    // This does not follow ECMA-262, but we do the same for compatibility.
+  if (n_arguments == 1) {
     DCHECK(len - actual_start >= 0);
     actual_delete_count = len - actual_start;
   } else {
-    int delete_count = 0;
-    DisallowHeapAllocation no_gc;
-    if (argument_count > 1) {
-      if (!ClampedToInteger(args[2], &delete_count)) {
+    int value = 0;  // ToInteger(undefined) == 0
+    if (n_arguments > 1) {
+      DisallowHeapAllocation no_gc;
+      Object* arg2 = args[2];
+      if (arg2->IsSmi()) {
+        value = Smi::cast(arg2)->value();
+      } else {
         AllowHeapAllocation allow_allocation;
         return CallJsBuiltin(isolate, "$arraySplice", args);
       }
     }
-    actual_delete_count = Min(Max(delete_count, 0), len - actual_start);
-  }
-
-  int add_count = (argument_count > 1) ? (argument_count - 2) : 0;
-  int new_length = len - actual_delete_count + add_count;
+    actual_delete_count = Min(Max(value, 0), len - actual_start);
+  }
+
+  ElementsKind elements_kind = array->GetElementsKind();
+
+  int item_count = (n_arguments > 1) ? (n_arguments - 2) : 0;
+  int new_length = len - actual_delete_count + item_count;
+
+  // For double mode we do not support changing the length.
+  if (new_length > len && IsFastDoubleElementsKind(elements_kind)) {
+    return CallJsBuiltin(isolate, "$arraySplice", args);
+  }
 
   if (new_length != len && JSArray::HasReadOnlyLength(array)) {
     AllowHeapAllocation allow_allocation;
     return CallJsBuiltin(isolate, "$arraySplice", args);
   }
-  ElementsAccessor* accessor = array->GetElementsAccessor();
-  Handle<JSArray> result = accessor->Splice(
-      array, elms_obj, actual_start, actual_delete_count, args, add_count);
-  return *result;
+
+  if (new_length == 0) {
+    Handle<JSArray> result = isolate->factory()->NewJSArrayWithElements(
+        elms_obj, elements_kind, actual_delete_count);
+    array->set_elements(heap->empty_fixed_array());
+    array->set_length(Smi::FromInt(0));
+    return *result;
+  }
+
+  Handle<JSArray> result_array =
+      isolate->factory()->NewJSArray(elements_kind,
+                                     actual_delete_count,
+                                     actual_delete_count);
+
+  if (actual_delete_count > 0) {
+    DisallowHeapAllocation no_gc;
+    ElementsAccessor* accessor = array->GetElementsAccessor();
+    accessor->CopyElements(
+        elms_obj, actual_start, elements_kind,
+        handle(result_array->elements(), isolate), 0, actual_delete_count);
+  }
+
+  bool elms_changed = false;
+  if (item_count < actual_delete_count) {
+    // Shrink the array.
+    const bool trim_array = !heap->lo_space()->Contains(*elms_obj) &&
+      ((actual_start + item_count) <
+          (len - actual_delete_count - actual_start));
+    if (trim_array) {
+      const int delta = actual_delete_count - item_count;
+
+      if (elms_obj->IsFixedDoubleArray()) {
+        Handle<FixedDoubleArray> elms =
+            Handle<FixedDoubleArray>::cast(elms_obj);
+        MoveDoubleElements(*elms, delta, *elms, 0, actual_start);
+      } else {
+        Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
+        DisallowHeapAllocation no_gc;
+        heap->MoveElements(*elms, delta, 0, actual_start);
+      }
+
+      if (heap->CanMoveObjectStart(*elms_obj)) {
+        // On the fast path we move the start of the object in memory.
+        elms_obj = handle(heap->LeftTrimFixedArray(*elms_obj, delta));
+      } else {
+        // This is the slow path. We are going to move the elements to the left
+        // by copying them. For trimmed values we store the hole.
+        if (elms_obj->IsFixedDoubleArray()) {
+          Handle<FixedDoubleArray> elms =
+              Handle<FixedDoubleArray>::cast(elms_obj);
+          MoveDoubleElements(*elms, 0, *elms, delta, len - delta);
+          elms->FillWithHoles(len - delta, len);
+        } else {
+          Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
+          DisallowHeapAllocation no_gc;
+          heap->MoveElements(*elms, 0, delta, len - delta);
+          elms->FillWithHoles(len - delta, len);
+        }
+      }
+      elms_changed = true;
+    } else {
+      if (elms_obj->IsFixedDoubleArray()) {
+        Handle<FixedDoubleArray> elms =
+            Handle<FixedDoubleArray>::cast(elms_obj);
+        MoveDoubleElements(*elms, actual_start + item_count,
+                           *elms, actual_start + actual_delete_count,
+                           (len - actual_delete_count - actual_start));
+        elms->FillWithHoles(new_length, len);
+      } else {
+        Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
+        DisallowHeapAllocation no_gc;
+        heap->MoveElements(*elms, actual_start + item_count,
+                           actual_start + actual_delete_count,
+                           (len - actual_delete_count - actual_start));
+        elms->FillWithHoles(new_length, len);
+      }
+    }
+  } else if (item_count > actual_delete_count) {
+    Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
+    // Currently fixed arrays cannot grow too big, so
+    // we should never hit this case.
+    DCHECK((item_count - actual_delete_count) <= (Smi::kMaxValue - len));
+
+    // Check if array need to grow.
+    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);
+
+      DisallowHeapAllocation no_gc;
+
+      ElementsKind kind = array->GetElementsKind();
+      ElementsAccessor* accessor = array->GetElementsAccessor();
+      if (actual_start > 0) {
+        // Copy the part before actual_start as is.
+        accessor->CopyElements(
+            elms, 0, kind, new_elms, 0, actual_start);
+      }
+      accessor->CopyElements(
+          elms, actual_start + actual_delete_count, kind,
+          new_elms, actual_start + item_count,
+          ElementsAccessor::kCopyToEndAndInitializeToHole);
+
+      elms_obj = new_elms;
+      elms_changed = true;
+    } else {
+      DisallowHeapAllocation no_gc;
+      heap->MoveElements(*elms, actual_start + item_count,
+                         actual_start + actual_delete_count,
+                         (len - actual_delete_count - actual_start));
+    }
+  }
+
+  if (IsFastDoubleElementsKind(elements_kind)) {
+    Handle<FixedDoubleArray> elms = Handle<FixedDo…
(message too large)

[Camillo Bruni, 2015-08-25 11:09:17]

cbruni@chromium.org changed reviewers:
+ hablich@chromium.org

[Camillo Bruni, 2015-08-25 11:18:17]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2015-08-25 11:18:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315823004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315823004/1

[commit-bot: I haz the power, 2015-08-25 11:18:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-08-25 11:18:49]

Patchset 1 (id:??) landed as
https://crrev.com/cfdcc874a99846592f41478112613d5da0c07018
Cr-Commit-Position: refs/heads/master@{#30351}

[Camillo Bruni, 2015-08-26 13:15:41]

On 2015/08/25 at 11:18:49, commit-bot wrote:
> Patchset 1 (id:??) landed as
https://crrev.com/cfdcc874a99846592f41478112613d5da0c07018
> Cr-Commit-Position: refs/heads/master@{#30351}

I rechecked again the blink layout tests and I could not reproduce the failures
with this CL.
However, https://codereview.chromium.org/1293683005 seems to consistently
reproduce it.