Cache fewer StoreIC-Transition handlers

Many handlers are not used again, so we can improve the cache hit rate
by caching fewer handlers. Specifically, in this CL, when a StoreIC
miss causes a new map transition to be created, then the handler is not
cached right away yet (it will be cached next time, when the transition
exists already).

Also, fix an embarrassing bug where growing a TransitionArray dropped
cached handlers. That further improves the cache hit rate. ;-)

Bug: chromium:752867, chromium:753819
Change-Id: Id8db5ca1e780a5fe8fc61db7f20996e61c65a90e
Reviewed-on: https://chromium-review.googlesource.com/619851Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47433}

src/ic/ic.cc

@@ -1562,7 +1562,8 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle<Object> value,
         if (it->HolderIsReceiverOrHiddenPrototype()) return false;
 
         if (it->ExtendingNonExtensible(receiver)) return false;
-        it->PrepareTransitionToDataProperty(receiver, value, NONE, store_mode);
+        created_new_transition_ = it->PrepareTransitionToDataProperty(
+            receiver, value, NONE, store_mode);
         return it->IsCacheableTransition();
       }
     }
@@ -1570,7 +1571,8 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle<Object> value,
 
   receiver = it->GetStoreTarget();
   if (it->ExtendingNonExtensible(receiver)) return false;
-  it->PrepareTransitionToDataProperty(receiver, value, NONE, store_mode);
+  created_new_transition_ =
+      it->PrepareTransitionToDataProperty(receiver, value, NONE, store_mode);
   return it->IsCacheableTransition();
 }
 
@@ -1793,8 +1795,10 @@ Handle<Object> StoreIC::GetMapIndependentHandler(LookupIterator* lookup) {
       TRACE_HANDLER_STATS(isolate(), StoreIC_StoreTransitionDH);
       Handle<Object> handler =
           StoreTransition(receiver_map(), holder, transition, lookup->name());
-      TransitionsAccessor(receiver_map())
-          .UpdateHandler(*lookup->name(), *handler);
+      if (!created_new_transition_) {
+        TransitionsAccessor(receiver_map())
+            .UpdateHandler(*lookup->name(), *handler);
+      }
       return handler;
     }
 

src/ic/ic.h

@@ -373,6 +373,8 @@ class StoreIC : public IC {
                                  Handle<Map> transition, Handle<Name> name);
 
   friend class IC;
