[cppgc] Make clearing of weak persistents concurrent

nickie · V8 LUCI CQ · commit 9144af854007 · 2024-08-29T13:20:59.000Z
Oilpan's weakness clearing phase often takes around 10% of the overall mark-compact pause. A small portion is taken by the clearing of same-thread WeakPersistent handles. This CL introduces a concurrent job for clearing such handles, in parallel to weak container callbacks. Bug: 349952799 Change-Id: I497b199edd674379d552281125cdca65bc766bd3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5822092 Reviewed-by: Anton Bikineev <bikineev@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org> Cr-Commit-Position: refs/heads/main@{#95872}
diff --git a/include/cppgc/internal/persistent-node.h b/include/cppgc/internal/persistent-node.h
@@ -158,6 +158,12 @@ class V8_EXPORT PersistentRegion final : public PersistentRegionBase {
     PersistentRegionBase::FreeNode(node);
   }
 
+  // During GC, nodes can be freed from a concurrent thread, so no thread check
+  // takes place here.
+  V8_INLINE void FreeNodeFromGC(PersistentNode* node) {
+    PersistentRegionBase::FreeNode(node);
+  }
+
  private:
   bool IsCreationThread();
 
diff --git a/include/cppgc/persistent.h b/include/cppgc/persistent.h
@@ -256,7 +256,7 @@ class BasicPersistent final : public PersistentBase,
 
   void ClearFromGC() const {
     if (IsValid()) {
-      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
+      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNodeFromGC(GetNode());
       PersistentBase::ClearFromGC();
     }
   }
diff --git a/src/heap/cppgc/marker.cc b/src/heap/cppgc/marker.cc
@@ -378,6 +378,33 @@ class WeakCallbackJobTask final : public cppgc::JobTask {
   LivenessBroker& broker_;
 };
 
