[compiler] Thread-safe FindElementsKindTransitionedMap

Re-enable the creation of elements transition groups in
JSHeapBroker::ProcessFeedbackMapsForElementAccess. This turned out to be
quite important for performance.

Bug: v8:7790,v8:12031
Change-Id: I4d24837a668a5f7e78a5078212a7dc34b767d703
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3085262
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#76215}

src/compiler/js-heap-broker.cc

@@ -904,29 +904,23 @@ ElementAccessFeedback const& JSHeapBroker::ProcessFeedbackMapsForElementAccess(
 
   // Separate the actual receiver maps and the possible transition sources.
   for (const MapRef& map : maps) {
+    Map transition_target;
+
     // Don't generate elements kind transitions from stable maps.
-    if (is_concurrent_inlining()) {
+    if (!map.is_stable()) {
-      // TODO(jgruber): Bring back elements kind transition generation when
+      transition_target = map.object()->FindElementsKindTransitionedMap(
-      // concurrent inlining (see FindElementsKindTransitionedMap).
+          isolate(), possible_transition_targets, ConcurrencyMode::kConcurrent);
+    }
+
+    if (transition_target.is_null()) {
       TransitionGroup group(1, map.object(), zone());
       transition_groups.insert({map.object(), group});
     } else {
-      Map transition_target;
+      Handle<Map> target = CanonicalPersistentHandle(transition_target);
-      if (!map.is_stable()) {
+      TransitionGroup new_group(1, target, zone());
-        transition_target = map.object()->FindElementsKindTransitionedMap(
+      TransitionGroup& actual_group =
-            isolate(), possible_transition_targets);
+          transition_groups.insert({target, new_group}).first->second;
-      }
+      actual_group.push_back(map.object());
-
-      if (transition_target.is_null()) {
-        TransitionGroup group(1, map.object(), zone());
-        transition_groups.insert({map.object(), group});
-      } else {
-        Handle<Map> target(transition_target, isolate());
-        TransitionGroup new_group(1, target, zone());
-        TransitionGroup& actual_group =
-            transition_groups.insert({target, new_group}).first->second;
-        actual_group.push_back(map.object());
-      }
     }
   }
 

src/ic/ic.cc

@@ -738,8 +738,8 @@ bool IC::IsTransitionOfMonomorphicTarget(Map source_map, Map target_map) {
   if (more_general_transition) {
     MapHandles map_list;
     map_list.push_back(handle(target_map, isolate_));
-    transitioned_map =
+    transitioned_map = source_map.FindElementsKindTransitionedMap(
-        source_map.FindElementsKindTransitionedMap(isolate(), map_list);
+        isolate(), map_list, ConcurrencyMode::kNotConcurrent);
   }
   return transitioned_map == target_map;
 }
@@ -1395,8 +1395,8 @@ void KeyedLoadIC::LoadElementPolymorphicHandlers(
     // among receiver_maps as unstable because the optimizing compilers may
     // generate an elements kind transition for this kind of receivers.
     if (receiver_map->is_stable()) {
-      Map tmap = receiver_map->FindElementsKindTransitionedMap(isolate(),
+      Map tmap = receiver_map->FindElementsKindTransitionedMap(
-                                                               *receiver_maps);
+          isolate(), *receiver_maps, ConcurrencyMode::kNotConcurrent);
       if (!tmap.is_null()) {
         receiver_map->NotifyLeafMapLayoutChange(isolate());
       }
@@ -2244,8 +2244,8 @@ void KeyedStoreIC::StoreElementPolymorphicHandlers(
 
     } else {
       {
-        Map tmap = receiver_map->FindElementsKindTransitionedMap(isolate(),
+        Map tmap = receiver_map->FindElementsKindTransitionedMap(
-                                                                 receiver_maps);
+            isolate(), receiver_maps, ConcurrencyMode::kNotConcurrent);
         if (!tmap.is_null()) {
           if (receiver_map->is_stable()) {
             receiver_map->NotifyLeafMapLayoutChange(isolate());

src/objects/map-inl.h

@@ -741,9 +741,10 @@ void Map::SetBackPointer(HeapObject value, WriteBarrierMode mode) {
 }
 
 // static
-Map Map::ElementsTransitionMap(Isolate* isolate) {
+Map Map::ElementsTransitionMap(Isolate* isolate, ConcurrencyMode cmode) {
   DisallowGarbageCollection no_gc;
-  return TransitionsAccessor(isolate, *this, &no_gc)
+  return TransitionsAccessor(isolate, *this, &no_gc,
+                             cmode == ConcurrencyMode::kConcurrent)
       .SearchSpecial(ReadOnlyRoots(isolate).elements_transition_symbol());
 }
 

src/objects/map.cc

@@ -757,14 +757,16 @@ Map Map::TryUpdateSlow(Isolate* isolate, Map old_map) {
   }
   if (from_kind != to_kind) {
     // Try to follow existing elements kind transitions.
-    root_map = root_map.LookupElementsTransitionMap(isolate, to_kind);
+    root_map = root_map.LookupElementsTransitionMap(
+        isolate, to_kind, ConcurrencyMode::kNotConcurrent);
     if (root_map.is_null()) return Map();
     // From here on, use the map with correct elements kind as root map.
   }
 
   // Replay the transitions as they were before the integrity level transition.
   Map result = root_map.TryReplayPropertyTransitions(
-      isolate, info.integrity_level_source_map);
+      isolate, info.integrity_level_source_map,
+      ConcurrencyMode::kNotConcurrent);
   if (result.is_null()) return Map();
 
   if (info.has_integrity_level_transition) {
@@ -780,25 +782,29 @@ Map Map::TryUpdateSlow(Isolate* isolate, Map old_map) {
   return result;
 }
 
-Map Map::TryReplayPropertyTransitions(Isolate* isolate, Map old_map) {
+Map Map::TryReplayPropertyTransitions(Isolate* isolate, Map old_map,
+                                      ConcurrencyMode cmode) {
   DisallowGarbageCollection no_gc;
-  DisallowDeoptimization no_deoptimization(isolate);
 
+  const bool is_concurrent = cmode == ConcurrencyMode::kConcurrent;
   int root_nof = NumberOfOwnDescriptors();
-
   int old_nof = old_map.NumberOfOwnDescriptors();
-  DescriptorArray old_descriptors = old_map.instance_descriptors(isolate);
+  DescriptorArray old_descriptors =
+      is_concurrent ? old_map.instance_descriptors(isolate, kAcquireLoad)
+                    : old_map.instance_descriptors(isolate);
 
   Map new_map = *this;
   for (InternalIndex i : InternalIndex::Range(root_nof, old_nof)) {
     PropertyDetails old_details = old_descriptors.GetDetails(i);
     Map transition =
-        TransitionsAccessor(isolate, new_map, &no_gc)
+        TransitionsAccessor(isolate, new_map, &no_gc, is_concurrent)
             .SearchTransition(old_descriptors.GetKey(i), old_details.kind(),
                               old_details.attributes());
     if (transition.is_null()) return Map();
     new_map = transition;
-    DescriptorArray new_descriptors = new_map.instance_descriptors(isolate);
+    DescriptorArray new_descriptors =
+        is_concurrent ? new_map.instance_descriptors(isolate, kAcquireLoad)
+                      : new_map.instance_descriptors(isolate);
 
     PropertyDetails new_details = new_descriptors.GetDetails(i);
     DCHECK_EQ(old_details.kind(), new_details.kind());
@@ -957,39 +963,42 @@ static bool HasElementsKind(MapHandles const& maps,
 }
 
 Map Map::FindElementsKindTransitionedMap(Isolate* isolate,
-                                         MapHandles const& candidates) {
+                                         MapHandles const& candidates,
+                                         ConcurrencyMode cmode) {
   DisallowGarbageCollection no_gc;
-  DisallowDeoptimization no_deoptimization(isolate);
 
   if (IsDetached(isolate)) return Map();
 
   ElementsKind kind = elements_kind();
-  bool packed = IsFastPackedElementsKind(kind);
+  bool is_packed = IsFastPackedElementsKind(kind);
 
   Map transition;
   if (IsTransitionableFastElementsKind(kind)) {
     // Check the state of the root map.
     Map root_map = FindRootMap(isolate);
     if (!EquivalentToForElementsKindTransition(root_map)) return Map();
-    root_map = root_map.LookupElementsTransitionMap(isolate, kind);
+    root_map = root_map.LookupElementsTransitionMap(isolate, kind, cmode);
     DCHECK(!root_map.is_null());
     // Starting from the next existing elements kind transition try to
     // replay the property transitions that does not involve instance rewriting
     // (ElementsTransitionAndStoreStub does not support that).
-    for (root_map = root_map.ElementsTransitionMap(isolate);
+    for (root_map = root_map.ElementsTransitionMap(isolate, cmode);
          !root_map.is_null() && root_map.has_fast_elements();
-         root_map = root_map.ElementsTransitionMap(isolate)) {
+         root_map = root_map.ElementsTransitionMap(isolate, cmode)) {
       // If root_map's elements kind doesn't match any of the elements kind in
       // the candidates there is no need to do any additional work.
       if (!HasElementsKind(candidates, root_map.elements_kind())) continue;
-      Map current = root_map.TryReplayPropertyTransitions(isolate, *this);
+      Map current =
+          root_map.TryReplayPropertyTransitions(isolate, *this, cmode);
       if (current.is_null()) continue;
       if (InstancesNeedRewriting(current)) continue;
 
+      const bool current_is_packed =
+          IsFastPackedElementsKind(current.elements_kind());
       if (ContainsMap(candidates, current) &&
-          (packed || !IsFastPackedElementsKind(current.elements_kind()))) {
+          (is_packed || !current_is_packed)) {
         transition = current;
-        packed = packed && IsFastPackedElementsKind(current.elements_kind());
+        is_packed = is_packed && current_is_packed;
       }
     }
   }
@@ -997,7 +1006,8 @@ Map Map::FindElementsKindTransitionedMap(Isolate* isolate,
 }
 
 static Map FindClosestElementsTransition(Isolate* isolate, Map map,
-                                         ElementsKind to_kind) {
+                                         ElementsKind to_kind,
+                                         ConcurrencyMode cmode) {
   // Ensure we are requested to search elements kind transition "near the root".
   DCHECK_EQ(map.FindRootMap(isolate).NumberOfOwnDescriptors(),
             map.NumberOfOwnDescriptors());
@@ -1005,7 +1015,7 @@ static Map FindClosestElementsTransition(Isolate* isolate, Map map,
 
   ElementsKind kind = map.elements_kind();
   while (kind != to_kind) {
-    Map next_map = current_map.ElementsTransitionMap(isolate);
+    Map next_map = current_map.ElementsTransitionMap(isolate, cmode);
     if (next_map.is_null()) return current_map;
     kind = next_map.elements_kind();
     current_map = next_map;
@@ -1015,8 +1025,9 @@ static Map FindClosestElementsTransition(Isolate* isolate, Map map,
   return current_map;
 }
 
-Map Map::LookupElementsTransitionMap(Isolate* isolate, ElementsKind to_kind) {
+Map Map::LookupElementsTransitionMap(Isolate* isolate, ElementsKind to_kind,
-  Map to_map = FindClosestElementsTransition(isolate, *this, to_kind);
+                                     ConcurrencyMode cmode) {
+  Map to_map = FindClosestElementsTransition(isolate, *this, to_kind, cmode);
   if (to_map.elements_kind() == to_kind) return to_map;
   return Map();
 }
@@ -1119,8 +1130,10 @@ static Handle<Map> AddMissingElementsTransitions(Isolate* isolate,
 // static
 Handle<Map> Map::AsElementsKind(Isolate* isolate, Handle<Map> map,
                                 ElementsKind kind) {
-  Handle<Map> closest_map(FindClosestElementsTransition(isolate, *map, kind),
+  Handle<Map> closest_map(
-                          isolate);
+      FindClosestElementsTransition(isolate, *map, kind,
+                                    ConcurrencyMode::kNotConcurrent),
+      isolate);
 
   if (closest_map->elements_kind() == kind) {
     return closest_map;
@@ -1591,7 +1604,8 @@ Handle<Map> Map::CopyAsElementsKind(Isolate* isolate, Handle<Map> map,
     DCHECK_EQ(map->FindRootMap(isolate).NumberOfOwnDescriptors(),
               map->NumberOfOwnDescriptors());
 
-    maybe_elements_transition_map = map->ElementsTransitionMap(isolate);
+    maybe_elements_transition_map =
+        map->ElementsTransitionMap(isolate, ConcurrencyMode::kNotConcurrent);
     DCHECK(
         maybe_elements_transition_map.is_null() ||
         (maybe_elements_transition_map.elements_kind() == DICTIONARY_ELEMENTS &&

src/objects/map.h

@@ -436,7 +436,7 @@ class Map : public TorqueGeneratedMap<Map, HeapObject> {
   // elements or an object with any frozen elements, or a slow arguments object.
   bool MayHaveReadOnlyElementsInPrototypeChain(Isolate* isolate);
 
-  inline Map ElementsTransitionMap(Isolate* isolate);
+  inline Map ElementsTransitionMap(Isolate* isolate, ConcurrencyMode cmode);
 
   inline FixedArrayBase GetInitialElements() const;
 
@@ -768,7 +768,7 @@ class Map : public TorqueGeneratedMap<Map, HeapObject> {
   // elements_kind that's found in |candidates|, or |nullptr| if no match is
   // found at all.
   V8_EXPORT_PRIVATE Map FindElementsKindTransitionedMap(
-      Isolate* isolate, MapHandles const& candidates);
+      Isolate* isolate, MapHandles const& candidates, ConcurrencyMode cmode);
 
   inline bool CanTransition() const;
 
@@ -862,14 +862,16 @@ class Map : public TorqueGeneratedMap<Map, HeapObject> {
 
   // Returns the map that this (root) map transitions to if its elements_kind
   // is changed to |elements_kind|, or |nullptr| if no such map is cached yet.
-  Map LookupElementsTransitionMap(Isolate* isolate, ElementsKind elements_kind);
+  Map LookupElementsTransitionMap(Isolate* isolate, ElementsKind elements_kind,
+                                  ConcurrencyMode cmode);
 
   // Tries to replay property transitions starting from this (root) map using
   // the descriptor array of the |map|. The |root_map| is expected to have
   // proper elements kind and therefore elements kinds transitions are not
   // taken by this function. Returns |nullptr| if matching transition map is
   // not found.
-  Map TryReplayPropertyTransitions(Isolate* isolate, Map map);
+  Map TryReplayPropertyTransitions(Isolate* isolate, Map map,
+                                   ConcurrencyMode cmode);
 
   static void ConnectTransition(Isolate* isolate, Handle<Map> parent,
                                 Handle<Map> child, Handle<Name> name,

src/objects/objects.cc

@@ -4706,7 +4706,8 @@ Handle<Object> CacheInitialJSArrayMaps(Isolate* isolate,
        i < kFastElementsKindCount; ++i) {
     Handle<Map> new_map;
     ElementsKind next_kind = GetFastElementsKindFromSequenceIndex(i);
-    Map maybe_elements_transition = current_map->ElementsTransitionMap(isolate);
+    Map maybe_elements_transition = current_map->ElementsTransitionMap(
+        isolate, ConcurrencyMode::kNotConcurrent);
     if (!maybe_elements_transition.is_null()) {
       new_map = handle(maybe_elements_transition, isolate);
     } else {

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

@@ -1831,8 +1831,8 @@ static void TestReconfigureElementsKind_GeneralizeFieldInPlace(
   {
     MapHandles map_list;
     map_list.push_back(updated_map);
-    Map transitioned_map =
+    Map transitioned_map = map2->FindElementsKindTransitionedMap(
-        map2->FindElementsKindTransitionedMap(isolate, map_list);
+        isolate, map_list, ConcurrencyMode::kNotConcurrent);
     CHECK_EQ(*updated_map, transitioned_map);
   }
 }

test/mjsunit/mjsunit.status

@@ -213,14 +213,6 @@
   # dispatcher *without* aborting existing jobs.
   'interrupt-budget-override': [PASS,FAIL],
   'never-optimize': [PASS,FAIL],
-
-  # TODO(v8:12031): Reimplement elements kinds transitions when concurrent
-  # inlining.
-  'compiler/call-with-arraylike-or-spread-4': [SKIP],
-  'elements-kind': [SKIP],
-  'elements-transition-hoisting': [SKIP],
-  'regress/regress-7254': [SKIP],
-  'regress/regress-7510': [SKIP],
 }],  # ALWAYS
 
 ##############################################################################