[compiler] Make Map a background-serialized-object

.. by locking the MapUpdater lock during MapData construction.

Note this only applies to basic MapRef/MapData construction. Some
methods, in particular MapRef::SerializeFoo methods, are not yet
background-serializable in general and require more work.

Bug: v8:7790
Change-Id: I473e78c82012ab6abc5a0633a4d34c4a40a3fb77
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2839553
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74164}

src/compiler/access-info.cc

@@ -722,9 +722,9 @@ PropertyAccessInfo AccessInfoFactory::ComputePropertyAccessInfo(
     Handle<Map> map, Handle<Name> name, AccessMode access_mode) const {
   CHECK(name->IsUniqueName());
 
+  JSHeapBroker::MapUpdaterMutexDepthScope mumd_scope(broker());
   base::SharedMutexGuardIf<base::kShared> mutex_guard(
-      isolate()->map_updater_access(), should_lock_mutex());
-  MapUpdaterMutexDepthScope mumd_scope(this);
+      isolate()->map_updater_access(), mumd_scope.should_lock());
 
   if (access_mode == AccessMode::kHas && !map->IsJSReceiverMap()) {
     return Invalid();
@@ -864,8 +864,7 @@ PropertyAccessInfo AccessInfoFactory::ComputePropertyAccessInfo(
     }
 
     // Walk up the prototype chain.
-    ObjectData* data = broker()->TryGetOrCreateData(
-        map, false, ObjectRef::BackgroundSerialization::kAllowed);
+    ObjectData* data = broker()->TryGetOrCreateData(map, false);
     if (data == nullptr) return Invalid();
 
     if (!MapRef(broker(), data).TrySerializePrototype()) {

src/compiler/access-info.h

@@ -317,28 +317,6 @@ class AccessInfoFactory final {
                               Handle<Name> name, InternalIndex* index_out,
                               PropertyDetails* details_out) const;
 
-  bool should_lock_mutex() const { return map_updater_mutex_depth_ == 0; }
-
-  class MapUpdaterMutexDepthScope final {
-   public:
-    explicit MapUpdaterMutexDepthScope(const AccessInfoFactory* ptr)
-        : ptr_(ptr),
-          initial_map_updater_mutex_depth_(ptr->map_updater_mutex_depth_) {
-      ptr_->map_updater_mutex_depth_++;
-    }
-
-    ~MapUpdaterMutexDepthScope() {
-      ptr_->map_updater_mutex_depth_--;
-      DCHECK_EQ(initial_map_updater_mutex_depth_,
-                ptr_->map_updater_mutex_depth_);
-      USE(initial_map_updater_mutex_depth_);
-    }
-
-   private:
-    const AccessInfoFactory* const ptr_;
-    const int initial_map_updater_mutex_depth_;
-  };
-
   CompilationDependencies* dependencies() const { return dependencies_; }
   JSHeapBroker* broker() const { return broker_; }
   Isolate* isolate() const;
@@ -349,12 +327,6 @@ class AccessInfoFactory final {
   TypeCache const* const type_cache_;
   Zone* const zone_;
 
-  // ComputePropertyAccessInfo can be called recursively, thus we need to
-  // emulate a recursive mutex. This field holds the locking depth, i.e. how
-  // many times the mutex has been recursively locked. Only the outermost
-  // locker actually locks underneath.
-  mutable int map_updater_mutex_depth_ = 0;
-
   // TODO(nicohartmann@): Move to public
   AccessInfoFactory(const AccessInfoFactory&) = delete;
   AccessInfoFactory& operator=(const AccessInfoFactory&) = delete;

src/compiler/heap-refs.cc

@@ -1184,21 +1184,24 @@ class MapData : public HeapObjectData {
   void SerializeForElementStore(JSHeapBroker* broker);
 
  private:
-  InstanceType const instance_type_;
-  int const instance_size_;
-  byte const bit_field_;
-  byte const bit_field2_;
-  uint32_t const bit_field3_;
-  bool const can_be_deprecated_;
-  bool const can_transition_;
-  int const in_object_properties_start_in_words_;
-  int const in_object_properties_;
-  int const constructor_function_index_;
-  int const next_free_property_index_;
-  int const unused_property_fields_;
-  bool const supports_fast_array_iteration_;
-  bool const supports_fast_array_resize_;
-  bool const is_abandoned_prototype_map_;
+  // The following fields should be const in principle, but construction
+  // requires locking the MapUpdater lock. For this reason, it's easier to
+  // initialize these inside the constructor body, not in the initializer list.
+  InstanceType instance_type_;
+  int instance_size_;
+  byte bit_field_;
+  byte bit_field2_;
+  uint32_t bit_field3_;
+  bool can_be_deprecated_;
+  bool can_transition_;
+  int in_object_properties_start_in_words_;
+  int in_object_properties_;
+  int constructor_function_index_;
+  int next_free_property_index_;
+  int unused_property_fields_;
+  bool supports_fast_array_iteration_;
+  bool supports_fast_array_resize_;
+  bool is_abandoned_prototype_map_;
 
   bool serialized_elements_kind_generalizations_ = false;
   ZoneVector<ObjectData*> elements_kind_generalizations_;
@@ -1274,7 +1277,7 @@ HeapObjectData::HeapObjectData(JSHeapBroker* broker, ObjectData** storage,
       // instance_type_ member. In the case of constructing the MapData for the
       // meta map (whose map is itself), this member has not yet been
       // initialized.
-      map_(broker->GetOrCreateData(object->map())) {
+      map_(broker->GetOrCreateData(object->synchronized_map())) {
   CHECK_IMPLIES(kind == kSerializedHeapObject,
                 broker->mode() == JSHeapBroker::kSerializing);
   CHECK_IMPLIES(broker->mode() == JSHeapBroker::kSerialized,
@@ -1302,17 +1305,22 @@ bool IsReadOnlyLengthDescriptor(Isolate* isolate, Handle<Map> jsarray_map) {
   return descriptors.GetDetails(offset).IsReadOnly();
 }
 
-bool SupportsFastArrayIteration(Isolate* isolate, Handle<Map> map) {
+// Important: this predicate does not check Protectors::IsNoElementsIntact. The
+// compiler checks protectors through the compilation dependency mechanism; it
+// doesn't make sense to do that here as part of every MapData construction.
+// Callers *must* take care to take the correct dependency themselves.
+bool SupportsFastArrayIteration(JSHeapBroker* broker, Handle<Map> map) {
   return map->instance_type() == JS_ARRAY_TYPE &&
          IsFastElementsKind(map->elements_kind()) &&
          map->prototype().IsJSArray() &&
-         isolate->IsAnyInitialArrayPrototype(JSArray::cast(map->prototype())) &&
-         Protectors::IsNoElementsIntact(isolate);
+         broker->IsArrayOrObjectPrototype(broker->CanonicalPersistentHandle(
+             JSArray::cast(map->prototype())));
 }
 
-bool SupportsFastArrayResize(Isolate* isolate, Handle<Map> map) {
-  return SupportsFastArrayIteration(isolate, map) && map->is_extensible() &&
-         !map->is_dictionary_map() && !IsReadOnlyLengthDescriptor(isolate, map);
+bool SupportsFastArrayResize(JSHeapBroker* broker, Handle<Map> map) {
+  return SupportsFastArrayIteration(broker, map) && map->is_extensible() &&
+         !map->is_dictionary_map() &&
+         !IsReadOnlyLengthDescriptor(broker->isolate(), map);
 }
 
 }  // namespace
@@ -1320,37 +1328,42 @@ bool SupportsFastArrayResize(Isolate* isolate, Handle<Map> map) {
 MapData::MapData(JSHeapBroker* broker, ObjectData** storage, Handle<Map> object,
                  ObjectDataKind kind)
     : HeapObjectData(broker, storage, object, kind),
-      instance_type_(object->instance_type()),
-      instance_size_(object->instance_size()),
-      // We read the bit_field as relaxed since `has_non_instance_prototype` can
-      // be modified in live objects, and because we serialize some maps on the
-      // background. Those background-serialized maps are the native context's
-      // maps for which this bit is "set" but it doesn't change value (i.e. it
-      // is set to false when it was already false).
-      bit_field_(object->relaxed_bit_field()),
-      bit_field2_(object->bit_field2()),
-      // Similar to the bit_field comment above.
-      bit_field3_(object->relaxed_bit_field3()),
-      can_be_deprecated_(object->NumberOfOwnDescriptors() > 0
-                             ? object->CanBeDeprecated()
-                             : false),
-      can_transition_(object->CanTransition()),
-      in_object_properties_start_in_words_(
-          object->IsJSObjectMap() ? object->GetInObjectPropertiesStartInWords()
-                                  : 0),
-      in_object_properties_(
-          object->IsJSObjectMap() ? object->GetInObjectProperties() : 0),
-      constructor_function_index_(object->IsPrimitiveMap()
-                                      ? object->GetConstructorFunctionIndex()
-                                      : Map::kNoConstructorFunctionIndex),
-      next_free_property_index_(object->NextFreePropertyIndex()),
-      unused_property_fields_(object->UnusedPropertyFields()),
-      supports_fast_array_iteration_(
-          SupportsFastArrayIteration(broker->isolate(), object)),
-      supports_fast_array_resize_(
-          SupportsFastArrayResize(broker->isolate(), object)),
-      is_abandoned_prototype_map_(object->is_abandoned_prototype_map()),
-      elements_kind_generalizations_(broker->zone()) {}
+      elements_kind_generalizations_(broker->zone()) {
+  // This lock ensure that MapData can always be background-serialized, i.e.
+  // while the lock is held the Map object may not be modified (except in
+  // benign ways).
+  // TODO(jgruber): Consider removing this lock by being smrt.
+  JSHeapBroker::MapUpdaterMutexDepthScope mumd_scope(broker);
+  base::SharedMutexGuardIf<base::kShared> mutex_guard(
+      broker->isolate()->map_updater_access(), mumd_scope.should_lock());
+
+  instance_type_ = object->instance_type();
+  instance_size_ = object->instance_size();
+  // We read the bit_field as relaxed since `has_non_instance_prototype` can
+  // be modified in live objects, and because we serialize some maps on the
+  // background. Those background-serialized maps are the native context's
+  // maps for which this bit is "set" but it doesn't change value (i.e. it
+  // is set to false when it was already false).
+  bit_field_ = object->relaxed_bit_field();
+  bit_field2_ = object->bit_field2();
+  // Similar to the bit_field comment above.
+  bit_field3_ = object->relaxed_bit_field3();
+  can_be_deprecated_ =
+      object->NumberOfOwnDescriptors() > 0 ? object->CanBeDeprecated() : false;
+  can_transition_ = object->CanTransition();
+  in_object_properties_start_in_words_ =
+      object->IsJSObjectMap() ? object->GetInObjectPropertiesStartInWords() : 0;
+  in_object_properties_ =
+      object->IsJSObjectMap() ? object->GetInObjectProperties() : 0;
+  constructor_function_index_ = object->IsPrimitiveMap()
+                                    ? object->GetConstructorFunctionIndex()
+                                    : Map::kNoConstructorFunctionIndex;
+  next_free_property_index_ = object->NextFreePropertyIndex();
+  unused_property_fields_ = object->UnusedPropertyFields();
+  supports_fast_array_iteration_ = SupportsFastArrayIteration(broker, object);
+  supports_fast_array_resize_ = SupportsFastArrayResize(broker, object);
+  is_abandoned_prototype_map_ = object->is_abandoned_prototype_map();
+}
 
 JSFunctionData::JSFunctionData(JSHeapBroker* broker, ObjectData** storage,
                                Handle<JSFunction> object)
@@ -2293,7 +2306,7 @@ bool MapData::TrySerializeOwnDescriptor(JSHeapBroker* broker,
 
   if (instance_descriptors_ == nullptr) {
     instance_descriptors_ =
-        broker->TryGetOrCreateData(map->instance_descriptors(isolate));
+        broker->TryGetOrCreateData(map->instance_descriptors(kAcquireLoad));
     if (instance_descriptors_ == nullptr) return false;
   }
 
@@ -2628,6 +2641,8 @@ void JSHeapBroker::InitializeAndStartSerializing(
   refs_ =
       zone()->New<RefsMap>(kInitialRefsBucketCount, AddressMatcher(), zone());
 
+  CollectArrayAndObjectPrototypes();
+
   SetTargetNativeContextRef(native_context);
   target_native_context().Serialize();
   if (!FLAG_turbo_direct_heap_access) {
@@ -2636,8 +2651,6 @@ void JSHeapBroker::InitializeAndStartSerializing(
     target_native_context().SerializeOnBackground();
   }
 
-  CollectArrayAndObjectPrototypes();
-
   Factory* const f = isolate()->factory();
   if (!FLAG_turbo_direct_heap_access) {
     ObjectData* data;
@@ -2895,14 +2908,14 @@ bool MapRef::HasOnlyStablePrototypesWithFastElements(
 
 bool MapRef::supports_fast_array_iteration() const {
   if (data_->should_access_heap()) {
-    return SupportsFastArrayIteration(broker()->isolate(), object());
+    return SupportsFastArrayIteration(broker(), object());
   }
   return data()->AsMap()->supports_fast_array_iteration();
 }
 
 bool MapRef::supports_fast_array_resize() const {
   if (data_->should_access_heap()) {
-    return SupportsFastArrayResize(broker()->isolate(), object());
+    return SupportsFastArrayResize(broker(), object());
   }
   return data()->AsMap()->supports_fast_array_resize();
 }

src/compiler/heap-refs.h

@@ -105,14 +105,14 @@ enum class OddballType : uint8_t {
 #define HEAP_BROKER_POSSIBLY_BACKGROUND_SERIALIZED_OBJECT_LIST(V) \
   /* Subtypes of HeapObject */                                    \
   V(BigInt)                                                       \
-  V(HeapNumber)                                                   \
-  V(Map)
+  V(HeapNumber)
 
 // This list is sorted such that subtypes appear before their supertypes.
 // DO NOT VIOLATE THIS PROPERTY!
 // Types in this list can be serialized on demand from the background thread.
 #define HEAP_BROKER_BACKGROUND_SERIALIZED_OBJECT_LIST(V) \
   /* Subtypes of HeapObject */                           \
+  V(Map)                                                 \
   V(PropertyCell)
 
 // This list is sorted such that subtypes appear before their supertypes.

src/compiler/js-call-reducer.cc

@@ -5717,15 +5717,17 @@ Reduction JSCallReducer::ReduceArrayPrototypeSlice(Node* node) {
   bool can_be_holey = false;
   for (Handle<Map> map : receiver_maps) {
     MapRef receiver_map(broker(), map);
-    if (!receiver_map.supports_fast_array_iteration())
+    if (!receiver_map.supports_fast_array_iteration()) {
       return inference.NoChange();
+    }
     if (IsHoleyElementsKind(receiver_map.elements_kind())) {
       can_be_holey = true;
     }
   }
 
-  if (!dependencies()->DependOnArraySpeciesProtector())
+  if (!dependencies()->DependOnArraySpeciesProtector()) {
     return inference.NoChange();
+  }
   if (can_be_holey && !dependencies()->DependOnNoElementsProtector()) {
     return inference.NoChange();
   }

src/compiler/js-heap-broker.cc

@@ -213,14 +213,18 @@ bool JSHeapBroker::IsSerializedForCompilation(
 }
 
 bool JSHeapBroker::IsArrayOrObjectPrototype(const JSObjectRef& object) const {
+  return IsArrayOrObjectPrototype(object.object());
+}
+
+bool JSHeapBroker::IsArrayOrObjectPrototype(Handle<JSObject> object) const {
   if (mode() == kDisabled) {
-    return isolate()->IsInAnyContext(*object.object(),
+    return isolate()->IsInAnyContext(*object,
                                      Context::INITIAL_ARRAY_PROTOTYPE_INDEX) ||
-           isolate()->IsInAnyContext(*object.object(),
+           isolate()->IsInAnyContext(*object,
                                      Context::INITIAL_OBJECT_PROTOTYPE_INDEX);
   }
   CHECK(!array_and_object_prototypes_.empty());
-  return array_and_object_prototypes_.find(object.object()) !=
+  return array_and_object_prototypes_.find(object) !=
          array_and_object_prototypes_.end();
 }
 

src/compiler/js-heap-broker.h

@@ -173,6 +173,7 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   // Check if {object} is any native context's %ArrayPrototype% or
   // %ObjectPrototype%.
   bool IsArrayOrObjectPrototype(const JSObjectRef& object) const;
+  bool IsArrayOrObjectPrototype(Handle<JSObject> object) const;
 
   bool HasFeedback(FeedbackSource const& source) const;
   void SetFeedback(FeedbackSource const& source,
@@ -342,6 +343,29 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
 
   RootIndexMap const& root_index_map() { return root_index_map_; }
 
+  class MapUpdaterMutexDepthScope final {
+   public:
+    explicit MapUpdaterMutexDepthScope(JSHeapBroker* ptr)
+        : ptr_(ptr),
+          initial_map_updater_mutex_depth_(ptr->map_updater_mutex_depth_) {
+      ptr_->map_updater_mutex_depth_++;
+    }
+
+    ~MapUpdaterMutexDepthScope() {
+      ptr_->map_updater_mutex_depth_--;
+      DCHECK_EQ(initial_map_updater_mutex_depth_,
+                ptr_->map_updater_mutex_depth_);
+    }
+
+    // Whether the MapUpdater mutex should be physically locked (if not, we
+    // already hold the lock).
+    bool should_lock() const { return initial_map_updater_mutex_depth_ == 0; }
+
+   private:
+    JSHeapBroker* const ptr_;
+    const int initial_map_updater_mutex_depth_;
+  };
+
  private:
   friend class HeapObjectRef;
   friend class ObjectRef;
@@ -456,9 +480,19 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   };
   ZoneMultimap<SerializedFunction, HintsVector> serialized_functions_;
 
-  static const size_t kMaxSerializedFunctionsCacheSize = 200;
-  static const uint32_t kMinimalRefsBucketCount = 8;     // must be power of 2
-  static const uint32_t kInitialRefsBucketCount = 1024;  // must be power of 2
+  // The MapUpdater mutex is used in recursive patterns; for example,
+  // ComputePropertyAccessInfo may call itself recursively. Thus we need to
+  // emulate a recursive mutex, which we do by checking if this heap broker
+  // instance already holds the mutex when a lock is requested. This field
+  // holds the locking depth, i.e. how many times the mutex has been
+  // recursively locked. Only the outermost locker actually locks underneath.
+  int map_updater_mutex_depth_ = 0;
+
+  static constexpr size_t kMaxSerializedFunctionsCacheSize = 200;
+  static constexpr uint32_t kMinimalRefsBucketCount = 8;
+  STATIC_ASSERT(base::bits::IsPowerOfTwo(kMinimalRefsBucketCount));
+  static constexpr uint32_t kInitialRefsBucketCount = 1024;
+  STATIC_ASSERT(base::bits::IsPowerOfTwo(kInitialRefsBucketCount));
 };
 
 class V8_NODISCARD TraceScope {

src/objects/map.cc

@@ -1141,7 +1141,7 @@ Handle<Map> Map::AsElementsKind(Isolate* isolate, Handle<Map> map,
 
 int Map::NumberOfEnumerableProperties() const {
   int result = 0;
-  DescriptorArray descs = instance_descriptors(kRelaxedLoad);
+  DescriptorArray descs = instance_descriptors(kAcquireLoad);
   for (InternalIndex i : IterateOwnDescriptors()) {
     if ((descs.GetDetails(i).attributes() & ONLY_ENUMERABLE) == 0 &&
         !descs.GetKey(i).FilterKey(ENUMERABLE_STRINGS)) {
@@ -1153,7 +1153,7 @@ int Map::NumberOfEnumerableProperties() const {
 
 int Map::NextFreePropertyIndex() const {
   int number_of_own_descriptors = NumberOfOwnDescriptors();
-  DescriptorArray descs = instance_descriptors();
+  DescriptorArray descs = instance_descriptors(kAcquireLoad);
   // Search properties backwards to find the last field.
   for (int i = number_of_own_descriptors - 1; i >= 0; --i) {
     PropertyDetails details = descs.GetDetails(InternalIndex(i));