+class WeakPersistentJobTask final : public cppgc::JobTask {
+ public:
+  WeakPersistentJobTask(MarkerBase* marker,
+                        RootMarkingVisitor* root_marking_visitor)
+      : marker_(marker),
+        root_marking_visitor_(root_marking_visitor),
+        pending_(true) {}
+
+  void Run(JobDelegate* delegate) override {
+    StatsCollector::EnabledConcurrentScope stats_scope(
+        marker_->heap().stats_collector(),
+        StatsCollector::kConcurrentWeakPersistent);
+    marker_->heap().GetWeakPersistentRegion().Iterate(*root_marking_visitor_);
+    pending_.store(false);
+  }
+
+  size_t GetMaxConcurrency(size_t worker_count) const override {
+    return std::min(static_cast<size_t>(1),
+                    (pending_.load() ? 1 : 0) + worker_count);
+  }
+
+ private:
+  MarkerBase* marker_;
+  RootMarkingVisitor* root_marking_visitor_;
+  std::atomic<bool> pending_;
+};
+
 void MarkerBase::ProcessWeakness() {
   DCHECK_EQ(MarkingConfig::MarkingType::kAtomic, config_.marking_type);
 
@@ -393,21 +420,43 @@ void MarkerBase::ProcessWeakness() {
   heap().GetWeakCrossThreadPersistentRegion().Iterate(root_marking_visitor);
   g_process_mutex.Pointer()->Unlock();
 
-  // Launch the parallel job before anything else to provide the maximum time
-  // slice for processing.
+  // Launch two parallel jobs before anything else to provide the maximum time
+  // slice for processing: one for weak callbacks and one for same-thread weak
+  // persistents.
   LivenessBroker broker = LivenessBrokerFactory::Create();
-  std::unique_ptr<cppgc::JobHandle> job_handle{nullptr};
+  std::unique_ptr<cppgc::JobHandle> weak_callback_job_handle{nullptr};
+  std::unique_ptr<cppgc::JobHandle> weak_persistent_job_handle{nullptr};
   if (heap().marking_support() ==
       cppgc::Heap::MarkingType::kIncrementalAndConcurrent) {
-    job_handle = platform_->PostJob(
+    weak_callback_job_handle = platform_->PostJob(
         cppgc::TaskPriority::kUserBlocking,
         std::make_unique<WeakCallbackJobTask>(
             this, marking_worklists_.parallel_weak_callback_worklist(),
             broker));
+    weak_persistent_job_handle = platform_->PostJob(
+        cppgc::TaskPriority::kUserBlocking,
+        std::make_unique<WeakPersistentJobTask>(this, &root_marking_visitor));
+  }
+
+  {
+    // Process weak container callbacks.
+    StatsCollector::EnabledScope stats_scope(
+        heap().stats_collector(),
+        StatsCollector::kWeakContainerCallbacksProcessing);
+    MarkingWorklists::WeakCallbackItem item;
+    MarkingWorklists::WeakCallbackWorklist::Local& collections_local =
+        mutator_marking_state_.weak_container_callback_worklist();
+    while (collections_local.Pop(&item)) {
+      item.callback(broker, item.parameter);
+    }
   }
 
-  // Process same-thread roots.
-  heap().GetWeakPersistentRegion().Iterate(root_marking_visitor);
+  // Finish processing same-thread WeakPersistents.
+  if (weak_persistent_job_handle) {
+    weak_persistent_job_handle->Join();
+  } else {
+    heap().GetWeakPersistentRegion().Iterate(root_marking_visitor);
+  }
 
   // Call weak callbacks on objects that may now be pointing to dead objects.
 #if defined(CPPGC_YOUNG_GENERATION)
@@ -430,19 +479,10 @@ void MarkerBase::ProcessWeakness() {
 #endif  // defined(CPPGC_YOUNG_GENERATION)
 
   {
-    // First, process weak container callbacks.
-    StatsCollector::EnabledScope stats_scope(
-        heap().stats_collector(),
-        StatsCollector::kWeakContainerCallbacksProcessing);
-    MarkingWorklists::WeakCallbackItem item;
-    MarkingWorklists::WeakCallbackWorklist::Local& collections_local =
-        mutator_marking_state_.weak_container_callback_worklist();
-    while (collections_local.Pop(&item)) {
-      item.callback(broker, item.parameter);
-    }
-  }
-  {
-    // Then, process custom weak callbacks.
+    // Process custom weak callbacks.
+    // Processing same-thread WeakPersistents must be finished before this, as
+    // custom weak callbacks may allocate new WeakPersistents, thus leading to
+    // data races.
     StatsCollector::EnabledScope stats_scope(
         heap().stats_collector(), StatsCollector::kCustomCallbacksProcessing);
     MarkingWorklists::WeakCallbackItem item;
@@ -457,8 +497,9 @@ void MarkerBase::ProcessWeakness() {
     }
   }
 
-  if (job_handle) {
-    job_handle->Join();
+  // Finish processing parallel weak callbacks.
+  if (weak_callback_job_handle) {
+    weak_callback_job_handle->Join();
   } else {
     MarkingWorklists::WeakCallbackItem item;
     MarkingWorklists::WeakCallbackWorklist::Local& local =
diff --git a/src/heap/cppgc/stats-collector.h b/src/heap/cppgc/stats-collector.h
@@ -67,7 +67,8 @@ namespace internal {
 #define CPPGC_FOR_ALL_HISTOGRAM_CONCURRENT_SCOPES(V) \
   V(ConcurrentMark)                                  \
   V(ConcurrentSweep)                                 \
-  V(ConcurrentWeakCallback)
+  V(ConcurrentWeakCallback)                          \
+  V(ConcurrentWeakPersistent)
 
 #define CPPGC_FOR_ALL_CONCURRENT_SCOPES(V) V(ConcurrentMarkProcessEphemerons)
 

Original file line number	Diff line number	Diff line change
`@@ -256,7 +256,7 @@ class BasicPersistent final : public PersistentBase,`
`256`	`256`
`257`	`257`	`void ClearFromGC() const {`
`258`	`258`	`if (IsValid()) {`
`259`		`- WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());`
	`259`	`+ WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNodeFromGC(GetNode());`
`260`	`260`	`PersistentBase::ClearFromGC();`
`261`	`261`	`}`
`262`	`262`	`}`