Array() in optimized code can create with wrong ElementsKind in corner cases.

Calling new Array(JSObject::kInitialMaxFastElementArray) in optimized code
makes a stub call that bails out due to the length. Currently, the bailout
code a) doesn't have the allocation site, and b) wouldn't use it if it did
because the length is perceived to be too high.

This CL passes the allocation site to the stub call (rather than undefined),
and alters the bailout code to utilize the feedback.

BUG=

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

Cr-Commit-Position: refs/heads/master@{#27857}

diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
index 261425a..2aa549a 100644 (file)
--- a/src/arm/lithium-codegen-arm.cc
+++ b/src/arm/lithium-codegen-arm.cc
@@ -4091,7 +4091,14 @@ void LCodeGen::DoCallNewArray(LCallNewArray* instr) {
   DCHECK(ToRegister(instr->result()).is(r0));
 
   __ mov(r0, Operand(instr->arity()));
-  __ LoadRoot(r2, Heap::kUndefinedValueRootIndex);
+  if (instr->arity() == 1) {
+    // We only need the allocation site for the case we have a length argument.
+    // The case may bail out to the runtime, which will determine the correct
+    // elements kind with the site.
+    __ Move(r2, instr->hydrogen()->site());
+  } else {
+    __ LoadRoot(r2, Heap::kUndefinedValueRootIndex);
+  }
   ElementsKind kind = instr->hydrogen()->elements_kind();
   AllocationSiteOverrideMode override_mode =
       (AllocationSite::GetMode(kind) == TRACK_ALLOCATION_SITE)

diff --git a/src/arm64/lithium-codegen-arm64.cc b/src/arm64/lithium-codegen-arm64.cc
index b38770b..44937d4 100644 (file)
--- a/src/arm64/lithium-codegen-arm64.cc
+++ b/src/arm64/lithium-codegen-arm64.cc
@@ -460,7 +460,15 @@ void LCodeGen::DoCallNewArray(LCallNewArray* instr) {
   DCHECK(ToRegister(instr->constructor()).is(x1));
 
   __ Mov(x0, Operand(instr->arity()));
-  __ LoadRoot(x2, Heap::kUndefinedValueRootIndex);
+  if (instr->arity() == 1) {
+    // We only need the allocation site for the case we have a length argument.
+    // The case may bail out to the runtime, which will determine the correct
+    // elements kind with the site.
+    __ Mov(x2, instr->hydrogen()->site());
+  } else {
+    __ LoadRoot(x2, Heap::kUndefinedValueRootIndex);
+  }
+
 
   ElementsKind kind = instr->hydrogen()->elements_kind();
   AllocationSiteOverrideMode override_mode =

diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index 0e5e1b6..2307f9d 100644 (file)
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -2430,10 +2430,9 @@ class HCallNew FINAL : public HBinaryCall {
 
 class HCallNewArray FINAL : public HBinaryCall {
  public:
-  DECLARE_INSTRUCTION_WITH_CONTEXT_FACTORY_P3(HCallNewArray,
-                                              HValue*,
-                                              int,
-                                              ElementsKind);
+  DECLARE_INSTRUCTION_WITH_CONTEXT_FACTORY_P4(HCallNewArray, HValue*, int,
+                                              ElementsKind,
+                                              Handle<AllocationSite>);
 
   HValue* context() { return first(); }
   HValue* constructor() { return second(); }
@@ -2441,16 +2440,19 @@ class HCallNewArray FINAL : public HBinaryCall {
   std::ostream& PrintDataTo(std::ostream& os) const OVERRIDE;  // NOLINT
 
   ElementsKind elements_kind() const { return elements_kind_; }
+  Handle<AllocationSite> site() const { return site_; }
 
   DECLARE_CONCRETE_INSTRUCTION(CallNewArray)
 
  private:
   HCallNewArray(HValue* context, HValue* constructor, int argument_count,
-                ElementsKind elements_kind)
+                ElementsKind elements_kind, Handle<AllocationSite> site)
       : HBinaryCall(context, constructor, argument_count),
-        elements_kind_(elements_kind) {}
+        elements_kind_(elements_kind),
+        site_(site) {}
 
   ElementsKind elements_kind_;
+  Handle<AllocationSite> site_;
 };
 
 

diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 3e87d69..b7130b4 100644 (file)
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -8971,7 +8971,7 @@ void HOptimizedGraphBuilder::BuildArrayCall(Expression* expression,
   }
 
   HInstruction* call = PreProcessCall(New<HCallNewArray>(
-      function, arguments_count + 1, site->GetElementsKind()));
+      function, arguments_count + 1, site->GetElementsKind(), site));
   if (expression->IsCall()) {
     Drop(1);
   }

diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc
index 2672601..a46cd4b 100644 (file)
--- a/src/ia32/lithium-codegen-ia32.cc
+++ b/src/ia32/lithium-codegen-ia32.cc
@@ -3977,7 +3977,15 @@ void LCodeGen::DoCallNewArray(LCallNewArray* instr) {
   DCHECK(ToRegister(instr->result()).is(eax));
 
   __ Move(eax, Immediate(instr->arity()));
-  __ mov(ebx, isolate()->factory()->undefined_value());
+  if (instr->arity() == 1) {
+    // We only need the allocation site for the case we have a length argument.
+    // The case may bail out to the runtime, which will determine the correct
+    // elements kind with the site.
+    __ mov(ebx, instr->hydrogen()->site());
+  } else {
+    __ mov(ebx, isolate()->factory()->undefined_value());
+  }
+
   ElementsKind kind = instr->hydrogen()->elements_kind();
   AllocationSiteOverrideMode override_mode =
       (AllocationSite::GetMode(kind) == TRACK_ALLOCATION_SITE)

diff --git a/src/mips/lithium-codegen-mips.cc b/src/mips/lithium-codegen-mips.cc
index 37b75b8..2b2e767 100644 (file)
--- a/src/mips/lithium-codegen-mips.cc
+++ b/src/mips/lithium-codegen-mips.cc
@@ -4066,7 +4066,14 @@ void LCodeGen::DoCallNewArray(LCallNewArray* instr) {
   DCHECK(ToRegister(instr->result()).is(v0));
 
   __ li(a0, Operand(instr->arity()));
-  __ LoadRoot(a2, Heap::kUndefinedValueRootIndex);
+  if (instr->arity() == 1) {
+    // We only need the allocation site for the case we have a length argument.
+    // The case may bail out to the runtime, which will determine the correct
+    // elements kind with the site.
+    __ li(a2, instr->hydrogen()->site());
+  } else {
+    __ LoadRoot(a2, Heap::kUndefinedValueRootIndex);
+  }
   ElementsKind kind = instr->hydrogen()->elements_kind();
   AllocationSiteOverrideMode override_mode =
       (AllocationSite::GetMode(kind) == TRACK_ALLOCATION_SITE)

diff --git a/src/mips64/lithium-codegen-mips64.cc b/src/mips64/lithium-codegen-mips64.cc
index ab4d496..2e6ede5 100644 (file)
--- a/src/mips64/lithium-codegen-mips64.cc
+++ b/src/mips64/lithium-codegen-mips64.cc
@@ -4128,7 +4128,14 @@ void LCodeGen::DoCallNewArray(LCallNewArray* instr) {
   DCHECK(ToRegister(instr->result()).is(v0));
 
   __ li(a0, Operand(instr->arity()));
-  __ LoadRoot(a2, Heap::kUndefinedValueRootIndex);
+  if (instr->arity() == 1) {
+    // We only need the allocation site for the case we have a length argument.
+    // The case may bail out to the runtime, which will determine the correct
+    // elements kind with the site.
+    __ li(a2, instr->hydrogen()->site());
+  } else {
+    __ LoadRoot(a2, Heap::kUndefinedValueRootIndex);
+  }
   ElementsKind kind = instr->hydrogen()->elements_kind();
   AllocationSiteOverrideMode override_mode =
       (AllocationSite::GetMode(kind) == TRACK_ALLOCATION_SITE)

diff --git a/src/objects-inl.h b/src/objects-inl.h
index e5e8235..f284f65 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -7201,6 +7201,17 @@ void JSArray::set_length(Smi* length) {
 }
 
 
+bool JSArray::SetElementsLengthWouldNormalize(
+    Heap* heap, Handle<Object> new_length_handle) {
+  // If the new array won't fit in a some non-trivial fraction of the max old
+  // space size, then force it to go dictionary mode.
+  int max_fast_array_size =
+      static_cast<int>((heap->MaxOldGenerationSize() / kDoubleSize) / 4);
+  return new_length_handle->IsNumber() &&
+         NumberToInt32(*new_length_handle) >= max_fast_array_size;
+}
+
+
 bool JSArray::AllowsSetElementsLength() {
   bool result = elements()->IsFixedArray() || elements()->IsFixedDoubleArray();
   DCHECK(result == !HasExternalArrayElements());

diff --git a/src/objects.cc b/src/objects.cc
index 1b8b5c5..8f8b6c8 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -12032,15 +12032,9 @@ MUST_USE_RESULT static MaybeHandle<Object> EndPerformSplice(
 MaybeHandle<Object> JSArray::SetElementsLength(
     Handle<JSArray> array,
     Handle<Object> new_length_handle) {
-  if (array->HasFastElements()) {
-    // If the new array won't fit in a some non-trivial fraction of the max old
-    // space size, then force it to go dictionary mode.
-    int max_fast_array_size = static_cast<int>(
-        (array->GetHeap()->MaxOldGenerationSize() / kDoubleSize) / 4);
-    if (new_length_handle->IsNumber() &&
-        NumberToInt32(*new_length_handle) >= max_fast_array_size) {
-      NormalizeElements(array);
-    }
+  if (array->HasFastElements() &&
+      SetElementsLengthWouldNormalize(array->GetHeap(), new_length_handle)) {
+    NormalizeElements(array);
   }
 
   // We should never end in here with a pixel or external array.

diff --git a/src/objects.h b/src/objects.h
index f36cd1f..66c04d6 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -10416,6 +10416,11 @@ class JSArray: public JSObject {
   // capacity is non-zero.
   static void Initialize(Handle<JSArray> array, int capacity, int length = 0);
 
+  // If the JSArray has fast elements, and new_length would result in
+  // normalization, returns true.
+  static inline bool SetElementsLengthWouldNormalize(
+      Heap* heap, Handle<Object> new_length_handle);
+
   // Initializes the array to a certain length.
   inline bool AllowsSetElementsLength();
   // Can cause GC.

diff --git a/src/runtime/runtime-array.cc b/src/runtime/runtime-array.cc
index 010c186..6f80443 100644 (file)
--- a/src/runtime/runtime-array.cc
+++ b/src/runtime/runtime-array.cc
@@ -1045,15 +1045,20 @@ static Object* ArrayConstructorCommon(Isolate* isolate,
 
   bool holey = false;
   bool can_use_type_feedback = true;
+  bool can_inline_array_constructor = true;
   if (caller_args->length() == 1) {
     Handle<Object> argument_one = caller_args->at<Object>(0);
     if (argument_one->IsSmi()) {
       int value = Handle<Smi>::cast(argument_one)->value();
-      if (value < 0 || value >= JSObject::kInitialMaxFastElementArray) {
+      if (value < 0 || JSArray::SetElementsLengthWouldNormalize(isolate->heap(),
+                                                                argument_one)) {
         // the array is a dictionary in this case.
         can_use_type_feedback = false;
       } else if (value != 0) {
         holey = true;
+        if (value >= JSObject::kInitialMaxFastElementArray) {
+          can_inline_array_constructor = false;
+        }
       }
     } else {
       // Non-smi length argument produces a dictionary
@@ -1104,7 +1109,8 @@ static Object* ArrayConstructorCommon(Isolate* isolate,
   RETURN_FAILURE_ON_EXCEPTION(
       isolate, ArrayConstructInitializeElements(array, caller_args));
   if (!site.is_null() &&
-      (old_kind != array->GetElementsKind() || !can_use_type_feedback)) {
+      (old_kind != array->GetElementsKind() || !can_use_type_feedback ||
+       !can_inline_array_constructor)) {
     // The arguments passed in caused a transition. This kind of complexity
     // can't be dealt with in the inlined hydrogen array constructor case.
     // We must mark the allocationsite as un-inlinable.

diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc
index b8aed96..a5a38cc 100644 (file)
--- a/src/x64/lithium-codegen-x64.cc
+++ b/src/x64/lithium-codegen-x64.cc
@@ -4063,7 +4063,15 @@ void LCodeGen::DoCallNewArray(LCallNewArray* instr) {
   DCHECK(ToRegister(instr->result()).is(rax));
 
   __ Set(rax, instr->arity());
-  __ LoadRoot(rbx, Heap::kUndefinedValueRootIndex);
+  if (instr->arity() == 1) {
+    // We only need the allocation site for the case we have a length argument.
+    // The case may bail out to the runtime, which will determine the correct
+    // elements kind with the site.
+    __ Move(rbx, instr->hydrogen()->site());
+  } else {
+    __ LoadRoot(rbx, Heap::kUndefinedValueRootIndex);
+  }
+
   ElementsKind kind = instr->hydrogen()->elements_kind();
   AllocationSiteOverrideMode override_mode =
       (AllocationSite::GetMode(kind) == TRACK_ALLOCATION_SITE)

diff --git a/test/mjsunit/array-constructor-feedback.js b/test/mjsunit/array-constructor-feedback.js
index e14c344..4c3a28f 100644 (file)
--- a/test/mjsunit/array-constructor-feedback.js
+++ b/test/mjsunit/array-constructor-feedback.js
@@ -216,3 +216,21 @@ function assertKind(expected, obj, name_opt) {
     assertFalse(isHoley(a));
   }
 })();
+
+// Test: Make sure that crankshaft continues with feedback for large arrays.
+(function() {
+  function bar(len) { return new Array(len); }
+  var size = 100001;
+  // Perform a gc, because we are allocating a very large array and if a gc
+  // happens during the allocation we could lose our memento.
+  gc();
+  bar(size)[0] = 'string';
+  var res = bar(size);
+  assertKind(elements_kind.fast, bar(size));
+    %OptimizeFunctionOnNextCall(bar);
+  assertKind(elements_kind.fast, bar(size));
+  // But there is a limit, based on the size of the old generation, currently
+  // 22937600, but double it to prevent the test being too brittle.
+  var large_size = 22937600 * 2;
+  assertKind(elements_kind.dictionary, bar(large_size));
+})();

diff --git a/test/mjsunit/array-feedback.js b/test/mjsunit/array-feedback.js
index 8121f86..f0a859e 100644 (file)
--- a/test/mjsunit/array-feedback.js
+++ b/test/mjsunit/array-feedback.js
@@ -91,7 +91,7 @@ function assertKind(expected, obj, name_opt) {
   assertKind(elements_kind.fast, b);
 
   a = create1(100000);
-  assertKind(elements_kind.fast_smi_only, a);
+  assertKind(elements_kind.fast, a);
 
   function create3(arg1, arg2, arg3) {
     return Array(arg1, arg2, arg3);