[compiler] Support concurrent map updates

.. to attempt to update deprecated maps. Used in
JSHeapBroker::ReadFeedbackForPropertyAccess.

Drive-by: Move Map::TryUpdate to MapUpdater to address
an old TODO.

Bug: v8:7790
Change-Id: Iaa791e204dd133f067014c0abdb23ef3b807a315
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3085274
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#76224}

src/compiler/js-heap-broker.cc

@@ -19,6 +19,7 @@
 #include "src/objects/feedback-cell.h"
 #include "src/objects/js-array-inl.h"
 #include "src/objects/literal-objects-inl.h"
+#include "src/objects/map-updater.h"
 #include "src/objects/objects-inl.h"
 #include "src/objects/oddball.h"
 #include "src/objects/property-cell.h"
@@ -562,17 +563,19 @@ ProcessedFeedback const& JSHeapBroker::ReadFeedbackForPropertyAccess(
 
     for (const MapAndHandler& map_and_handler : maps_and_handlers_unfiltered) {
       MapRef map = MakeRefAssumeMemoryFence(this, *map_and_handler.first);
-      if (!is_concurrent_inlining()) {
-        // TODO(jgruber): Consider replaying transitions on deprecated maps
-        // when concurrent inlining (see Map::TryUpdate).
-        Handle<Map> map_handle;
-        if (!Map::TryUpdate(isolate(), map.object()).ToHandle(&map_handle))
-          continue;
-        map = MakeRefAssumeMemoryFence(this, *map_handle);
-      }
       // May change concurrently at any time - must be guarded by a dependency
       // if non-deprecation is important.
-      if (map.is_deprecated()) continue;
+      if (map.is_deprecated()) {
+        // TODO(ishell): support fast map updating if we enable it.
+        CHECK(!FLAG_fast_map_update);
+        base::Optional<Map> maybe_map = MapUpdater::TryUpdateNoLock(
+            isolate(), *map.object(), ConcurrencyMode::kConcurrent);
+        if (maybe_map.has_value()) {
+          map = MakeRefAssumeMemoryFence(this, maybe_map.value());
+        } else {
+          continue;  // Couldn't update the deprecated map.
+        }
+      }
       if (map.is_abandoned_prototype_map()) continue;
       maps_and_handlers.push_back({map, map_and_handler.second});
       maps.push_back(map);

src/objects/map-updater.cc

@@ -282,6 +282,125 @@ Handle<Map> MapUpdater::UpdateImpl() {
   return result_map_;
 }
 
+namespace {
+
+struct IntegrityLevelTransitionInfo {
+  explicit IntegrityLevelTransitionInfo(Map map)
+      : integrity_level_source_map(map) {}
+
+  bool has_integrity_level_transition = false;
+  PropertyAttributes integrity_level = NONE;
+  Map integrity_level_source_map;
+  Symbol integrity_level_symbol;
+};
+
+IntegrityLevelTransitionInfo DetectIntegrityLevelTransitions(
+    Map map, Isolate* isolate, DisallowGarbageCollection* no_gc,
+    ConcurrencyMode cmode) {
+  const bool is_concurrent = cmode == ConcurrencyMode::kConcurrent;
+  IntegrityLevelTransitionInfo info(map);
+
+  // Figure out the most restrictive integrity level transition (it should
+  // be the last one in the transition tree).
+  DCHECK(!map.is_extensible());
+  Map previous = Map::cast(map.GetBackPointer(isolate));
+  TransitionsAccessor last_transitions(isolate, previous, no_gc, is_concurrent);
+  if (!last_transitions.HasIntegrityLevelTransitionTo(
+          map, &info.integrity_level_symbol, &info.integrity_level)) {
+    // The last transition was not integrity level transition - just bail out.
+    // This can happen in the following cases:
+    // - there are private symbol transitions following the integrity level
+    //   transitions (see crbug.com/v8/8854).
+    // - there is a getter added in addition to an existing setter (or a setter
+    //   in addition to an existing getter).
+    return info;
+  }
+
+  Map source_map = previous;
+  // Now walk up the back pointer chain and skip all integrity level
+  // transitions. If we encounter any non-integrity level transition interleaved
+  // with integrity level transitions, just bail out.
+  while (!source_map.is_extensible()) {
+    previous = Map::cast(source_map.GetBackPointer(isolate));
+    TransitionsAccessor transitions(isolate, previous, no_gc, is_concurrent);
+    if (!transitions.HasIntegrityLevelTransitionTo(source_map)) {
+      return info;
+    }
+    source_map = previous;
+  }
+
+  // Integrity-level transitions never change number of descriptors.
+  CHECK_EQ(map.NumberOfOwnDescriptors(), source_map.NumberOfOwnDescriptors());
+
+  info.has_integrity_level_transition = true;
+  info.integrity_level_source_map = source_map;
+  return info;
+}
+
+}  // namespace
+
+// static
+base::Optional<Map> MapUpdater::TryUpdateNoLock(Isolate* isolate, Map old_map,
+                                                ConcurrencyMode cmode) {
+  DisallowGarbageCollection no_gc;
+
+  // Check the state of the root map.
+  Map root_map = old_map.FindRootMap(isolate);
+  if (root_map.is_deprecated()) {
+    JSFunction constructor = JSFunction::cast(root_map.GetConstructor());
+    DCHECK(constructor.has_initial_map());
+    DCHECK(constructor.initial_map().is_dictionary_map());
+    if (constructor.initial_map().elements_kind() != old_map.elements_kind()) {
+      return {};
+    }
+    return constructor.initial_map();
+  }
+  if (!old_map.EquivalentToForTransition(root_map)) return {};
+
+  ElementsKind from_kind = root_map.elements_kind();
+  ElementsKind to_kind = old_map.elements_kind();
+
+  IntegrityLevelTransitionInfo info(old_map);
+  if (root_map.is_extensible() != old_map.is_extensible()) {
+    DCHECK(!old_map.is_extensible());
+    DCHECK(root_map.is_extensible());
+    info = DetectIntegrityLevelTransitions(old_map, isolate, &no_gc, cmode);
+    // Bail out if there were some private symbol transitions mixed up
+    // with the integrity level transitions.
+    if (!info.has_integrity_level_transition) return Map();
+    // Make sure to replay the original elements kind transitions, before
+    // the integrity level transition sets the elements to dictionary mode.
+    DCHECK(to_kind == DICTIONARY_ELEMENTS ||
+           to_kind == SLOW_STRING_WRAPPER_ELEMENTS ||
+           IsTypedArrayElementsKind(to_kind) ||
+           IsAnyHoleyNonextensibleElementsKind(to_kind));
+    to_kind = info.integrity_level_source_map.elements_kind();
+  }
+  if (from_kind != to_kind) {
+    // Try to follow existing elements kind transitions.
+    root_map = root_map.LookupElementsTransitionMap(isolate, to_kind, cmode);
+    if (root_map.is_null()) return {};
+    // 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, cmode);
+  if (result.is_null()) return {};
+
+  if (info.has_integrity_level_transition) {
+    // Now replay the integrity level transition.
+    result = TransitionsAccessor(isolate, result, &no_gc,
+                                 cmode == ConcurrencyMode::kConcurrent)
+                 .SearchSpecial(info.integrity_level_symbol);
+  }
+  if (result.is_null()) return {};
+
+  DCHECK_EQ(old_map.elements_kind(), result.elements_kind());
+  DCHECK_EQ(old_map.instance_type(), result.instance_type());
+  return result;
+}
+
 void MapUpdater::GeneralizeField(Handle<Map> map, InternalIndex modify_index,
                                  PropertyConstness new_constness,
                                  Representation new_representation,

src/objects/map-updater.h

@@ -67,6 +67,12 @@ class V8_EXPORT_PRIVATE MapUpdater {
   // version and performs the steps 1-6.
   Handle<Map> Update();
 
+  // As above but does not mutate maps; instead, we attempt to replay existing
+  // transitions to find an updated map. No lock is taken.
+  static base::Optional<Map> TryUpdateNoLock(Isolate* isolate, Map old_map,
+                                             ConcurrencyMode cmode)
+      V8_WARN_UNUSED_RESULT;
+
   static Handle<Map> ReconfigureExistingProperty(Isolate* isolate,
                                                  Handle<Map> map,
                                                  InternalIndex descriptor,

src/objects/map.cc

@@ -609,7 +609,6 @@ namespace {
 
 Map SearchMigrationTarget(Isolate* isolate, Map old_map) {
   DisallowGarbageCollection no_gc;
-  DisallowDeoptimization no_deoptimization(isolate);
 
   Map target = old_map;
   do {
@@ -636,12 +635,12 @@ Map SearchMigrationTarget(Isolate* isolate, Map old_map) {
     }
   }
 
-  SLOW_DCHECK(Map::TryUpdateSlow(isolate, old_map) == target);
+  SLOW_DCHECK(MapUpdater::TryUpdateNoLock(
+                  isolate, old_map, ConcurrencyMode::kNotConcurrent) == target);
   return target;
 }
 }  // namespace
 
-// TODO(ishell): Move TryUpdate() and friends to MapUpdater
 // static
 MaybeHandle<Map> Map::TryUpdate(Isolate* isolate, Handle<Map> old_map) {
   DisallowGarbageCollection no_gc;
@@ -656,130 +655,14 @@ MaybeHandle<Map> Map::TryUpdate(Isolate* isolate, Handle<Map> old_map) {
     }
   }
 
-  Map new_map = TryUpdateSlow(isolate, *old_map);
+  base::Optional<Map> new_map = MapUpdater::TryUpdateNoLock(
-  if (new_map.is_null()) return MaybeHandle<Map>();
+      isolate, *old_map, ConcurrencyMode::kNotConcurrent);
+  if (!new_map.has_value()) return MaybeHandle<Map>();
   if (FLAG_fast_map_update) {
-    TransitionsAccessor(isolate, *old_map, &no_gc).SetMigrationTarget(new_map);
+    TransitionsAccessor(isolate, *old_map, &no_gc)
+        .SetMigrationTarget(new_map.value());
   }
-  return handle(new_map, isolate);
+  return handle(new_map.value(), isolate);
-}
-
-namespace {
-
-struct IntegrityLevelTransitionInfo {
-  explicit IntegrityLevelTransitionInfo(Map map)
-      : integrity_level_source_map(map) {}
-
-  bool has_integrity_level_transition = false;
-  PropertyAttributes integrity_level = NONE;
-  Map integrity_level_source_map;
-  Symbol integrity_level_symbol;
-};
-
-IntegrityLevelTransitionInfo DetectIntegrityLevelTransitions(
-    Map map, Isolate* isolate, DisallowGarbageCollection* no_gc) {
-  IntegrityLevelTransitionInfo info(map);
-
-  // Figure out the most restrictive integrity level transition (it should
-  // be the last one in the transition tree).
-  DCHECK(!map.is_extensible());
-  Map previous = Map::cast(map.GetBackPointer(isolate));
-  TransitionsAccessor last_transitions(isolate, previous, no_gc);
-  if (!last_transitions.HasIntegrityLevelTransitionTo(
-          map, &(info.integrity_level_symbol), &(info.integrity_level))) {
-    // The last transition was not integrity level transition - just bail out.
-    // This can happen in the following cases:
-    // - there are private symbol transitions following the integrity level
-    //   transitions (see crbug.com/v8/8854).
-    // - there is a getter added in addition to an existing setter (or a setter
-    //   in addition to an existing getter).
-    return info;
-  }
-
-  Map source_map = previous;
-  // Now walk up the back pointer chain and skip all integrity level
-  // transitions. If we encounter any non-integrity level transition interleaved
-  // with integrity level transitions, just bail out.
-  while (!source_map.is_extensible()) {
-    previous = Map::cast(source_map.GetBackPointer(isolate));
-    TransitionsAccessor transitions(isolate, previous, no_gc);
-    if (!transitions.HasIntegrityLevelTransitionTo(source_map)) {
-      return info;
-    }
-    source_map = previous;
-  }
-
-  // Integrity-level transitions never change number of descriptors.
-  CHECK_EQ(map.NumberOfOwnDescriptors(), source_map.NumberOfOwnDescriptors());
-
-  info.has_integrity_level_transition = true;
-  info.integrity_level_source_map = source_map;
-  return info;
-}
-
-}  // namespace
-
-Map Map::TryUpdateSlow(Isolate* isolate, Map old_map) {
-  DisallowGarbageCollection no_gc;
-  DisallowDeoptimization no_deoptimization(isolate);
-
-  // Check the state of the root map.
-  Map root_map = old_map.FindRootMap(isolate);
-  if (root_map.is_deprecated()) {
-    JSFunction constructor = JSFunction::cast(root_map.GetConstructor());
-    DCHECK(constructor.has_initial_map());
-    DCHECK(constructor.initial_map().is_dictionary_map());
-    if (constructor.initial_map().elements_kind() != old_map.elements_kind()) {
-      return Map();
-    }
-    return constructor.initial_map();
-  }
-  if (!old_map.EquivalentToForTransition(root_map)) return Map();
-
-  ElementsKind from_kind = root_map.elements_kind();
-  ElementsKind to_kind = old_map.elements_kind();
-
-  IntegrityLevelTransitionInfo info(old_map);
-  if (root_map.is_extensible() != old_map.is_extensible()) {
-    DCHECK(!old_map.is_extensible());
-    DCHECK(root_map.is_extensible());
-    info = DetectIntegrityLevelTransitions(old_map, isolate, &no_gc);
-    // Bail out if there were some private symbol transitions mixed up
-    // with the integrity level transitions.
-    if (!info.has_integrity_level_transition) return Map();
-    // Make sure to replay the original elements kind transitions, before
-    // the integrity level transition sets the elements to dictionary mode.
-    DCHECK(to_kind == DICTIONARY_ELEMENTS ||
-           to_kind == SLOW_STRING_WRAPPER_ELEMENTS ||
-           IsTypedArrayElementsKind(to_kind) ||
-           IsAnyHoleyNonextensibleElementsKind(to_kind));
-    to_kind = info.integrity_level_source_map.elements_kind();
-  }
-  if (from_kind != to_kind) {
-    // Try to follow existing elements kind transitions.
-    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,
-      ConcurrencyMode::kNotConcurrent);
-  if (result.is_null()) return Map();
-
-  if (info.has_integrity_level_transition) {
-    // Now replay the integrity level transition.
-    result = TransitionsAccessor(isolate, result, &no_gc)
-                 .SearchSpecial(info.integrity_level_symbol);
-  }
-
-  DCHECK_IMPLIES(!result.is_null(),
-                 old_map.elements_kind() == result.elements_kind());
-  DCHECK_IMPLIES(!result.is_null(),
-                 old_map.instance_type() == result.instance_type());
-  return result;
 }
 
 Map Map::TryReplayPropertyTransitions(Isolate* isolate, Map old_map,

src/objects/map.h

@@ -653,6 +653,7 @@ class Map : public TorqueGeneratedMap<Map, HeapObject> {
 
   DECL_BOOLEAN_ACCESSORS(is_deprecated)
   inline bool CanBeDeprecated() const;
+
   // Returns a non-deprecated version of the input. If the input was not
   // deprecated, it is directly returned. Otherwise, the non-deprecated version
   // is found by re-transitioning from the root of the transition tree using the
@@ -660,8 +661,6 @@ class Map : public TorqueGeneratedMap<Map, HeapObject> {
   // is found.
   V8_EXPORT_PRIVATE static MaybeHandle<Map> TryUpdate(
       Isolate* isolate, Handle<Map> map) V8_WARN_UNUSED_RESULT;
-  V8_EXPORT_PRIVATE static Map TryUpdateSlow(Isolate* isolate,
-                                             Map map) V8_WARN_UNUSED_RESULT;
 
   // Returns a non-deprecated version of the input. This method may deprecate
   // existing maps along the way if encodings conflict. Not for use while

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

@@ -70,7 +70,9 @@ static void CheckMigrationTarget(Isolate* isolate, Map old_map, Map new_map) {
                    .GetMigrationTarget();
   if (target.is_null()) return;
   CHECK_EQ(new_map, target);
-  CHECK_EQ(Map::TryUpdateSlow(isolate, old_map), target);
+  CHECK_EQ(MapUpdater::TryUpdateNoLock(isolate, old_map,
+                                       ConcurrencyMode::kNotConcurrent),
+           target);
 }
 
 class Expectations {