Revert "cppgc: Minor fix in cppgc efficiency calculation"

This reverts commit 543acf34.

Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Mac%20-%20arm64%20-%20release/10365/overview

Original change's description:
> 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/+/3714356
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#81329}

Bug: chromium:1338256
Change-Id: Ie9a23651494fc28a11bb59485a9812ee1a7cff48
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3721697
Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
Owners-Override: Nico Hartmann <nicohartmann@chromium.org>
Auto-Submit: Nico Hartmann <nicohartmann@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#81331}

src/heap/cppgc/stats-collector.cc

@@ -160,10 +160,6 @@ 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;
 }
@@ -219,23 +215,11 @@ MetricRecorder::GCCycle GetCycleEventForMetricRecorder(
       static_cast<double>(event.objects.after_bytes) /
       event.objects.before_bytes;
   // Efficiency:
-  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;
-  }
+  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);
   return event;
 }
 

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

@@ -55,14 +55,6 @@ 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();
@@ -289,11 +281,9 @@ 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);
@@ -306,11 +296,9 @@ 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);