+
+  bool created_new_transition_ = false;
 };
 
 class StoreGlobalIC : public StoreIC {

src/lookup.cc

@@ -382,11 +382,13 @@ void LookupIterator::ReconfigureDataProperty(Handle<Object> value,
 
 // 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.
-void LookupIterator::PrepareTransitionToDataProperty(
+// Returns true if a new transition has been created, or false if an existing
+// transition was followed.
+bool LookupIterator::PrepareTransitionToDataProperty(
     Handle<JSObject> receiver, Handle<Object> value,
     PropertyAttributes attributes, Object::StoreFromKeyed store_mode) {
   DCHECK(receiver.is_identical_to(GetStoreTarget()));
-  if (state_ == TRANSITION) return;
+  if (state_ == TRANSITION) return false;
 
   if (!IsElement() && name()->IsPrivate()) {
     attributes = static_cast<PropertyAttributes>(attributes | DONT_ENUM);
@@ -432,11 +434,13 @@ void LookupIterator::PrepareTransitionToDataProperty(
           PropertyDetails(kData, attributes, PropertyCellType::kNoCell);
       transition_ = map;
     }
-    return;
+    return false;
   }
 
+  bool created_new_map;
   Handle<Map> transition = Map::TransitionToDataProperty(
-      map, name_, value, attributes, kDefaultFieldConstness, store_mode);
+      map, name_, value, attributes, kDefaultFieldConstness, store_mode,
+      &created_new_map);
   state_ = TRANSITION;
   transition_ = transition;
 
@@ -448,6 +452,7 @@ void LookupIterator::PrepareTransitionToDataProperty(
     property_details_ = transition->GetLastDescriptorDetails();
     has_property_ = true;
   }
+  return created_new_map;
 }
 
 void LookupIterator::ApplyTransitionToDataProperty(Handle<JSObject> receiver) {

src/lookup.h

@@ -214,7 +214,7 @@ class V8_EXPORT_PRIVATE LookupIterator final BASE_EMBEDDED {
            (IsElement() || !name_->IsPrivate());
   }
   void PrepareForDataProperty(Handle<Object> value);
-  void PrepareTransitionToDataProperty(Handle<JSObject> receiver,
+  bool PrepareTransitionToDataProperty(Handle<JSObject> receiver,
                                        Handle<Object> value,
                                        PropertyAttributes attributes,
                                        Object::StoreFromKeyed store_mode);

src/objects.cc

@@ -9477,7 +9477,8 @@ Handle<Map> Map::TransitionToDataProperty(Handle<Map> map, Handle<Name> name,
                                           Handle<Object> value,
                                           PropertyAttributes attributes,
                                           PropertyConstness constness,
-                                          StoreFromKeyed store_mode) {
+                                          StoreFromKeyed store_mode,
+                                          bool* created_new_map) {
   RuntimeCallTimerScope stats_scope(
       *map, map->is_prototype_map()
                 ? &RuntimeCallStats::PrototypeMap_TransitionToDataProperty
@@ -9492,6 +9493,7 @@ Handle<Map> Map::TransitionToDataProperty(Handle<Map> map, Handle<Name> name,
   Map* maybe_transition =
       TransitionsAccessor(map).SearchTransition(*name, kData, attributes);
   if (maybe_transition != NULL) {
+    *created_new_map = false;
     Handle<Map> transition(maybe_transition);
     int descriptor = transition->LastAdded();
 
@@ -9502,6 +9504,7 @@ Handle<Map> Map::TransitionToDataProperty(Handle<Map> map, Handle<Name> name,
     return UpdateDescriptorForValue(transition, descriptor, constness, value);
   }
 
+  *created_new_map = true;
   TransitionFlag flag = INSERT_TRANSITION;
   MaybeHandle<Map> maybe_map;
   if (!FLAG_track_constant_fields && value->IsJSFunction()) {

src/objects/map.h

@@ -528,12 +528,10 @@ class Map : public HeapObject {
   // transitions to avoid an explosion in the number of maps for objects used as
   // dictionaries.
   inline bool TooManyFastProperties(StoreFromKeyed store_mode) const;
-  static Handle<Map> TransitionToDataProperty(Handle<Map> map,
-                                              Handle<Name> name,
-                                              Handle<Object> value,
-                                              PropertyAttributes attributes,
-                                              PropertyConstness constness,
-                                              StoreFromKeyed store_mode);
+  static Handle<Map> TransitionToDataProperty(
+      Handle<Map> map, Handle<Name> name, Handle<Object> value,
+      PropertyAttributes attributes, PropertyConstness constness,
+      StoreFromKeyed store_mode, bool* created_new_map);
   static Handle<Map> TransitionToAccessorProperty(
       Isolate* isolate, Handle<Map> map, Handle<Name> name, int descriptor,
       Handle<Object> getter, Handle<Object> setter,

src/transitions.cc

@@ -200,11 +200,11 @@ void TransitionsAccessor::Insert(Handle<Name> name, Handle<Map> target,
 
   DCHECK_NE(kNotFound, insertion_index);
   for (int i = 0; i < insertion_index; ++i) {
-    result->Set(i, array->GetKey(i), array->GetTarget(i));
+    result->Set(i, array->GetKey(i), array->GetRawTarget(i));
   }
   result->Set(insertion_index, *name, *target);
   for (int i = insertion_index; i < number_of_transitions; ++i) {
-    result->Set(i + 1, array->GetKey(i), array->GetTarget(i));
+    result->Set(i + 1, array->GetKey(i), array->GetRawTarget(i));
   }
 
   SLOW_DCHECK(result->IsSortedNoDuplicates());
@@ -258,7 +258,7 @@ Object* TransitionsAccessor::SearchHandler(Name* name,
                                                       out_transition);
       }
       if (raw_handler->IsFixedArray()) {
-        return StoreHandler::ValidFixedArrayHandlerOrNull(raw_handler, name,
+        return StoreHandler::ValidFixedArrayHandlerOrNull(raw_handler, nullptr,
                                                           out_transition);
       }
       return nullptr;

test/cctest/test-field-type-tracking.cc

@@ -366,9 +366,10 @@ class Expectations {
                  heap_type);
 
     Handle<String> name = MakeName("prop", property_index);
+    bool created_new_map;
     return Map::TransitionToDataProperty(
         map, name, value, attributes, constness,
-        Object::CERTAINLY_NOT_STORE_FROM_KEYED);
+        Object::CERTAINLY_NOT_STORE_FROM_KEYED, &created_new_map);
   }
 
   Handle<Map> TransitionToDataConstant(Handle<Map> map,
@@ -379,9 +380,10 @@ class Expectations {
     SetDataConstant(property_index, attributes, value);
 
     Handle<String> name = MakeName("prop", property_index);
-    return Map::TransitionToDataProperty(
-        map, name, value, attributes, kConst,
-        Object::CERTAINLY_NOT_STORE_FROM_KEYED);
+    bool created_new_map;
+    return Map::TransitionToDataProperty(map, name, value, attributes, kConst,
+                                         Object::CERTAINLY_NOT_STORE_FROM_KEYED,
+                                         &created_new_map);
   }
 
   Handle<Map> FollowDataTransition(Handle<Map> map,