Ensure large Smi-only arrays don't transition to FAST_DOUBLE_ARRAY

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index ae5aa78cae57fc078a87ed41d290a3c9b10a1597..2323484ee565a2bd0b2dbef150c82a1625e71710 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -8188,10 +8188,13 @@ MaybeObject* JSObject::SetFastElementsCapacityAndLength(
   Map* new_map = NULL;
   if (elements()->map() != heap->non_strict_arguments_elements_map()) {
     Object* object;
+    // The resized array has FAST_SMI_ONLY_ELEMENTS if the capacity mode forces
+    // it, or if it's allowed and the old elements array contained only SMIs.
     bool has_fast_smi_only_elements =
-        (set_capacity_mode == kAllowSmiOnlyElements) &&
-        (elements()->map()->has_fast_smi_only_elements() ||
-         elements() == heap->empty_fixed_array());
+        (set_capacity_mode == kForceSmiOnlyElements) ||
+        ((set_capacity_mode == kAllowSmiOnlyElements) &&
+         (elements()->map()->has_fast_smi_only_elements() ||
+          elements() == heap->empty_fixed_array()));
     ElementsKind elements_kind = has_fast_smi_only_elements
         ? FAST_SMI_ONLY_ELEMENTS
         : FAST_ELEMENTS;
@@ -9241,11 +9244,20 @@ MaybeObject* JSObject::SetDictionaryElement(uint32_t index,
     } else {
       new_length = dictionary->max_number_key() + 1;
     }
-    MaybeObject* result = CanConvertToFastDoubleElements()
+    SetFastElementsCapacityMode set_capacity_mode = FLAG_smi_only_arrays
+        ? kAllowSmiOnlyElements
+        : kDontAllowSmiOnlyElements;
+    bool has_smi_only_elements = false;
+    bool should_convert_to_fast_double_elements =
+        ShouldConvertToFastDoubleElements(&has_smi_only_elements);
+    if (has_smi_only_elements) {

[Jakob Kummerow, 2011/12/29 15:18:41]

I think you need an additional "&& FLAG_smi_only_arrays" here, otherwise it'll
transition to SMI elements even if the flag is off.

[danno, 2011/12/30 14:12:02]

There's no harm in going to smi elements for very large arrays when the flag
isn't on. We already do this for very large array literals. The smi_only_arrays
flag has become a bit of a misnomer, it now means "enable the smi/double
features that are known to still cause performance regressions".

On 2011/12/29 15:18:41, Jakob wrote:

+      set_capacity_mode = kForceSmiOnlyElements;
+    }
+    MaybeObject* result = should_convert_to_fast_double_elements
         ? SetFastDoubleElementsCapacityAndLength(new_length, new_length)
         : SetFastElementsCapacityAndLength(new_length,
                                            new_length,
-                                           kDontAllowSmiOnlyElements);
+                                           set_capacity_mode);
     if (result->IsFailure()) return result;
 #ifdef DEBUG
     if (FLAG_trace_normalization) {
@@ -9724,17 +9736,25 @@ bool JSObject::ShouldConvertToFastElements() {
 }
 
 
-bool JSObject::CanConvertToFastDoubleElements() {
+bool JSObject::ShouldConvertToFastDoubleElements(
+    bool* has_smi_only_elements) {
+  *has_smi_only_elements = false;
   if (FLAG_unbox_double_arrays) {
     ASSERT(HasDictionaryElements());
     NumberDictionary* dictionary = NumberDictionary::cast(elements());
+    bool found_double = false;
     for (int i = 0; i < dictionary->Capacity(); i++) {
       Object* key = dictionary->KeyAt(i);
       if (key->IsNumber()) {
-        if (!dictionary->ValueAt(i)->IsNumber()) return false;
+        Object* value = dictionary->ValueAt(i);
+        if (!value->IsNumber()) return false;
+        if (!value->IsSmi()) {
+          found_double = true;
+        }
       }
     }
-    return true;
+    *has_smi_only_elements = !found_double;
+    return found_double;
   } else {
     return false;
   }