cppgc, heap: Batch incremental events for UMA.

Reporting an event requires virtual calls. Frequent incremental events
seem to cause performance regression. Mitigate by batching events
reporting.

See usage in crrev.com/c/2992193

Bug: chromium:1214693
Change-Id: Iff212d0e9f90a2716956458c6e828fbe87a7b780
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2992712
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75695}

include/v8-metrics.h

@@ -50,11 +50,19 @@ struct GarbageCollectionFullMainThreadIncrementalMark {
   int64_t cpp_wall_clock_duration_in_us = -1;
 };
 
+struct GarbageCollectionFullMainThreadBatchedIncrementalMark {
+  std::vector<GarbageCollectionFullMainThreadIncrementalMark> events;
+};
+
 struct GarbageCollectionFullMainThreadIncrementalSweep {
   int64_t wall_clock_duration_in_us = -1;
   int64_t cpp_wall_clock_duration_in_us = -1;
 };
 
+struct GarbageCollectionFullMainThreadBatchedIncrementalSweep {
+  std::vector<GarbageCollectionFullMainThreadIncrementalSweep> events;
+};
+
 struct GarbageCollectionYoungCycle {
   int64_t total_wall_clock_duration_in_us = -1;
   int64_t main_thread_wall_clock_duration_in_us = -1;
@@ -101,14 +109,16 @@ struct WasmModulesPerIsolate {
   size_t count = 0;
 };
 
-#define V8_MAIN_THREAD_METRICS_EVENTS(V)             \
-  V(GarbageCollectionFullCycle)                      \
-  V(GarbageCollectionFullMainThreadIncrementalMark)  \
-  V(GarbageCollectionFullMainThreadIncrementalSweep) \
-  V(GarbageCollectionYoungCycle)                     \
-  V(WasmModuleDecoded)                               \
-  V(WasmModuleCompiled)                              \
-  V(WasmModuleInstantiated)                          \
+#define V8_MAIN_THREAD_METRICS_EVENTS(V)                    \
+  V(GarbageCollectionFullCycle)                             \
+  V(GarbageCollectionFullMainThreadIncrementalMark)         \
+  V(GarbageCollectionFullMainThreadBatchedIncrementalMark)  \
+  V(GarbageCollectionFullMainThreadIncrementalSweep)        \
+  V(GarbageCollectionFullMainThreadBatchedIncrementalSweep) \
+  V(GarbageCollectionYoungCycle)                            \
+  V(WasmModuleDecoded)                                      \
+  V(WasmModuleCompiled)                                     \
+  V(WasmModuleInstantiated)                                 \
   V(WasmModuleTieredUp)
 
 #define V8_THREAD_SAFE_METRICS_EVENTS(V) V(WasmModulesPerIsolate)

src/heap/cppgc-js/cpp-heap.cc

@@ -42,7 +42,6 @@
 #include "src/heap/marking-worklist.h"
 #include "src/heap/sweeper.h"
 #include "src/init/v8.h"
-#include "src/logging/metrics.h"
 #include "src/profiler/heap-profiler.h"
 
 namespace v8 {
@@ -220,65 +219,96 @@ void UnifiedHeapMarker::AddObject(void* object) {
 
 }  // namespace
 
-class CppHeap::MetricRecorderAdapter final
-    : public cppgc::internal::MetricRecorder {
- public:
-  explicit MetricRecorderAdapter(CppHeap& cpp_heap) : cpp_heap_(cpp_heap) {}
+void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(
+    const FullCycle& cppgc_event) {
+  last_full_gc_event_ = cppgc_event;
+  GetIsolate()->heap()->tracer()->NotifyGCCompleted();
+}
 
-  void AddMainThreadEvent(const FullCycle& cppgc_event) final {
-    cpp_heap_.last_cppgc_full_gc_event_ = cppgc_event;
-    GetIsolate()->heap()->tracer()->NotifyGCCompleted();
+void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(
+    const MainThreadIncrementalMark& cppgc_event) {
+  // Incremental marking steps might be nested in V8 marking steps. In such
+  // cases, stash the relevant values and delegate to V8 to report them. For
+  // non-nested steps, report to the Recorder directly.
+  if (cpp_heap_.is_in_v8_marking_step_) {
+    last_incremental_mark_event_ = cppgc_event;
+    return;
   }
-
-  void AddMainThreadEvent(const MainThreadIncrementalMark& cppgc_event) final {
-    // Incremental marking steps might be nested in V8 marking steps. In such
-    // cases, stash the relevant values and delegate to V8 to report them. For
-    // non-nested steps, report to the Recorder directly.
-    if (cpp_heap_.is_in_v8_marking_step_) {
-      cpp_heap_.last_cppgc_incremental_mark_event_ = cppgc_event;
-      return;
-    }
-    // This is a standalone incremental marking step.
-    const std::shared_ptr<metrics::Recorder>& recorder =
-        GetIsolate()->metrics_recorder();
-    DCHECK_NOT_NULL(recorder);
-    if (!recorder->HasEmbedderRecorder()) return;
-    ::v8::metrics::GarbageCollectionFullMainThreadIncrementalMark event;
-    event.cpp_wall_clock_duration_in_us = cppgc_event.duration_us;
-    // TODO(chromium:1154636): Populate event.wall_clock_duration_in_us.
-    recorder->AddMainThreadEvent(event, GetContextId());
+  // This is a standalone incremental marking step.
+  const std::shared_ptr<metrics::Recorder>& recorder =
+      GetIsolate()->metrics_recorder();
+  DCHECK_NOT_NULL(recorder);
+  if (!recorder->HasEmbedderRecorder()) return;
+  incremental_mark_batched_events_.events.emplace_back();
+  incremental_mark_batched_events_.events.back().cpp_wall_clock_duration_in_us =
+      cppgc_event.duration_us;
+  // TODO(chromium:1154636): Populate event.wall_clock_duration_in_us.
+  if (incremental_mark_batched_events_.events.size() == kMaxBatchedEvents) {
+    recorder->AddMainThreadEvent(std::move(incremental_mark_batched_events_),
+                                 GetContextId());
   }
+}
 
-  void AddMainThreadEvent(const MainThreadIncrementalSweep& cppgc_event) final {
-    // Incremental sweeping steps are never nested inside V8 sweeping steps, so
-    // report to the Recorder directly.
-    const std::shared_ptr<metrics::Recorder>& recorder =
-        GetIsolate()->metrics_recorder();
-    DCHECK_NOT_NULL(recorder);
-    if (!recorder->HasEmbedderRecorder()) return;
-    ::v8::metrics::GarbageCollectionFullMainThreadIncrementalSweep event;
-    event.cpp_wall_clock_duration_in_us = cppgc_event.duration_us;
-    // TODO(chromium:1154636): Populate event.wall_clock_duration_in_us.
-    recorder->AddMainThreadEvent(event, GetContextId());
+void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(
+    const MainThreadIncrementalSweep& cppgc_event) {
+  // Incremental sweeping steps are never nested inside V8 sweeping steps, so
+  // report to the Recorder directly.
+  const std::shared_ptr<metrics::Recorder>& recorder =
+      GetIsolate()->metrics_recorder();
+  DCHECK_NOT_NULL(recorder);
+  if (!recorder->HasEmbedderRecorder()) return;
+  incremental_sweep_batched_events_.events.emplace_back();
+  incremental_sweep_batched_events_.events.back()
+      .cpp_wall_clock_duration_in_us = cppgc_event.duration_us;
+  // TODO(chromium:1154636): Populate event.wall_clock_duration_in_us.
+  if (incremental_sweep_batched_events_.events.size() == kMaxBatchedEvents) {
+    recorder->AddMainThreadEvent(std::move(incremental_sweep_batched_events_),
+                                 GetContextId());
   }
+}
 
- private:
-  Isolate* GetIsolate() {
-    DCHECK_NOT_NULL(cpp_heap_.isolate());
-    return reinterpret_cast<Isolate*>(cpp_heap_.isolate());
+void CppHeap::MetricRecorderAdapter::FlushBatchedIncrementalEvents() {
+  const std::shared_ptr<metrics::Recorder>& recorder =
+      GetIsolate()->metrics_recorder();
+  DCHECK_NOT_NULL(recorder);
+  if (!incremental_mark_batched_events_.events.empty()) {
+    recorder->AddMainThreadEvent(std::move(incremental_mark_batched_events_),
+                                 GetContextId());
   }
-
-  v8::metrics::Recorder::ContextId GetContextId() {
-    DCHECK_NOT_NULL(GetIsolate());
-    if (GetIsolate()->context().is_null())
-      return v8::metrics::Recorder::ContextId::Empty();
-    HandleScope scope(GetIsolate());
-    return GetIsolate()->GetOrRegisterRecorderContextId(
-        GetIsolate()->native_context());
+  if (!incremental_sweep_batched_events_.events.empty()) {
+    recorder->AddMainThreadEvent(std::move(incremental_sweep_batched_events_),
+                                 GetContextId());
   }
+}
 
-  CppHeap& cpp_heap_;
-};
+bool CppHeap::MetricRecorderAdapter::MetricsReportPending() const {
+  return last_full_gc_event_.has_value();
+}
+
+const base::Optional<cppgc::internal::MetricRecorder::FullCycle>
+CppHeap::MetricRecorderAdapter::ExtractLastFullGcEvent() {
+  return std::move(last_full_gc_event_);
+}
+
+const base::Optional<cppgc::internal::MetricRecorder::MainThreadIncrementalMark>
+CppHeap::MetricRecorderAdapter::ExtractLastIncrementalMarkEvent() {
+  return std::move(last_incremental_mark_event_);
+}
+
+Isolate* CppHeap::MetricRecorderAdapter::GetIsolate() const {
+  DCHECK_NOT_NULL(cpp_heap_.isolate());
+  return reinterpret_cast<Isolate*>(cpp_heap_.isolate());
+}
+
+v8::metrics::Recorder::ContextId CppHeap::MetricRecorderAdapter::GetContextId()
+    const {
+  DCHECK_NOT_NULL(GetIsolate());
+  if (GetIsolate()->context().is_null())
+    return v8::metrics::Recorder::ContextId::Empty();
+  HandleScope scope(GetIsolate());
+  return GetIsolate()->GetOrRegisterRecorderContextId(
+      GetIsolate()->native_context());
+}
 
 CppHeap::CppHeap(
     v8::Platform* platform,
@@ -650,5 +680,10 @@ void CppHeap::CollectCustomSpaceStatisticsAtLastGC(
                               std::move(receiver));
 }
 
+CppHeap::MetricRecorderAdapter* CppHeap::GetMetricRecorder() const {
+  return static_cast<MetricRecorderAdapter*>(
+      stats_collector_->GetMetricRecorder());
+}
+
 }  // namespace internal
 }  // namespace v8

src/heap/cppgc-js/cpp-heap.h

@@ -15,6 +15,7 @@ static_assert(
 #include "src/base/macros.h"
 #include "src/heap/cppgc/heap-base.h"
 #include "src/heap/cppgc/stats-collector.h"
+#include "src/logging/metrics.h"
 
 namespace v8 {
 
@@ -29,6 +30,45 @@ class V8_EXPORT_PRIVATE CppHeap final
       public v8::EmbedderHeapTracer,
       public cppgc::internal::StatsCollector::AllocationObserver {
  public:
+  class MetricRecorderAdapter final : public cppgc::internal::MetricRecorder {
+   public:
+    static constexpr int kMaxBatchedEvents = 16;
+
+    explicit MetricRecorderAdapter(CppHeap& cpp_heap) : cpp_heap_(cpp_heap) {}
+
+    void AddMainThreadEvent(const FullCycle& cppgc_event) final;
+    void AddMainThreadEvent(const MainThreadIncrementalMark& cppgc_event) final;
+    void AddMainThreadEvent(
+        const MainThreadIncrementalSweep& cppgc_event) final;
+
+    void FlushBatchedIncrementalEvents();
+
+    // The following 3 methods are only used for reporting nested cpp events
+    // through V8. Standalone events are reported directly.
+    bool MetricsReportPending() const;
+
+    const base::Optional<cppgc::internal::MetricRecorder::FullCycle>
+    ExtractLastFullGcEvent();
+    const base::Optional<
+        cppgc::internal::MetricRecorder::MainThreadIncrementalMark>
+    ExtractLastIncrementalMarkEvent();
+
+   private:
+    Isolate* GetIsolate() const;
+
+    v8::metrics::Recorder::ContextId GetContextId() const;
+
+    CppHeap& cpp_heap_;
+    v8::metrics::GarbageCollectionFullMainThreadBatchedIncrementalMark
+        incremental_mark_batched_events_;
+    v8::metrics::GarbageCollectionFullMainThreadBatchedIncrementalSweep
+        incremental_sweep_batched_events_;
+    base::Optional<cppgc::internal::MetricRecorder::FullCycle>
+        last_full_gc_event_;
+    base::Optional<cppgc::internal::MetricRecorder::MainThreadIncrementalMark>
+        last_incremental_mark_event_;
+  };
+
   static CppHeap* From(v8::CppHeap* heap) {
     return static_cast<CppHeap*>(heap);
   }
@@ -76,24 +116,9 @@ class V8_EXPORT_PRIVATE CppHeap final
   void AllocatedObjectSizeDecreased(size_t) final;
   void ResetAllocatedObjectSize(size_t) final {}
 
-  // The following 3 methods are only used for reporting nested cpp events
-  // through V8. Standalone events are reported directly.
-  bool MetricsReportPending() const {
-    return last_cppgc_full_gc_event_.has_value();
-  }
-  const base::Optional<cppgc::internal::MetricRecorder::FullCycle>
-  ExtractLastCppgcFullGcEvent() {
-    return std::move(last_cppgc_full_gc_event_);
-  }
-  const base::Optional<
-      cppgc::internal::MetricRecorder::MainThreadIncrementalMark>
-  ExtractLastCppgcIncrementalMarkEvent() {
-    return std::move(last_cppgc_incremental_mark_event_);
-  }
+  MetricRecorderAdapter* GetMetricRecorder() const;
 
  private:
-  class MetricRecorderAdapter;
-
   void FinalizeIncrementalGarbageCollectionIfNeeded(
       cppgc::Heap::StackState) final {
     // For unified heap, CppHeap shouldn't finalize independently (i.e.
@@ -120,10 +145,6 @@ class V8_EXPORT_PRIVATE CppHeap final
   bool force_incremental_marking_for_testing_ = false;
 
   bool is_in_v8_marking_step_ = false;
-  base::Optional<cppgc::internal::MetricRecorder::FullCycle>
-      last_cppgc_full_gc_event_;
-  base::Optional<cppgc::internal::MetricRecorder::MainThreadIncrementalMark>
-      last_cppgc_incremental_mark_event_;
 
   friend class MetricRecorderAdapter;
 };

src/heap/cppgc/stats-collector.h

@@ -304,6 +304,8 @@ class V8_EXPORT_PRIVATE StatsCollector final {
     metric_recorder_ = std::move(histogram_recorder);
   }
 
+  MetricRecorder* GetMetricRecorder() const { return metric_recorder_.get(); }
+
  private:
   enum class GarbageCollectionState : uint8_t {
     kNotRunning,

src/heap/gc-tracer.cc

@@ -1310,7 +1310,8 @@ void GCTracer::NotifyGCCompleted() {
     return;
   }
   const auto* cpp_heap = heap_->cpp_heap();
-  if (cpp_heap && !CppHeap::From(cpp_heap)->MetricsReportPending()) {
+  if (cpp_heap &&
+      !CppHeap::From(cpp_heap)->GetMetricRecorder()->MetricsReportPending()) {
     // Cppgc sweeping is not done yet.
     return;
   }
@@ -1362,6 +1363,16 @@ void CopySizeMetrics(
   return isolate->GetOrRegisterRecorderContextId(isolate->native_context());
 }
 
+void FlushBatchedIncrementalEvents(
+    v8::metrics::GarbageCollectionFullMainThreadBatchedIncrementalMark&
+        batched_events,
+    Isolate* isolate) {
+  DCHECK_NOT_NULL(isolate->metrics_recorder());
+  DCHECK(!batched_events.events.empty());
+  isolate->metrics_recorder()->AddMainThreadEvent(std::move(batched_events),
+                                                  GetContextId(isolate));
+}
+
 }  // namespace
 
 void GCTracer::ReportFullCycleToRecorder() {
@@ -1369,11 +1380,17 @@ void GCTracer::ReportFullCycleToRecorder() {
       heap_->isolate()->metrics_recorder();
   DCHECK_NOT_NULL(recorder);
   if (!recorder->HasEmbedderRecorder()) return;
-  ::v8::metrics::GarbageCollectionFullCycle event;
+  if (!incremental_mark_batched_events_.events.empty()) {
+    FlushBatchedIncrementalEvents(incremental_mark_batched_events_,
+                                  heap_->isolate());
+  }
+  v8::metrics::GarbageCollectionFullCycle event;
   if (heap_->cpp_heap()) {
+    auto* cpp_heap = v8::internal::CppHeap::From(heap_->cpp_heap());
+    cpp_heap->GetMetricRecorder()->FlushBatchedIncrementalEvents();
     const base::Optional<cppgc::internal::MetricRecorder::FullCycle>
-        optional_cppgc_event = v8::internal::CppHeap::From(heap_->cpp_heap())
-                                   ->ExtractLastCppgcFullGcEvent();
+        optional_cppgc_event =
+            cpp_heap->GetMetricRecorder()->ExtractLastFullGcEvent();
     DCHECK(optional_cppgc_event.has_value());
     const cppgc::internal::MetricRecorder::FullCycle& cppgc_event =
         optional_cppgc_event.value();
@@ -1401,23 +1418,30 @@ void GCTracer::ReportFullCycleToRecorder() {
 }
 
 void GCTracer::ReportIncrementalMarkingStepToRecorder() {
+  static constexpr int kMaxBatchedEvents =
+      CppHeap::MetricRecorderAdapter::kMaxBatchedEvents;
   const std::shared_ptr<metrics::Recorder>& recorder =
       heap_->isolate()->metrics_recorder();
   DCHECK_NOT_NULL(recorder);
   if (!recorder->HasEmbedderRecorder()) return;
-  ::v8::metrics::GarbageCollectionFullMainThreadIncrementalMark event;
+  incremental_mark_batched_events_.events.emplace_back();
   if (heap_->cpp_heap()) {
     const base::Optional<
         cppgc::internal::MetricRecorder::MainThreadIncrementalMark>
         cppgc_event = v8::internal::CppHeap::From(heap_->cpp_heap())
-                          ->ExtractLastCppgcIncrementalMarkEvent();
+                          ->GetMetricRecorder()
+                          ->ExtractLastIncrementalMarkEvent();
     if (cppgc_event.has_value()) {
       DCHECK_NE(-1, cppgc_event.value().duration_us);
-      event.cpp_wall_clock_duration_in_us = cppgc_event.value().duration_us;
+      incremental_mark_batched_events_.events.back()
+          .cpp_wall_clock_duration_in_us = cppgc_event.value().duration_us;
     }
   }
   // TODO(chromium:1154636): Populate event.wall_clock_duration_in_us.
-  recorder->AddMainThreadEvent(event, GetContextId(heap_->isolate()));
+  if (incremental_mark_batched_events_.events.size() == kMaxBatchedEvents) {
+    FlushBatchedIncrementalEvents(incremental_mark_batched_events_,
+                                  heap_->isolate());
+  }
 }
 
 }  // namespace internal

src/heap/gc-tracer.h

@@ -5,6 +5,7 @@
 #ifndef V8_HEAP_GC_TRACER_H_
 #define V8_HEAP_GC_TRACER_H_
 
+#include "include/v8-metrics.h"
 #include "src/base/compiler-specific.h"
 #include "src/base/optional.h"
 #include "src/base/platform/platform.h"
@@ -486,6 +487,9 @@ class V8_EXPORT_PRIVATE GCTracer {
 
   bool metrics_report_pending_ = false;
 
+  v8::metrics::GarbageCollectionFullMainThreadBatchedIncrementalMark
+      incremental_mark_batched_events_;
+
   base::Mutex background_counter_mutex_;
   BackgroundCounter background_counter_[Scope::NUMBER_OF_SCOPES];
 };