Array Builtin Refactoring: Creating API methods on ElementsAccessor

src/elements.cc

Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
index 7ef13b30dedc38511410ef9a5bfb91cdd0297f4b..ad66094ef83548a75401575b3b020692bba8c34e 100644
--- a/src/elements.cc
+++ b/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,22 @@ class ElementsAccessorBase : public ElementsAccessor {
     UNREACHABLE();
   }
 
+  virtual uint32_t Push(Handle<JSObject> receiver,
+                        Handle<FixedArrayBase> backing_store, Object** objects,
+                        uint32_t start, uint32_t push_size,
+                        uint32_t direction) {
+    return ElementsAccessorSubclass::PushImpl(receiver, backing_store, objects,
+                                              start, push_size, direction);
+  }
+
+  static uint32_t PushImpl(Handle<JSObject> receiver,
+                           Handle<FixedArrayBase> elms_obj, Object** objects,
+                           uint32_t start, uint32_t push_size,
+                           uint32_t direction) {
+    UNREACHABLE();
+    return 0;
+  }
+
   virtual void SetLength(Handle<JSArray> array, uint32_t length) final {
     ElementsAccessorSubclass::SetLengthImpl(array, length,
                                             handle(array->elements()));
@@ -1198,6 +1215,58 @@ class FastSmiOrObjectElementsAccessor
 #undef TYPED_ARRAY_CASE
     }
   }
+
+  static uint32_t PushImpl(Handle<JSObject> receiver,
+                           Handle<FixedArrayBase> backing_store,
+                           Object** objects, uint32_t start, uint32_t push_size,
+                           uint32_t direction) {
+    Handle<JSArray> array = Handle<JSArray>::cast(receiver);

[Jakob Kummerow, 2015/07/30 11:54:59]

Just take a Handle<JSArray> (instead of Handle<JSObject>) as a parameter.

+    int len = Smi::cast(array->length())->value();

[Jakob Kummerow, 2015/07/30 11:54:59]

Let's use the opportunity to clean up the types here. Use uint32_t for array
lengths, element counts, and capacities.

+    int elms_len = backing_store->length();
+    if (push_size == 0) {
+      return len;
+    }
+    // Currently fixed arrays cannot grow too big, so
+    // we should never hit this case.
+    DCHECK(static_cast<int>(push_size) <= (Smi::kMaxValue - len));

[Jakob Kummerow, 2015/07/30 11:54:59]

This should cast the other way round: 
DCHECK(push_size <= static_cast<uint32_t>(Smi::kMaxValue) - len);

+    int new_length = len + push_size;
+    Handle<FixedArrayBase> new_elms;
+
+    if (new_length > elms_len) {
+      // New backing storage is needed.
+      int capacity = new_length + (new_length >> 1) + 16;
+      new_elms = receiver->GetIsolate()->factory()->NewUninitializedFixedArray(

[Jakob Kummerow, 2015/07/30 11:54:59]

This should use ElementsAccessorBase::ConvertElementsWithCapacity to do the
heavy lifting.

+          capacity);
+      CopyElementsImpl(*backing_store, 0, *new_elms, KindTraits::Kind, 0,
+                       kPackedSizeNotKnown,
+                       ElementsAccessor::kCopyToEndAndInitializeToHole);
+    } else {
+      // to_add is > 0 and new_length <= elms_len, so elms_obj cannot be the
+      // empty_fixed_array.
+      new_elms = Handle<FixedArray>::cast(backing_store);
+    }
+
+    // Add the provided values.
+    DisallowHeapAllocation no_gc;
+    WriteBarrierMode mode = new_elms->GetWriteBarrierMode(no_gc);
+    Object* object;
+    int offset;
+    for (uint32_t index = 0; index < push_size; index++) {
+      if (direction == -1) {

[Jakob Kummerow, 2015/07/30 11:54:59]

s/-1/kDirectionReverse/

Or consider this:

STATIC_ASSERT(kDirectionForward == 1);
STATIC_ASSERT(kDirectionBackward == -1);
Object* object = objects[direction * index];

+        offset = 0 - index - start;
+      } else {
+        offset = index + start;
+      }
+      object = objects[offset];
+      Handle<FixedArray>::cast(new_elms)->set(index + len, object, mode);

[Jakob Kummerow, 2015/07/30 11:54:59]

This should use ElementsAccessorBase::SetImpl to abstract away the elements
kind.

+    }
+    if (*new_elms != array->elements()) {

[Jakob Kummerow, 2015/07/30 11:54:59]

if (!new_elms.is_identical_to(backing_store)) {

If you want, you can guard that with a DCHECK(array->elements() ==
*backing_store) to prevent API abuse.

+      array->set_elements(*new_elms);
+    }
+    // Set the length.
+    array->set_length(Smi::FromInt(new_length));
+    return new_length;
+  }
 };
 
 
@@ -1294,6 +1363,56 @@ class FastDoubleElementsAccessor
 #undef TYPED_ARRAY_CASE
     }
   }
+
+  static uint32_t PushImpl(Handle<JSObject> receiver,

[Jakob Kummerow, 2015/07/30 11:54:59]

This is not needed at all if you move the other implementation up into
FastElementsAccessor.

+                           Handle<FixedArrayBase> backing_store,
+                           Object** objects, uint32_t start, uint32_t push_size,
+                           uint32_t direction) {
+    Handle<JSArray> array = Handle<JSArray>::cast(receiver);
+    int len = Smi::cast(array->length())->value();
+    int elms_len = backing_store->length();
+    if (push_size == 0) {
+      return len;
+    }
+    // Currently fixed arrays cannot grow too big, so
+    // we should never hit this case.
+    DCHECK(static_cast<int>(push_size) <= (Smi::kMaxValue - len));
+    int new_length = len + push_size;
+    Handle<FixedArrayBase> new_elms;
+
+    if (new_length > elms_len) {
+      // New backing storage is needed.
+      int capacity = new_length + (new_length >> 1) + 16;
+      new_elms =
+          receiver->GetIsolate()->factory()->NewFixedDoubleArray(capacity);
+      CopyElementsImpl(*backing_store, 0, *new_elms, KindTraits::Kind, 0,
+                       kPackedSizeNotKnown,
+                       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(backing_store);
+    }
+
+    // Add the provided values.
+    DisallowHeapAllocation no_gc;
+    int offset;
+    for (uint32_t index = 0; index < push_size; index++) {
+      if (direction == ElementsAccessor::kDirectionReverse) {
+        offset = 0 - index - start;
+      } else {
+        offset = index + start;
+      }
+      Handle<FixedDoubleArray>::cast(new_elms)
+          ->set(index + len, objects[offset]->Number());
+    }
+    if (*new_elms != array->elements()) {
+      array->set_elements(*new_elms);
+    }
+    // Set the length.
+    array->set_length(Smi::FromInt(new_length));
+    return new_length;
+  }
 };