TraversableMap only dependent on DescriptorArray

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index cb87c71fb1193c38953701a88ffac4e1f465d9cd..4e4fb385b966f7f5949a35a7e2ad1e8ad9fed9c1 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -4889,39 +4889,39 @@ class IntrusiveMapTransitionIterator {
 
   void Start() {
     ASSERT(!IsIterating());
-    if (HasContentArray()) *ContentHeader() = Smi::FromInt(0);
+    if (HasDescriptors()) *DescriptorArrayHeader() = Smi::FromInt(0);
   }
 
   bool IsIterating() {
-    return HasContentArray() && (*ContentHeader())->IsSmi();
+    return HasDescriptors() && (*DescriptorArrayHeader())->IsSmi();
   }
 
   Map* Next() {
     ASSERT(IsIterating());
-    FixedArray* contents = ContentArray();
-    // Attention, tricky index manipulation ahead: Every entry in the contents
-    // array consists of a value/details pair, so the index is typically even.
-    // An exception is made for CALLBACKS entries: An even index means we look
-    // at its getter, and an odd index means we look at its setter.
-    int index = Smi::cast(*ContentHeader())->value();
-    while (index < contents->length()) {
-      PropertyDetails details(Smi::cast(contents->get(index | 1)));
+    // Attention, tricky index manipulation ahead: Two two consecutive indices

[Michael Starzinger, 2012/05/24 14:02:25]

Only one "two".

[Toon Verwaest, 2012/05/25 11:36:46]

Done.

+    // are assigned to each descriptor. Most descriptors directly advance to the
+    // next descriptor by adding 2 to the index. The exceptions are the
+    // CALLBACKS entries: An even index means we look at its getter, and an odd
+    // index means we look at its setter.
+    int index = Smi::cast(*DescriptorArrayHeader())->value();
+    while ((index / 2) < descriptor_array_->number_of_descriptors()) {

[Michael Starzinger, 2012/05/24 14:02:25]

Can we hoist the division by two into a local variable? That would improve
readability. Like this:

int index = Smi::cast(*DescriptorArrayHeader())->value();
int descriptor_number = raw_index / 2;

[Toon Verwaest, 2012/05/25 11:36:46]

Done.

+      PropertyDetails details(descriptor_array_->RawGetDetails(index / 2));
       switch (details.type()) {
         case MAP_TRANSITION:
         case CONSTANT_TRANSITION:
         case ELEMENTS_TRANSITION:
           // We definitely have a map transition.
-          *ContentHeader() = Smi::FromInt(index + 2);
-          return static_cast<Map*>(contents->get(index));
+          *DescriptorArrayHeader() = Smi::FromInt(index + 2);
+          return static_cast<Map*>(descriptor_array_->RawGetValue(index / 2));
         case CALLBACKS: {
           // We might have a map transition in a getter or in a setter.
           AccessorPair* accessors =
-              static_cast<AccessorPair*>(contents->get(index & ~1));
+              static_cast<AccessorPair*>(descriptor_array_->RawGetValue(index / 2));

[Michael Starzinger, 2012/05/24 14:02:25]

More than 80 characters.

[Toon Verwaest, 2012/05/25 11:36:46]

Done.

           Object* accessor =
               ((index & 1) == 0) ? accessors->getter() : accessors->setter();
           index++;
           if (accessor->IsMap()) {
-            *ContentHeader() = Smi::FromInt(index);
+            *DescriptorArrayHeader() = Smi::FromInt(index);
             return static_cast<Map*>(accessor);
           }
           break;
@@ -4937,22 +4937,17 @@ class IntrusiveMapTransitionIterator {
           break;
       }
     }
-    *ContentHeader() = descriptor_array_->GetHeap()->fixed_array_map();
+    *DescriptorArrayHeader() = descriptor_array_->GetHeap()->fixed_array_map();
     return NULL;
   }
 
  private:
-  bool HasContentArray() {
-    return descriptor_array_-> length() > DescriptorArray::kContentArrayIndex;
+  bool HasDescriptors() {
+    return descriptor_array_-> length() > DescriptorArray::kFirstIndex;
   }
 
-  FixedArray* ContentArray() {
-    Object* array = descriptor_array_->get(DescriptorArray::kContentArrayIndex);
-    return static_cast<FixedArray*>(array);
-  }
-
-  Object** ContentHeader() {
-    return HeapObject::RawField(ContentArray(), DescriptorArray::kMapOffset);
+  Object** DescriptorArrayHeader() {
+    return HeapObject::RawField(descriptor_array_, DescriptorArray::kMapOffset);
   }
 
   DescriptorArray* descriptor_array_;
@@ -5053,6 +5048,20 @@ class TraversableMap : public Map {
     return old_parent;
   }
 
+  // Can either be Smi (no instance descriptors), or a descriptor array with the
+  // header overwritten as a Smi (thus iterating).
+  DescriptorArray* MutatedInstanceDescriptors() {
+    Object* object = *HeapObject::RawField(this, kInstanceDescriptorsOrBitField3Offset);
+    if (object->IsSmi()) {
+      return GetHeap()->empty_descriptor_array();
+    } else {
+      DescriptorArray* descriptor_array = static_cast<DescriptorArray*>(object);
+      Object* map = *HeapObject::RawField(descriptor_array, DescriptorArray::kMapOffset);
+      ASSERT(map->IsSmi());
+      return descriptor_array;
+    }
+  }
+
   // Start iterating over this map's children, possibly destroying a FixedArray
   // map (see explanation above).
   void ChildIteratorStart() {
@@ -5064,17 +5073,17 @@ class TraversableMap : public Map {
   // If we have an unvisited child map, return that one and advance. If we have
   // none, return NULL and reset any destroyed FixedArray maps.
   TraversableMap* ChildIteratorNext() {
-    IntrusiveMapTransitionIterator descriptor_iterator(instance_descriptors());
-    if (descriptor_iterator.IsIterating()) {
-      Map* next = descriptor_iterator.Next();
-      if (next != NULL) return static_cast<TraversableMap*>(next);
-    }
     IntrusivePrototypeTransitionIterator
         proto_iterator(unchecked_prototype_transitions());
     if (proto_iterator.IsIterating()) {
       Map* next = proto_iterator.Next();
       if (next != NULL) return static_cast<TraversableMap*>(next);
     }
+    IntrusiveMapTransitionIterator descriptor_iterator(MutatedInstanceDescriptors());

[Michael Starzinger, 2012/05/24 14:02:25]

Could we just pass the map itself to the constructor of the child iterator and
do the raw access of the instance descriptor field in there?

More than 80 characters.

[Toon Verwaest, 2012/05/25 11:36:46]

As discussed offline, leaving it as it is.

On 2012/05/24 14:02:25, Michael Starzinger wrote:

+    if (descriptor_iterator.IsIterating()) {
+      Map* next = descriptor_iterator.Next();
+      if (next != NULL) return static_cast<TraversableMap*>(next);
+    }
     return NULL;
   }
 };