cppgc: Minor fix in cppgc efficiency calculation

Efficiency calculation (freed bytes over GC duration) assumes that the
duration of the GC is non zero. However, if the clock resolution is
not small enough and the entire GC is very short, the timed value
appears to be zero. This leads to NaN values showing in metrics and
CHECKs failing. This CL fixes the issue.

Bug: chromium:1338256
Change-Id: I1dbc52072fcde3411aa38fa0c11da25afd107ca8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3714356Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81329}

src/heap/cppgc/stats-collector.cc

@@ -160,6 +160,10 @@ double StatsCollector::GetRecentAllocationSpeedInBytesPerMs() const {
 namespace {
 
 int64_t SumPhases(const MetricRecorder::GCCycle::Phases& phases) {
+  DCHECK_LE(0, phases.mark_duration_us);
+  DCHECK_LE(0, phases.weak_duration_us);
+  DCHECK_LE(0, phases.compact_duration_us);
+  DCHECK_LE(0, phases.sweep_duration_us);
   return phases.mark_duration_us + phases.weak_duration_us +
          phases.compact_duration_us + phases.sweep_duration_us;
 }
@@ -215,11 +219,23 @@ MetricRecorder::GCCycle GetCycleEventForMetricRecorder(
       static_cast<double>(event.objects.after_bytes) /
       event.objects.before_bytes;
   // Efficiency:
-  event.efficiency_in_bytes_per_us =
-      static_cast<double>(event.objects.freed_bytes) / SumPhases(event.total);
-  event.main_thread_efficiency_in_bytes_per_us =
-      static_cast<double>(event.objects.freed_bytes) /
-      SumPhases(event.main_thread);
+  if (event.objects.freed_bytes == 0) {
+    event.efficiency_in_bytes_per_us = 0;
+    event.main_thread_efficiency_in_bytes_per_us = 0;
+  } else {
+    const int64_t sum_total = SumPhases(event.total);
+    const int64_t sum_main_thread = SumPhases(event.main_thread);
+    // The following check could be violated if the clock resolution is not
+    // small enough and the entire GC was very short, so the timed value was
+    // zero. See crbug.com/1338256. In this case, we expect that there won't be
+    // any freed bytes, so we will not be in this branch. If however the CHECK
+    // fails, we will need to provide an arbitrary value for efficiency here.
+    CHECK_NE(0, sum_main_thread);
+    event.efficiency_in_bytes_per_us =
+        static_cast<double>(event.objects.freed_bytes) / sum_total;
+    event.main_thread_efficiency_in_bytes_per_us =
+        static_cast<double>(event.objects.freed_bytes) / sum_main_thread;
+  }
   return event;
 }
 

test/unittests/heap/cppgc/metric-recorder-unittest.cc

@@ -55,6 +55,14 @@ class MetricRecorderTest : public testing::TestWithHeap {
         GarbageCollector::Config::CollectionType::kMajor,
         GarbageCollector::Config::IsForcedGC::kNotForced);
   }
+  void WaitGC(StatsCollector::ScopeId scope_id) {
+    // This ensures that the scope contains a non zero value.
+    StatsCollector::EnabledScope scope(stats, scope_id);
+    v8::base::TimeTicks time = v8::base::TimeTicks::Now();
+    while (time == v8::base::TimeTicks::Now()) {
+      // Force time to progress before destroying scope.
+    }
+  }
   void EndGC(size_t marked_bytes) {
     stats->NotifyMarkingCompleted(marked_bytes);
     stats->NotifySweepingCompleted();
@@ -281,9 +289,11 @@ TEST_F(MetricRecorderTest, CycleEndHistogramReportsCorrectValues) {
 TEST_F(MetricRecorderTest, ObjectSizeMetricsNoAllocations) {
   // Populate previous event.
   StartGC();
+  WaitGC(StatsCollector::kAtomicMark);
   EndGC(1000);
   // Populate current event.
   StartGC();
+  WaitGC(StatsCollector::kAtomicMark);
   EndGC(800);
   EXPECT_EQ(1000u, MetricRecorderImpl::GCCycle_event.objects.before_bytes);
   EXPECT_EQ(800u, MetricRecorderImpl::GCCycle_event.objects.after_bytes);
@@ -296,9 +306,11 @@ TEST_F(MetricRecorderTest, ObjectSizeMetricsNoAllocations) {
 TEST_F(MetricRecorderTest, ObjectSizeMetricsWithAllocations) {
   // Populate previous event.
   StartGC();
+  WaitGC(StatsCollector::kAtomicMark);
   EndGC(1000);
   // Populate current event.
   StartGC();
+  WaitGC(StatsCollector::kAtomicMark);
   stats->NotifyAllocation(300);
   stats->NotifyAllocatedMemory(1400);
   stats->NotifyFreedMemory(700);