Per-Isolate cache for polymorphic stubs

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index 0e87e5121a6ad77252f8bf8cea8f3eeabcb68867..70fd427297be68a2f9bade577f41ff722758190e 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -1629,79 +1629,40 @@ MaybeObject* KeyedIC::ComputeStub(JSObject* receiver,
     return generic_stub;
   }
 
-  // TODO(1385): Currently MEGAMORPHIC stubs are cached in the receiver map stub
-  // cache, but that can put receiver types together from unrelated call sites
-  // into the same stub--they always handle the union of all receiver maps seen
-  // at all call sites involving the receiver map. This is only an
-  // approximation: ideally, there would be a global cache that mapped sets of
-  // receiver maps to MEGAMORPHIC stubs. The complexity of the MEGAMORPHIC stub
-  // computation also leads to direct manipulation of the stub cache from the IC
-  // code, which the global cache solution would avoid.
-  Code::Kind kind = this->kind();
-  Code::Flags flags = Code::ComputeFlags(kind,
-                                         NOT_IN_LOOP,
-                                         MEGAMORPHIC,
-                                         strict_mode);
-  String* megamorphic_name = GetStubNameForCache(MEGAMORPHIC);
-  Object* maybe_cached_stub = receiver->map()->FindInCodeCache(megamorphic_name,
-                                                               flags);
-
-  // Create a set of all receiver maps that have been seen at the IC call site
-  // and those seen by the MEGAMORPHIC cached stub, if that's the stub that's
-  // been selected.
-  MapList receiver_maps;
-  if (!maybe_cached_stub->IsUndefined()) {
-    GetReceiverMapsForStub(Code::cast(maybe_cached_stub), &receiver_maps);
-  }
-  bool added_map = false;
-  for (int i = 0; i < target_receiver_maps.length(); ++i) {
-    if (AddOneReceiverMapIfMissing(&receiver_maps,
-                                   target_receiver_maps.at(i))) {
-      added_map = true;
-    }
-  }
-  ASSERT(receiver_maps.length() > 0);
-
   // If the maximum number of receiver maps has been exceeded, use the Generic

[Mads Ager (chromium), 2011/06/01 08:45:53]

Generic -> generic?

[Jakob Kummerow, 2011/06/01 10:15:27]

Done.

   // version of the IC.
-  if (receiver_maps.length() > KeyedIC::kMaxKeyedPolymorphism) {
+  if (target_receiver_maps.length() > KeyedIC::kMaxKeyedPolymorphism) {
     return generic_stub;
   }
 
-  // If no maps have been seen at the call site that aren't in the cached
-  // stub, then use it.
-  if (!added_map) {
-    ASSERT(!maybe_cached_stub->IsUndefined());
+  PolymorphicCodeCache* cache = isolate()->heap()->polymorphic_code_cache();
+  Code::Flags flags = Code::ComputeFlags(this->kind(),
+                                         NOT_IN_LOOP,
+                                         MEGAMORPHIC,
+                                         strict_mode);
+  Object* maybe_cached_stub = cache->Lookup(&target_receiver_maps, flags);
+  // If there is a cached stub, use it.
+  if (!maybe_cached_stub->IsUndefined()) {
     ASSERT(maybe_cached_stub->IsCode());
     return Code::cast(maybe_cached_stub);
   }
-
-  // Lookup all of the receiver maps in the cache, they should all already
-  // have MONOMORPHIC stubs.
+  // Collect MONOMORPHIC stubs for all target_receiver_maps.
   CodeList handler_ics(KeyedIC::kMaxKeyedPolymorphism);

[Mads Ager (chromium), 2011/06/01 08:45:53]

Not your code. :-) Don't we always have target_receiver_maps.length() handler
ics? If so, we don't need to over allocate for the list.

[Jakob Kummerow, 2011/06/01 10:15:27]

Done.

-  for (int current = 0; current < receiver_maps.length(); ++current) {
-    Map* receiver_map(receiver_maps.at(current));
+  for (int i = 0; i < target_receiver_maps.length(); ++i) {
+    Map* receiver_map(target_receiver_maps.at(i));
     MaybeObject* maybe_cached_stub = ComputeMonomorphicStubWithoutMapCheck(
-        receiver_map,
-        strict_mode,
-        generic_stub);
+        receiver_map, strict_mode, generic_stub);
     Code* cached_stub;
-    if (!maybe_cached_stub->To(&cached_stub)) {
-      return maybe_cached_stub;
-    }
+    if (!maybe_cached_stub->To(&cached_stub)) return maybe_cached_stub;
     handler_ics.Add(cached_stub);
   }
-
-  Code* stub;
   // Build the MEGAMORPHIC stub.
-  maybe_stub = ConstructMegamorphicStub(&receiver_maps,
+  Code* stub;
+  maybe_stub = ConstructMegamorphicStub(&target_receiver_maps,
                                         &handler_ics,
                                         strict_mode);
   if (!maybe_stub->To(&stub)) return maybe_stub;
-
-  MaybeObject* maybe_update = receiver->UpdateMapCodeCache(
-      megamorphic_name,
-      stub);
+  MaybeObject* maybe_update = cache->Update(&target_receiver_maps, flags, stub);

[Mads Ager (chromium), 2011/06/01 08:45:53]

Not sure I understand the ownership for |target_receiver_maps|. You are passing
a pointer to a local and using that as part of the key in a hash table? Is there
copying going on somewhere? If not, this looks unsafe.

[Jakob Kummerow, 2011/06/01 10:15:27]

Ownership is local. cache->Update takes a weak reference, and only makes use of
it before the call returns (--> while the local variable is still alive).
(See also my response to your comment in objects.cc on the same issue.)

[Mads Ager (chromium), 2011/06/01 10:36:54]

Thanks for explaining. I forgot how the HashTableKey stuff works. :-)

   if (maybe_update->IsFailure()) return maybe_update;
   return stub;
 }