From 1a8bed477ea23e27a37a12267faca891e1c3814a Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Tue, 19 Aug 2014 17:02:04 +0000 Subject: [PATCH] Use LookupIterator to transition to accessors BUG= R=jkummerow@chromium.org Review URL: https://codereview.chromium.org/490533002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23210 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/lookup-inl.h | 1 + src/lookup.cc | 86 ++++++++--- src/lookup.h | 17 ++- src/objects.cc | 259 ++++++++++++++-------------------- src/objects.h | 19 +-- src/runtime.cc | 2 +- src/stub-cache.cc | 12 -- src/stub-cache.h | 2 - src/v8natives.js | 14 +- test/mjsunit/deopt-global-accessor.js | 23 +++ 10 files changed, 229 insertions(+), 206 deletions(-) create mode 100644 test/mjsunit/deopt-global-accessor.js diff --git a/src/lookup-inl.h b/src/lookup-inl.h index ccd1fbb..cdd32f7 100644 --- a/src/lookup-inl.h +++ b/src/lookup-inl.h @@ -29,6 +29,7 @@ JSReceiver* LookupIterator::NextHolder(Map* map) { LookupIterator::State LookupIterator::LookupInHolder(Map* map) { + STATIC_ASSERT(INTERCEPTOR == BEFORE_PROPERTY); DisallowHeapAllocation no_gc; switch (state_) { case NOT_FOUND: diff --git a/src/lookup.cc b/src/lookup.cc index c3e0fdb..3943bf3 100644 --- a/src/lookup.cc +++ b/src/lookup.cc @@ -5,6 +5,7 @@ #include "src/v8.h" #include "src/bootstrapper.h" +#include "src/deoptimizer.h" #include "src/lookup.h" #include "src/lookup-inl.h" @@ -109,6 +110,14 @@ bool LookupIterator::HasProperty() { } +void LookupIterator::ReloadPropertyInformation() { + state_ = BEFORE_PROPERTY; + state_ = LookupInHolder(*holder_map_); + DCHECK(IsFound()); + HasProperty(); +} + + void LookupIterator::PrepareForDataProperty(Handle value) { DCHECK(has_property_); DCHECK(HolderIsReceiverOrHiddenPrototype()); @@ -116,13 +125,7 @@ void LookupIterator::PrepareForDataProperty(Handle value) { holder_map_ = Map::PrepareForDataProperty(holder_map_, descriptor_number(), value); JSObject::MigrateToMap(GetHolder(), holder_map_); - // Reload property information. - if (holder_map_->is_dictionary_map()) { - property_encoding_ = DICTIONARY; - } else { - property_encoding_ = DESCRIPTOR; - } - CHECK(HasProperty()); + ReloadPropertyInformation(); } @@ -137,17 +140,12 @@ void LookupIterator::ReconfigureDataProperty(Handle value, JSObject::MigrateToMap(holder, holder_map_); } - // Reload property information and update the descriptor if in dictionary - // mode. if (holder_map_->is_dictionary_map()) { - property_encoding_ = DICTIONARY; PropertyDetails details(attributes, NORMAL, 0); JSObject::SetNormalizedProperty(holder, name(), value, details); - } else { - property_encoding_ = DESCRIPTOR; } - CHECK(HasProperty()); + ReloadPropertyInformation(); } @@ -172,12 +170,62 @@ void LookupIterator::TransitionToDataProperty( value, attributes, store_mode); JSObject::MigrateToMap(receiver, holder_map_); - // Reload the information. - state_ = NOT_FOUND; - configuration_ = CHECK_PROPERTY; - state_ = LookupInHolder(*holder_map_); - DCHECK(IsFound()); - HasProperty(); + ReloadPropertyInformation(); +} + + +void LookupIterator::TransitionToAccessorProperty( + AccessorComponent component, Handle accessor, + PropertyAttributes attributes) { + DCHECK(!accessor->IsNull()); + // Can only be called when the receiver is a JSObject. JSProxy has to be + // handled via a trap. Adding properties to primitive values is not + // observable. + Handle receiver = Handle::cast(GetReceiver()); + + if (receiver->IsJSGlobalProxy()) { + PrototypeIterator iter(isolate(), receiver); + receiver = + Handle::cast(PrototypeIterator::GetCurrent(iter)); + } + + maybe_holder_ = receiver; + holder_map_ = Map::TransitionToAccessorProperty( + handle(receiver->map()), name_, component, accessor, attributes); + JSObject::MigrateToMap(receiver, holder_map_); + + ReloadPropertyInformation(); + + if (!holder_map_->is_dictionary_map()) return; + + // We have to deoptimize since accesses to data properties may have been + // inlined without a corresponding map-check. + if (holder_map_->IsGlobalObjectMap()) { + Deoptimizer::DeoptimizeGlobalObject(*receiver); + } + + // Install the accessor into the dictionary-mode object. + PropertyDetails details(attributes, CALLBACKS, 0); + Handle pair; + if (IsFound() && HasProperty() && property_kind() == ACCESSOR && + GetAccessors()->IsAccessorPair()) { + pair = Handle::cast(GetAccessors()); + // If the component and attributes are identical, nothing has to be done. + if (pair->get(component) == *accessor) { + if (property_details().attributes() == attributes) return; + } else { + pair = AccessorPair::Copy(pair); + pair->set(component, *accessor); + } + } else { + pair = isolate()->factory()->NewAccessorPair(); + pair->set(component, *accessor); + } + JSObject::SetNormalizedProperty(receiver, name_, pair, details); + + JSObject::ReoptimizeIfPrototype(receiver); + holder_map_ = handle(receiver->map()); + ReloadPropertyInformation(); } diff --git a/src/lookup.h b/src/lookup.h index 53f1a68..5fbc10b 100644 --- a/src/lookup.h +++ b/src/lookup.h @@ -31,11 +31,14 @@ class LookupIterator V8_FINAL BASE_EMBEDDED { }; enum State { + ACCESS_CHECK, + INTERCEPTOR, + JSPROXY, NOT_FOUND, PROPERTY, - INTERCEPTOR, - ACCESS_CHECK, - JSPROXY + // Set state_ to BEFORE_PROPERTY to ensure that the next lookup will be a + // PROPERTY lookup. + BEFORE_PROPERTY = INTERCEPTOR }; enum PropertyKind { @@ -100,6 +103,10 @@ class LookupIterator V8_FINAL BASE_EMBEDDED { bool IsFound() const { return state_ != NOT_FOUND; } void Next(); + void NotFound() { + has_property_ = false; + state_ = NOT_FOUND; + } Heap* heap() const { return isolate_->heap(); } Factory* factory() const { return isolate_->factory(); } @@ -130,6 +137,9 @@ class LookupIterator V8_FINAL BASE_EMBEDDED { Object::StoreFromKeyed store_mode); void ReconfigureDataProperty(Handle value, PropertyAttributes attributes); + void TransitionToAccessorProperty(AccessorComponent component, + Handle accessor, + PropertyAttributes attributes); PropertyKind property_kind() const { DCHECK(has_property_); return property_kind_; @@ -162,6 +172,7 @@ class LookupIterator V8_FINAL BASE_EMBEDDED { MUST_USE_RESULT inline JSReceiver* NextHolder(Map* map); inline State LookupInHolder(Map* map); Handle FetchValue() const; + void ReloadPropertyInformation(); bool IsBootstrapping() const; diff --git a/src/objects.cc b/src/objects.cc index ac4982d..e8c3838 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -6102,51 +6102,6 @@ void JSObject::DefineElementAccessor(Handle object, } -Handle JSObject::CreateAccessorPairFor(Handle object, - Handle name) { - Isolate* isolate = object->GetIsolate(); - LookupResult result(isolate); - object->LookupOwnRealNamedProperty(name, &result); - if (result.IsPropertyCallbacks()) { - // Note that the result can actually have IsDontDelete() == true when we - // e.g. have to fall back to the slow case while adding a setter after - // successfully reusing a map transition for a getter. Nevertheless, this is - // OK, because the assertion only holds for the whole addition of both - // accessors, not for the addition of each part. See first comment in - // DefinePropertyAccessor below. - Object* obj = result.GetCallbackObject(); - if (obj->IsAccessorPair()) { - return AccessorPair::Copy(handle(AccessorPair::cast(obj), isolate)); - } - } - return isolate->factory()->NewAccessorPair(); -} - - -void JSObject::DefinePropertyAccessor(Handle object, - Handle name, - Handle getter, - Handle setter, - PropertyAttributes attributes) { - // We could assert that the property is configurable here, but we would need - // to do a lookup, which seems to be a bit of overkill. - bool only_attribute_changes = getter->IsNull() && setter->IsNull(); - if (object->HasFastProperties() && !only_attribute_changes && - (object->map()->NumberOfOwnDescriptors() <= kMaxNumberOfDescriptors)) { - bool getterOk = getter->IsNull() || - DefineFastAccessor(object, name, ACCESSOR_GETTER, getter, attributes); - bool setterOk = !getterOk || setter->IsNull() || - DefineFastAccessor(object, name, ACCESSOR_SETTER, setter, attributes); - if (getterOk && setterOk) return; - } - - Handle accessors = CreateAccessorPairFor(object, name); - accessors->SetComponents(*getter, *setter); - - SetPropertyCallback(object, name, accessors, attributes); -} - - bool Map::DictionaryElementsInPrototypeChainOnly() { if (IsDictionaryElementsKind(elements_kind())) { return false; @@ -6305,7 +6260,19 @@ MaybeHandle JSObject::DefineAccessor(Handle object, if (is_element) { DefineElementAccessor(object, index, getter, setter, attributes); } else { - DefinePropertyAccessor(object, name, getter, setter, attributes); + DCHECK(getter->IsSpecFunction() || getter->IsUndefined() || + getter->IsNull()); + DCHECK(setter->IsSpecFunction() || setter->IsUndefined() || + setter->IsNull()); + // At least one of the accessors needs to be a new value. + DCHECK(!getter->IsNull() || !setter->IsNull()); + LookupIterator it(object, name, LookupIterator::CHECK_PROPERTY); + if (!getter->IsNull()) { + it.TransitionToAccessorProperty(ACCESSOR_GETTER, getter, attributes); + } + if (!setter->IsNull()) { + it.TransitionToAccessorProperty(ACCESSOR_SETTER, setter, attributes); + } } if (is_observed) { @@ -6317,111 +6284,6 @@ MaybeHandle JSObject::DefineAccessor(Handle object, } -static bool TryAccessorTransition(Handle self, - Handle transitioned_map, - int target_descriptor, - AccessorComponent component, - Handle accessor, - PropertyAttributes attributes) { - DescriptorArray* descs = transitioned_map->instance_descriptors(); - PropertyDetails details = descs->GetDetails(target_descriptor); - - // If the transition target was not callbacks, fall back to the slow case. - if (details.type() != CALLBACKS) return false; - Object* descriptor = descs->GetCallbacksObject(target_descriptor); - if (!descriptor->IsAccessorPair()) return false; - - Object* target_accessor = AccessorPair::cast(descriptor)->get(component); - PropertyAttributes target_attributes = details.attributes(); - - // Reuse transition if adding same accessor with same attributes. - if (target_accessor == *accessor && target_attributes == attributes) { - JSObject::MigrateToMap(self, transitioned_map); - return true; - } - - // If either not the same accessor, or not the same attributes, fall back to - // the slow case. - return false; -} - - -bool JSObject::DefineFastAccessor(Handle object, - Handle name, - AccessorComponent component, - Handle accessor, - PropertyAttributes attributes) { - DCHECK(accessor->IsSpecFunction() || accessor->IsUndefined()); - Isolate* isolate = object->GetIsolate(); - LookupResult result(isolate); - object->LookupOwn(name, &result); - - if (result.IsFound() && !result.IsPropertyCallbacks()) { - return false; - } - - // Return success if the same accessor with the same attributes already exist. - AccessorPair* source_accessors = NULL; - if (result.IsPropertyCallbacks()) { - Object* callback_value = result.GetCallbackObject(); - if (callback_value->IsAccessorPair()) { - source_accessors = AccessorPair::cast(callback_value); - Object* entry = source_accessors->get(component); - if (entry == *accessor && result.GetAttributes() == attributes) { - return true; - } - } else { - return false; - } - - int descriptor_number = result.GetDescriptorIndex(); - - object->map()->LookupTransition(*object, *name, &result); - - if (result.IsFound()) { - Handle target(result.GetTransitionTarget()); - DCHECK(target->NumberOfOwnDescriptors() == - object->map()->NumberOfOwnDescriptors()); - // This works since descriptors are sorted in order of addition. - DCHECK(Name::Equals( - handle(object->map()->instance_descriptors()->GetKey( - descriptor_number)), - name)); - return TryAccessorTransition(object, target, descriptor_number, - component, accessor, attributes); - } - } else { - // If not, lookup a transition. - object->map()->LookupTransition(*object, *name, &result); - - // If there is a transition, try to follow it. - if (result.IsFound()) { - Handle target(result.GetTransitionTarget()); - int descriptor_number = target->LastAdded(); - DCHECK(Name::Equals(name, - handle(target->instance_descriptors()->GetKey(descriptor_number)))); - return TryAccessorTransition(object, target, descriptor_number, - component, accessor, attributes); - } - } - - // If there is no transition yet, add a transition to the a new accessor pair - // containing the accessor. Allocate a new pair if there were no source - // accessors. Otherwise, copy the pair and modify the accessor. - Handle accessors = source_accessors != NULL - ? AccessorPair::Copy(Handle(source_accessors)) - : isolate->factory()->NewAccessorPair(); - accessors->set(component, *accessor); - - CallbacksDescriptor new_accessors_desc(name, accessors, attributes); - Handle new_map = Map::CopyInsertDescriptor( - handle(object->map()), &new_accessors_desc, INSERT_TRANSITION); - - JSObject::MigrateToMap(object, new_map); - return true; -} - - MaybeHandle JSObject::SetAccessor(Handle object, Handle info) { Isolate* isolate = object->GetIsolate(); @@ -7062,6 +6924,101 @@ Handle Map::ReconfigureDataProperty(Handle map, int descriptor, } +Handle Map::TransitionToAccessorProperty(Handle map, + Handle name, + AccessorComponent component, + Handle accessor, + PropertyAttributes attributes) { + Isolate* isolate = name->GetIsolate(); + + // Dictionary maps can always have additional data properties. + if (map->is_dictionary_map()) { + // For global objects, property cells are inlined. We need to change the + // map. + if (map->IsGlobalObjectMap()) return Copy(map); + return map; + } + + // Migrate to the newest map before transitioning to the new property. + if (map->is_deprecated()) map = Update(map); + + PropertyNormalizationMode mode = map->is_prototype_map() + ? KEEP_INOBJECT_PROPERTIES + : CLEAR_INOBJECT_PROPERTIES; + + int index = map->SearchTransition(*name); + if (index != TransitionArray::kNotFound) { + Handle transition(map->GetTransition(index)); + DescriptorArray* descriptors = transition->instance_descriptors(); + // Fast path, assume that we're modifying the last added descriptor. + int descriptor = transition->LastAdded(); + if (descriptors->GetKey(descriptor) != *name) { + // If not, search for the descriptor. + descriptor = descriptors->SearchWithCache(*name, *transition); + } + + if (descriptors->GetDetails(descriptor).type() != CALLBACKS) { + return Map::Normalize(map, mode); + } + + // TODO(verwaest): Handle attributes better. + if (descriptors->GetDetails(descriptor).attributes() != attributes) { + return Map::Normalize(map, mode); + } + + Handle maybe_pair(descriptors->GetValue(descriptor), isolate); + if (!maybe_pair->IsAccessorPair()) { + return Map::Normalize(map, mode); + } + + Handle pair = Handle::cast(maybe_pair); + if (pair->get(component) != *accessor) { + return Map::Normalize(map, mode); + } + + return transition; + } + + Handle pair; + DescriptorArray* old_descriptors = map->instance_descriptors(); + int descriptor = old_descriptors->SearchWithCache(*name, *map); + if (descriptor != DescriptorArray::kNotFound) { + PropertyDetails old_details = old_descriptors->GetDetails(descriptor); + if (old_details.type() != CALLBACKS) { + return Map::Normalize(map, mode); + } + + if (old_details.attributes() != attributes) { + return Map::Normalize(map, mode); + } + + Handle maybe_pair(old_descriptors->GetValue(descriptor), isolate); + if (!maybe_pair->IsAccessorPair()) { + return Map::Normalize(map, mode); + } + + Object* current = Handle::cast(maybe_pair)->get(component); + if (current == *accessor) return map; + + if (!current->IsTheHole()) { + return Map::Normalize(map, mode); + } + + pair = AccessorPair::Copy(Handle::cast(maybe_pair)); + } else if (map->NumberOfOwnDescriptors() >= kMaxNumberOfDescriptors || + map->TooManyFastProperties(CERTAINLY_NOT_STORE_FROM_KEYED)) { + return Map::Normalize(map, CLEAR_INOBJECT_PROPERTIES); + } else { + pair = isolate->factory()->NewAccessorPair(); + } + + pair->set(component, *accessor); + TransitionFlag flag = INSERT_TRANSITION; + CallbacksDescriptor new_desc(name, pair, attributes); + return Map::CopyInsertDescriptor(map, &new_desc, flag); +} + + Handle Map::CopyAddDescriptor(Handle map, Descriptor* descriptor, TransitionFlag flag) { diff --git a/src/objects.h b/src/objects.h index af1152d..92d5ab5 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2724,22 +2724,6 @@ class JSObject: public JSReceiver { Handle getter, Handle setter, PropertyAttributes attributes); - static Handle CreateAccessorPairFor(Handle object, - Handle name); - static void DefinePropertyAccessor(Handle object, - Handle name, - Handle getter, - Handle setter, - PropertyAttributes attributes); - - // Try to define a single accessor paying attention to map transitions. - // Returns false if this was not possible and we have to use the slow case. - static bool DefineFastAccessor(Handle object, - Handle name, - AccessorComponent component, - Handle accessor, - PropertyAttributes attributes); - // Return the hash table backing store or the inline stored identity hash, // whatever is found. @@ -6462,6 +6446,9 @@ class Map: public HeapObject { Handle value, PropertyAttributes attributes, StoreFromKeyed store_mode); + static Handle TransitionToAccessorProperty( + Handle map, Handle name, AccessorComponent component, + Handle accessor, PropertyAttributes attributes); static Handle ReconfigureDataProperty(Handle map, int descriptor, PropertyAttributes attributes); diff --git a/src/runtime.cc b/src/runtime.cc index 87d8ce4..4aeef9d 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -4948,7 +4948,7 @@ static bool IsValidAccessor(Handle obj) { // Transform getter or setter into something DefineAccessor can handle. static Handle InstantiateAccessorComponent(Isolate* isolate, Handle component) { - if (component->IsUndefined()) return isolate->factory()->null_value(); + if (component->IsUndefined()) return isolate->factory()->undefined_value(); Handle info = Handle::cast(component); return Utils::OpenHandle(*Utils::ToLocal(info)->GetFunction()); diff --git a/src/stub-cache.cc b/src/stub-cache.cc index d812588..e89a6b6 100644 --- a/src/stub-cache.cc +++ b/src/stub-cache.cc @@ -1193,18 +1193,6 @@ void ElementHandlerCompiler::GenerateStoreDictionaryElement( } -CallOptimization::CallOptimization(LookupResult* lookup) { - if (lookup->IsFound() && - lookup->IsCacheable() && - lookup->IsConstantFunction()) { - // We only optimize constant function calls. - Initialize(Handle(lookup->GetConstantFunction())); - } else { - Initialize(Handle::null()); - } -} - - CallOptimization::CallOptimization(Handle function) { Initialize(function); } diff --git a/src/stub-cache.h b/src/stub-cache.h index 030e1dc..d8aafdf 100644 --- a/src/stub-cache.h +++ b/src/stub-cache.h @@ -626,8 +626,6 @@ class ElementHandlerCompiler : public PropertyHandlerCompiler { // Holds information about possible function call optimizations. class CallOptimization BASE_EMBEDDED { public: - explicit CallOptimization(LookupResult* lookup); - explicit CallOptimization(Handle function); bool is_constant_call() const { diff --git a/src/v8natives.js b/src/v8natives.js index 9612f16..00d8a88 100644 --- a/src/v8natives.js +++ b/src/v8natives.js @@ -840,8 +840,18 @@ function DefineObjectProperty(obj, p, desc, should_throw) { // property. // Step 12 - updating an existing accessor property with an accessor // descriptor. - var getter = desc.hasGetter() ? desc.getGet() : null; - var setter = desc.hasSetter() ? desc.getSet() : null; + var getter = null; + if (desc.hasGetter()) { + getter = desc.getGet(); + } else if (IsAccessorDescriptor(current) && current.hasGetter()) { + getter = current.getGet(); + } + var setter = null; + if (desc.hasSetter()) { + setter = desc.getSet(); + } else if (IsAccessorDescriptor(current) && current.hasSetter()) { + setter = current.getSet(); + } %DefineAccessorPropertyUnchecked(obj, p, getter, setter, flag); } return true; diff --git a/test/mjsunit/deopt-global-accessor.js b/test/mjsunit/deopt-global-accessor.js new file mode 100644 index 0000000..5c544a0 --- /dev/null +++ b/test/mjsunit/deopt-global-accessor.js @@ -0,0 +1,23 @@ +// Copyright 2014 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax + +x = 1; +x = 2; +x = 3; + +function f() { + return x; +} + +f(); +f(); +f(); +%OptimizeFunctionOnNextCall(f); +f(); + +Object.defineProperty(this, "x", {get:function() { return 100; }}); + +assertEquals(100, f()); -- 2.7.4