[elements] Minor hardening and cleanup of concat

src/elements.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/elements.h"
 
 #include "src/arguments.h"
 #include "src/conversions.h"
 #include "src/factory.h"
 #include "src/messages.h"
@@ skipping 2836 matching lines @@
   if (elements_accessors_ == NULL) return;
 #define ACCESSOR_DELETE(Class, Kind, Store) delete elements_accessors_[Kind];
   ELEMENTS_LIST(ACCESSOR_DELETE)
 #undef ACCESSOR_DELETE
   elements_accessors_ = NULL;
 }
 
 
 Handle<JSArray> ElementsAccessor::Concat(Isolate* isolate, Arguments* args,
                                          uint32_t concat_size) {
-  int result_len = 0;
+  uint32_t result_len = 0;
   ElementsKind elements_kind = GetInitialFastElementsKind();
-  bool has_double = false;
   {
     DisallowHeapAllocation no_gc;
     // Iterate through all the arguments performing checks
     // and calculating total length.
-    bool is_holey = false;
     for (uint32_t i = 0; i < concat_size; i++) {
-      Object* arg = (*args)[i];
-      int len = Smi::cast(JSArray::cast(arg)->length())->value();
+      JSArray* array = JSArray::cast((*args)[i]);
+      uint32_t len;
+      array->length()->ToArrayLength(&len);
 
       // We shouldn't overflow when adding another len.
       const int kHalfOfMaxInt = 1 << (kBitsPerInt - 2);
       STATIC_ASSERT(FixedArray::kMaxLength < kHalfOfMaxInt);
       USE(kHalfOfMaxInt);
       result_len += len;
-      DCHECK(0 <= result_len);
+      DCHECK_LE(0, result_len);
       DCHECK(result_len <= FixedDoubleArray::kMaxLength);
 
-      ElementsKind arg_kind = JSArray::cast(arg)->map()->elements_kind();
-      has_double = has_double || IsFastDoubleElementsKind(arg_kind);
-      is_holey = is_holey || IsFastHoleyElementsKind(arg_kind);
+      ElementsKind arg_kind = array->GetElementsKind();
+      if (IsFastHoleyElementsKind(arg_kind)) {
+        elements_kind = GetHoleyElementsKind(elements_kind);
+      }
       elements_kind = GetMoreGeneralElementsKind(elements_kind, arg_kind);
     }
-    if (is_holey) {
-      elements_kind = GetHoleyElementsKind(elements_kind);
-    }
   }
 
   // If a double array is concatted into a fast elements array, the fast
   // elements array needs to be initialized to contain proper holes, since
   // boxing doubles may cause incremental marking.
-  ArrayStorageAllocationMode mode =
-      has_double && IsFastObjectElementsKind(elements_kind)
-          ? INITIALIZE_ARRAY_ELEMENTS_WITH_HOLE
-          : DONT_INITIALIZE_ARRAY_ELEMENTS;
+  ArrayStorageAllocationMode mode = IsFastDoubleElementsKind(elements_kind)

[Jakob Kummerow, 2016/03/18 10:36:39]

This is not the same. Consider the scenario mentioned in the comment right
above: a large double array and a small array with object elements are
concatenated. |elements_kind| at this point will be FAST_ELEMENTS (or holey),
but the new backing store must be initialized with hole values, because one of
the heap number allocations could cause the GC to scan the partially populated
new backing store. You may want to add a test case for this :-)
(Come to think of it, I'm not sure if the above is still true with the recently
introduced black allocation... Please talk to a GC expert to decide whether it's
OK to rely on this.)

[Camillo Bruni, 2016/03/22 12:47:10]

Wonder what I was doing... I actually got that part.. but now looking at the
code it doesn't make sense...

+                                        ? INITIALIZE_ARRAY_ELEMENTS_WITH_HOLE
+                                        : DONT_INITIALIZE_ARRAY_ELEMENTS;
   Handle<JSArray> result_array = isolate->factory()->NewJSArray(
       elements_kind, result_len, result_len, mode);
   if (result_len == 0) return result_array;
-  int j = 0;
+
+  uint32_t insertion_index = 0;
   Handle<FixedArrayBase> storage(result_array->elements(), isolate);
   ElementsAccessor* accessor = ElementsAccessor::ForKind(elements_kind);
   for (uint32_t i = 0; i < concat_size; i++) {
     // 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();
-    if (len > 0) {
-      ElementsKind from_kind = array->GetElementsKind();
-      accessor->CopyElements(array, 0, from_kind, storage, j, len);
-      j += len;
-    }
+    uint32_t len;
+    array->length()->ToArrayLength(&len);
+    if (len == 0) continue;
+    ElementsKind from_kind = array->GetElementsKind();
+    accessor->CopyElements(array, 0, from_kind, storage, insertion_index, len);
+    insertion_index += len;
   }
 
-  DCHECK(j == result_len);
+  DCHECK_EQ(insertion_index, result_len);
   return result_array;
 }
 
 ElementsAccessor** ElementsAccessor::elements_accessors_ = NULL;
 }  // namespace internal
 }  // namespace v8