heap: Fix reporting of incremental and background scopes

Method GCTracer::UpdateStatistics was responsible for copying
incremental and background scopes to the current event, before
reporting. It was called, however, at the end of the atomic pause and,
as a result, some of these scopes would be prematurely copied to the
current event (e.g., incremental and background sweeping scopes) and
misreported.

This CL fixes this by splitting the update of statistics and the
copying of incremental and background scopes. It introduces the
method GCTracer::FinalizeCurrentEvent which does the latter, which
is called from GCTracer::StopCycle. It also introduces methods for
correctly accessing and updating scopes, before the current event is
finalized, and eliminates the distinction between
GCTracer::AddScopeSample and GCTracer::AddScopeSampleBackground.

Bug: chromium:1154636
Change-Id: I2a6d9abb3daa2c48b2dce12dc2685cfc84130abf
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3576792Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79938}

src/heap/gc-tracer.h

@@ -103,7 +103,8 @@ class V8_EXPORT_PRIVATE GCTracer {
       LAST_TOP_MC_SCOPE = MC_SWEEP,
       FIRST_MINOR_GC_BACKGROUND_SCOPE = MINOR_MC_BACKGROUND_EVACUATE_COPY,
       LAST_MINOR_GC_BACKGROUND_SCOPE = SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL,
-      FIRST_BACKGROUND_SCOPE = FIRST_GENERAL_BACKGROUND_SCOPE
+      FIRST_BACKGROUND_SCOPE = FIRST_GENERAL_BACKGROUND_SCOPE,
+      LAST_BACKGROUND_SCOPE = LAST_MINOR_GC_BACKGROUND_SCOPE
     };
 
     Scope(GCTracer* tracer, ScopeId scope, ThreadKind thread_kind);
