[compiler] Thread-safe Map::ComputeMinObjectSlack

ComputeMinObjectSlack is called concurrently from background threads
(when --concurrent-inlining) and must therefore be thread-safe.

This CL adds a compiler-specific thread-safe variant
of ComputeMinObjectSlack in addition to the plain old non-thread-safe
one. Thread-safety is achieved through locking: on the bg thread, a
shared lock when traversing transitions, and on the main thread, an
additional exclusive critical section when overwriting prototype
transitions.

Tbr: leszeks@chromium.org
Bug: v8:7790,v8:12010,chromium:1231901
Change-Id: If5af83df1ab896b22477921449fb5ba4c8d3e8a3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3045342
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75949}

src/compiler/heap-refs.cc

@@ -1056,6 +1056,43 @@ class MapData : public HeapObjectData {
   bool serialized_for_element_store_ = false;
 };
 
+namespace {
+
+int InstanceSizeWithMinSlack(JSHeapBroker* broker, MapRef map) {
+  // This operation is split into two phases (1. map collection, 2. map
+  // processing). This is to avoid having to take two locks
+  // (full_transition_array_access and map_updater_access) at once and thus
+  // having to deal with related deadlock issues.
+  ZoneVector<Handle<Map>> maps(broker->zone());
+  maps.push_back(map.object());
+
+  {
+    DisallowGarbageCollection no_gc;
+
+    // Has to be an initial map.
+    DCHECK(map.object()->GetBackPointer().IsUndefined(broker->isolate()));
+
+    static constexpr bool kConcurrentAccess = true;
+    TransitionsAccessor(broker->isolate(), *map.object(), &no_gc,
+                        kConcurrentAccess)
+        .TraverseTransitionTree([&](Map m) {
+          maps.push_back(broker->CanonicalPersistentHandle(m));
+        });
+  }
+
+  // The lock is needed for UnusedPropertyFields and InstanceSizeFromSlack.
+  JSHeapBroker::MapUpdaterGuardIfNeeded mumd_scope(broker);
+
+  int slack = std::numeric_limits<int>::max();
+  for (Handle<Map> m : maps) {
+    slack = std::min(slack, m->UnusedPropertyFields());
+  }
+
+  return map.object()->InstanceSizeFromSlack(slack);
+}
+
+}  // namespace
+
 // IMPORTANT: Keep this sync'd with JSFunctionData::IsConsistentWithHeapState.
 void JSFunctionData::Cache(JSHeapBroker* broker) {
   CHECK(!serialized_);
@@ -1096,13 +1133,12 @@ void JSFunctionData::Cache(JSHeapBroker* broker) {
       MapRef initial_map_ref = TryMakeRef<Map>(broker, initial_map_).value();
       if (initial_map_ref.IsInobjectSlackTrackingInProgress()) {
         initial_map_instance_size_with_min_slack_ =
-            initial_map_ref.object()->InstanceSizeFromSlack(
-                initial_map_ref.object()->ComputeMinObjectSlack(
-                    broker->isolate()));
+            InstanceSizeWithMinSlack(broker, initial_map_ref);
       } else {
         initial_map_instance_size_with_min_slack_ =
             initial_map_ref.instance_size();
       }
+      CHECK_GT(initial_map_instance_size_with_min_slack_, 0);
 
       if (!initial_map_->should_access_heap() &&
           !broker->is_concurrent_inlining()) {

src/objects/transitions-inl.h

@@ -191,8 +191,9 @@ int TransitionArray::SearchName(Name name, int* out_insertion_index) {
 }
 
 TransitionsAccessor::TransitionsAccessor(Isolate* isolate, Map map,
-                                         DisallowGarbageCollection* no_gc)
-    : isolate_(isolate), map_(map), concurrent_access_(false) {
+                                         DisallowGarbageCollection* no_gc,
+                                         bool concurrent_access)
+    : isolate_(isolate), map_(map), concurrent_access_(concurrent_access) {
   Initialize();
   USE(no_gc);
 }

src/objects/transitions.cc

@@ -382,6 +382,9 @@ void TransitionsAccessor::PutPrototypeTransition(Handle<Object> prototype,
   int capacity = cache->length() - header;
   int transitions = TransitionArray::NumberOfPrototypeTransitions(*cache) + 1;
 
+  base::SharedMutexGuard<base::kExclusive> scope(
+      isolate_->full_transition_array_access());
+
   if (transitions > capacity) {
     // Grow the array if compacting it doesn't free space.
     if (!TransitionArray::CompactPrototypeTransitionArray(isolate_, *cache)) {
@@ -518,7 +521,7 @@ void TransitionsAccessor::TraverseTransitionTreeInternal(
     case kWeakRef: {
       Map simple_target =
           Map::cast(raw_transitions_->GetHeapObjectAssumeWeak());
-      TransitionsAccessor(isolate_, simple_target, no_gc)
+      TransitionsAccessor(isolate_, simple_target, no_gc, concurrent_access_)
           .TraverseTransitionTreeInternal(callback, no_gc);
       break;
     }
@@ -531,7 +534,8 @@ void TransitionsAccessor::TraverseTransitionTreeInternal(
           MaybeObject target = proto_trans.Get(index);
           HeapObject heap_object;
           if (target->GetHeapObjectIfWeak(&heap_object)) {
-            TransitionsAccessor(isolate_, Map::cast(heap_object), no_gc)
+            TransitionsAccessor(isolate_, Map::cast(heap_object), no_gc,
+                                concurrent_access_)
                 .TraverseTransitionTreeInternal(callback, no_gc);
           } else {
             DCHECK(target->IsCleared());
@@ -539,7 +543,8 @@ void TransitionsAccessor::TraverseTransitionTreeInternal(
         }
       }
       for (int i = 0; i < transitions().number_of_transitions(); ++i) {
-        TransitionsAccessor(isolate_, transitions().GetTarget(i), no_gc)
+        TransitionsAccessor(isolate_, transitions().GetTarget(i), no_gc,
+                            concurrent_access_)
             .TraverseTransitionTreeInternal(callback, no_gc);
       }
       break;

src/objects/transitions.h

@@ -6,6 +6,7 @@
 #define V8_OBJECTS_TRANSITIONS_H_
 
 #include "src/common/checks.h"
+#include "src/execution/isolate.h"
 #include "src/objects/descriptor-array.h"
 #include "src/objects/elements-kind.h"
 #include "src/objects/map.h"
@@ -45,12 +46,12 @@ using ForEachTransitionCallback = std::function<void(Map)>;
 // 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,
-                             DisallowGarbageCollection* no_gc);
   // {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, Map map,
+                             DisallowGarbageCollection* no_gc,
+                             bool concurrent_access = false);
   inline TransitionsAccessor(Isolate* isolate, Handle<Map> map,
                              bool concurrent_access = false);
   // Insert a new transition into |map|'s transition array, extending it
@@ -110,6 +111,8 @@ class V8_EXPORT_PRIVATE TransitionsAccessor {
   void TraverseTransitionTree(TraverseCallback callback) {
     // Make sure that we do not allocate in the callback.
     DisallowGarbageCollection no_gc;
+    base::SharedMutexGuardIf<base::kShared> scope(
+        isolate_->full_transition_array_access(), concurrent_access_);
     TraverseTransitionTreeInternal(callback, &no_gc);
   }
 

test/mjsunit/regress/regress-1231901.js

+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Flags: --allow-natives-syntax --interrupt-budget=100
+
+function __f_9() {}
+function __f_10() {}
+
+function f(a, b) {
+  if (b) {
+    new __f_10().__proto__ = a;
+  } else {
+    __f_10.prototype = a;
+  }
+}
+
+function g(a, b, c) {
+    var d = a ? new __f_9() : {};
+  if (b) { g(d); }
+  f(d, c);
+}
+
+g(false, true, false);
+g(false, false, false);
+g(false, false, false, undefined);
+
+g(false, true, true);
+g(false, false, true);
+g(false, false, true, undefined);
+
+g(true, true, false);
+g(true, false, false);
+g(true, false, false, undefined);
+
+g(true, true, true);
+g(true, false, true);
+g(true, false, true, undefined);