[throughput metrics] Accumulate metrics across sequences of same type.

If a sequence does not have enough frames, then instead of discarding
them completely, retain the throughput-data, and accumulate them with
the throughput metrics of the subsequent sequences (of the same
interaction-type) until there are enough frames to report the metric.

Bug: 1031251
Change-Id: I4bbe1d0b024e04afd92677426a546699d9cc85d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952529Reviewed-by: Behdad Bakhshinategh <behdadb@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722588}

cc/BUILD.gn

@@ -645,6 +645,7 @@ cc_test("cc_unittests") {
     "metrics/compositor_frame_reporter_unittest.cc",
     "metrics/compositor_frame_reporting_controller_unittest.cc",
     "metrics/compositor_timing_history_unittest.cc",
+    "metrics/frame_sequence_metrics_unittest.cc",
     "metrics/frame_sequence_tracker_unittest.cc",
     "mojo_embedder/async_layer_tree_frame_sink_unittest.cc",
     "paint/discardable_image_map_unittest.cc",

cc/metrics/frame_sequence_metrics_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "cc/metrics/frame_sequence_tracker.h"
+
+#include "base/macros.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cc {
+
+TEST(FrameSequenceMetricsTest, MergeMetrics) {
+  // Create a metric with only a small number of frames. It shouldn't report any
+  // metrics.
+  FrameSequenceMetrics first(FrameSequenceTrackerType::kTouchScroll, nullptr,
+                             nullptr);
+  first.impl_throughput().frames_expected = 20;
+  first.impl_throughput().frames_produced = 10;
+  EXPECT_FALSE(first.HasEnoughDataForReporting());
+
+  // Create a second metric with too few frames to report any metrics.
+  auto second = std::make_unique<FrameSequenceMetrics>(
+      FrameSequenceTrackerType::kTouchScroll, nullptr, nullptr);
+  second->impl_throughput().frames_expected = 90;
+  second->impl_throughput().frames_produced = 60;
+  EXPECT_FALSE(second->HasEnoughDataForReporting());
+
+  // Merge the two metrics. The result should have enough frames to report
+  // metrics.
+  first.Merge(std::move(second));
+  EXPECT_TRUE(first.HasEnoughDataForReporting());
+}
+
+TEST(FrameSequenceMetricsTest, AllMetricsReported) {
+  base::HistogramTester histograms;
+
+  // Create a metric with enough frames on impl to be reported, but not enough
+  // on main.
+  FrameSequenceMetrics first(FrameSequenceTrackerType::kTouchScroll, nullptr,
+                             nullptr);
+  first.impl_throughput().frames_expected = 120;
+  first.impl_throughput().frames_produced = 80;
+  first.main_throughput().frames_expected = 20;
+  first.main_throughput().frames_produced = 10;
+  EXPECT_TRUE(first.HasEnoughDataForReporting());
+  first.ReportMetrics();
+
+  // The compositor-thread metric should be reported, but not the main-thread
+  // metric.
+  histograms.ExpectTotalCount(
+      "Graphics.Smoothness.Throughput.CompositorThread.TouchScroll", 1u);
+  histograms.ExpectTotalCount(
+      "Graphics.Smoothness.Throughput.MainThread.TouchScroll", 0u);
+
+  // There should still be data left over for the main-thread.
+  EXPECT_TRUE(first.HasDataLeftForReporting());
+
+  auto second = std::make_unique<FrameSequenceMetrics>(
+      FrameSequenceTrackerType::kTouchScroll, nullptr, nullptr);
+  second->impl_throughput().frames_expected = 110;
+  second->impl_throughput().frames_produced = 100;
+  second->main_throughput().frames_expected = 90;
+  first.Merge(std::move(second));
+  EXPECT_TRUE(first.HasEnoughDataForReporting());
+  first.ReportMetrics();
+  histograms.ExpectTotalCount(
+      "Graphics.Smoothness.Throughput.CompositorThread.TouchScroll", 2u);
+  histograms.ExpectTotalCount(
+      "Graphics.Smoothness.Throughput.MainThread.TouchScroll", 1u);
+  // All the metrics have now been reported. No data should be left over.
+  EXPECT_FALSE(first.HasDataLeftForReporting());
+}
+
+}  // namespace cc

cc/metrics/frame_sequence_tracker.cc

