From b34e202b204490737102387722563fd1b5ae9b55 Mon Sep 17 00:00:00 2001 From: "svenpanne@chromium.org" Date: Fri, 3 Feb 2012 13:37:13 +0000 Subject: [PATCH] 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 | 35 ++++++++++++++--------------- src/objects-inl.h | 24 ++++++++++++++++++-- src/objects.cc | 60 ++++++++++++++++++++++++++++++++------------------ src/objects.h | 4 +++- src/property-details.h | 28 +---------------------- src/property.cc | 29 +++++++++++++++++++++++- src/property.h | 8 +++++-- 7 files changed, 115 insertions(+), 73 deletions(-) diff --git a/src/ast.cc b/src/ast.cc index 811193b..18eb90c 100644 --- a/src/ast.cc +++ b/src/ast.cc @@ -730,33 +730,32 @@ void CaseClause::RecordTypeFeedback(TypeFeedbackOracle* oracle) { bool Call::ComputeTarget(Handle type, Handle 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::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::cast(type->prototype())); - type = Handle(holder()->map()); - } else if (lookup.IsFound() && lookup.type() == CONSTANT_FUNCTION) { - target_ = Handle(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(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::cast(type->prototype())); + type = Handle(holder()->map()); } } diff --git a/src/objects-inl.h b/src/objects-inl.h index 7901f08..6eeba20 100644 --- a/src/objects-inl.h +++ b/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; } diff --git a/src/objects.cc b/src/objects.cc index a1e37b1..5e472ba 100644 --- a/src/objects.cc +++ b/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 MaybeObject* HashTable::Rehash(HashTable* new_table, Key key) { ASSERT(NumberOfElements() < new_table->Capacity()); diff --git a/src/objects.h b/src/objects.h index 9cd8d72..6edd6cc 100644 --- a/src/objects.h +++ b/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 { // Find entry for key, otherwise return kNotFound. Optimized version of // HashTable::FindEntry. int FindEntry(String* key); + + bool ContainsTransition(int entry); }; diff --git a/src/property-details.h b/src/property-details.h index 135c2ca..d4a4125 100644 --- a/src/property-details.h +++ b/src/property-details.h @@ -1,4 +1,4 @@ -// 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()); } diff --git a/src/property.cc b/src/property.cc index 6e043e2..78f237d 100644 --- a/src/property.cc +++ b/src/property.cc @@ -1,4 +1,4 @@ -// 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 diff --git a/src/property.h b/src/property.h index 120734d..bcf1d66 100644 --- a/src/property.h +++ b/src/property.h @@ -1,4 +1,4 @@ -// 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()); } -- 2.7.4