Preserve order of fields when doing slow-to-fast object migration.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 9f25145bf2c197048ee858906c251158c8e8570e..457efe0c4f86d867ffcb5e771ae5af3e56233a71 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -4280,25 +4280,27 @@ void JSObject::MigrateSlowToFast(Handle<JSObject> object,
   int number_of_elements = dictionary->NumberOfElements();
   if (number_of_elements > kMaxNumberOfDescriptors) return;
 
+  Handle<FixedArray> iteration_order;
   if (number_of_elements != dictionary->NextEnumerationIndex()) {
-    NameDictionary::DoGenerateNewEnumerationIndices(dictionary);
+    iteration_order =
+        NameDictionary::DoGenerateNewEnumerationIndices(dictionary);
+  } else {
+    iteration_order = NameDictionary::BuildNewIterationIndices(dictionary);
   }
 
-  int instance_descriptor_length = 0;
+  int instance_descriptor_length = iteration_order->length();
   int number_of_fields = 0;
 
   // Compute the length of the instance descriptor.
-  int capacity = dictionary->Capacity();
-  for (int i = 0; i < capacity; i++) {
-    Object* k = dictionary->KeyAt(i);
-    if (dictionary->IsKey(k)) {
-      Object* value = dictionary->ValueAt(i);
-      PropertyType type = dictionary->DetailsAt(i).type();
-      DCHECK(type != FIELD);
-      instance_descriptor_length++;
-      if (type == NORMAL && !value->IsJSFunction()) {
-        number_of_fields += 1;
-      }
+  for (int i = 0; i < instance_descriptor_length; i++) {
+    int index = Smi::cast(iteration_order->get(i))->value();
+    DCHECK(dictionary->IsKey(dictionary->KeyAt(index)));
+
+    Object* value = dictionary->ValueAt(index);
+    PropertyType type = dictionary->DetailsAt(index).type();
+    DCHECK(type != FIELD);
+    if (type == NORMAL && !value->IsJSFunction()) {
+      number_of_fields += 1;
     }
   }
 
@@ -4338,51 +4340,45 @@ void JSObject::MigrateSlowToFast(Handle<JSObject> object,
 
   // Fill in the instance descriptor and the fields.
   int current_offset = 0;
-  for (int i = 0; i < capacity; i++) {
-    Object* k = dictionary->KeyAt(i);
-    if (dictionary->IsKey(k)) {
-      Object* value = dictionary->ValueAt(i);
-      Handle<Name> key;
-      if (k->IsSymbol()) {
-        key = handle(Symbol::cast(k));
-      } else {
-        // Ensure the key is a unique name before writing into the
-        // instance descriptor.
-        key = factory->InternalizeString(handle(String::cast(k)));
-      }
+  for (int i = 0; i < instance_descriptor_length; i++) {
+    int index = Smi::cast(iteration_order->get(i))->value();
+    Object* k = dictionary->KeyAt(index);
+    DCHECK(dictionary->IsKey(k));
 
-      PropertyDetails details = dictionary->DetailsAt(i);
-      int enumeration_index = details.dictionary_index();
-      PropertyType type = details.type();
-
-      if (value->IsJSFunction()) {
-        ConstantDescriptor d(key,
-                             handle(value, isolate),
-                             details.attributes());
-        descriptors->Set(enumeration_index - 1, &d);
-      } else if (type == NORMAL) {
-        if (current_offset < inobject_props) {
-          object->InObjectPropertyAtPut(current_offset,
-                                        value,
-                                        UPDATE_WRITE_BARRIER);
-        } else {
-          int offset = current_offset - inobject_props;
-          fields->set(offset, value);
-        }
-        FieldDescriptor d(key,
-                          current_offset++,
-                          details.attributes(),
-                          // TODO(verwaest): value->OptimalRepresentation();
-                          Representation::Tagged());
-        descriptors->Set(enumeration_index - 1, &d);
-      } else if (type == CALLBACKS) {
-        CallbacksDescriptor d(key,
-                              handle(value, isolate),
-                              details.attributes());
-        descriptors->Set(enumeration_index - 1, &d);
+    Object* value = dictionary->ValueAt(index);
+    Handle<Name> key;
+    if (k->IsSymbol()) {
+      key = handle(Symbol::cast(k));
+    } else {
+      // Ensure the key is a unique name before writing into the
+      // instance descriptor.
+      key = factory->InternalizeString(handle(String::cast(k)));
+    }
+
+    PropertyDetails details = dictionary->DetailsAt(index);
+    int enumeration_index = details.dictionary_index();
+    PropertyType type = details.type();
+
+    if (value->IsJSFunction()) {
+      ConstantDescriptor d(key, handle(value, isolate), details.attributes());
+      descriptors->Set(enumeration_index - 1, &d);
+    } else if (type == NORMAL) {
+      if (current_offset < inobject_props) {
+        object->InObjectPropertyAtPut(current_offset, value,
+                                      UPDATE_WRITE_BARRIER);
       } else {
-        UNREACHABLE();
+        int offset = current_offset - inobject_props;
+        fields->set(offset, value);
       }
+      FieldDescriptor d(key, current_offset++, details.attributes(),
+                        // TODO(verwaest): value->OptimalRepresentation();
+                        Representation::Tagged());
+      descriptors->Set(enumeration_index - 1, &d);
+    } else if (type == CALLBACKS) {
+      CallbacksDescriptor d(key, handle(value, isolate), details.attributes());
+      descriptors->Set(enumeration_index - 1, &d);
+    } else {
+      UNREACHABLE();
     }
   }
   DCHECK(current_offset == number_of_fields);
@@ -14106,9 +14102,13 @@ template Handle<NameDictionary>
 Dictionary<NameDictionary, NameDictionaryShape, Handle<Name> >::Add(
     Handle<NameDictionary>, Handle<Name>, Handle<Object>, PropertyDetails);
 
-template void
-Dictionary<NameDictionary, NameDictionaryShape, Handle<Name> >::
-    GenerateNewEnumerationIndices(Handle<NameDictionary>);
+template Handle<FixedArray>
+    Dictionary<NameDictionary, NameDictionaryShape,
+               Handle<Name> >::BuildNewIterationIndices(Handle<NameDictionary>);
+
+template Handle<FixedArray> Dictionary<
+    NameDictionary, NameDictionaryShape,
+    Handle<Name> >::GenerateNewEnumerationIndices(Handle<NameDictionary>);
 
 template int
 Dictionary<SeededNumberDictionary, SeededNumberDictionaryShape, uint32_t>::
@@ -14914,56 +14914,60 @@ Handle<Derived> Dictionary<Derived, Shape, Key>::New(
 }
 
 
-template<typename Derived, typename Shape, typename Key>
-void Dictionary<Derived, Shape, Key>::GenerateNewEnumerationIndices(
+template <typename Derived, typename Shape, typename Key>
+Handle<FixedArray> Dictionary<Derived, Shape, Key>::BuildNewIterationIndices(

[Jakob Kummerow, 2014/10/06 11:47:35]

naming: BuildNewIterationIndices sounds confusingly similar to
GenerateNewInterationIndices. How about "BuildIterationIndicesArray" instead?

[Igor Sheludko, 2014/10/20 14:05:24]

Done. Thanks!

     Handle<Derived> dictionary) {
   Factory* factory = dictionary->GetIsolate()->factory();
   int length = dictionary->NumberOfElements();
 
-  // Allocate and initialize iteration order array.
   Handle<FixedArray> iteration_order = factory->NewFixedArray(length);
-  for (int i = 0; i < length; i++) {
-    iteration_order->set(i, Smi::FromInt(i));
-  }
-
-  // Allocate array with enumeration order.
   Handle<FixedArray> enumeration_order = factory->NewFixedArray(length);
 
-  // Fill the enumeration order array with property details.
+  // Fill both the iteration order array and the enumeration order array
+  // with property details.
   int capacity = dictionary->Capacity();
   int pos = 0;
   for (int i = 0; i < capacity; i++) {
     if (dictionary->IsKey(dictionary->KeyAt(i))) {
       int index = dictionary->DetailsAt(i).dictionary_index();
-      enumeration_order->set(pos++, Smi::FromInt(index));
+      iteration_order->set(pos, Smi::FromInt(i));
+      enumeration_order->set(pos, Smi::FromInt(index));
+      pos++;
     }
   }
+  DCHECK(pos == length);
 
   // Sort the arrays wrt. enumeration order.
   iteration_order->SortPairs(*enumeration_order, enumeration_order->length());
+  return iteration_order;
+}
+

[Jakob Kummerow, 2014/10/06 11:47:35]

nit: two empty lines (doesn't presubmit complain?)

[Igor Sheludko, 2014/10/20 14:05:24]

Done.

+template <typename Derived, typename Shape, typename Key>
+Handle<FixedArray>
+Dictionary<Derived, Shape, Key>::GenerateNewEnumerationIndices(
+    Handle<Derived> dictionary) {
+  int length = dictionary->NumberOfElements();
 
-  // Overwrite the enumeration_order with the enumeration indices.
+  Handle<FixedArray> iteration_order = BuildNewIterationIndices(dictionary);
+  DCHECK(iteration_order->length() == length);
+
+  // Iterate over the dictionary using the enumeration order and update
+  // the dictionary with new enumeration indices.
   for (int i = 0; i < length; i++) {
     int index = Smi::cast(iteration_order->get(i))->value();
+    DCHECK(dictionary->IsKey(dictionary->KeyAt(index)));
+
     int enum_index = PropertyDetails::kInitialIndex + i;
-    enumeration_order->set(index, Smi::FromInt(enum_index));
-  }
 
-  // Update the dictionary with new indices.
-  capacity = dictionary->Capacity();
-  pos = 0;
-  for (int i = 0; i < capacity; i++) {
-    if (dictionary->IsKey(dictionary->KeyAt(i))) {
-      int enum_index = Smi::cast(enumeration_order->get(pos++))->value();
-      PropertyDetails details = dictionary->DetailsAt(i);
-      PropertyDetails new_details = PropertyDetails(
-          details.attributes(), details.type(), enum_index);
-      dictionary->DetailsAtPut(i, new_details);
-    }
+    PropertyDetails details = dictionary->DetailsAt(index);
+    PropertyDetails new_details =
+        PropertyDetails(details.attributes(), details.type(), enum_index);
+    dictionary->DetailsAtPut(index, new_details);
   }
 
   // Set the next enumeration index.
   dictionary->SetNextEnumerationIndex(PropertyDetails::kInitialIndex+length);
+  return iteration_order;
 }