@@ -57,9 +57,9 @@ const char* FrameSequenceTracker::GetFrameSequenceTrackerTypeName(
 
 namespace {
 
-// Avoid reporting any throughput metric for sequences that had a small amount
-// of frames.
-constexpr int kMinFramesForThroughputMetric = 4;
+// Avoid reporting any throughput metric for sequences that do not have a
+// sufficient number of frames.
+constexpr int kMinFramesForThroughputMetric = 100;
 
 constexpr int kBuiltinSequenceNum = FrameSequenceTrackerType::kMaxType + 1;
 constexpr int kMaximumHistogramIndex = 3 * kBuiltinSequenceNum;
@@ -110,17 +110,42 @@ FrameSequenceMetrics::FrameSequenceMetrics(FrameSequenceTrackerType type,
 }
 
 FrameSequenceMetrics::~FrameSequenceMetrics() {
+  if (HasDataLeftForReporting())
+    ReportMetrics();
+}
+
+void FrameSequenceMetrics::Merge(
+    std::unique_ptr<FrameSequenceMetrics> metrics) {
+  DCHECK_EQ(type_, metrics->type_);
+  impl_throughput_.Merge(metrics->impl_throughput_);
+  main_throughput_.Merge(metrics->main_throughput_);
+  frames_checkerboarded_ += metrics->frames_checkerboarded_;
+
+  // Reset the state of |metrics| before destroying it, so that it doesn't end
+  // up reporting the metrics.
+  metrics->impl_throughput_ = {};
+  metrics->main_throughput_ = {};
+  metrics->frames_checkerboarded_ = 0;
+}
+
+bool FrameSequenceMetrics::HasEnoughDataForReporting() const {
+  return impl_throughput_.frames_expected >= kMinFramesForThroughputMetric ||
+         main_throughput_.frames_expected >= kMinFramesForThroughputMetric;
+}
+
+bool FrameSequenceMetrics::HasDataLeftForReporting() const {
+  return impl_throughput_.frames_expected > 0 ||
+         main_throughput_.frames_expected > 0;
+}
+
+void FrameSequenceMetrics::ReportMetrics() {
   DCHECK_LE(impl_throughput_.frames_produced, impl_throughput_.frames_expected);
   DCHECK_LE(main_throughput_.frames_produced, main_throughput_.frames_expected);
-  DCHECK_LE(main_throughput_.frames_produced, impl_throughput_.frames_produced);
   TRACE_EVENT_ASYNC_END2(
       "cc,benchmark", "FrameSequenceTracker", this, "args",
       ThroughputData::ToTracedValue(impl_throughput_, main_throughput_),
       "checkerboard", frames_checkerboarded_);
-  ReportMetrics();
-}
 
-void FrameSequenceMetrics::ReportMetrics() {
   // Report the throughput metrics.
   base::Optional<int> impl_throughput_percent = ThroughputData::ReportHistogram(
       type_, "CompositorThread",
@@ -169,7 +194,14 @@ void FrameSequenceMetrics::ReportMetrics() {
         base::LinearHistogram::FactoryGet(
             GetCheckerboardingHistogramName(type_), 1, 100, 101,
             base::HistogramBase::kUmaTargetedHistogramFlag));
+    frames_checkerboarded_ = 0;
   }
+
+  // Reset the metrics that have already been reported.
+  if (impl_throughput_percent.has_value())
+    impl_throughput_ = {};
+  if (main_throughput_percent.has_value())
+    main_throughput_ = {};
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -275,6 +307,26 @@ void FrameSequenceTrackerCollection::NotifyFramePresented(
   for (auto& tracker : removal_trackers_)
     tracker->ReportFramePresented(frame_token, feedback);
 
+  for (auto& tracker : removal_trackers_) {
+    if (tracker->termination_status() ==
+        FrameSequenceTracker::TerminationStatus::kReadyForTermination) {
+      // The tracker is ready to be terminated. Take the metrics from the
+      // tracker, merge with any outstanding metrics from previous trackers of
+      // the same type. If there are enough frames to report the metrics, then
+      // report the metrics and destroy it. Otherwise, retain it to be merged
+      // with follow-up sequences.
+      auto metrics = tracker->TakeMetrics();
+      if (accumulated_metrics_.contains(tracker->type())) {
+        metrics->Merge(std::move(accumulated_metrics_[tracker->type()]));
+        accumulated_metrics_.erase(tracker->type());
+      }
+      if (metrics->HasEnoughDataForReporting())
+        metrics->ReportMetrics();
+      if (metrics->HasDataLeftForReporting())
+        accumulated_metrics_[tracker->type()] = std::move(metrics);
+    }
+  }
+
   // Destroy the trackers that are ready to be terminated.
   base::EraseIf(
       removal_trackers_,
@@ -321,7 +373,11 @@ FrameSequenceTracker::FrameSequenceTracker(
     FrameSequenceTrackerType type,
     UkmManager* manager,
     ThroughputUkmReporter* throughput_ukm_reporter)
-    : type_(type), metrics_(type, manager, throughput_ukm_reporter) {
+    : type_(type),
+      metrics_(
+          std::make_unique<FrameSequenceMetrics>(type,
+                                                 manager,
+                                                 throughput_ukm_reporter)) {
   DCHECK_LT(type_, FrameSequenceTrackerType::kMaxType);
 }
 
@@ -329,7 +385,7 @@ FrameSequenceTracker::~FrameSequenceTracker() {
 }
 
 void FrameSequenceTracker::ReportMetrics() {
-  metrics_.ReportMetrics();
+  metrics_->ReportMetrics();
 }
 
 void FrameSequenceTracker::ReportBeginImplFrame(
@@ -446,7 +502,7 @@ void FrameSequenceTracker::ReportFramePresented(
   TRACKER_TRACE_STREAM << 'P';
 
   TRACE_EVENT_ASYNC_STEP_INTO_WITH_TIMESTAMP0(
-      "cc,benchmark", "FrameSequenceTracker", this, "FramePresented",
+      "cc,benchmark", "FrameSequenceTracker", metrics_.get(), "FramePresented",
       feedback.timestamp);
   const bool was_presented = !feedback.timestamp.is_null();
   if (was_presented && last_submitted_frame_) {
@@ -490,7 +546,7 @@ void FrameSequenceTracker::ReportFramePresented(
       DCHECK(!interval.is_zero()) << TRACKER_DCHECK_MSG;
       constexpr base::TimeDelta kEpsilon = base::TimeDelta::FromMilliseconds(1);
       int64_t frames = (difference + kEpsilon) / interval;
-      metrics_.add_checkerboarded_frames(frames);
+      metrics_->add_checkerboarded_frames(frames);
     }
 
     const bool frame_had_checkerboarding =
@@ -614,10 +670,13 @@ FrameSequenceMetrics::ThroughputData::ToTracedValue(
 
 bool FrameSequenceTracker::ShouldReportMetricsNow(
     const viz::BeginFrameArgs& args) const {
-  if (!first_frame_timestamp_.is_null() &&
-      args.frame_time - first_frame_timestamp_ >= time_delta_to_report_)
-    return true;
-  return false;
+  return metrics_->HasEnoughDataForReporting() &&
+         !first_frame_timestamp_.is_null() &&
+         args.frame_time - first_frame_timestamp_ >= time_delta_to_report_;
+}
+
+std::unique_ptr<FrameSequenceMetrics> FrameSequenceTracker::TakeMetrics() {
+  return std::move(metrics_);
 }
 
 base::Optional<int> FrameSequenceMetrics::ThroughputData::ReportHistogram(

cc/metrics/frame_sequence_tracker.h

@@ -61,6 +61,7 @@ class CC_EXPORT FrameSequenceMetrics {
     static std::unique_ptr<base::trace_event::TracedValue> ToTracedValue(
         const ThroughputData& impl,
         const ThroughputData& main);
+
     // Returns the throughput in percent, a return value of base::nullopt
     // indicates that no throughput metric is reported.
     static base::Optional<int> ReportHistogram(
@@ -68,6 +69,12 @@ class CC_EXPORT FrameSequenceMetrics {
         const char* thread_name,
         int metric_index,
         const ThroughputData& data);
+
+    void Merge(const ThroughputData& data) {
+      frames_expected += data.frames_expected;
+      frames_produced += data.frames_produced;
+    }
+
     // Tracks the number of frames that were expected to be shown during this
     // frame-sequence.
     uint32_t frames_expected = 0;
@@ -77,6 +84,9 @@ class CC_EXPORT FrameSequenceMetrics {
     uint32_t frames_produced = 0;
   };
 
+  void Merge(std::unique_ptr<FrameSequenceMetrics> metrics);
+  bool HasEnoughDataForReporting() const;
+  bool HasDataLeftForReporting() const;
   void ReportMetrics();
 
   ThroughputData& impl_throughput() { return impl_throughput_; }
@@ -178,6 +188,10 @@ class CC_EXPORT FrameSequenceTrackerCollection {
   // LayerTreeHostImpl::ukm_manager_ lives as long as the LayerTreeHostImpl, so
   // this pointer should never be null as long as LayerTreeHostImpl is alive.
   UkmManager* ukm_manager_ = nullptr;
+
+  base::flat_map<FrameSequenceTrackerType,
+                 std::unique_ptr<FrameSequenceMetrics>>
+      accumulated_metrics_;
 };
 
 // Tracks a sequence of frames to determine the throughput. It tracks this by
@@ -250,6 +264,11 @@ class CC_EXPORT FrameSequenceTracker {
   // Returns true if we should ask this tracker to report its throughput data.
   bool ShouldReportMetricsNow(const viz::BeginFrameArgs& args) const;
 
+  FrameSequenceMetrics* metrics() { return metrics_.get(); }
+  FrameSequenceTrackerType type() const { return type_; }
+
+  std::unique_ptr<FrameSequenceMetrics> TakeMetrics();
+
  private:
   friend class FrameSequenceTrackerCollection;
   friend class FrameSequenceTrackerTest;
@@ -259,10 +278,10 @@ class CC_EXPORT FrameSequenceTracker {
                        ThroughputUkmReporter* throughput_ukm_reporter);
 
   FrameSequenceMetrics::ThroughputData& impl_throughput() {
-    return metrics_.impl_throughput();
+    return metrics_->impl_throughput();
   }
   FrameSequenceMetrics::ThroughputData& main_throughput() {
-    return metrics_.main_throughput();
+    return metrics_->main_throughput();
   }
 
   void ScheduleTerminate() {
@@ -313,7 +332,7 @@ class CC_EXPORT FrameSequenceTracker {
   TrackedFrameData begin_impl_frame_data_;
   TrackedFrameData begin_main_frame_data_;
 
-  FrameSequenceMetrics metrics_;
+  std::unique_ptr<FrameSequenceMetrics> metrics_;
 
   CheckerboardingData checkerboarding_;
 

cc/metrics/frame_sequence_tracker_unittest.cc

@@ -128,7 +128,9 @@ class FrameSequenceTrackerTest : public testing::Test {
         "Graphics.Smoothness.Throughput.SlowerThread.TouchScroll", 1u);
 
     // Test that both are reported.
-    tracker_->main_throughput().frames_expected = 50u;
+    tracker_->impl_throughput().frames_expected = 100u;
+    tracker_->impl_throughput().frames_produced = 85u;
+    tracker_->main_throughput().frames_expected = 150u;
     tracker_->main_throughput().frames_produced = 25u;
     tracker_->ReportMetrics();
     histogram_tester.ExpectTotalCount(
@@ -152,10 +154,10 @@ class FrameSequenceTrackerTest : public testing::Test {
         "Graphics.Smoothness.Throughput.SlowerThread.TouchScroll", 2u);
 
     // Test the case where compositor and main thread have the same throughput.
-    tracker_->impl_throughput().frames_expected = 20u;
-    tracker_->impl_throughput().frames_produced = 18u;
-    tracker_->main_throughput().frames_expected = 20u;
-    tracker_->main_throughput().frames_produced = 18u;
+    tracker_->impl_throughput().frames_expected = 120u;
+    tracker_->impl_throughput().frames_produced = 118u;
+    tracker_->main_throughput().frames_expected = 120u;
+    tracker_->main_throughput().frames_produced = 118u;
     tracker_->ReportMetrics();
     histogram_tester.ExpectTotalCount(
         "Graphics.Smoothness.Throughput.CompositorThread.TouchScroll", 3u);
@@ -187,16 +189,16 @@ class FrameSequenceTrackerTest : public testing::Test {
     return tracker_->ignored_frame_tokens_;
   }
 
-  FrameSequenceMetrics::ThroughputData ImplThroughput() const {
+  FrameSequenceMetrics::ThroughputData& ImplThroughput() const {
     return tracker_->impl_throughput();
   }
-  FrameSequenceMetrics::ThroughputData MainThroughput() const {
+  FrameSequenceMetrics::ThroughputData& MainThroughput() const {
     return tracker_->main_throughput();
   }
 
  protected:
   uint32_t number_of_frames_checkerboarded() const {
-    return tracker_->metrics_.frames_checkerboarded();
+    return tracker_->metrics_->frames_checkerboarded();
   }
 
   std::unique_ptr<CompositorFrameReportingController>
@@ -374,6 +376,7 @@ TEST_F(FrameSequenceTrackerTest, ReportMetricsAtFixedInterval) {
   EXPECT_EQ(NumberOfTrackers(), 1u);
   EXPECT_EQ(NumberOfRemovalTrackers(), 0u);
 
+  ImplThroughput().frames_expected += 100;
   // Now args.frame_time is 5s since the tracker creation time, so this tracker
   // should be scheduled to report its throughput.
   args = CreateBeginFrameArgs(source, ++sequence,

tools/perf/page_sets/rendering/throughput_test_cases.py

@@ -12,7 +12,7 @@ class ThroughputMetricStory(rendering_story.RenderingStory):
   def RunPageInteractions(self, action_runner):
     with action_runner.CreateGestureInteraction('AnimationOnTap'):
       action_runner.PressKey(' ')
-      action_runner.Wait(5)
+      action_runner.Wait(10)
       action_runner.PressKey(' ')