Fix garbage collection of unused maps. Null descriptors, created

by map collection, are now handled correctly everywhere.  The 
map-collect flag is now true by default.
Review URL: http://codereview.chromium.org/40218

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1459 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/factory.cc

@@ -536,7 +536,9 @@ Handle<DescriptorArray> Factory::CopyAppendCallbackDescriptors(
   // Copy the descriptors from the array.
   DescriptorWriter w(*result);
   for (DescriptorReader r(*array); !r.eos(); r.advance()) {
-    w.WriteFrom(&r);
+    if (!r.IsNullDescriptor()) {
+      w.WriteFrom(&r);
+    }
     descriptor_count++;
   }
 

src/flag-definitions.h

@@ -151,7 +151,7 @@ DEFINE_bool(gc_global, false, "always perform global GCs")
 DEFINE_int(gc_interval, -1, "garbage collect after <n> allocations")
 DEFINE_bool(trace_gc, false,
             "print one trace line following each garbage collection")
-DEFINE_bool(collect_maps, false,
+DEFINE_bool(collect_maps, true,
             "garbage collect maps from which no objects can be reached")
 
 // ic.cc

src/handles.cc

@@ -467,7 +467,7 @@ Handle<FixedArray> GetEnumPropertyKeys(Handle<JSObject> object) {
     for (DescriptorReader r(object->map()->instance_descriptors());
          !r.eos();
          r.advance()) {
-      if (!r.IsTransition() && !r.IsDontEnum()) {
+      if (r.IsProperty() && !r.IsDontEnum()) {
         (*storage)->set(index, r.GetKey());
         (*sort_array)->set(index, Smi::FromInt(r.GetDetails().index()));
         index++;

src/objects.cc

@@ -2391,7 +2391,7 @@ bool JSObject::IsSimpleEnum() {
 int Map::NumberOfDescribedProperties() {
   int result = 0;
   for (DescriptorReader r(instance_descriptors()); !r.eos(); r.advance()) {
-    if (!r.IsTransition()) result++;
+    if (r.IsProperty()) result++;
   }
   return result;
 }
@@ -2399,7 +2399,7 @@ int Map::NumberOfDescribedProperties() {
 
 int Map::PropertyIndexFor(String* name) {
   for (DescriptorReader r(instance_descriptors()); !r.eos(); r.advance()) {
-    if (r.Equals(name)) return r.GetFieldIndex();
+    if (r.Equals(name) && !r.IsNullDescriptor()) return r.GetFieldIndex();
   }
   return -1;
 }
@@ -2933,6 +2933,7 @@ Object* DescriptorArray::CopyInsert(Descriptor* descriptor,
   int new_size = number_of_descriptors() - transitions - null_descriptors;
 
   // If key is in descriptor, we replace it in-place when filtering.
+  // Count a null descriptor for key as inserted, not replaced.
   int index = Search(descriptor->GetKey());
   const bool inserting = (index == kNotFound);
   const bool replacing = !inserting;
@@ -2949,9 +2950,9 @@ Object* DescriptorArray::CopyInsert(Descriptor* descriptor,
         t == CALLBACKS ||
         t == INTERCEPTOR) {
       keep_enumeration_index = true;
-    } else if (t == NULL_DESCRIPTOR || remove_transitions) {
-     // Replaced descriptor has been counted as removed if it is null
-     // or a transition that will be replaced.  Adjust count in this case.
+    } else if (remove_transitions) {
+     // Replaced descriptor has been counted as removed if it is
+     // a transition that will be replaced.  Adjust count in this case.
       ++new_size;
     }
   }
@@ -2990,7 +2991,9 @@ Object* DescriptorArray::CopyInsert(Descriptor* descriptor,
     ASSERT(r.GetKey() == descriptor->GetKey());
     r.advance();
   } else {
-    ASSERT(r.eos() || r.GetKey()->Hash() > descriptor_hash);
+    ASSERT(r.eos() ||
+           r.GetKey()->Hash() > descriptor_hash ||
+           r.IsNullDescriptor());
   }
   for (; !r.eos(); r.advance()) {
     if (r.IsNullDescriptor()) continue;
@@ -3004,24 +3007,25 @@ Object* DescriptorArray::CopyInsert(Descriptor* descriptor,
 
 
 Object* DescriptorArray::RemoveTransitions() {
-  // Remove all transitions.  Return a copy of the array with all transitions
-  // removed, or a Failure object if the new array could not be allocated.
+  // Remove all transitions and null descriptors. Return a copy of the array
+  // with all transitions removed, or a Failure object if the new array could
+  // not be allocated.
 
   // Compute the size of the map transition entries to be removed.
-  int count_transitions = 0;
+  int num_removed = 0;
   for (DescriptorReader r(this); !r.eos(); r.advance()) {
-    if (r.IsTransition()) count_transitions++;
+    if (!r.IsProperty()) num_removed++;
   }
 
   // Allocate the new descriptor array.
-  Object* result = Allocate(number_of_descriptors() - count_transitions);
+  Object* result = Allocate(number_of_descriptors() - num_removed);
   if (result->IsFailure()) return result;
   DescriptorArray* new_descriptors = DescriptorArray::cast(result);
 
   // Copy the content.
   DescriptorWriter w(new_descriptors);
   for (DescriptorReader r(this); !r.eos(); r.advance()) {
-    if (!r.IsTransition()) w.WriteFrom(&r);
+    if (r.IsProperty()) w.WriteFrom(&r);
   }
   ASSERT(w.eos());
 
@@ -3097,10 +3101,10 @@ int DescriptorArray::BinarySearch(String* name, int low, int high) {
     ASSERT(hash == mid_hash);
     // There might be more, so we find the first one and
     // check them all to see if we have a match.
-    if (name == mid_name) return mid;
+    if (name == mid_name  && !is_null_descriptor(mid)) return mid;
     while ((mid > low) && (GetKey(mid - 1)->Hash() == hash)) mid--;
     for (; (mid <= high) && (GetKey(mid)->Hash() == hash); mid++) {
-      if (GetKey(mid)->Equals(name)) return mid;
+      if (GetKey(mid)->Equals(name) && !is_null_descriptor(mid)) return mid;
     }
     break;
   }
@@ -3110,7 +3114,9 @@ int DescriptorArray::BinarySearch(String* name, int low, int high) {
 
 int DescriptorArray::LinearSearch(String* name, int len) {
   for (int number = 0; number < len; number++) {
-    if (name->Equals(GetKey(number))) return number;
+    if (name->Equals(GetKey(number)) && !is_null_descriptor(number)) {
+      return number;
+    }
   }
   return kNotFound;
 }
@@ -5643,7 +5649,8 @@ int JSObject::NumberOfLocalProperties(PropertyAttributes filter) {
          !r.eos();
          r.advance()) {
       PropertyDetails details = r.GetDetails();
-      if (!details.IsTransition() && (details.attributes() & filter) == 0) {
+      if (details.IsProperty() &&
+          (details.attributes() & filter) == 0) {
         result++;
       }
     }
@@ -5785,7 +5792,7 @@ void JSObject::GetLocalPropertyNames(FixedArray* storage, int index) {
     for (DescriptorReader r(map()->instance_descriptors());
          !r.eos();
          r.advance()) {
-      if (!r.IsTransition()) {
+      if (r.IsProperty()) {
         storage->set(index++, r.GetKey());
       }
     }

src/objects.h

@@ -144,6 +144,10 @@ class PropertyDetails BASE_EMBEDDED {
     return t == MAP_TRANSITION || t == CONSTANT_TRANSITION;
   }
 
+  bool IsProperty() {
+    return type() < FIRST_PHANTOM_PROPERTY_TYPE;
+  }
+
   PropertyAttributes attributes() { return AttributesField::decode(value_); }
 
   int index() { return IndexField::decode(value_); }
@@ -1718,6 +1722,10 @@ class DescriptorArray: public FixedArray {
     return( descriptor_number << 1) + 1;
   }
 
+  bool is_null_descriptor(int descriptor_number) {
+    return PropertyDetails(GetDetails(descriptor_number)).type() ==
+        NULL_DESCRIPTOR;
+  }
   // Swap operation on FixedArray without using write barriers.
   static inline void fast_swap(FixedArray* array, int first, int second);
 

src/property.h

@@ -356,6 +356,10 @@ class DescriptorReader: public DescriptorStream {
     return type() == NULL_DESCRIPTOR;
   }
 
+  bool IsProperty() {
+    return type() < FIRST_PHANTOM_PROPERTY_TYPE;
+  }
+
   JSFunction* GetConstantFunction() { return JSFunction::cast(GetValue()); }
 
   AccessorDescriptor* GetCallbacks() {

test/mjsunit/regress/regress-1493017.js

+// Copyright 2008 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Test for collection of abandoned maps
+
+// This test makes a wide, shallow tree of map transitions and maps
+// by adding the properties "a" through "j" in a pseudorandom order
+// to a new A() object.  This should create map transitions forming
+// a partial denary tree.  These objects only stick around for about
+// 1000 iterations, with each iteration creating a new object.  Therefore,
+// some of the maps going to leaves will become abandoned.
+// There are still map transitions going to them though, so
+// only the new map-collection code will remove them.
+
+// Every 101 object creations, the object is created again, and tested
+// after each property addition to make sure that no map transitions
+// are visible as properties.  This is a regression test for a bug.
+
+// Flags: --expose-gc --collect-maps
+
+function dotest() {
+  function A() {
+  }
+
+  function B() {
+    this.x = 3;
+  }
+
+  var a_B = new B();
+  var r = 1;
+  var i = 0;
+  var holder = new Array();
+  while (i++ < 15000) {
+    if (i == 14000) {
+      gc();
+    }
+    var s = r % 100000000;
+    var obj = new A();
+    holder[i % 1000] = obj;
+    while (s > 0) {
+      var property_name = String.fromCharCode(s % 10 + 97);
+      obj[property_name] = a_B;
+      s = s / 10;
+    }
+    if ( i % 101 == 0 ) {
+      // Check that all object maps have no undefined properties
+      s = r % 100000000;
+      obj = new A();
+      while (s > 0) {
+        for (var p in obj) {
+          assertEquals(a_B, obj[p] );
+        }
+        property_name = String.fromCharCode(s % 10 + 97);
+        obj[property_name] = a_B;
+      s = s / 10;
+      }
+    }
+    r = r * 7 % 100000000;
+  }
+}
+
+dotest();