Monomorphic prototype failures should be reserved for already-seen keys.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index f01c3d120af1fe6bd00647712ab0e4622b4bb761..a678f37de991f62a57e71f7f446607fa35f30ac6 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -248,13 +248,7 @@ static void LookupForRead(Handle<Object> object,
 
 bool IC::TryRemoveInvalidPrototypeDependentStub(Handle<Object> receiver,
                                                 Handle<String> name) {
-  if (target()->is_keyed_stub()) {
-    // Determine whether the failure is due to a name failure.
-    if (!name->IsName()) return false;
-    Name* stub_name = target()->FindFirstName();
-    if (*name != stub_name) return false;
-  }
-
+  ASSERT(IsNameCompatibleWithMonomorphicPrototypeFailure(name));
   InlineCacheHolderFlag cache_holder =
       Code::ExtractCacheHolderFromFlags(target()->flags());
 
@@ -336,6 +330,18 @@ void IC::TryRemoveInvalidHandlers(Handle<Map> map, Handle<String> name) {
 }
 
 
+bool IC::IsNameCompatibleWithMonomorphicPrototypeFailure(Handle<Object> name) {
+  if (target()->is_keyed_stub()) {
+    // Determine whether the failure is due to a name failure.
+    if (!name->IsName()) return false;
+    Name* stub_name = target()->FindFirstName();
+    if (*name != stub_name) return false;
+  }
+
+  return true;
+}
+
+
 void IC::UpdateState(Handle<Object> receiver, Handle<Object> name) {
   if (!name->IsString()) return;
   if (state() != MONOMORPHIC) {
@@ -351,9 +357,11 @@ void IC::UpdateState(Handle<Object> receiver, Handle<Object> name) {
   // Remove the target from the code cache if it became invalid
   // because of changes in the prototype chain to avoid hitting it
   // again.
-  if (TryRemoveInvalidPrototypeDependentStub(
+  if (IsNameCompatibleWithMonomorphicPrototypeFailure(name) &&
+      TryRemoveInvalidPrototypeDependentStub(
           receiver, Handle<String>::cast(name))) {
-    return MarkMonomorphicPrototypeFailure();
+    MarkMonomorphicPrototypeFailure(name);
+    return;
   }
 
   // The builtins object is special.  It only changes when JavaScript
@@ -1175,6 +1183,8 @@ static bool LookupForWrite(Handle<JSObject> receiver,
   // Ensure the instance and its map were migrated before trying to update the
   // transition target.
   ASSERT(!receiver->map()->is_deprecated());
+
+  if (!ic->IsNameCompatibleWithMonomorphicPrototypeFailure(name)) return false;

[Toon Verwaest, 2014/03/31 12:34:44]

This is not exactly the same...
The name only has to be compatible for the MarkMonomorphicPrototypeFailure(name)
below. That's why I suggested (and still suggest) moving it inside of
MarkMonomorphicPrototypeFailure(name), so that we don't need to explicitly guard
it at every use of MarkMonomorphicPrototypeFailure(name) (and asserting inside
of the method).

[mvstanton, 2014/03/31 13:30:57]

I ran into an issue with that idea. TryRemoiveInvalidPrototypeDependentStub()
carries out a lot of actions and is better served with a guard against going
forward if the name isn't found in the stub, hence the two-part nature of the
fix.

What I've done now is to

1) change MarkMonomorphicPrototypeFailure to TryMarkMonomorphicPrototypeFailure,
so this code site doesn't need to call
IsNameCompatibleWithMonomorphicPrototypeFailure().
2) In TryRemoiveInvalidPrototypeDependentStub(), an explicit call to
IsNameCompatibleWithMonomorphicPrototypeFailure is made.
3) No more dubious asserts.

   if (!value->FitsRepresentation(target_details.representation())) {
     Handle<Map> target(lookup->GetTransitionTarget());
     Map::GeneralizeRepresentation(
@@ -1184,7 +1194,7 @@ static bool LookupForWrite(Handle<JSObject> receiver,
     // entirely by the migration above.
     receiver->map()->LookupTransition(*holder, *name, lookup);
     if (!lookup->IsTransition()) return false;
-    ic->MarkMonomorphicPrototypeFailure();
+    ic->MarkMonomorphicPrototypeFailure(name);
   }
   return true;
 }