Avoid one repeated property lookup when computing load ICs.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index c11f58244e05b2299d61280f5fa0044d98a6c8ff..f2303d89289c1e12092f1d291052ffaa7784767b 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -190,48 +190,32 @@ Code* IC::GetOriginalCode() const {
 }
 
 
-static bool HasInterceptorGetter(JSObject* object) {
-  return !object->GetNamedInterceptor()->getter()->IsUndefined();
-}
-
-
 static bool HasInterceptorSetter(JSObject* object) {
   return !object->GetNamedInterceptor()->setter()->IsUndefined();
 }
 
 
-static void LookupForRead(Handle<Object> object,
-                          Handle<String> name,
-                          LookupResult* lookup) {
-  // Skip all the objects with named interceptors, but
-  // without actual getter.
-  while (true) {
-    object->Lookup(name, lookup);
-    // Besides normal conditions (property not found or it's not
-    // an interceptor), bail out if lookup is not cacheable: we won't
-    // be able to IC it anyway and regular lookup should work fine.
-    if (!lookup->IsInterceptor() || !lookup->IsCacheable()) {
-      return;
-    }
-
-    Handle<JSObject> holder(lookup->holder(), lookup->isolate());
-    if (HasInterceptorGetter(*holder)) {
-      return;
-    }
-
-    holder->LookupOwnRealNamedProperty(name, lookup);
-    if (lookup->IsFound()) {
-      ASSERT(!lookup->IsInterceptor());
-      return;
-    }
-
-    PrototypeIterator iter(lookup->isolate(), holder);
-    if (iter.IsAtEnd()) {
-      ASSERT(!lookup->IsFound());
-      return;
+static void LookupForRead(LookupIterator* it) {
+  for (; it->IsFound(); it->Next()) {
+    switch (it->state()) {
+      case LookupIterator::NOT_FOUND:
+        UNREACHABLE();
+      case LookupIterator::JSPROXY:
+        return;
+      case LookupIterator::INTERCEPTOR: {
+        // If there is a getter, return; otherwise loop to perform the lookup.
+        Handle<JSObject> holder = it->GetHolder<JSObject>();
+        if (!holder->GetNamedInterceptor()->getter()->IsUndefined()) {

[Toon Verwaest, 2014/07/30 16:54:52]

In the future we should probably get the property from the interceptor to see if
it's there. If not, we should continue the lookup behind the interceptor and
compile this into the IC as well.

Since this matches what we did before, it's fine for now.

[Jakob Kummerow, 2014/07/31 13:55:51]

But that assumes that the interceptor never changes its behavior, right? I'm not
sure we should make that assumption.
Acknowledged.

+          return;
+        }
+        break;
+      }
+      case LookupIterator::ACCESS_CHECK:
+        return;
+      case LookupIterator::PROPERTY:
+        if (it->HasProperty()) return;  // Yay!
+        break;
     }
-
-    object = PrototypeIterator::GetCurrent(iter);
   }
 }
 
@@ -574,11 +558,11 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<String> name) {
   bool use_ic = MigrateDeprecated(object) ? false : FLAG_use_ic;
 
   // Named lookup in the object.
-  LookupResult lookup(isolate());
-  LookupForRead(object, name, &lookup);
+  LookupIterator it(object, name);
+  LookupForRead(&it);
 
   // If we did not find a property, check if we need to throw an exception.
-  if (!lookup.IsFound()) {
+  if (!it.IsFound()) {
     if (IsUndeclaredGlobal(object)) {
       return ReferenceError("not_defined", name);
     }
@@ -586,16 +570,14 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<String> name) {
   }
 
   // Update inline cache and stub cache.
-  if (use_ic) UpdateCaches(&lookup, object, name);
+  if (use_ic) UpdateCaches(&it, object, name);
 
   // Get the property.
-  LookupIterator it(object, name);
   Handle<Object> result;
   ASSIGN_RETURN_ON_EXCEPTION(
       isolate(), result, Object::GetProperty(&it), Object);
   // If the property is not present, check if we need to throw an exception.
-  if ((lookup.IsInterceptor() || lookup.IsHandler()) &&
-      !it.IsFound() && IsUndeclaredGlobal(object)) {
+  if (!it.IsFound() && IsUndeclaredGlobal(object)) {
     return ReferenceError("not_defined", name);
   }
 
@@ -828,8 +810,7 @@ Handle<Code> LoadIC::SimpleFieldLoad(FieldIndex index) {
 }
 
 
-void LoadIC::UpdateCaches(LookupResult* lookup,
-                          Handle<Object> object,
+void LoadIC::UpdateCaches(LookupIterator* lookup, Handle<Object> object,
                           Handle<String> name) {
   if (state() == UNINITIALIZED) {
     // This is the first time we execute this inline cache.
@@ -841,10 +822,10 @@ void LoadIC::UpdateCaches(LookupResult* lookup,
   }
 
   Handle<Code> code;
-  if (!lookup->IsCacheable()) {
-    // Bail out if the result is not cacheable.
+  if (lookup->state() == LookupIterator::JSPROXY ||
+      lookup->state() == LookupIterator::ACCESS_CHECK) {
     code = slow_stub();
-  } else if (!lookup->IsProperty()) {
+  } else if (!lookup->IsFound()) {
     if (kind() == Code::LOAD_IC) {
       code = NamedLoadHandlerCompiler::ComputeLoadNonexistent(name,
                                                               receiver_type());
@@ -869,10 +850,52 @@ void IC::UpdateMegamorphicCache(HeapType* type, Name* name, Code* code) {
 }
 
 
-Handle<Code> IC::ComputeHandler(LookupResult* lookup,
-                                Handle<Object> object,
-                                Handle<String> name,
-                                Handle<Object> value) {
+Handle<Code> IC::ComputeHandler(LookupIterator* lookup, Handle<Object> object,
+                                Handle<String> name, Handle<Object> value) {
+  bool receiver_is_holder = lookup->HolderIsReceiver();

[Toon Verwaest, 2014/07/30 16:54:53]

These 2 things are different.
Rename HolderIsReceiver to HolderIsReceiverOrHiddenPrototype() along the way to
make it explicit?

[Jakob Kummerow, 2014/07/31 13:55:51]

Done.

+  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, kind(), flag,
+      lookup->has_fast_properties() ? Code::FAST : Code::NORMAL);

[Toon Verwaest, 2014/07/30 16:54:53]

What about just doing lookup->holder_map()->is_dictionary_map() and removing
this method again from the LookupIterator?

The difference between Code::FAST and Code::NORMAL should go away any time soon
now anyway ;)

[Jakob Kummerow, 2014/07/31 13:55:51]

Done.

+  // 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 = CompileHandler(lookup, object, name, value, flag);
+  ASSERT(code->is_handler());
+
+  if (code->type() != Code::NORMAL) {
+    Map::UpdateCodeCache(stub_holder_map, name, code);
+  }
+
+  return code;
+}
+
+
+Handle<Code> IC::ComputeStoreHandler(LookupResult* lookup,
+                                     Handle<Object> object, Handle<String> name,
+                                     Handle<Object> value) {
   bool receiver_is_holder = lookup->ReceiverIsHolder(object);
   CacheHolderFlag flag;
   Handle<Map> stub_holder_map = IC::GetHandlerCacheHolder(
@@ -903,7 +926,7 @@ Handle<Code> IC::ComputeHandler(LookupResult* lookup,
     }
   }
 
-  code = CompileHandler(lookup, object, name, value, flag);
+  code = CompileStoreHandler(lookup, object, name, value, flag);
   ASSERT(code->is_handler());
 
   if (code->type() != Code::NORMAL) {
@@ -914,8 +937,9 @@ Handle<Code> IC::ComputeHandler(LookupResult* lookup,
 }
 
 
-Handle<Code> LoadIC::CompileHandler(LookupResult* lookup, Handle<Object> object,
-                                    Handle<String> name, Handle<Object> unused,
+Handle<Code> LoadIC::CompileHandler(LookupIterator* lookup,
+                                    Handle<Object> object, Handle<String> name,
+                                    Handle<Object> unused,
                                     CacheHolderFlag cache_holder) {
   if (object->IsString() &&
       String::Equals(isolate()->factory()->length_string(), name)) {
@@ -940,11 +964,17 @@ Handle<Code> LoadIC::CompileHandler(LookupResult* lookup, Handle<Object> object,
   }
 
   Handle<HeapType> type = receiver_type();
-  Handle<JSObject> holder(lookup->holder());
+  Handle<JSObject> holder(lookup->GetHolder<JSObject>());

[Toon Verwaest, 2014/07/30 16:54:52]

Handle<JSObject> holder = lookup->GetHolder<JSObject>();

[Jakob Kummerow, 2014/07/31 13:55:50]

Done.

   bool receiver_is_holder = object.is_identical_to(holder);
   NamedLoadHandlerCompiler compiler(isolate(), cache_holder);
 
-  switch (lookup->type()) {
+  if (lookup->state() == LookupIterator::INTERCEPTOR) {
+    ASSERT(!holder->GetNamedInterceptor()->getter()->IsUndefined());
+    return compiler.CompileLoadInterceptor(type, holder, name);
+  }
+  ASSERT(lookup->state() == LookupIterator::PROPERTY);
+
+  switch (lookup->property_details().type()) {

[Toon Verwaest, 2014/07/30 16:54:52]

What about doing the same as GetProperty; using property_kind() to switch
between callbacks and others, then using property_encoding for dict vs
field/constant; and only at the end using property_details?

I'd like to get rid of the current form of PropertyType at some point.

[Jakob Kummerow, 2014/07/31 13:55:51]

Done.

     case FIELD: {
       FieldIndex field = lookup->GetFieldIndex();
       if (receiver_is_holder) {
@@ -954,7 +984,7 @@ Handle<Code> LoadIC::CompileHandler(LookupResult* lookup, Handle<Object> object,
           type, holder, name, field, lookup->representation());
     }
     case CONSTANT: {
-      Handle<Object> constant(lookup->GetConstant(), isolate());
+      Handle<Object> constant = lookup->GetDataValue();
       return compiler.CompileLoadConstant(type, holder, name, constant);
     }
     case NORMAL:
@@ -964,7 +994,7 @@ Handle<Code> LoadIC::CompileHandler(LookupResult* lookup, Handle<Object> object,
         Handle<PropertyCell> cell(
             global->GetPropertyCell(lookup), isolate());
         Handle<Code> code = compiler.CompileLoadGlobal(
-            type, global, cell, name, lookup->IsDontDelete());
+            type, global, cell, name, !lookup->IsConfigurable());

[Toon Verwaest, 2014/07/30 16:54:52]

What about changing the interface of CompileLoadGlobal? Otherwise you negate the
flag 3 times :)
!is_dont_delete -> !configurable -> !is_dont_delete

[Jakob Kummerow, 2014/07/31 13:55:51]

Done.

         // TODO(verwaest): Move caching of these NORMAL stubs outside as well.
         CacheHolderFlag flag;
         Handle<Map> stub_holder_map =
@@ -992,15 +1022,15 @@ Handle<Code> LoadIC::CompileHandler(LookupResult* lookup, Handle<Object> object,
         }
       }
 
-      Handle<Object> callback(lookup->GetCallbackObject(), isolate());
-      if (callback->IsExecutableAccessorInfo()) {
+      Handle<Object> accessors = lookup->GetAccessors();
+      if (accessors->IsExecutableAccessorInfo()) {
         Handle<ExecutableAccessorInfo> info =
-            Handle<ExecutableAccessorInfo>::cast(callback);
+            Handle<ExecutableAccessorInfo>::cast(accessors);
         if (v8::ToCData<Address>(info->getter()) == 0) break;
         if (!info->IsCompatibleReceiver(*object)) break;
         return compiler.CompileLoadCallback(type, holder, name, info);
-      } else if (callback->IsAccessorPair()) {
-        Handle<Object> getter(Handle<AccessorPair>::cast(callback)->getter(),
+      } else if (accessors->IsAccessorPair()) {
+        Handle<Object> getter(Handle<AccessorPair>::cast(accessors)->getter(),
                               isolate());
         if (!getter->IsJSFunction()) break;
         if (holder->IsGlobalObject()) break;
@@ -1022,12 +1052,9 @@ Handle<Code> LoadIC::CompileHandler(LookupResult* lookup, Handle<Object> object,
         return compiler.CompileLoadViaGetter(type, holder, name, function);
       }
       // TODO(dcarney): Handle correctly.
-      ASSERT(callback->IsDeclaredAccessorInfo());
+      ASSERT(accessors->IsDeclaredAccessorInfo());
       break;
     }
-    case INTERCEPTOR:
-      ASSERT(HasInterceptorGetter(*holder));
-      return compiler.CompileLoadInterceptor(type, holder, name);
     default:
       break;
   }
@@ -1374,17 +1401,18 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
   // These are not cacheable, so we never see such LookupResults here.
   ASSERT(!lookup->IsHandler());
 
-  Handle<Code> code = ComputeHandler(lookup, receiver, name, value);
+  Handle<Code> code = ComputeStoreHandler(lookup, receiver, name, value);
 
   PatchCache(name, code);
   TRACE_IC("StoreIC", name);
 }
 
 
-Handle<Code> StoreIC::CompileHandler(LookupResult* lookup,
-                                     Handle<Object> object, Handle<String> name,
-                                     Handle<Object> value,
-                                     CacheHolderFlag cache_holder) {
+Handle<Code> StoreIC::CompileStoreHandler(LookupResult* lookup,
+                                          Handle<Object> object,
+                                          Handle<String> name,
+                                          Handle<Object> value,
+                                          CacheHolderFlag cache_holder) {
   if (object->IsAccessCheckNeeded()) return slow_stub();
   ASSERT(cache_holder == kCacheOnReceiver || lookup->type() == CALLBACKS ||
          (object->IsJSGlobalProxy() && lookup->holder()->IsJSGlobalObject()));