cppgc: Delay CrossThreadPersistent processing

During a final atomic pause CrossThreadPersistent handles need to be
frozen after they have been marked to avoid any
WeakCrossThreadPersistent handles creating new strong references
(through their Lock() call) that would retain objects.

Handles are frozen by acquiring a lock. Since this lock is also taking
by other threads on WCTP::Lock() this can introduce jank.

This CL improves the situation by delaying processing of CTP
references until absolutely necessary, i.e., when we have otherwise no
more objects to mark.

Bug: chromium:1252743
Change-Id: I872f38c6d24d7955bea74fd59685abd3019b385e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3194253Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77139}

src/heap/cppgc/marker.cc

@@ -244,12 +244,6 @@ void MarkerBase::EnterAtomicPause(MarkingConfig::StackState stack_state) {
   config_.stack_state = stack_state;
   config_.marking_type = MarkingConfig::MarkingType::kAtomic;
 
-  // Lock guards against changes to {Weak}CrossThreadPersistent handles, that
-  // may conflict with marking. E.g., a WeakCrossThreadPersistent may be
-  // converted into a CrossThreadPersistent which requires that the handle
-  // is either cleared or the object is retained.
-  g_process_mutex.Pointer()->Lock();
-
   {
     // VisitRoots also resets the LABs.
     VisitRoots(config_.stack_state);
@@ -307,6 +301,7 @@ void MarkerBase::ProcessWeakness() {
   heap().GetWeakPersistentRegion().Trace(&visitor());
   // Processing cross-thread handles requires taking the process lock.
   g_process_mutex.Get().AssertHeld();
+  CHECK(visited_cross_thread_persistents_in_atomic_pause_);
   heap().GetWeakCrossThreadPersistentRegion().Trace(&visitor());
 
   // Call weak callbacks on objects that may now be pointing to dead objects.
@@ -336,13 +331,6 @@ void MarkerBase::VisitRoots(MarkingConfig::StackState stack_state) {
           heap().stats_collector(), StatsCollector::kMarkVisitPersistents);
       heap().GetStrongPersistentRegion().Trace(&visitor());
     }
-    if (config_.marking_type == MarkingConfig::MarkingType::kAtomic) {
-      StatsCollector::DisabledScope inner_stats_scope(
-          heap().stats_collector(),
-          StatsCollector::kMarkVisitCrossThreadPersistents);
-      g_process_mutex.Get().AssertHeld();
-      heap().GetStrongCrossThreadPersistentRegion().Trace(&visitor());
-    }
   }
 
   if (stack_state != MarkingConfig::StackState::kNoHeapPointers) {
@@ -355,6 +343,24 @@ void MarkerBase::VisitRoots(MarkingConfig::StackState stack_state) {
   }
 }
 
+bool MarkerBase::VisitCrossThreadPersistentsIfNeeded() {
+  if (config_.marking_type != MarkingConfig::MarkingType::kAtomic ||
+      visited_cross_thread_persistents_in_atomic_pause_)
+    return false;
+
+  StatsCollector::DisabledScope inner_stats_scope(
+      heap().stats_collector(),
+      StatsCollector::kMarkVisitCrossThreadPersistents);
+  // Lock guards against changes to {Weak}CrossThreadPersistent handles, that
+  // may conflict with marking. E.g., a WeakCrossThreadPersistent may be
+  // converted into a CrossThreadPersistent which requires that the handle
+  // is either cleared or the object is retained.
+  g_process_mutex.Pointer()->Lock();
+  heap().GetStrongCrossThreadPersistentRegion().Trace(&visitor());
+  visited_cross_thread_persistents_in_atomic_pause_ = true;
+  return true;
+}
+
 void MarkerBase::ScheduleIncrementalMarkingTask() {
   DCHECK(platform_);
   if (!foreground_task_runner_ || incremental_marking_handle_) return;
@@ -399,8 +405,13 @@ bool MarkerBase::AdvanceMarkingWithLimits(v8::base::TimeDelta max_duration,
         heap().stats_collector(),
         StatsCollector::kMarkTransitiveClosureWithDeadline, "deadline_ms",
         max_duration.InMillisecondsF());
-    is_done = ProcessWorklistsWithDeadline(
-        marked_bytes_limit, v8::base::TimeTicks::Now() + max_duration);
+    const auto deadline = v8::base::TimeTicks::Now() + max_duration;
+    is_done = ProcessWorklistsWithDeadline(marked_bytes_limit, deadline);
+    if (is_done && VisitCrossThreadPersistentsIfNeeded()) {
+      // Both limits are absolute and hence can be passed along without further
+      // adjustment.
+      is_done = ProcessWorklistsWithDeadline(marked_bytes_limit, deadline);
+    }
     schedule_.UpdateMutatorThreadMarkedBytes(
         mutator_marking_state_.marked_bytes());
   }

src/heap/cppgc/marker.h

@@ -164,6 +164,8 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   void VisitRoots(MarkingConfig::StackState);
 
+  bool VisitCrossThreadPersistentsIfNeeded();
+
   void MarkNotFullyConstructedObjects();
 
   void ScheduleIncrementalMarkingTask();
@@ -186,6 +188,7 @@ class V8_EXPORT_PRIVATE MarkerBase {
   std::unique_ptr<ConcurrentMarkerBase> concurrent_marker_{nullptr};
 
   bool main_marking_disabled_for_testing_{false};
+  bool visited_cross_thread_persistents_in_atomic_pause_{false};
 
   friend class MarkerFactory;
 };