@@ -113,6 +114,12 @@ class V8_EXPORT_PRIVATE GCTracer {
     static const char* Name(ScopeId id);
     static bool NeedsYoungEpoch(ScopeId id);
 
+    static constexpr int IncrementalOffset(ScopeId id) {
+      DCHECK_LE(FIRST_INCREMENTAL_SCOPE, id);
+      DCHECK_GE(LAST_INCREMENTAL_SCOPE, id);
+      return id - FIRST_INCREMENTAL_SCOPE;
+    }
+
    private:
 #if DEBUG
     void AssertMainThread();
@@ -214,7 +221,7 @@ class V8_EXPORT_PRIVATE GCTracer {
 
     // Holds details for incremental marking scopes.
     IncrementalMarkingInfos
-        incremental_marking_scopes[Scope::NUMBER_OF_INCREMENTAL_SCOPES];
+        incremental_scopes[Scope::NUMBER_OF_INCREMENTAL_SCOPES];
   };
 
   class RecordGCPhasesInfo {
@@ -260,6 +267,7 @@ class V8_EXPORT_PRIVATE GCTracer {
                           const char* collector_reason);
 
   void UpdateStatistics(GarbageCollector collector);
+  void FinalizeCurrentEvent();
 
   enum class MarkingType { kAtomic, kIncremental };
 
@@ -411,19 +419,20 @@ class V8_EXPORT_PRIVATE GCTracer {
   double AverageMarkCompactMutatorUtilization() const;
   double CurrentMarkCompactMutatorUtilization() const;
 
-  V8_INLINE void AddScopeSample(Scope::ScopeId scope, double duration) {
-    DCHECK(scope < Scope::NUMBER_OF_SCOPES);
-    if (scope >= Scope::FIRST_INCREMENTAL_SCOPE &&
-        scope <= Scope::LAST_INCREMENTAL_SCOPE) {
-      incremental_marking_scopes_[scope - Scope::FIRST_INCREMENTAL_SCOPE]
-          .Update(duration);
+  V8_INLINE void AddScopeSample(Scope::ScopeId id, double duration) {
+    if (Scope::FIRST_INCREMENTAL_SCOPE <= id &&
+        id <= Scope::LAST_INCREMENTAL_SCOPE) {
+      incremental_scopes_[Scope::IncrementalOffset(id)].Update(duration);
+    } else if (Scope::FIRST_BACKGROUND_SCOPE <= id &&
+               id <= Scope::LAST_BACKGROUND_SCOPE) {
+      base::MutexGuard guard(&background_counter_mutex_);
+      background_counter_[id].total_duration_ms += duration;
     } else {
-      current_.scopes[scope] += duration;
+      DCHECK_GT(Scope::NUMBER_OF_SCOPES, id);
+      current_.scopes[id] += duration;
     }
   }
 
-  void AddScopeSampleBackground(Scope::ScopeId scope, double duration);
-
   void RecordGCPhasesHistograms(RecordGCPhasesInfo::Mode mode);
 
   void RecordEmbedderSpeed(size_t bytes, double duration);
@@ -462,6 +471,30 @@ class V8_EXPORT_PRIVATE GCTracer {
 
   void StopCycle(GarbageCollector collector);
 
+  // Statistics for incremental and background scopes are kept out of the
+  // current event and only copied there by FinalizeCurrentEvent, at StopCycle.
+  // This method can be used to access scopes correctly, before this happens.
+  // Note: when accessing a background scope via this method, the caller is
+  // responsible for avoiding data races, e.g., by acquiring
+  // background_counter_mutex_.
+  constexpr double current_scope(Scope::ScopeId id) const {
+    if (Scope::FIRST_INCREMENTAL_SCOPE <= id &&
+        id <= Scope::LAST_INCREMENTAL_SCOPE) {
+      return incremental_scope(id).duration;
+    } else if (Scope::FIRST_BACKGROUND_SCOPE <= id &&
+               id <= Scope::LAST_BACKGROUND_SCOPE) {
+      return background_counter_[id].total_duration_ms;
+    } else {
+      DCHECK_GT(Scope::NUMBER_OF_SCOPES, id);
+      return current_.scopes[id];
+    }
+  }
+
+  constexpr const IncrementalMarkingInfos& incremental_scope(
+      Scope::ScopeId id) const {
+    return incremental_scopes_[Scope::IncrementalOffset(id)];
+  }
+
   // Returns the average speed of the events in the buffer.
   // If the buffer is empty, the result is 0.
   // Otherwise, the result is between 1 byte/ms and 1 GB/ms.
@@ -494,14 +527,6 @@ class V8_EXPORT_PRIVATE GCTracer {
   // it can be included in later crash dumps.
   void PRINTF_FORMAT(2, 3) Output(const char* format, ...) const;
 
-  double TotalExternalTime() const {
-    return current_.scopes[Scope::HEAP_EXTERNAL_WEAK_GLOBAL_HANDLES] +
-           current_.scopes[Scope::HEAP_EXTERNAL_EPILOGUE] +
-           current_.scopes[Scope::HEAP_EXTERNAL_PROLOGUE] +
-           current_.scopes[Scope::MC_INCREMENTAL_EXTERNAL_EPILOGUE] +
-           current_.scopes[Scope::MC_INCREMENTAL_EXTERNAL_PROLOGUE];
-  }
-
   void FetchBackgroundCounters(int first_scope, int last_scope);
   void FetchBackgroundMinorGCCounters();
   void FetchBackgroundMarkCompactCounters();
@@ -549,7 +574,7 @@ class V8_EXPORT_PRIVATE GCTracer {
   // Incremental scopes carry more information than just the duration. The infos
   // here are merged back upon starting/stopping the GC tracer.
   IncrementalMarkingInfos
-      incremental_marking_scopes_[Scope::NUMBER_OF_INCREMENTAL_SCOPES];
+      incremental_scopes_[Scope::NUMBER_OF_INCREMENTAL_SCOPES];
 
   // Timestamp and allocation counter at the last sampled allocation event.
   double allocation_time_ms_;
@@ -605,7 +630,7 @@ class V8_EXPORT_PRIVATE GCTracer {
   v8::metrics::GarbageCollectionFullMainThreadBatchedIncrementalSweep
       incremental_sweep_batched_events_;
 
-  base::Mutex background_counter_mutex_;
+  mutable base::Mutex background_counter_mutex_;
   BackgroundCounter background_counter_[Scope::NUMBER_OF_SCOPES];
 };
 

test/unittests/heap/gc-tracer-unittest.cc

@@ -263,20 +263,18 @@ TEST_F(GCTracerTest, IncrementalMarkingDetails) {
   tracer->AddScopeSample(GCTracer::Scope::MC_INCREMENTAL_FINALIZE, 100);
   StopTracing(tracer, GarbageCollector::MARK_COMPACTOR);
   EXPECT_DOUBLE_EQ(
-      100,
-      tracer->current_
-          .incremental_marking_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
-          .longest_step);
-  EXPECT_EQ(
-      2,
-      tracer->current_
-          .incremental_marking_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
-          .steps);
+      100, tracer->current_
+               .incremental_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
+               .longest_step);
+  EXPECT_EQ(2, tracer->current_
+                   .incremental_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
+                   .steps);
+  EXPECT_DOUBLE_EQ(
+      150, tracer->current_
+               .incremental_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
+               .duration);
   EXPECT_DOUBLE_EQ(
-      150,
-      tracer->current_
-          .incremental_marking_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
-          .duration);
+      150, tracer->current_.scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]);
 
   // Round 2. Numbers should be reset.
   tracer->AddScopeSample(GCTracer::Scope::MC_INCREMENTAL_FINALIZE, 13);
@@ -286,20 +284,18 @@ TEST_F(GCTracerTest, IncrementalMarkingDetails) {
   tracer->AddScopeSample(GCTracer::Scope::MC_INCREMENTAL_FINALIZE, 122);
   StopTracing(tracer, GarbageCollector::MARK_COMPACTOR);
   EXPECT_DOUBLE_EQ(
-      122,
-      tracer->current_
-          .incremental_marking_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
-          .longest_step);
-  EXPECT_EQ(
-      3,
-      tracer->current_
-          .incremental_marking_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
-          .steps);
+      122, tracer->current_
+               .incremental_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
+               .longest_step);
+  EXPECT_EQ(3, tracer->current_
+                   .incremental_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
+                   .steps);
+  EXPECT_DOUBLE_EQ(
+      150, tracer->current_
+               .incremental_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
+               .duration);
   EXPECT_DOUBLE_EQ(
-      150,
-      tracer->current_
-          .incremental_marking_scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]
-          .duration);
+      150, tracer->current_.scopes[GCTracer::Scope::MC_INCREMENTAL_FINALIZE]);
 }
 
 TEST_F(GCTracerTest, IncrementalMarkingSpeed) {
@@ -392,9 +388,9 @@ TEST_F(GCTracerTest, BackgroundScavengerScope) {
   GCTracer* tracer = i_isolate()->heap()->tracer();
   tracer->ResetForTesting();
   StartTracing(tracer, GarbageCollector::SCAVENGER, StartTracingMode::kAtomic);
-  tracer->AddScopeSampleBackground(
+  tracer->AddScopeSample(
       GCTracer::Scope::SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL, 10);
-  tracer->AddScopeSampleBackground(
+  tracer->AddScopeSample(
       GCTracer::Scope::SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL, 1);
   StopTracing(tracer, GarbageCollector::SCAVENGER);
   EXPECT_DOUBLE_EQ(
@@ -407,17 +403,14 @@ TEST_F(GCTracerTest, BackgroundMinorMCScope) {
   tracer->ResetForTesting();
   StartTracing(tracer, GarbageCollector::MINOR_MARK_COMPACTOR,
                StartTracingMode::kAtomic);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MINOR_MC_BACKGROUND_MARKING,
-                                   10);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MINOR_MC_BACKGROUND_MARKING,
-                                   1);
-  tracer->AddScopeSampleBackground(
-      GCTracer::Scope::MINOR_MC_BACKGROUND_EVACUATE_COPY, 20);
-  tracer->AddScopeSampleBackground(
-      GCTracer::Scope::MINOR_MC_BACKGROUND_EVACUATE_COPY, 2);
-  tracer->AddScopeSampleBackground(
+  tracer->AddScopeSample(GCTracer::Scope::MINOR_MC_BACKGROUND_MARKING, 10);
+  tracer->AddScopeSample(GCTracer::Scope::MINOR_MC_BACKGROUND_MARKING, 1);
+  tracer->AddScopeSample(GCTracer::Scope::MINOR_MC_BACKGROUND_EVACUATE_COPY,
+                         20);
+  tracer->AddScopeSample(GCTracer::Scope::MINOR_MC_BACKGROUND_EVACUATE_COPY, 2);
+  tracer->AddScopeSample(
       GCTracer::Scope::MINOR_MC_BACKGROUND_EVACUATE_UPDATE_POINTERS, 30);
-  tracer->AddScopeSampleBackground(
+  tracer->AddScopeSample(
       GCTracer::Scope::MINOR_MC_BACKGROUND_EVACUATE_UPDATE_POINTERS, 3);
   StopTracing(tracer, GarbageCollector::MINOR_MARK_COMPACTOR);
   EXPECT_DOUBLE_EQ(
@@ -434,25 +427,22 @@ TEST_F(GCTracerTest, BackgroundMinorMCScope) {
 TEST_F(GCTracerTest, BackgroundMajorMCScope) {
   GCTracer* tracer = i_isolate()->heap()->tracer();
   tracer->ResetForTesting();
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_MARKING, 100);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_SWEEPING,
-                                   200);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_MARKING, 10);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_MARKING, 100);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_SWEEPING, 200);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_MARKING, 10);
   // Scavenger should not affect the major mark-compact scopes.
   StartTracing(tracer, GarbageCollector::SCAVENGER, StartTracingMode::kAtomic);
   StopTracing(tracer, GarbageCollector::SCAVENGER);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_SWEEPING, 20);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_MARKING, 1);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_SWEEPING, 2);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_SWEEPING, 20);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_MARKING, 1);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_SWEEPING, 2);
   StartTracing(tracer, GarbageCollector::MARK_COMPACTOR,
                StartTracingMode::kAtomic);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_EVACUATE_COPY,
-                                   30);
-  tracer->AddScopeSampleBackground(GCTracer::Scope::MC_BACKGROUND_EVACUATE_COPY,
-                                   3);
-  tracer->AddScopeSampleBackground(
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_EVACUATE_COPY, 30);
+  tracer->AddScopeSample(GCTracer::Scope::MC_BACKGROUND_EVACUATE_COPY, 3);
+  tracer->AddScopeSample(
       GCTracer::Scope::MC_BACKGROUND_EVACUATE_UPDATE_POINTERS, 40);
-  tracer->AddScopeSampleBackground(
+  tracer->AddScopeSample(
       GCTracer::Scope::MC_BACKGROUND_EVACUATE_UPDATE_POINTERS, 4);
   StopTracing(tracer, GarbageCollector::MARK_COMPACTOR);
   EXPECT_DOUBLE_EQ(