[maps] Lock map_updater_access in CompleteInobjectSlackTracking

CompleteInobjectSlackTracking potentially shrinks multiple maps, and
the relation between these maps should be preserved in a concurrent
environment. Thus it is not enough to make each modification
atomically, but all related map modifications must be within a
critical section.

We do this by locking the map_updater_access mutex
CompleteInobjectSlackTracking, and hence moving the function to the
MapUpdater class.

Bug: chromium:1274445,v8:7990
Change-Id: If99bb8b55e03180128ee397d845fa4c269c4241e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3379819Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78597}

src/objects/js-function-inl.h

@@ -14,6 +14,7 @@
 #include "src/ic/ic.h"
 #include "src/init/bootstrapper.h"
 #include "src/objects/feedback-cell-inl.h"
+#include "src/objects/map-updater.h"
 #include "src/objects/shared-function-info-inl.h"
 
 // Has to be the last include (doesn't have include guards):
@@ -123,7 +124,7 @@ bool JSFunction::IsInOptimizationQueue() {
 void JSFunction::CompleteInobjectSlackTrackingIfActive() {
   if (!has_prototype_slot()) return;
   if (has_initial_map() && initial_map().IsInobjectSlackTrackingInProgress()) {
-    initial_map().CompleteInobjectSlackTracking(GetIsolate());
+    MapUpdater::CompleteInobjectSlackTracking(GetIsolate(), initial_map());
   }
 }
 

src/objects/map-inl.h

@@ -12,6 +12,7 @@
 #include "src/objects/field-type.h"
 #include "src/objects/instance-type-inl.h"
 #include "src/objects/js-function-inl.h"
+#include "src/objects/map-updater.h"
 #include "src/objects/map.h"
 #include "src/objects/objects-inl.h"
 #include "src/objects/property.h"
@@ -857,7 +858,7 @@ void Map::InobjectSlackTrackingStep(Isolate* isolate) {
   int counter = construction_counter();
   set_construction_counter(counter - 1);
   if (counter == kSlackTrackingCounterEnd) {
-    CompleteInobjectSlackTracking(isolate);
+    MapUpdater::CompleteInobjectSlackTracking(isolate, *this);
   }
 }
 

src/objects/map-updater.cc

@@ -420,21 +420,50 @@ MapUpdater::State MapUpdater::Normalize(const char* reason) {
   return state_;  // Done.
 }
 
-void MapUpdater::ShrinkInstanceSize(base::SharedMutex* map_updater_access,
-                                    Map map, int slack) {
+// static
+void MapUpdater::CompleteInobjectSlackTracking(Isolate* isolate,
+                                               Map initial_map) {
+  DisallowGarbageCollection no_gc;
+  // Has to be an initial map.
+  DCHECK(initial_map.GetBackPointer().IsUndefined(isolate));
+
+  const int slack = initial_map.ComputeMinObjectSlack(isolate);
   DCHECK_GE(slack, 0);
+
+  TransitionsAccessor transitions(isolate, initial_map, &no_gc);
+  TransitionsAccessor::TraverseCallback callback;
+  if (slack != 0) {
+    // Resize the initial map and all maps in its transition tree.
+    callback = [slack](Map map) {
 #ifdef DEBUG
-  int old_visitor_id = Map::GetVisitorId(map);
-  int new_unused = map.UnusedPropertyFields() - slack;
+      int old_visitor_id = Map::GetVisitorId(map);
+      int new_unused = map.UnusedPropertyFields() - slack;
 #endif
+      map.set_instance_size(map.InstanceSizeFromSlack(slack));
+      map.set_construction_counter(Map::kNoSlackTracking);
+      DCHECK_EQ(old_visitor_id, Map::GetVisitorId(map));
+      DCHECK_EQ(new_unused, map.UnusedPropertyFields());
+    };
+  } else {
+    // Stop slack tracking for this map.
+    callback = [](Map map) {
+      map.set_construction_counter(Map::kNoSlackTracking);
+    };
+  }
 
   {
-    base::SharedMutexGuard<base::kExclusive> mutex_guard(map_updater_access);
-    map.set_instance_size(map.InstanceSizeFromSlack(slack));
+    // The map_updater_access lock is taken here to guarantee atomicity of all
+    // related map changes (instead of guaranteeing only atomicity of each
+    // single map change). This is needed e.g. by InstancesNeedsRewriting,
+    // which expects certain relations between maps to hold.
+    //
+    // Note: Avoid locking the full_transition_array_access lock inside this
+    // call to TraverseTransitionTree to prevent dependencies between the two
+    // locks.
+    base::SharedMutexGuard<base::kExclusive> mutex_guard(
+        isolate->map_updater_access());
+    transitions.TraverseTransitionTree(callback);
   }
-  map.set_construction_counter(Map::kNoSlackTracking);
-  DCHECK_EQ(old_visitor_id, Map::GetVisitorId(map));
-  DCHECK_EQ(new_unused, map.UnusedPropertyFields());
 }
 
 MapUpdater::State MapUpdater::TryReconfigureToDataFieldInplace() {

src/objects/map-updater.h

@@ -86,8 +86,9 @@ class V8_EXPORT_PRIVATE MapUpdater {
                               Representation new_representation,
                               Handle<FieldType> new_field_type);
 
-  static void ShrinkInstanceSize(base::SharedMutex* map_updater_access, Map map,
-                                 int slack);
+  // Completes inobject slack tracking for the transition tree starting at the
+  // initial map.
+  static void CompleteInobjectSlackTracking(Isolate* isolate, Map initial_map);
 
  private:
   enum State {

src/objects/map.cc

@@ -2156,28 +2156,6 @@ int Map::ComputeMinObjectSlack(Isolate* isolate) {
   return slack;
 }
 
-void Map::CompleteInobjectSlackTracking(Isolate* isolate) {
-  DisallowGarbageCollection no_gc;
-  // Has to be an initial map.
-  DCHECK(GetBackPointer().IsUndefined(isolate));
-
-  int slack = ComputeMinObjectSlack(isolate);
-  TransitionsAccessor transitions(isolate, *this, &no_gc);
-  TransitionsAccessor::TraverseCallback callback;
-  if (slack != 0) {
-    // Resize the initial map and all maps in its transition tree.
-    callback = [&](Map map) {
-      MapUpdater::ShrinkInstanceSize(isolate->map_updater_access(), map, slack);
-    };
-  } else {
-    callback = [](Map map) {
-      // Stop slack tracking for this map.
-      map.set_construction_counter(Map::kNoSlackTracking);
-    };
-  }
-  transitions.TraverseTransitionTree(callback);
-}
-
 void Map::SetInstanceDescriptors(Isolate* isolate, DescriptorArray descriptors,
                                  int number_of_own_descriptors) {
   set_instance_descriptors(descriptors, kReleaseStore);

src/objects/map.h

@@ -356,10 +356,6 @@ class Map : public TorqueGeneratedMap<Map, HeapObject> {
   int ComputeMinObjectSlack(Isolate* isolate);
   inline int InstanceSizeFromSlack(int slack) const;
 
-  // Completes inobject slack tracking for the transition tree starting at this
-  // initial map.
-  V8_EXPORT_PRIVATE void CompleteInobjectSlackTracking(Isolate* isolate);
-
   // Tells whether the object in the prototype property will be used
   // for instances created from this function.  If the prototype
   // property is set to a value that is not a JSObject, the prototype

src/runtime/runtime-object.cc

@@ -1035,7 +1035,7 @@ RUNTIME_FUNCTION(Runtime_CompleteInobjectSlackTrackingForMap) {
   DCHECK_EQ(1, args.length());
 
   CONVERT_ARG_HANDLE_CHECKED(Map, initial_map, 0);
-  initial_map->CompleteInobjectSlackTracking(isolate);
+  MapUpdater::CompleteInobjectSlackTracking(isolate, *initial_map);
 
   return ReadOnlyRoots(isolate).undefined_value();
 }

src/runtime/runtime-test.cc

@@ -1390,7 +1390,7 @@ RUNTIME_FUNCTION(Runtime_CompleteInobjectSlackTracking) {
   DCHECK_EQ(1, args.length());
 
   CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 0);
-  object->map().CompleteInobjectSlackTracking(isolate);
+  MapUpdater::CompleteInobjectSlackTracking(isolate, object->map());
 
   return ReadOnlyRoots(isolate).undefined_value();
 }

test/cctest/test-api.cc

@@ -71,6 +71,7 @@
 #include "src/objects/js-array-inl.h"
 #include "src/objects/js-promise-inl.h"
 #include "src/objects/lookup.h"
+#include "src/objects/map-updater.h"
 #include "src/objects/module-inl.h"
 #include "src/objects/objects-inl.h"
 #include "src/objects/string-inl.h"
@@ -2981,9 +2982,9 @@ TEST(InternalFieldsSubclassing) {
         CHECK_LE(i_value->map().GetInObjectProperties(), kMaxNofProperties);
       }
 
-      // Make Sure we get the precise property count.
-      i_value->map().FindRootMap(i_isolate).CompleteInobjectSlackTracking(
-          i_isolate);
+      // Make sure we get the precise property count.
+      i::MapUpdater::CompleteInobjectSlackTracking(
+          i_isolate, i_value->map().FindRootMap(i_isolate));
       // TODO(cbruni): fix accounting to make this condition true.
       // CHECK_EQ(0, i_value->map()->UnusedPropertyFields());
       if (in_object_only) {