Reviewers: Michael Starzinger,

Message:
The added patch fixes the iteration bug introduced in the second patch of the
original issue. Now seems to run stable. Please verify.

Description:
TraversableMap only dependent on DescriptorArray
This is the first step towards merging the ContentArray into the
DescriptorArray, (hence reclaiming 3 words per descriptor array).


Please review this at https://chromiumcodereview.appspot.com/10455005/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
  M src/objects.h
  M src/objects.cc


Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index a8970584d8b3e206d6dc0abc89084d8046b5887e..46eb2be34b0a383645e9f91c1dac53cbfb9b8d97 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -5088,39 +5088,46 @@ 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 consecutive indices 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 raw_index = Smi::cast(*DescriptorArrayHeader())->value();
+    int index = raw_index / 2;
+    int num = descriptor_array_->number_of_descriptors();
+    while (index < num) {
+      PropertyDetails details(RawGetDetails(index));
       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(raw_index + 2);
+          return static_cast<Map*>(RawGetValue(index));
         case CALLBACKS: {
           // We might have a map transition in a getter or in a setter.
           AccessorPair* accessors =
-              static_cast<AccessorPair*>(contents->get(index & ~1));
-          Object* accessor =
- ((index & 1) == 0) ? accessors->getter() : accessors->setter();
-          index++;
+              static_cast<AccessorPair*>(RawGetValue(index));
+          Object* accessor;
+          if ((raw_index & 1) == 0) {
+            accessor = accessors->getter();
+          } else {
+            ++index;
+            accessor = accessors->setter();
+          }
+          ++raw_index;
           if (accessor->IsMap()) {
-            *ContentHeader() = Smi::FromInt(index);
+             *DescriptorArrayHeader() = Smi::FromInt(raw_index);
             return static_cast<Map*>(accessor);
           }
           break;
@@ -5132,28 +5139,42 @@ class IntrusiveMapTransitionIterator {
         case INTERCEPTOR:
         case NULL_DESCRIPTOR:
           // We definitely have no map transition.
-          index += 2;
+          raw_index += 2;
+          ++index;
           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;
+  }
+
+  Object** DescriptorArrayHeader() {
+ return HeapObject::RawField(descriptor_array_, DescriptorArray::kMapOffset);
+  }
+
+  FixedArray* RawGetContentArray() {
+      Object* array =
+          descriptor_array_->get(DescriptorArray::kContentArrayIndex);
+      return static_cast<FixedArray*>(array);
   }

-  FixedArray* ContentArray() {
- Object* array = descriptor_array_->get(DescriptorArray::kContentArrayIndex);
-    return static_cast<FixedArray*>(array);
+  Object* RawGetValue(int descriptor_number) {
+    return RawGetContentArray()->get(
+        DescriptorArray::ToValueIndex(descriptor_number));
   }

-  Object** ContentHeader() {
- return HeapObject::RawField(ContentArray(), DescriptorArray::kMapOffset);
+  PropertyDetails RawGetDetails(int descriptor_number) {
+    Object* details = RawGetContentArray()->get(
+        DescriptorArray::ToDetailsIndex(descriptor_number));
+    return PropertyDetails(Smi::cast(details));
   }

+
   DescriptorArray* descriptor_array_;
 };

@@ -5252,6 +5273,22 @@ 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);
+      ASSERT((*HeapObject::RawField(descriptor_array,
+ DescriptorArray::kMapOffset))->IsSmi());
+      return descriptor_array;
+    }
+  }
+
// Start iterating over this map's children, possibly destroying a FixedArray
   // map (see explanation above).
   void ChildIteratorStart() {
@@ -5263,7 +5300,8 @@ 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());
+    IntrusiveMapTransitionIterator
+        descriptor_iterator(MutatedInstanceDescriptors());
     if (descriptor_iterator.IsIterating()) {
       Map* next = descriptor_iterator.Next();
       if (next != NULL) return static_cast<TraversableMap*>(next);
Index: src/objects.h
diff --git a/src/objects.h b/src/objects.h
index e9dfe6ca55b94b8a923137b3cdfbcd47355ae79e..b6462dd78770e318d70c97187ea0ce97a914961d 100644
--- a/src/objects.h
+++ b/src/objects.h
@@ -2588,6 +2588,8 @@ class DescriptorArray: public FixedArray {
   static const int kMaxNumberOfDescriptors = 1024 + 512;

  private:
+  friend class IntrusiveMapTransitionIterator;
+
   // An entry in a DescriptorArray, represented as an (array, index) pair.
   class Entry {
    public:
@@ -2626,6 +2628,7 @@ class DescriptorArray: public FixedArray {
   FixedArray* GetContentArray() {
     return FixedArray::cast(get(kContentArrayIndex));
   }
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(DescriptorArray);
 };



--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to