Avoid one repeated property lookup when computing store ICs.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index ceed585d056b9dae7501e7b121b9292037171323..1beb2190cac3ec2b6c99f158b0bb0143896c1287 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -202,15 +202,11 @@ Code* IC::GetOriginalCode() const {
 }
 
 
-static bool HasInterceptorSetter(JSObject* object) {
-  return !object->GetNamedInterceptor()->setter()->IsUndefined();
-}
-
-
 static void LookupForRead(LookupIterator* it) {
   for (; it->IsFound(); it->Next()) {
     switch (it->state()) {
       case LookupIterator::NOT_FOUND:
+      case LookupIterator::TRANSITION:
         UNREACHABLE();
       case LookupIterator::JSPROXY:
         return;
@@ -302,7 +298,7 @@ bool IC::IsNameCompatibleWithPrototypeFailure(Handle<Object> name) {
 
 
 void IC::UpdateState(Handle<Object> receiver, Handle<Object> name) {
-  receiver_type_ = CurrentTypeOf(receiver, isolate());
+  update_receiver_type(receiver);
   if (!name->IsString()) return;
   if (state() != MONOMORPHIC && state() != POLYMORPHIC) return;
   if (receiver->IsUndefined() || receiver->IsNull()) return;
@@ -955,50 +951,6 @@ Handle<Code> IC::ComputeHandler(LookupIterator* lookup, Handle<Object> object,
 }
 
 
-Handle<Code> IC::ComputeStoreHandler(LookupResult* lookup,
-                                     Handle<Object> object, Handle<Name> name,
-                                     Handle<Object> value) {
-  bool receiver_is_holder = lookup->ReceiverIsHolder(object);
-  CacheHolderFlag flag;
-  Handle<Map> stub_holder_map = IC::GetHandlerCacheHolder(
-      *receiver_type(), receiver_is_holder, isolate(), &flag);
-
-  Handle<Code> code = PropertyHandlerCompiler::Find(
-      name, stub_holder_map, handler_kind(), flag,
-      lookup->holder()->HasFastProperties() ? Code::FAST : Code::NORMAL);
-  // Use the cached value if it exists, and if it is different from the
-  // handler that just missed.
-  if (!code.is_null()) {
-    if (!maybe_handler_.is_null() &&
-        !maybe_handler_.ToHandleChecked().is_identical_to(code)) {
-      return code;
-    }
-    if (maybe_handler_.is_null()) {
-      // maybe_handler_ is only populated for MONOMORPHIC and POLYMORPHIC ICs.
-      // In MEGAMORPHIC case, check if the handler in the megamorphic stub
-      // cache (which just missed) is different from the cached handler.
-      if (state() == MEGAMORPHIC && object->IsHeapObject()) {
-        Map* map = Handle<HeapObject>::cast(object)->map();
-        Code* megamorphic_cached_code =
-            isolate()->stub_cache()->Get(*name, map, code->flags());
-        if (megamorphic_cached_code != *code) return code;
-      } else {
-        return code;
-      }
-    }
-  }
-
-  code = CompileStoreHandler(lookup, object, name, value, flag);
-  DCHECK(code->is_handler());
-
-  if (code->type() != Code::NORMAL) {
-    Map::UpdateCodeCache(stub_holder_map, name, code);
-  }
-
-  return code;
-}
-
-
 Handle<Code> LoadIC::CompileHandler(LookupIterator* lookup,
                                     Handle<Object> object, Handle<Name> name,
                                     Handle<Object> unused,
@@ -1040,9 +992,9 @@ Handle<Code> LoadIC::CompileHandler(LookupIterator* lookup,
     LookupForRead(&it);
     return compiler.CompileLoadInterceptor(&it, name);
   }
-  DCHECK(lookup->state() == LookupIterator::PROPERTY);
 
   // -------------- Accessors --------------
+  DCHECK(lookup->state() == LookupIterator::PROPERTY);
   if (lookup->property_kind() == LookupIterator::ACCESSOR) {
     // Use simple field loads for some well-known callback properties.
     if (receiver_is_holder) {
@@ -1284,70 +1236,64 @@ MaybeHandle<Object> KeyedLoadIC::Load(Handle<Object> object,
 }
 
 
-static bool LookupForWrite(Handle<Object> object, Handle<Name> name,
-                           Handle<Object> value, LookupResult* lookup, IC* ic) {
+bool StoreIC::LookupForWrite(Handle<Object> object, Handle<Name> name,
+                             Handle<Object> value, LookupIterator* it,
+                             JSReceiver::StoreFromKeyed store_mode) {
   // Disable ICs for non-JSObjects for now.
   if (!object->IsJSObject()) return false;
   Handle<JSObject> receiver = Handle<JSObject>::cast(object);
+  DCHECK(!receiver->map()->is_deprecated());
 
-  Handle<JSObject> holder = receiver;
-  receiver->Lookup(name, lookup);
-  if (lookup->IsFound()) {
-    if (lookup->IsInterceptor() && !HasInterceptorSetter(lookup->holder())) {
-      receiver->LookupOwnRealNamedProperty(name, lookup);
-      if (!lookup->IsFound()) return false;
-    }
-
-    if (lookup->IsReadOnly() || !lookup->IsCacheable()) return false;
-    if (lookup->holder() == *receiver) return lookup->CanHoldValue(value);
-    if (lookup->IsPropertyCallbacks()) return true;
-    // JSGlobalProxy either stores on the global object in the prototype, or
-    // goes into the runtime if access checks are needed, so this is always
-    // safe.
-    if (receiver->IsJSGlobalProxy()) {
-      PrototypeIterator iter(lookup->isolate(), receiver);
-      return lookup->holder() == *PrototypeIterator::GetCurrent(iter);
+  for (; it->IsFound(); it->Next()) {
+    switch (it->state()) {
+      case LookupIterator::NOT_FOUND:
+      case LookupIterator::TRANSITION:
+        UNREACHABLE();
+      case LookupIterator::JSPROXY:
+        return false;
+      case LookupIterator::INTERCEPTOR: {
+        Handle<JSObject> holder = it->GetHolder<JSObject>();
+        InterceptorInfo* info = holder->GetNamedInterceptor();
+        if (holder.is_identical_to(receiver)) {
+          if (!info->setter()->IsUndefined()) return true;
+        } else if (!info->getter()->IsUndefined() ||
+                   !info->query()->IsUndefined()) {
+          return false;
+        }
+        break;
+      }
+      case LookupIterator::ACCESS_CHECK:
+        if (it->GetHolder<JSObject>()->IsAccessCheckNeeded()) return false;
+        break;
+      case LookupIterator::PROPERTY: {
+        if (!it->HasProperty()) break;
+        if (it->IsReadOnly()) return false;
+        if (it->property_kind() == LookupIterator::ACCESSOR) return true;
+        if (it->GetHolder<JSObject>().is_identical_to(receiver)) {
+          it->PrepareForDataProperty(value);
+          // The previous receiver map might just have been deprecated,
+          // so reload it.
+          update_receiver_type(receiver);
+          return true;
+        } else {
+          // Receiver != holder.
+          if (receiver->IsJSGlobalProxy()) {
+            PrototypeIterator iter(it->isolate(), receiver);
+            return it->GetHolder<Object>().is_identical_to(
+                PrototypeIterator::GetCurrent(iter));
+          }
+          if (it->property_encoding() == LookupIterator::DICTIONARY) {
+            // DICTIONARY on the prototype chain -> return false
+            return false;
+          }

[Toon Verwaest, 2014/08/21 15:07:05]

You already found a property, so you need to make a decision. I presume you want
to return true?
So basically:
return it->property_encoding() == LookupIterator::DESCRIPTOR?

[Jakob Kummerow, 2014/08/21 16:14:53]

Done.

+        }
+        break;
+      }
     }
-    // Currently normal holders in the prototype chain are not supported. They
-    // would require a runtime positive lookup and verification that the details
-    // have not changed.
-    if (lookup->IsInterceptor() || lookup->IsNormal()) return false;
-    holder = Handle<JSObject>(lookup->holder(), lookup->isolate());
-  }
-
-  // While normally LookupTransition gets passed the receiver, in this case we
-  // pass the holder of the property that we overwrite. This keeps the holder in
-  // the LookupResult intact so we can later use it to generate a prototype
-  // chain check. This avoids a double lookup, but requires us to pass in the
-  // receiver when trying to fetch extra information from the transition.
-  receiver->map()->LookupTransition(*holder, *name, lookup);
-  if (!lookup->IsTransition() || lookup->IsReadOnly()) return false;
-
-  // If the value that's being stored does not fit in the field that the
-  // instance would transition to, create a new transition that fits the value.
-  // This has to be done before generating the IC, since that IC will embed the
-  // transition target.
-  // Ensure the instance and its map were migrated before trying to update the
-  // transition target.
-  DCHECK(!receiver->map()->is_deprecated());
-  if (!lookup->CanHoldValue(value)) {
-    Handle<Map> target(lookup->GetTransitionTarget());
-    Representation field_representation = value->OptimalRepresentation();
-    Handle<HeapType> field_type = value->OptimalType(
-        lookup->isolate(), field_representation);
-    Map::GeneralizeRepresentation(
-        target, target->LastAdded(),
-        field_representation, field_type, FORCE_FIELD);
-    // Lookup the transition again since the transition tree may have changed
-    // entirely by the migration above.
-    receiver->map()->LookupTransition(*holder, *name, lookup);
-    if (!lookup->IsTransition()) return false;
-    if (!ic->IsNameCompatibleWithPrototypeFailure(name)) return false;
-    ic->MarkPrototypeFailure(name);
-    return true;
   }
 
-  return true;
+  it->PrepareTransitionToDataProperty(value, NONE, store_mode);
+  return it->IsCacheableTransition();
 }
 
 
@@ -1399,15 +1345,16 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object,
     return result;
   }
 
-  LookupResult lookup(isolate());
-  bool can_store = LookupForWrite(object, name, value, &lookup, this);
-  if (!can_store &&
-      strict_mode() == STRICT &&
-      !(lookup.IsProperty() && lookup.IsReadOnly()) &&
-      object->IsGlobalObject()) {
+  // LookupResult lookup(isolate());

[Toon Verwaest, 2014/08/21 15:07:05]

Remove leftover

[Jakob Kummerow, 2014/08/21 16:14:53]

Done.

+  LookupIterator lookup(object, name);
+  bool can_store = LookupForWrite(object, name, value, &lookup, store_mode);
+  if (!can_store && strict_mode() == STRICT && object->IsGlobalObject() &&

[Toon Verwaest, 2014/08/21 15:07:05]

This shouldn't depend on "can_store". Before the reference error you could
DCHECK(!can_store) though.

[Jakob Kummerow, 2014/08/21 16:14:53]

It does, actually, because we're interested in nonexistent properties. Removing
the condition would mean that we erroneously throw when trying to overwrite
writable global properties.

+      !(lookup.state() == LookupIterator::PROPERTY && lookup.has_property() &&

[Toon Verwaest, 2014/08/21 15:07:06]

You shouldn't need to expose has_property(). Any iteration that doesn't
has_property after visiting PROPERTY should have called Next(); continuing to
NOT_FOUND.

[Jakob Kummerow, 2014/08/21 16:14:53]

Done.

+        lookup.IsReadOnly())) {
     // Strict mode doesn't allow setting non-existent global property.
     return ReferenceError("not_defined", name);
   }
+
   if (FLAG_use_ic) {
     if (state() == UNINITIALIZED) {
       Handle<Code> stub = pre_monomorphic_stub();
@@ -1415,10 +1362,9 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object,
       TRACE_IC("StoreIC", name);
     } else if (can_store) {
       UpdateCaches(&lookup, Handle<JSObject>::cast(object), name, value);
-    } else if (lookup.IsNormal() ||
-               (lookup.IsField() && lookup.CanHoldValue(value))) {
-      Handle<Code> stub = generic_stub();
-      set_target(*stub);
+    } else {
+      PatchCache(name, slow_stub());
+      TRACE_IC("StoreIC", name);
     }
   }
 
@@ -1426,8 +1372,7 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object,
   Handle<Object> result;
   ASSIGN_RETURN_ON_EXCEPTION(
       isolate(), result,
-      Object::SetProperty(object, name, value, strict_mode(), store_mode),
-      Object);
+      Object::SetProperty(&lookup, value, strict_mode(), store_mode), Object);
   return result;
 }
 
@@ -1475,133 +1420,116 @@ Handle<Code> StoreIC::pre_monomorphic_stub(Isolate* isolate,
 }
 
 
-void StoreIC::UpdateCaches(LookupResult* lookup,
-                           Handle<JSObject> receiver,
-                           Handle<Name> name,
-                           Handle<Object> value) {
-  DCHECK(lookup->IsFound());
-
+void StoreIC::UpdateCaches(LookupIterator* lookup, Handle<JSObject> receiver,
+                           Handle<Name> name, Handle<Object> value) {
   // These are not cacheable, so we never see such LookupResults here.
-  DCHECK(!lookup->IsHandler());
+  DCHECK(lookup->state() != LookupIterator::JSPROXY);

[Toon Verwaest, 2014/08/21 15:07:06]

DCHECK_NE

[Jakob Kummerow, 2014/08/21 16:14:53]

Done.

 
-  Handle<Code> code = ComputeStoreHandler(lookup, receiver, name, value);
+  Handle<Code> code = ComputeHandler(lookup, receiver, name, value);
 
   PatchCache(name, code);
   TRACE_IC("StoreIC", name);
 }
 
 
-Handle<Code> StoreIC::CompileStoreHandler(LookupResult* lookup,
-                                          Handle<Object> object,
-                                          Handle<Name> name,
-                                          Handle<Object> value,
-                                          CacheHolderFlag cache_holder) {
-  if (object->IsAccessCheckNeeded()) return slow_stub();
-  DCHECK(cache_holder == kCacheOnReceiver || lookup->type() == CALLBACKS ||
-         (object->IsJSGlobalProxy() && lookup->holder()->IsJSGlobalObject()));
+Handle<Code> StoreIC::CompileHandler(LookupIterator* lookup,
+                                     Handle<Object> object, Handle<Name> name,
+                                     Handle<Object> value,
+                                     CacheHolderFlag cache_holder) {
+  DCHECK(!object->IsAccessCheckNeeded());
   // This is currently guaranteed by checks in StoreIC::Store.
   Handle<JSObject> receiver = Handle<JSObject>::cast(object);
+  Handle<JSObject> holder(lookup->GetHolder<JSObject>());
 
-  Handle<JSObject> holder(lookup->holder());
+  // -------------- Transition --------------
+  if (lookup->state() == LookupIterator::TRANSITION) {
+    Handle<Map> transition = lookup->transition_map();
 
-  if (lookup->IsTransition()) {
-    // Explicitly pass in the receiver map since LookupForWrite may have
-    // stored something else than the receiver in the holder.
-    Handle<Map> transition(lookup->GetTransitionTarget());
-    PropertyDetails details = lookup->GetPropertyDetails();
+    DCHECK(lookup->IsCacheableTransition());
+    NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
+    return compiler.CompileStoreTransition(transition, name);
+  }
 
-    if (details.type() != CALLBACKS && details.attributes() == NONE &&
-        holder->HasFastProperties()) {
-      NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
-      return compiler.CompileStoreTransition(transition, name);
-    }
-  } else {
-    switch (lookup->type()) {
-      case FIELD: {
-        bool use_stub = true;
-        if (lookup->representation().IsHeapObject()) {
-          // Only use a generic stub if no types need to be tracked.
-          HeapType* field_type = lookup->GetFieldType();
-          HeapType::Iterator<Map> it = field_type->Classes();
-          use_stub = it.Done();
-        }
-        if (use_stub) {
-          StoreFieldStub stub(isolate(), lookup->GetFieldIndex(),
-                              lookup->representation());
-          return stub.GetCode();
-        }
-        NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
-        return compiler.CompileStoreField(lookup, name);
-      }
-      case NORMAL:
-        if (receiver->IsJSGlobalProxy() || receiver->IsGlobalObject()) {
-          // The stub generated for the global object picks the value directly
-          // from the property cell. So the property must be directly on the
-          // global object.
-          PrototypeIterator iter(isolate(), receiver);
-          Handle<GlobalObject> global =
-              receiver->IsJSGlobalProxy()
-                  ? Handle<GlobalObject>::cast(
-                        PrototypeIterator::GetCurrent(iter))
-                  : Handle<GlobalObject>::cast(receiver);
-          Handle<PropertyCell> cell(global->GetPropertyCell(lookup), isolate());
-          Handle<HeapType> union_type = PropertyCell::UpdatedType(cell, value);
-          StoreGlobalStub stub(
-              isolate(), union_type->IsConstant(), receiver->IsJSGlobalProxy());
-          Handle<Code> code = stub.GetCodeCopyFromTemplate(global, cell);
-          // TODO(verwaest): Move caching of these NORMAL stubs outside as well.
-          HeapObject::UpdateMapCodeCache(receiver, name, code);
-          return code;
-        }
-        DCHECK(holder.is_identical_to(receiver));
-        return isolate()->builtins()->StoreIC_Normal();
-      case CALLBACKS: {
-        if (!holder->HasFastProperties()) break;
-        Handle<Object> callback(lookup->GetValueFromMap(holder->map()),
-                                isolate());
-        if (callback->IsExecutableAccessorInfo()) {
-          Handle<ExecutableAccessorInfo> info =
-              Handle<ExecutableAccessorInfo>::cast(callback);
-          if (v8::ToCData<Address>(info->setter()) == 0) break;
-          if (!ExecutableAccessorInfo::IsCompatibleReceiverType(
-                  isolate(), info, receiver_type())) {
-            break;
-          }
-          NamedStoreHandlerCompiler compiler(isolate(), receiver_type(),
-                                             holder);
-          return compiler.CompileStoreCallback(receiver, name, info);
-        } else if (callback->IsAccessorPair()) {
-          Handle<Object> setter(
-              Handle<AccessorPair>::cast(callback)->setter(), isolate());
-          if (!setter->IsJSFunction()) break;
-          Handle<JSFunction> function = Handle<JSFunction>::cast(setter);
-          CallOptimization call_optimization(function);
-          NamedStoreHandlerCompiler compiler(isolate(), receiver_type(),
-                                             holder);
-          if (call_optimization.is_simple_api_call() &&
-              call_optimization.IsCompatibleReceiver(receiver, holder)) {
-            return compiler.CompileStoreCallback(receiver, name,
-                                                 call_optimization);
-          }
-          return compiler.CompileStoreViaSetter(
-              receiver, name, Handle<JSFunction>::cast(setter));
-        }
-        // TODO(dcarney): Handle correctly.
-        DCHECK(callback->IsDeclaredAccessorInfo());
-        break;
+  // -------------- Interceptors --------------
+  if (lookup->state() == LookupIterator::INTERCEPTOR) {
+    DCHECK(!holder->GetNamedInterceptor()->setter()->IsUndefined());
+    NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
+    return compiler.CompileStoreInterceptor(name);
+  }
+
+  // -------------- Accessors --------------
+  DCHECK(lookup->state() == LookupIterator::PROPERTY);
+  if (lookup->property_kind() == LookupIterator::ACCESSOR) {
+    if (!holder->HasFastProperties()) return slow_stub();
+    Handle<Object> accessors = lookup->GetAccessors();
+    if (accessors->IsExecutableAccessorInfo()) {
+      Handle<ExecutableAccessorInfo> info =
+          Handle<ExecutableAccessorInfo>::cast(accessors);
+      if (v8::ToCData<Address>(info->setter()) == 0) return slow_stub();
+      if (!ExecutableAccessorInfo::IsCompatibleReceiverType(isolate(), info,
+                                                            receiver_type())) {
+        return slow_stub();
       }
-      case INTERCEPTOR: {
-        DCHECK(HasInterceptorSetter(*holder));
-        NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
-        return compiler.CompileStoreInterceptor(name);
+      NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
+      return compiler.CompileStoreCallback(receiver, name, info);
+    } else if (accessors->IsAccessorPair()) {
+      Handle<Object> setter(Handle<AccessorPair>::cast(accessors)->setter(),
+                            isolate());
+      if (!setter->IsJSFunction()) return slow_stub();
+      Handle<JSFunction> function = Handle<JSFunction>::cast(setter);
+      CallOptimization call_optimization(function);
+      NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
+      if (call_optimization.is_simple_api_call() &&
+          call_optimization.IsCompatibleReceiver(receiver, holder)) {
+        return compiler.CompileStoreCallback(receiver, name, call_optimization);
       }
-      case CONSTANT:
-        break;
-      case HANDLER:
-        UNREACHABLE();
-        break;
+      return compiler.CompileStoreViaSetter(receiver, name,
+                                            Handle<JSFunction>::cast(setter));
+    }
+    // TODO(dcarney): Handle correctly.
+    DCHECK(accessors->IsDeclaredAccessorInfo());
+    return slow_stub();
+  }
+
+  // -------------- Dictionary properties --------------
+  DCHECK(lookup->property_kind() == LookupIterator::DATA);
+  if (lookup->property_encoding() == LookupIterator::DICTIONARY) {
+    if (holder->IsGlobalObject()) {
+      Handle<PropertyCell> cell = lookup->GetPropertyCell();
+      Handle<HeapType> union_type = PropertyCell::UpdatedType(cell, value);
+      StoreGlobalStub stub(isolate(), union_type->IsConstant(),
+                           receiver->IsJSGlobalProxy());
+      Handle<Code> code = stub.GetCodeCopyFromTemplate(
+          Handle<GlobalObject>::cast(holder), cell);
+      // TODO(verwaest): Move caching of these NORMAL stubs outside as well.
+      HeapObject::UpdateMapCodeCache(receiver, name, code);
+      return code;
+    }
+    DCHECK(holder.is_identical_to(receiver));
+    return isolate()->builtins()->StoreIC_Normal();
+  }
+
+  // -------------- Fields --------------
+  DCHECK(lookup->property_encoding() == LookupIterator::DESCRIPTOR);
+  if (lookup->property_details().type() == FIELD) {
+    bool use_stub = true;
+    if (lookup->representation().IsHeapObject()) {
+      // Only use a generic stub if no types need to be tracked.
+      Handle<HeapType> field_type = lookup->GetFieldType();
+      HeapType::Iterator<Map> it = field_type->Classes();
+      use_stub = it.Done();
     }
+    if (use_stub) {
+      StoreFieldStub stub(isolate(), lookup->GetFieldIndex(),
+                          lookup->representation());
+      return stub.GetCode();
+    }
+    NamedStoreHandlerCompiler compiler(isolate(), receiver_type(), holder);
+    return compiler.CompileStoreField(lookup, name);
   }
+
+  // -------------- Constant properties --------------
+  DCHECK(lookup->property_details().type() == CONSTANT);
   return slow_stub();
 }