[runtime] Add thread-safety to TransitionAccessor via shared mutex

What we need is a multiple readers single writer (MRSW) lock. The
main thread is the only one that is going to be writing, while the
readers might be either the main thread or background threads.

The shared_mutex is in the isolate itself, so that different isolates
will not block each other.

Bug: v8:7790
Change-Id: Idd6bb1826bd0cc6279df1c0694a84e00d53a7eae
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2241513
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68639}

src/compiler/access-info.cc

@@ -769,7 +769,8 @@ PropertyAccessInfo AccessInfoFactory::LookupTransition(
     Handle<Map> map, Handle<Name> name, MaybeHandle<JSObject> holder) const {
   // Check if the {map} has a data transition with the given {name}.
   Map transition =
-      TransitionsAccessor(isolate(), map).SearchTransition(*name, kData, NONE);
+      TransitionsAccessor(isolate(), map, broker()->is_concurrent_inlining())
+          .SearchTransition(*name, kData, NONE);
   if (transition.is_null()) {
     return PropertyAccessInfo::Invalid(zone());
   }
@@ -777,7 +778,7 @@ PropertyAccessInfo AccessInfoFactory::LookupTransition(
   Handle<Map> transition_map(transition, isolate());
   InternalIndex const number = transition_map->LastAdded();
   PropertyDetails const details =
-      transition_map->instance_descriptors().GetDetails(number);
+      transition_map->synchronized_instance_descriptors().GetDetails(number);
   // Don't bother optimizing stores to read-only properties.
   if (details.IsReadOnly()) {
     return PropertyAccessInfo::Invalid(zone());
@@ -812,7 +813,9 @@ PropertyAccessInfo AccessInfoFactory::LookupTransition(
     // Extract the field type from the property details (make sure its
     // representation is TaggedPointer to reflect the heap object case).
     Handle<FieldType> descriptors_field_type(
-        transition_map->instance_descriptors().GetFieldType(number), isolate());
+        transition_map->synchronized_instance_descriptors().GetFieldType(
+            number),
+        isolate());
     if (descriptors_field_type->IsNone()) {
       // Store is not safe if the field type was cleared.
       return PropertyAccessInfo::Invalid(zone());

src/execution/isolate.h

@@ -608,6 +608,11 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   // Mutex for serializing access to break control structures.
   base::RecursiveMutex* break_access() { return &break_access_; }
 
+  // Shared mutex for allowing concurrent read/writes to TransitionArrays.
+  base::SharedMutex* transition_array_access() {
+    return &transition_array_access_;
+  }
+
   Address get_address_from_id(IsolateAddressId id);
 
   // Access to top context (where the current function object was created).
@@ -1640,6 +1645,7 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   CompilationCache* compilation_cache_ = nullptr;
   std::shared_ptr<Counters> async_counters_;
   base::RecursiveMutex break_access_;
+  base::SharedMutex transition_array_access_;
   Logger* logger_ = nullptr;
   StubCache* load_stub_cache_ = nullptr;
   StubCache* store_stub_cache_ = nullptr;

src/objects/map-inl.h

@@ -45,7 +45,8 @@ SYNCHRONIZED_ACCESSORS(Map, synchronized_instance_descriptors, DescriptorArray,
 SYNCHRONIZED_ACCESSORS_CHECKED(Map, layout_descriptor, LayoutDescriptor,
                                kLayoutDescriptorOffset,
                                FLAG_unbox_double_fields)
-WEAK_ACCESSORS(Map, raw_transitions, kTransitionsOrPrototypeInfoOffset)
+SYNCHRONIZED_WEAK_ACCESSORS(Map, raw_transitions,
+                            kTransitionsOrPrototypeInfoOffset)
 
 ACCESSORS_CHECKED2(Map, prototype, HeapObject, kPrototypeOffset, true,
                    value.IsNull() || value.IsJSReceiver())

src/objects/object-macros.h

@@ -196,6 +196,27 @@
 #define WEAK_ACCESSORS(holder, name, offset) \
   WEAK_ACCESSORS_CHECKED(holder, name, offset, true)
 
+#define SYNCHRONIZED_WEAK_ACCESSORS_CHECKED2(holder, name, offset,         \
+                                             get_condition, set_condition) \
+  DEF_GETTER(holder, name, MaybeObject) {                                  \
+    MaybeObject value =                                                    \
+        TaggedField<MaybeObject, offset>::Acquire_Load(isolate, *this);    \
+    DCHECK(get_condition);                                                 \
+    return value;                                                          \
+  }                                                                        \
+  void holder::set_##name(MaybeObject value, WriteBarrierMode mode) {      \
+    DCHECK(set_condition);                                                 \
+    TaggedField<MaybeObject, offset>::Release_Store(*this, value);         \
+    CONDITIONAL_WEAK_WRITE_BARRIER(*this, offset, value, mode);            \
+  }
+
+#define SYNCHRONIZED_WEAK_ACCESSORS_CHECKED(holder, name, offset, condition) \
+  SYNCHRONIZED_WEAK_ACCESSORS_CHECKED2(holder, name, offset, condition,      \
+                                       condition)
+
+#define SYNCHRONIZED_WEAK_ACCESSORS(holder, name, offset) \
+  SYNCHRONIZED_WEAK_ACCESSORS_CHECKED(holder, name, offset, true)
+
 // Getter that returns a Smi as an int and writes an int as a Smi.
 #define SMI_ACCESSORS_CHECKED(holder, name, offset, condition)   \
   int holder::name() const {                                     \

src/objects/transitions-inl.h

@@ -181,13 +181,17 @@ int TransitionArray::SearchName(Name name, int* out_insertion_index) {
 
 TransitionsAccessor::TransitionsAccessor(Isolate* isolate, Map map,
                                          DisallowHeapAllocation* no_gc)
-    : isolate_(isolate), map_(map) {
+    : isolate_(isolate), map_(map), concurrent_access_(false) {
   Initialize();
   USE(no_gc);
 }
 
-TransitionsAccessor::TransitionsAccessor(Isolate* isolate, Handle<Map> map)
-    : isolate_(isolate), map_handle_(map), map_(*map) {
+TransitionsAccessor::TransitionsAccessor(Isolate* isolate, Handle<Map> map,
+                                         bool concurrent_access)
+    : isolate_(isolate),
+      map_handle_(map),
+      map_(*map),
+      concurrent_access_(concurrent_access) {
   Initialize();
 }
 

src/objects/transitions.cc

@@ -35,6 +35,7 @@ bool TransitionsAccessor::HasSimpleTransitionTo(Map map) {
 
 void TransitionsAccessor::Insert(Handle<Name> name, Handle<Map> target,
                                  SimpleTransitionFlag flag) {
+  DCHECK(!concurrent_access_);
   DCHECK(!map_handle_.is_null());
   DCHECK_NE(kPrototypeInfo, encoding());
   target->SetBackPointer(map_);
@@ -47,16 +48,17 @@ void TransitionsAccessor::Insert(Handle<Name> name, Handle<Map> target,
     }
     // If the flag requires a full TransitionArray, allocate one.
     Handle<TransitionArray> result =
-        isolate_->factory()->NewTransitionArray(0, 1);
+        isolate_->factory()->NewTransitionArray(1, 0);
+    result->Set(0, *name, HeapObjectReference::Weak(*target));
     ReplaceTransitions(MaybeObject::FromObject(*result));
     Reload();
     DCHECK_EQ(kFullTransitionArray, encoding());
+    return;
   }
 
-  // If the map has a simple transition, check if it should be overwritten.
-  Map simple_transition = GetSimpleTransition();
-  if (!simple_transition.is_null()) {
-    DCHECK_EQ(kWeakRef, encoding());
+  if (encoding() == kWeakRef) {
+    Map simple_transition = GetSimpleTransition();
+    DCHECK(!simple_transition.is_null());
 
     if (flag == SIMPLE_PROPERTY_TRANSITION) {
       Name key = GetSimpleTransitionKey(simple_transition);
@@ -76,16 +78,46 @@ void TransitionsAccessor::Insert(Handle<Name> name, Handle<Map> target,
     // Reload state; allocations might have caused it to be cleared.
     Reload();
     simple_transition = GetSimpleTransition();
-    if (!simple_transition.is_null()) {
-      DCHECK_EQ(*map, simple_transition);
-      DCHECK_EQ(kWeakRef, encoding());
-      result->Set(0, GetSimpleTransitionKey(simple_transition),
-                  HeapObjectReference::Weak(simple_transition));
+    if (simple_transition.is_null()) {
+      result->Set(0, *name, HeapObjectReference::Weak(*target));
+      ReplaceTransitions(MaybeObject::FromObject(*result));
+      Reload();
+      DCHECK_EQ(kFullTransitionArray, encoding());
+      return;
+    }
+
+    // Insert the original transition in index 0.
+    result->Set(0, GetSimpleTransitionKey(simple_transition),
+                HeapObjectReference::Weak(simple_transition));
+
+    // Search for the correct index to insert the new transition.
+    int insertion_index;
+    int index;
+    if (flag == SPECIAL_TRANSITION) {
+      index = result->SearchSpecial(Symbol::cast(*name), &insertion_index);
     } else {
-      result->SetNumberOfTransitions(0);
+      PropertyDetails details = GetTargetDetails(*name, *target);
+      index = result->Search(details.kind(), *name, details.attributes(),
+                             &insertion_index);
     }
+    DCHECK_EQ(index, kNotFound);
+    USE(index);
+
+    result->SetNumberOfTransitions(2);
+    if (insertion_index == 0) {
+      // If the new transition will be inserted in index 0, move the original
+      // transition to index 1.
+      result->Set(1, GetSimpleTransitionKey(simple_transition),
+                  HeapObjectReference::Weak(simple_transition));
+    }
+    result->SetKey(insertion_index, *name);
+    result->SetRawTarget(insertion_index, HeapObjectReference::Weak(*target));
+
+    SLOW_DCHECK(result->IsSortedNoDuplicates());
     ReplaceTransitions(MaybeObject::FromObject(*result));
     Reload();
+    DCHECK_EQ(kFullTransitionArray, encoding());
+    return;
   }
 
   // At this point, we know that the map has a full TransitionArray.
@@ -112,6 +144,8 @@ void TransitionsAccessor::Insert(Handle<Name> name, Handle<Map> target,
                                    &insertion_index);
     // If an existing entry was found, overwrite it and return.
     if (index != kNotFound) {
+      base::SharedMutexGuard<base::kExclusive> shared_mutex_guard(
+          isolate_->transition_array_access());
       array.SetRawTarget(index, HeapObjectReference::Weak(*target));
       return;
     }
@@ -123,6 +157,8 @@ void TransitionsAccessor::Insert(Handle<Name> name, Handle<Map> target,
 
     // If there is enough capacity, insert new entry into the existing array.
     if (new_nof <= array.Capacity()) {
+      base::SharedMutexGuard<base::kExclusive> shared_mutex_guard(
+          isolate_->transition_array_access());
       array.SetNumberOfTransitions(new_nof);
       for (int i = number_of_transitions; i > insertion_index; --i) {
         array.SetKey(i, array.GetKey(i - 1));
@@ -194,7 +230,11 @@ Map TransitionsAccessor::SearchTransition(Name name, PropertyKind kind,
       return map;
     }
     case kFullTransitionArray: {
-      return transitions().SearchAndGetTarget(kind, name, attributes);
+      if (concurrent_access_) isolate_->transition_array_access()->LockShared();
+      Map result = transitions().SearchAndGetTarget(kind, name, attributes);
+      if (concurrent_access_)
+        isolate_->transition_array_access()->UnlockShared();
+      return result;
     }
   }
   UNREACHABLE();

src/objects/transitions.h

@@ -38,9 +38,14 @@ namespace internal {
 // cleared when the map they refer to is not otherwise reachable.
 class V8_EXPORT_PRIVATE TransitionsAccessor {
  public:
+  // For concurrent access, use the other constructor.
   inline TransitionsAccessor(Isolate* isolate, Map map,
                              DisallowHeapAllocation* no_gc);
-  inline TransitionsAccessor(Isolate* isolate, Handle<Map> map);
+  // {concurrent_access} signals that the TransitionsAccessor will only be used
+  // in background threads. It acquires a reader lock for critical paths, as
+  // well as blocking the accessor from modifying the TransitionsArray.
+  inline TransitionsAccessor(Isolate* isolate, Handle<Map> map,
+                             bool concurrent_access = false);
   // Insert a new transition into |map|'s transition array, extending it
   // as necessary.
   // Requires the constructor that takes a Handle<Map> to have been used.
@@ -182,6 +187,7 @@ class V8_EXPORT_PRIVATE TransitionsAccessor {
   Map map_;
   MaybeObject raw_transitions_;
   Encoding encoding_;
+  bool concurrent_access_;
 #if DEBUG
   bool needs_reload_;
 #endif