ArrayConcat regression recover after r20312 (appeared on dromaeo benchmarks).

BUG=chromium:358561
LOG=N
R=yangguo@chromium.org

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

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24269 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/builtins.cc b/src/builtins.cc
index f921640..c52d228 100644 (file)
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -987,12 +987,12 @@ BUILTIN(ArrayConcat) {
   Handle<FixedArrayBase> storage(result_array->elements(), isolate);
   ElementsAccessor* accessor = ElementsAccessor::ForKind(elements_kind);
   for (int i = 0; i < n_arguments; i++) {
-    // TODO(ishell): It is crucial to keep |array| as a raw pointer to avoid
-    // performance degradation. Revisit this later.
+    // It is crucial to keep |array| in a raw pointer form to avoid performance
+    // degradation.
     JSArray* array = JSArray::cast(args[i]);
     int len = Smi::cast(array->length())->value();
-    ElementsKind from_kind = array->GetElementsKind();
     if (len > 0) {
+      ElementsKind from_kind = array->GetElementsKind();
       accessor->CopyElements(array, 0, from_kind, storage, j, len);
       j += len;
     }

diff --git a/src/elements.cc b/src/elements.cc
index e2127c4..abb0467 100644 (file)
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -247,15 +247,18 @@ static void CopyDictionaryToObjectElements(
 }
 
 
-static void CopyDoubleToObjectElements(Handle<FixedArrayBase> from_base,
+// NOTE: this method violates the handlified function signature convention:
+// raw pointer parameters in the function that allocates.
+// See ElementsAccessorBase::CopyElements() for details.
+static void CopyDoubleToObjectElements(FixedArrayBase* from_base,
                                        uint32_t from_start,
-                                       Handle<FixedArrayBase> to_base,
-                                       ElementsKind to_kind,
-                                       uint32_t to_start,
+                                       FixedArrayBase* to_base,
+                                       ElementsKind to_kind, uint32_t to_start,
                                        int raw_copy_size) {
   DCHECK(IsFastSmiOrObjectElementsKind(to_kind));
   int copy_size = raw_copy_size;
   if (raw_copy_size < 0) {
+    DisallowHeapAllocation no_allocation;
     DCHECK(raw_copy_size == ElementsAccessor::kCopyToEnd ||
            raw_copy_size == ElementsAccessor::kCopyToEndAndInitializeToHole);
     copy_size = Min(from_base->length() - from_start,
@@ -268,7 +271,7 @@ static void CopyDoubleToObjectElements(Handle<FixedArrayBase> from_base,
       int length = to_base->length() - start;
       if (length > 0) {
         Heap* heap = from_base->GetHeap();
-        MemsetPointer(FixedArray::cast(*to_base)->data_start() + start,
+        MemsetPointer(FixedArray::cast(to_base)->data_start() + start,
                       heap->the_hole_value(), length);
       }
     }
@@ -276,9 +279,12 @@ static void CopyDoubleToObjectElements(Handle<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;
+
+  // From here on, the code below could actually allocate. Therefore the raw
+  // values are wrapped into handles.
   Isolate* isolate = from_base->GetIsolate();
-  Handle<FixedDoubleArray> from = Handle<FixedDoubleArray>::cast(from_base);
-  Handle<FixedArray> to = Handle<FixedArray>::cast(to_base);
+  Handle<FixedDoubleArray> from(FixedDoubleArray::cast(from_base), isolate);
+  Handle<FixedArray> to(FixedArray::cast(to_base), isolate);
   for (int i = 0; i < copy_size; ++i) {
     HandleScope scope(isolate);
     if (IsFastSmiElementsKind(to_kind)) {
@@ -702,12 +708,9 @@ class ElementsAccessorBase : public ElementsAccessor {
       uint32_t key,
       JSReceiver::DeleteMode mode) OVERRIDE = 0;
 
-  static void CopyElementsImpl(Handle<FixedArrayBase> from,
-                               uint32_t from_start,
-                               Handle<FixedArrayBase> to,
-                               ElementsKind from_kind,
-                               uint32_t to_start,
-                               int packed_size,
+  static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
+                               FixedArrayBase* to, ElementsKind from_kind,
+                               uint32_t to_start, int packed_size,
                                int copy_size) {
     UNREACHABLE();
   }
@@ -720,9 +723,15 @@ class ElementsAccessorBase : public ElementsAccessor {
       uint32_t to_start,
       int copy_size) FINAL OVERRIDE {
     DCHECK(!from.is_null());
-    ElementsAccessorSubclass::CopyElementsImpl(
-        from, from_start, to, from_kind, to_start, kPackedSizeNotKnown,
-        copy_size);
+    // NOTE: the ElementsAccessorSubclass::CopyElementsImpl() methods
+    // violate the handlified function signature convention:
+    // raw pointer parameters in the function that allocates. This is done
+    // intentionally to avoid ArrayConcat() builtin performance degradation.
+    // See the comment in another ElementsAccessorBase::CopyElements() for
+    // details.
+    ElementsAccessorSubclass::CopyElementsImpl(*from, from_start, *to,
+                                               from_kind, to_start,
+                                               kPackedSizeNotKnown, copy_size);
   }
 
   virtual void CopyElements(
@@ -742,9 +751,18 @@ class ElementsAccessorBase : public ElementsAccessor {
         packed_size = copy_size;
       }
     }
-    Handle<FixedArrayBase> from(from_holder->elements());
+    FixedArrayBase* from = from_holder->elements();
+    // NOTE: the ElementsAccessorSubclass::CopyElementsImpl() methods
+    // violate the handlified function signature convention:
+    // raw pointer parameters in the function that allocates. This is done
+    // intentionally to avoid ArrayConcat() builtin performance degradation.
+    //
+    // Details: The idea is that allocations actually happen only in case of
+    // copying from object with fast double elements to object with object
+    // elements. In all the other cases there are no allocations performed and
+    // handle creation causes noticeable performance degradation of the builtin.
     ElementsAccessorSubclass::CopyElementsImpl(
-        from, from_start, to, from_kind, to_start, packed_size, copy_size);
+        from, from_start, *to, from_kind, to_start, packed_size, copy_size);
   }
 
   virtual MaybeHandle<FixedArray> AddElementsToFixedArray(
@@ -1018,7 +1036,7 @@ class FastElementsAccessor
 };
 
 
-static inline ElementsKind ElementsKindForArray(Handle<FixedArrayBase> array) {
+static inline ElementsKind ElementsKindForArray(FixedArrayBase* array) {
   switch (array->map()->instance_type()) {
     case FIXED_ARRAY_TYPE:
       if (array->IsDictionary()) {
@@ -1054,38 +1072,42 @@ class FastSmiOrObjectElementsAccessor
       : FastElementsAccessor<FastElementsAccessorSubclass,
                              KindTraits>(name) {}
 
-  static void CopyElementsImpl(Handle<FixedArrayBase> from,
-                               uint32_t from_start,
-                               Handle<FixedArrayBase> to,
-                               ElementsKind from_kind,
-                               uint32_t to_start,
-                               int packed_size,
+  // NOTE: this method violates the handlified function signature convention:
+  // raw pointer parameters in the function that allocates.
+  // See ElementsAccessor::CopyElements() for details.
+  // This method could actually allocate if copying from double elements to
+  // object elements.
+  static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
+                               FixedArrayBase* to, ElementsKind from_kind,
+                               uint32_t to_start, int packed_size,
                                int copy_size) {
+    DisallowHeapAllocation no_gc;
     ElementsKind to_kind = KindTraits::Kind;
     switch (from_kind) {
       case FAST_SMI_ELEMENTS:
       case FAST_HOLEY_SMI_ELEMENTS:
       case FAST_ELEMENTS:
       case FAST_HOLEY_ELEMENTS:
-        CopyObjectToObjectElements(*from, from_kind, from_start, *to, to_kind,
+        CopyObjectToObjectElements(from, from_kind, from_start, to, to_kind,
                                    to_start, copy_size);
         break;
       case FAST_DOUBLE_ELEMENTS:
-      case FAST_HOLEY_DOUBLE_ELEMENTS:
+      case FAST_HOLEY_DOUBLE_ELEMENTS: {
+        AllowHeapAllocation allow_allocation;
         CopyDoubleToObjectElements(
             from, from_start, to, to_kind, to_start, copy_size);
         break;
+      }
       case DICTIONARY_ELEMENTS:
-        CopyDictionaryToObjectElements(*from, from_start, *to, to_kind,
-                                       to_start, copy_size);
+        CopyDictionaryToObjectElements(from, from_start, to, to_kind, to_start,
+                                       copy_size);
         break;
       case SLOPPY_ARGUMENTS_ELEMENTS: {
         // TODO(verwaest): This is a temporary hack to support extending
         // SLOPPY_ARGUMENTS_ELEMENTS in SetFastElementsCapacityAndLength.
         // This case should be UNREACHABLE().
-        Handle<FixedArray> parameter_map = Handle<FixedArray>::cast(from);
-        Handle<FixedArrayBase> arguments(
-            FixedArrayBase::cast(parameter_map->get(1)));
+        FixedArray* parameter_map = FixedArray::cast(from);
+        FixedArrayBase* arguments = FixedArrayBase::cast(parameter_map->get(1));
         ElementsKind from_kind = ElementsKindForArray(arguments);
         CopyElementsImpl(arguments, from_start, to, from_kind,
                          to_start, packed_size, copy_size);
@@ -1179,31 +1201,29 @@ class FastDoubleElementsAccessor
   }
 
  protected:
-  static void CopyElementsImpl(Handle<FixedArrayBase> from,
-                               uint32_t from_start,
-                               Handle<FixedArrayBase> to,
-                               ElementsKind from_kind,
-                               uint32_t to_start,
-                               int packed_size,
+  static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
+                               FixedArrayBase* to, ElementsKind from_kind,
+                               uint32_t to_start, int packed_size,
                                int copy_size) {
+    DisallowHeapAllocation no_allocation;
     switch (from_kind) {
       case FAST_SMI_ELEMENTS:
-        CopyPackedSmiToDoubleElements(*from, from_start, *to, to_start,
+        CopyPackedSmiToDoubleElements(from, from_start, to, to_start,
                                       packed_size, copy_size);
         break;
       case FAST_HOLEY_SMI_ELEMENTS:
-        CopySmiToDoubleElements(*from, from_start, *to, to_start, copy_size);
+        CopySmiToDoubleElements(from, from_start, to, to_start, copy_size);
         break;
       case FAST_DOUBLE_ELEMENTS:
       case FAST_HOLEY_DOUBLE_ELEMENTS:
-        CopyDoubleToDoubleElements(*from, from_start, *to, to_start, copy_size);
+        CopyDoubleToDoubleElements(from, from_start, to, to_start, copy_size);
         break;
       case FAST_ELEMENTS:
       case FAST_HOLEY_ELEMENTS:
-        CopyObjectToDoubleElements(*from, from_start, *to, to_start, copy_size);
+        CopyObjectToDoubleElements(from, from_start, to, to_start, copy_size);
         break;
       case DICTIONARY_ELEMENTS:
-        CopyDictionaryToDoubleElements(*from, from_start, *to, to_start,
+        CopyDictionaryToDoubleElements(from, from_start, to, to_start,
                                        copy_size);
         break;
       case SLOPPY_ARGUMENTS_ELEMENTS:
@@ -1439,12 +1459,9 @@ class DictionaryElementsAccessor
     return isolate->factory()->true_value();
   }
 
-  static void CopyElementsImpl(Handle<FixedArrayBase> from,
-                               uint32_t from_start,
-                               Handle<FixedArrayBase> to,
-                               ElementsKind from_kind,
-                               uint32_t to_start,
-                               int packed_size,
+  static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
+                               FixedArrayBase* to, ElementsKind from_kind,
+                               uint32_t to_start, int packed_size,
                                int copy_size) {
     UNREACHABLE();
   }
@@ -1654,12 +1671,9 @@ class SloppyArgumentsElementsAccessor : public ElementsAccessorBase<
     return isolate->factory()->true_value();
   }
 
-  static void CopyElementsImpl(Handle<FixedArrayBase> from,
-                               uint32_t from_start,
-                               Handle<FixedArrayBase> to,
-                               ElementsKind from_kind,
-                               uint32_t to_start,
-                               int packed_size,
+  static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
+                               FixedArrayBase* to, ElementsKind from_kind,
+                               uint32_t to_start, int packed_size,
                                int copy_size) {
     UNREACHABLE();
   }
@@ -1715,7 +1729,7 @@ class SloppyArgumentsElementsAccessor : public ElementsAccessorBase<
 
 
 ElementsAccessor* ElementsAccessor::ForArray(Handle<FixedArrayBase> array) {
-  return elements_accessors_[ElementsKindForArray(array)];
+  return elements_accessors_[ElementsKindForArray(*array)];
 }
 
 

diff --git a/src/elements.h b/src/elements.h
index d0bddf9..f4de4bb 100644 (file)
--- a/src/elements.h
+++ b/src/elements.h
@@ -146,9 +146,10 @@ class ElementsAccessor {
       uint32_t destination_start,
       int copy_size) = 0;
 
-  // TODO(ishell): Keeping |source_holder| parameter in a non-handlified form
-  // helps avoiding ArrayConcat() builtin performance degradation.
-  // Revisit this later.
+  // NOTE: this method violates the handlified function signature convention:
+  // raw pointer parameter |source_holder| in the function that allocates.
+  // This is done intentionally to avoid ArrayConcat() builtin performance
+  // degradation.
   virtual void CopyElements(
       JSObject* source_holder,
       uint32_t source_start,