[elements] Enable left-trimming again

src/elements.cc

Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
index 28700b8f0abe31e90e32a0f2c6be65d787c915dd..46382a066cbaee5dc8e2a61c7c96180cdaaf7cb5 100644
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -677,11 +677,12 @@ class ElementsAccessorBase : public ElementsAccessor {
   }
 
   void SetLength(Handle<JSArray> array, uint32_t length) final {
-    ElementsAccessorSubclass::SetLengthImpl(array, length,
+    ElementsAccessorSubclass::SetLengthImpl(array->GetIsolate(), array, length,
                                             handle(array->elements()));
   }
 
-  static void SetLengthImpl(Handle<JSArray> array, uint32_t length,
+  static void SetLengthImpl(Isolate* isolate, Handle<JSArray> array,
+                            uint32_t length,
                             Handle<FixedArrayBase> backing_store) {
     DCHECK(!array->SetLengthWouldNormalize(length));
     DCHECK(IsFastElementsKind(array->GetElementsKind()));
@@ -698,6 +699,7 @@ class ElementsAccessorBase : public ElementsAccessor {
 
     // Check whether the backing store should be shrunk.
     uint32_t capacity = backing_store->length();
+    old_length = Min(old_length, capacity);
     if (length == 0) {
       array->initialize_elements();
     } else if (length <= capacity) {
@@ -706,7 +708,7 @@ class ElementsAccessorBase : public ElementsAccessor {
       }
       if (2 * length <= capacity) {
         // If more than half the elements won't be used, trim the array.
-        array->GetHeap()->RightTrimFixedArray<Heap::CONCURRENT_TO_SWEEPER>(
+        isolate->heap()->RightTrimFixedArray<Heap::CONCURRENT_TO_SWEEPER>(
             *backing_store, capacity - length);
       } else {
         // Otherwise, fill the unused tail with holes.
@@ -972,11 +974,11 @@ class DictionaryElementsAccessor
       : ElementsAccessorBase<DictionaryElementsAccessor,
                              ElementsKindTraits<DICTIONARY_ELEMENTS> >(name) {}
 
-  static void SetLengthImpl(Handle<JSArray> array, uint32_t length,
+  static void SetLengthImpl(Isolate* isolate, Handle<JSArray> array,
+                            uint32_t length,
                             Handle<FixedArrayBase> backing_store) {
     Handle<SeededNumberDictionary> dict =
         Handle<SeededNumberDictionary>::cast(backing_store);
-    Isolate* isolate = array->GetIsolate();
     int capacity = dict->Capacity();
     uint32_t old_length = 0;
     CHECK(array->length()->ToArrayLength(&old_length));
@@ -1352,10 +1354,12 @@ class FastElementsAccessor
         receiver, backing_store, args, unshift_size, AT_START);
   }
 
-  static void MoveElements(Heap* heap, Handle<FixedArrayBase> backing_store,
-                           int dst_index, int src_index, int len,
-                           int hole_start, int hole_end) {
+  static Handle<FixedArrayBase> MoveElements(
+      Isolate* isolate, Handle<JSArray> receiver,
+      Handle<FixedArrayBase> backing_store, int dst_index, int src_index,
+      int len, int hole_start, int hole_end) {
     UNREACHABLE();
+    return backing_store;
   }
 
   static Handle<JSArray> SliceImpl(Handle<JSObject> receiver,
@@ -1403,13 +1407,13 @@ class FastElementsAccessor
 
     // Delete and move elements to make space for add_count new elements.
     if (add_count < delete_count) {
-      FastElementsAccessorSubclass::SpliceShrinkStep(backing_store, heap, start,
-                                                     delete_count, add_count,
-                                                     length, new_length);
+      backing_store = FastElementsAccessorSubclass::SpliceShrinkStep(
+          isolate, receiver, backing_store, start, delete_count, add_count,
+          length, new_length);
     } else if (add_count > delete_count) {
       backing_store = FastElementsAccessorSubclass::SpliceGrowStep(
-          receiver, backing_store, isolate, heap, start, delete_count,
-          add_count, length, new_length);
+          isolate, receiver, backing_store, start, delete_count, add_count,
+          length, new_length);
     }
 
     // Copy over the arguments.
@@ -1423,30 +1427,31 @@ class FastElementsAccessor
   }
 
  private:
-  static void SpliceShrinkStep(Handle<FixedArrayBase> backing_store, Heap* heap,
-                               uint32_t start, uint32_t delete_count,
-                               uint32_t add_count, uint32_t len,
-                               uint32_t new_length) {
+  static Handle<FixedArrayBase> SpliceShrinkStep(
+      Isolate* isolate, Handle<JSArray> receiver,
+      Handle<FixedArrayBase> backing_store, uint32_t start,
+      uint32_t delete_count, uint32_t add_count, uint32_t len,
+      uint32_t new_length) {
     const int move_left_count = len - delete_count - start;
     const int move_left_dst_index = start + add_count;
-    FastElementsAccessorSubclass::MoveElements(
-        heap, backing_store, move_left_dst_index, start + delete_count,
-        move_left_count, new_length, len);
+    return FastElementsAccessorSubclass::MoveElements(
+        isolate, receiver, backing_store, move_left_dst_index,
+        start + delete_count, move_left_count, new_length, len);
   }
 
 
   static Handle<FixedArrayBase> SpliceGrowStep(
-      Handle<JSArray> receiver, Handle<FixedArrayBase> backing_store,
-      Isolate* isolate, Heap* heap, uint32_t start, uint32_t delete_count,
-      uint32_t add_count, uint32_t length, uint32_t new_length) {
+      Isolate* isolate, Handle<JSArray> receiver,
+      Handle<FixedArrayBase> backing_store, uint32_t start,
+      uint32_t delete_count, uint32_t add_count, uint32_t length,
+      uint32_t new_length) {
     // Check we do not overflow the new_length.
     DCHECK((add_count - delete_count) <= (Smi::kMaxValue - length));
     // Check if backing_store is big enough.
     if (new_length <= static_cast<uint32_t>(backing_store->length())) {
-      FastElementsAccessorSubclass::MoveElements(
-          heap, backing_store, start + add_count, start + delete_count,
-          (length - delete_count - start), 0, 0);
-      return backing_store;
+      return FastElementsAccessorSubclass::MoveElements(
+          isolate, receiver, backing_store, start + add_count,
+          start + delete_count, (length - delete_count - start), 0, 0);
     }
     // New backing storage is needed.
     int capacity = JSObject::NewElementsCapacity(new_length);
@@ -1466,20 +1471,19 @@ class FastElementsAccessor
   static Handle<Object> RemoveElement(Handle<JSArray> receiver,
                                       Handle<FixedArrayBase> backing_store,
                                       Where remove_position) {
+    Isolate* isolate = receiver->GetIsolate();
     uint32_t length =
         static_cast<uint32_t>(Smi::cast(receiver->length())->value());
-    Isolate* isolate = receiver->GetIsolate();
     DCHECK(length > 0);
     int new_length = length - 1;
     int remove_index = remove_position == AT_START ? 0 : new_length;
     Handle<Object> result =
         FastElementsAccessorSubclass::GetImpl(backing_store, remove_index);
     if (remove_position == AT_START) {
-      Heap* heap = isolate->heap();
-      FastElementsAccessorSubclass::MoveElements(heap, backing_store, 0, 1,
-                                                 new_length, 0, 0);
+      backing_store = FastElementsAccessorSubclass::MoveElements(
+          isolate, receiver, backing_store, 0, 1, new_length, 0, 0);
     }
-    FastElementsAccessorSubclass::SetLengthImpl(receiver, new_length,
+    FastElementsAccessorSubclass::SetLengthImpl(isolate, receiver, new_length,
                                                 backing_store);
 
     if (IsHoleyElementsKind(KindTraits::Kind) && result->IsTheHole()) {
@@ -1513,8 +1517,8 @@ class FastElementsAccessor
       // If the backing store has enough capacity and we add elements to the
       // start we have to shift the existing objects.
       Isolate* isolate = receiver->GetIsolate();
-      FastElementsAccessorSubclass::MoveElements(isolate->heap(), backing_store,
-                                                 add_size, 0, length, 0, 0);
+      backing_store = FastElementsAccessorSubclass::MoveElements(
+          isolate, receiver, backing_store, add_size, 0, length, 0, 0);
     }
 
     int insertion_index = remove_position == AT_START ? 0 : length;
@@ -1567,17 +1571,26 @@ class FastSmiOrObjectElementsAccessor
     return backing_store->get(index);
   }
 
-  static void MoveElements(Heap* heap, Handle<FixedArrayBase> backing_store,
-                           int dst_index, int src_index, int len,
-                           int hole_start, int hole_end) {
+  static Handle<FixedArrayBase> MoveElements(
+      Isolate* isolate, Handle<JSArray> receiver,
+      Handle<FixedArrayBase> backing_store, int dst_index, int src_index,
+      int len, int hole_start, int hole_end) {
+    Heap* heap = isolate->heap();
     Handle<FixedArray> dst_elms = Handle<FixedArray>::cast(backing_store);
-    if (len != 0) {
+    if (heap->CanMoveObjectStart(*dst_elms) && dst_index == 0) {
+      dst_elms = Handle<FixedArray>(

[Igor Sheludko, 2015/12/22 10:02:36]

Since
1) the left trimming is not "safe" in a sense that we would crash anyway if we
ever create a handle to untrimmed object before entering elements accessors and
then try to use it after left trimming,
2) the approach to return an updated handle from MoveElements does not solve (1)
in general,
maybe it is better just to update the existing backing_store handle and keep the
signature of MoveElements unchanged.

+          FixedArray::cast(heap->LeftTrimFixedArray(*dst_elms, src_index)),
+          isolate);
+      receiver->set_elements(*dst_elms);
+      hole_end -= src_index;

[Igor Sheludko, 2015/12/22 10:02:36]

Shouldn't we update hole_start here as well?

[Camillo Bruni, 2015/12/22 14:21:01]

I added a DCHECK, the hole_start has to lie within length-removed elements (aka.
in the worst case right at the end of storage if we only trim).

+    } else if (len != 0) {
       DisallowHeapAllocation no_gc;
       heap->MoveElements(*dst_elms, dst_index, src_index, len);
     }
     if (hole_start != hole_end) {
       dst_elms->FillWithHoles(hole_start, hole_end);
     }
+    return dst_elms;
   }
 
   // NOTE: this method violates the handlified function signature convention:
@@ -1690,18 +1703,28 @@ class FastDoubleElementsAccessor
     FixedDoubleArray::cast(backing_store)->set(entry, value->Number());
   }
 
-  static void MoveElements(Heap* heap, Handle<FixedArrayBase> backing_store,
-                           int dst_index, int src_index, int len,
-                           int hole_start, int hole_end) {
+  static Handle<FixedArrayBase> MoveElements(
+      Isolate* isolate, Handle<JSArray> receiver,
+      Handle<FixedArrayBase> backing_store, int dst_index, int src_index,
+      int len, int hole_start, int hole_end) {
+    Heap* heap = isolate->heap();
     Handle<FixedDoubleArray> dst_elms =
         Handle<FixedDoubleArray>::cast(backing_store);
-    if (len != 0) {
+    if (heap->CanMoveObjectStart(*dst_elms) && dst_index == 0) {
+      dst_elms = Handle<FixedDoubleArray>(

[Igor Sheludko, 2015/12/22 10:02:36]

Ditto...

+          FixedDoubleArray::cast(
+              heap->LeftTrimFixedArray(*dst_elms, src_index)),
+          isolate);
+      receiver->set_elements(*dst_elms);
+      hole_end -= src_index;

[Igor Sheludko, 2015/12/22 10:02:36]

Ditto...

+    } else if (len != 0) {
       MemMove(dst_elms->data_start() + dst_index,
               dst_elms->data_start() + src_index, len * kDoubleSize);
     }
     if (hole_start != hole_end) {
       dst_elms->FillWithHoles(hole_start, hole_end);
     }
+    return dst_elms;
   }
 
   static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
@@ -1801,7 +1824,8 @@ class TypedElementsAccessor
     return PropertyDetails(DONT_DELETE, DATA, 0, PropertyCellType::kNoCell);
   }
 
-  static void SetLengthImpl(Handle<JSArray> array, uint32_t length,
+  static void SetLengthImpl(Isolate* isolate, Handle<JSArray> array,
+                            uint32_t length,
                             Handle<FixedArrayBase> backing_store) {
     // External arrays do not support changing their length.
     UNREACHABLE();
@@ -1915,7 +1939,8 @@ class SloppyArgumentsElementsAccessor
     }
   }
 
-  static void SetLengthImpl(Handle<JSArray> array, uint32_t length,
+  static void SetLengthImpl(Isolate* isolate, Handle<JSArray> array,
+                            uint32_t length,
                             Handle<FixedArrayBase> parameter_map) {
     // Sloppy arguments objects are not arrays.
     UNREACHABLE();