Removed IsTransitionType predicate.

With the upcoming changes to CALLBACKS properties, a predicate on the transition
type alone doesn't make sense anymore: For CALLBACKS one has to look into the
property's value to decide, and there is even the possibility of having a an
accessor function *and* a transition in the same property.

I am not completely happy with some parts of this CL, because they contain
redundant code, but given the various representations we currently have for
property type/value pairs, I can see no easy way around that. Perhaps one can
improve this a bit in a different CL, the current diversity really, really hurts
productivity...

As a bonus, this CL includes a few minor things:

 * CaseClause::RecordTypeFeedback has been cleaned up and it handles the
   NULL_DESCRIPTOR case correctly now. Under some (very unlikely) circumstances,
   we previously missed some opportunities for monomorphic calls. In general, it
   is rather unfortunate that NULL_DESCRIPTOR "shines through", it is just a
   hack for the inability to remove a descriptor entry during GC, something
   callers shouldn't have to be aware of.

 * DescriptorArray::CopyInsert has been cleaned up a bit, preparing it for later
   CALLBACKS-related changes.

 * LookupResult::Print is now more informative for CONSTANT_TRANSITION.

Review URL: https://chromiumcodereview.appspot.com/9320066

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

src/ast.cc

@@ -730,33 +730,32 @@ void CaseClause::RecordTypeFeedback(TypeFeedbackOracle* oracle) {
 
 
 bool Call::ComputeTarget(Handle<Map> type, Handle<String> name) {
-  // If there is an interceptor, we can't compute the target for
-  // a direct call.
+  // If there is an interceptor, we can't compute the target for a direct call.
   if (type->has_named_interceptor()) return false;
 
   if (check_type_ == RECEIVER_MAP_CHECK) {
-    // For primitive checks the holder is set up to point to the
-    // corresponding prototype object, i.e. one step of the algorithm
-    // below has been already performed.
-    // For non-primitive checks we clear it to allow computing targets
-    // for polymorphic calls.
+    // For primitive checks the holder is set up to point to the corresponding
+    // prototype object, i.e. one step of the algorithm below has been already
+    // performed. For non-primitive checks we clear it to allow computing
+    // targets for polymorphic calls.
     holder_ = Handle<JSObject>::null();
   }
+  LookupResult lookup(type->GetIsolate());
   while (true) {
-    LookupResult lookup(type->GetIsolate());
     type->LookupInDescriptors(NULL, *name, &lookup);
-    // If the function wasn't found directly in the map, we start
-    // looking upwards through the prototype chain.
-    if ((!lookup.IsFound() || IsTransitionType(lookup.type()))
-        && type->prototype()->IsJSObject()) {
-      holder_ = Handle<JSObject>(JSObject::cast(type->prototype()));
-      type = Handle<Map>(holder()->map());
-    } else if (lookup.IsFound() && lookup.type() == CONSTANT_FUNCTION) {
-      target_ = Handle<JSFunction>(lookup.GetConstantFunctionFromMap(*type));
-      return true;
-    } else {
+    // For properties we know the target iff we have a constant function.
+    if (lookup.IsFound() && lookup.IsProperty()) {
+      if (lookup.type() == CONSTANT_FUNCTION) {
+        target_ = Handle<JSFunction>(lookup.GetConstantFunctionFromMap(*type));
+        return true;
+      }
       return false;
     }
+    // If we reach the end of the prototype chain, we don't know the target.
+    if (!type->prototype()->IsJSObject()) return false;
+    // Go up the prototype chain, recording where we are currently.
+    holder_ = Handle<JSObject>(JSObject::cast(type->prototype()));
+    type = Handle<Map>(holder()->map());
   }
 }
 

src/objects-inl.h

@@ -1991,8 +1991,28 @@ bool DescriptorArray::IsProperty(int descriptor_number) {
 }
 
 
-bool DescriptorArray::IsTransition(int descriptor_number) {
-  return IsTransitionType(GetType(descriptor_number));
+bool DescriptorArray::IsTransitionOnly(int descriptor_number) {
+  switch (GetType(descriptor_number)) {
+    case MAP_TRANSITION:
+    case CONSTANT_TRANSITION:
+    case ELEMENTS_TRANSITION:
+      return true;
+    case CALLBACKS: {
+      Object* value = GetValue(descriptor_number);
+      if (!value->IsAccessorPair()) return false;
+      AccessorPair* accessors = AccessorPair::cast(value);
+      return accessors->getter()->IsMap() && accessors->setter()->IsMap();
+    }
+    case NORMAL:
+    case FIELD:
+    case CONSTANT_FUNCTION:
+    case HANDLER:
+    case INTERCEPTOR:
+    case NULL_DESCRIPTOR:
+      return false;
+  }
+  UNREACHABLE();  // Keep the compiler happy.
+  return false;
 }
 
 

src/objects.cc

@@ -1823,7 +1823,7 @@ MaybeObject* JSObject::ReplaceSlowProperty(String* name,
   int new_enumeration_index = 0;  // 0 means "Use the next available index."
   if (old_index != -1) {
     // All calls to ReplaceSlowProperty have had all transitions removed.
-    ASSERT(!dictionary->DetailsAt(old_index).IsTransition());
+    ASSERT(!dictionary->ContainsTransition(old_index));
     new_enumeration_index = dictionary->DetailsAt(old_index).index();
   }
 
@@ -5729,7 +5729,7 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
   // Conversely, we filter after replacing, so replacing a transition and
   // removing all other transitions is not supported.
   bool remove_transitions = transition_flag == REMOVE_TRANSITIONS;
-  ASSERT(remove_transitions == !descriptor->GetDetails().IsTransition());
+  ASSERT(remove_transitions == !descriptor->ContainsTransition());
   ASSERT(descriptor->GetDetails().type() != NULL_DESCRIPTOR);
 
   // Ensure the key is a symbol.
@@ -5738,29 +5738,18 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
     if (!maybe_result->ToObject(&result)) return maybe_result;
   }
 
-  int transitions = 0;
-  int null_descriptors = 0;
-  if (remove_transitions) {
-    for (int i = 0; i < number_of_descriptors(); i++) {
-      if (IsTransition(i)) transitions++;
-      if (IsNullDescriptor(i)) null_descriptors++;
-    }
-  } else {
-    for (int i = 0; i < number_of_descriptors(); i++) {
-      if (IsNullDescriptor(i)) null_descriptors++;
-    }
+  int new_size = 0;
+  for (int i = 0; i < number_of_descriptors(); i++) {
+    if (IsNullDescriptor(i)) continue;
+    if (remove_transitions && IsTransitionOnly(i)) continue;
+    new_size++;
   }
-  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;
+  const bool replacing = (index != kNotFound);
   bool keep_enumeration_index = false;
-  if (inserting) {
-    ++new_size;
-  }
   if (replacing) {
     // We are replacing an existing descriptor.  We keep the enumeration
     // index of a visible property.
@@ -5775,6 +5764,8 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
      // a transition that will be replaced.  Adjust count in this case.
       ++new_size;
     }
+  } else {
+    ++new_size;
   }
 
   DescriptorArray* new_descriptors;
@@ -5789,7 +5780,7 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
   // Set the enumeration index in the descriptors and set the enumeration index
   // in the result.
   int enumeration_index = NextEnumerationIndex();
-  if (!descriptor->GetDetails().IsTransition()) {
+  if (!descriptor->ContainsTransition()) {
     if (keep_enumeration_index) {
       descriptor->SetEnumerationIndex(
           PropertyDetails(GetDetails(index)).index());
@@ -5812,7 +5803,7 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
       break;
     }
     if (IsNullDescriptor(from_index)) continue;
-    if (remove_transitions && IsTransition(from_index)) continue;
+    if (remove_transitions && IsTransitionOnly(from_index)) continue;
     new_descriptors->CopyFrom(to_index++, this, from_index, witness);
   }
 
@@ -5821,7 +5812,7 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
 
   for (; from_index < number_of_descriptors(); from_index++) {
     if (IsNullDescriptor(from_index)) continue;
-    if (remove_transitions && IsTransition(from_index)) continue;
+    if (remove_transitions && IsTransitionOnly(from_index)) continue;
     new_descriptors->CopyFrom(to_index++, this, from_index, witness);
   }
 
@@ -11133,6 +11124,31 @@ int StringDictionary::FindEntry(String* key) {
 }
 
 
+bool StringDictionary::ContainsTransition(int entry) {
+  switch (DetailsAt(entry).type()) {
+    case MAP_TRANSITION:
+    case CONSTANT_TRANSITION:
+    case ELEMENTS_TRANSITION:
+      return true;
+    case CALLBACKS: {
+      Object* value = ValueAt(entry);
+      if (!value->IsAccessorPair()) return false;
+      AccessorPair* accessors = AccessorPair::cast(value);
+      return accessors->getter()->IsMap() || accessors->setter()->IsMap();
+    }
+    case NORMAL:
+    case FIELD:
+    case CONSTANT_FUNCTION:
+    case HANDLER:
+    case INTERCEPTOR:
+    case NULL_DESCRIPTOR:
+      return false;
+  }
+  UNREACHABLE();  // Keep the compiler happy.
+  return false;
+}
+
+
 template<typename Shape, typename Key>
 MaybeObject* HashTable<Shape, Key>::Rehash(HashTable* new_table, Key key) {
   ASSERT(NumberOfElements() < new_table->Capacity());

src/objects.h

@@ -2412,7 +2412,7 @@ class DescriptorArray: public FixedArray {
   inline Object* GetCallbacksObject(int descriptor_number);
   inline AccessorDescriptor* GetCallbacks(int descriptor_number);
   inline bool IsProperty(int descriptor_number);
-  inline bool IsTransition(int descriptor_number);
+  inline bool IsTransitionOnly(int descriptor_number);
   inline bool IsNullDescriptor(int descriptor_number);
   inline bool IsDontEnum(int descriptor_number);
 
@@ -3038,6 +3038,8 @@ class StringDictionary: public Dictionary<StringDictionaryShape, String*> {
   // Find entry for key, otherwise return kNotFound. Optimized version of
   // HashTable::FindEntry.
   int FindEntry(String* key);
+
+  bool ContainsTransition(int entry);
 };
 
 

src/property-details.h

-// Copyright 2011 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -73,26 +73,6 @@ enum PropertyType {
 };
 
 
-inline bool IsTransitionType(PropertyType type) {
-  switch (type) {
-    case MAP_TRANSITION:
-    case CONSTANT_TRANSITION:
-    case ELEMENTS_TRANSITION:
-      return true;
-    case NORMAL:
-    case FIELD:
-    case CONSTANT_FUNCTION:
-    case CALLBACKS:
-    case HANDLER:
-    case INTERCEPTOR:
-    case NULL_DESCRIPTOR:
-      return false;
-  }
-  UNREACHABLE();  // keep the compiler happy
-  return false;
-}
-
-
 inline bool IsRealProperty(PropertyType type) {
   switch (type) {
     case NORMAL:
@@ -139,12 +119,6 @@ class PropertyDetails BASE_EMBEDDED {
 
   PropertyType type() { return TypeField::decode(value_); }
 
-  bool IsTransition() {
-    PropertyType t = type();
-    ASSERT(t != INTERCEPTOR);
-    return IsTransitionType(t);
-  }
-
   bool IsProperty() {
     return IsRealProperty(type());
   }

src/property.cc

-// Copyright 2011 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -91,6 +91,9 @@ void LookupResult::Print(FILE* out) {
       break;
     case CONSTANT_TRANSITION:
       PrintF(out, " -type = constant property transition\n");
+      PrintF(out, " -map:\n");
+      GetTransitionMap()->Print(out);
+      PrintF(out, "\n");
       break;
     case NULL_DESCRIPTOR:
       PrintF(out, " =type = null descriptor\n");
@@ -111,4 +114,28 @@ void Descriptor::Print(FILE* out) {
 #endif
 
 
+bool Descriptor::ContainsTransition() {
+  switch (details_.type()) {
+    case MAP_TRANSITION:
+    case CONSTANT_TRANSITION:
+    case ELEMENTS_TRANSITION:
+      return true;
+    case CALLBACKS: {
+      if (!value_->IsAccessorPair()) return false;
+      AccessorPair* accessors = AccessorPair::cast(value_);
+      return accessors->getter()->IsMap() || accessors->setter()->IsMap();
+    }
+    case NORMAL:
+    case FIELD:
+    case CONSTANT_FUNCTION:
+    case HANDLER:
+    case INTERCEPTOR:
+    case NULL_DESCRIPTOR:
+      return false;
+  }
+  UNREACHABLE();  // Keep the compiler happy.
+  return false;
+}
+
+
 } }  // namespace v8::internal

src/property.h

-// Copyright 2011 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -71,6 +71,8 @@ class Descriptor BASE_EMBEDDED {
     details_ = PropertyDetails(details_.attributes(), details_.type(), index);
   }
 
+  bool ContainsTransition();
+
  private:
   String* key_;
   Object* value_;
@@ -290,7 +292,9 @@ class LookupResult BASE_EMBEDDED {
 
   Map* GetTransitionMap() {
     ASSERT(lookup_type_ == DESCRIPTOR_TYPE);
-    ASSERT(IsTransitionType(type()));
+    ASSERT(type() == MAP_TRANSITION ||
+           type() == ELEMENTS_TRANSITION ||
+           type() == CONSTANT_TRANSITION);
     return Map::cast(GetValue());
   }