[throughput] Improve slower thread throughput

Right now the slower thread throughput is simply taking the lower
throughput between the main and compositor throughput.

This CL improves that. Specifically, when we are expecting and
producing main frames, then we count the frames produced from the
main thread towards the slower thread. Otherwise, use the numbers
from the compositor thread towards the slower thread.

Bug: 1002994
Change-Id: Ie2de3ff958f920e6afa5234b7cf40aea05930ab9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2022618Reviewed-by: Annie Sullivan <sullivan@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754547}

cc/metrics/frame_sequence_metrics_unittest.cc

@@ -10,20 +10,16 @@
 
 namespace cc {
 
-TEST(FrameSequenceMetricsTest, SlowerThread) {
-  base::HistogramTester histogram_tester;
-
+TEST(FrameSequenceMetricsTest, AggregatedThroughput) {
   FrameSequenceMetrics first(FrameSequenceTrackerType::kTouchScroll, nullptr);
-  first.impl_throughput().frames_expected = 200;
-  first.impl_throughput().frames_produced = 190;
-  first.main_throughput().frames_expected = 100;
-  first.main_throughput().frames_produced = 50;
-
-  // slower thread throughput is computed at ReportMetrics().
-  first.ReportMetrics();
-  std::string metric =
-      "Graphics.Smoothness.PercentDroppedFrames.SlowerThread.TouchScroll";
-  EXPECT_EQ(histogram_tester.GetBucketCount(metric, 50), 1);
+  first.impl_throughput().frames_expected = 200u;
+  first.impl_throughput().frames_produced = 190u;
+  first.main_throughput().frames_expected = 100u;
+  first.main_throughput().frames_produced = 50u;
+
+  // The aggregated throughput is computed at ReportMetrics().
+  first.ComputeAggregatedThroughputForTesting();
+  EXPECT_EQ(first.aggregated_throughput().frames_expected, 200u);
 }
 
 TEST(FrameSequenceMetricsTest, MergeMetrics) {
@@ -67,8 +63,6 @@ TEST(FrameSequenceMetricsTest, AllMetricsReported) {
       1u);
   histograms.ExpectTotalCount(
       "Graphics.Smoothness.PercentDroppedFrames.MainThread.TouchScroll", 0u);
-  histograms.ExpectTotalCount(
-      "Graphics.Smoothness.PercentDroppedFrames.SlowerThread.TouchScroll", 1u);
 
   // There should still be data left over for the main-thread.
   EXPECT_TRUE(first.HasDataLeftForReporting());
@@ -86,8 +80,6 @@ TEST(FrameSequenceMetricsTest, AllMetricsReported) {
       2u);
   histograms.ExpectTotalCount(
       "Graphics.Smoothness.PercentDroppedFrames.MainThread.TouchScroll", 1u);
-  histograms.ExpectTotalCount(
-      "Graphics.Smoothness.PercentDroppedFrames.SlowerThread.TouchScroll", 2u);
   // All the metrics have now been reported. No data should be left over.
   EXPECT_FALSE(first.HasDataLeftForReporting());
 
@@ -104,8 +96,6 @@ TEST(FrameSequenceMetricsTest, AllMetricsReported) {
       1u);
   histograms.ExpectTotalCount(
       "Graphics.Smoothness.PercentDroppedFrames.MainThread.Universal", 1u);
-  histograms.ExpectTotalCount(
-      "Graphics.Smoothness.PercentDroppedFrames.SlowerThread.Universal", 1u);
 }
 
 TEST(FrameSequenceMetricsTest, IrrelevantMetricsNotReported) {

cc/metrics/frame_sequence_tracker.cc

@@ -143,8 +143,9 @@ FrameSequenceMetrics::FrameSequenceMetrics(FrameSequenceTrackerType type,
 }
 
 FrameSequenceMetrics::~FrameSequenceMetrics() {
-  if (HasDataLeftForReporting())
+  if (HasDataLeftForReporting()) {
     ReportMetrics();
+  }
 }
 
 void FrameSequenceMetrics::SetScrollingThread(ThreadType scrolling_thread) {
@@ -160,12 +161,14 @@ void FrameSequenceMetrics::Merge(
   DCHECK_EQ(type_, metrics->type_);
   impl_throughput_.Merge(metrics->impl_throughput_);
   main_throughput_.Merge(metrics->main_throughput_);
+  aggregated_throughput_.Merge(metrics->aggregated_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->aggregated_throughput_ = {};
   metrics->frames_checkerboarded_ = 0;
 }
 
@@ -179,6 +182,17 @@ bool FrameSequenceMetrics::HasDataLeftForReporting() const {
          main_throughput_.frames_expected > 0;
 }
 
+void FrameSequenceMetrics::ComputeAggregatedThroughput() {
+  // Whenever we are expecting and producing main frames, we are expecting and
+  // producing impl frames as well. As an example, if we expect one main frame
+  // to be produced, and when that main frame is presented, we are expecting 3
+  // impl frames, then the number of expected frames is 3 for the aggregated
+  // throughput.
+  aggregated_throughput_.frames_expected = impl_throughput_.frames_expected;
+  DCHECK_LE(aggregated_throughput_.frames_produced,
+            aggregated_throughput_.frames_produced);
+}
+
 void FrameSequenceMetrics::ReportMetrics() {
   DCHECK_LE(impl_throughput_.frames_produced, impl_throughput_.frames_expected);
   DCHECK_LE(main_throughput_.frames_produced, main_throughput_.frames_expected);
@@ -187,6 +201,8 @@ void FrameSequenceMetrics::ReportMetrics() {
       ThroughputData::ToTracedValue(impl_throughput_, main_throughput_),
       "checkerboard", frames_checkerboarded_);
 
+  ComputeAggregatedThroughput();
+
   // Report the throughput metrics.
   base::Optional<int> impl_throughput_percent = ThroughputData::ReportHistogram(
       throughput_ukm_reporter_, type_, ThreadType::kCompositor,
@@ -200,34 +216,15 @@ void FrameSequenceMetrics::ReportMetrics() {
   // Report for the 'slower thread' for the metrics where it makes sense.
   bool should_report_slower_thread =
       IsInteractionType(type_) || type_ == FrameSequenceTrackerType::kUniversal;
+  base::Optional<int> aggregated_throughput_percent;
   if (should_report_slower_thread) {
-    base::Optional<ThroughputData> slower_throughput;
-    base::Optional<int> slower_throughput_percent;
-    // The value is percent of dropped frames, slower throughput should have
-    // larger value.
-    if (impl_throughput_percent &&
-        (!main_throughput_percent ||
-         impl_throughput_percent.value() >= main_throughput_percent.value())) {
-      slower_throughput = impl_throughput_;
-    }
-    if (main_throughput_percent &&
-        (!impl_throughput_percent ||
-         main_throughput_percent.value() > impl_throughput_percent.value())) {
-      slower_throughput = main_throughput_;
-    }
-    if (slower_throughput.has_value()) {
-      slower_throughput_percent = ThroughputData::ReportHistogram(
-          throughput_ukm_reporter_, type_, ThreadType::kSlower,
-          GetIndexForMetric(FrameSequenceMetrics::ThreadType::kSlower, type_),
-          slower_throughput.value());
-      DCHECK(slower_throughput_percent.has_value())
-          << FrameSequenceTracker::GetFrameSequenceTrackerTypeName(type_);
-    }
-
-    // slower_throughput has value indicates that we have reported UMA.
-    if (slower_throughput.has_value() && throughput_ukm_reporter_) {
+    aggregated_throughput_percent = ThroughputData::ReportHistogram(
+        throughput_ukm_reporter_, type_, ThreadType::kSlower,
+        GetIndexForMetric(FrameSequenceMetrics::ThreadType::kSlower, type_),
+        aggregated_throughput_);
+    if (aggregated_throughput_percent.has_value() && throughput_ukm_reporter_) {
       throughput_ukm_reporter_->ReportThroughputUkm(
-          slower_throughput_percent, impl_throughput_percent,
+          aggregated_throughput_percent, impl_throughput_percent,
           main_throughput_percent, type_);
     }
   }
@@ -290,6 +287,8 @@ void FrameSequenceMetrics::ReportMetrics() {
     impl_throughput_ = {};
   if (main_throughput_.frames_expected >= kMinFramesForThroughputMetric)
     main_throughput_ = {};
+  if (aggregated_throughput_percent.has_value())
+    aggregated_throughput_ = {};
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -700,16 +699,26 @@ void FrameSequenceTracker::ReportSubmitFrame(
       last_no_main_damage_sequence_ != 0 &&
       origin_args.frame_id.sequence_number == last_no_main_damage_sequence_;
 
-  if (!ShouldIgnoreBeginFrameSource(origin_args.frame_id.source_id) &&
-      main_changes_after_sequence_started && main_changes_include_new_changes &&
-      !main_change_had_no_damage) {
-    submitted_frame_had_new_main_content_ = true;
-    TRACKER_TRACE_STREAM << "S(" << origin_args.frame_id.sequence_number << ")";
+  if (!ShouldIgnoreBeginFrameSource(origin_args.frame_id.source_id)) {
+    if (main_changes_after_sequence_started &&
+        main_changes_include_new_changes && !main_change_had_no_damage) {
+      submitted_frame_had_new_main_content_ = true;
+      TRACKER_TRACE_STREAM << "S(" << origin_args.frame_id.sequence_number
+                           << ")";
 
-    last_submitted_main_sequence_ = origin_args.frame_id.sequence_number;
-    main_frames_.push_back(frame_token);
-    DCHECK_GE(main_throughput().frames_expected, main_frames_.size())
-        << TRACKER_DCHECK_MSG;
+      last_submitted_main_sequence_ = origin_args.frame_id.sequence_number;
+      main_frames_.push_back(frame_token);
+      DCHECK_GE(main_throughput().frames_expected, main_frames_.size())
+          << TRACKER_DCHECK_MSG;
+    } else {
+      // If we have sent a BeginMainFrame which hasn't yet been submitted, or
+      // confirmed that it has no damage (previous_sequence is set to 0), then
+      // we are currently expecting a main frame.
+      const bool expecting_main = begin_main_frame_data_.previous_sequence >
+                                  last_submitted_main_sequence_;
+      if (expecting_main)
+        expecting_main_when_submit_impl_.push_back(frame_token);
+    }
   }
 
   if (has_missing_content) {
@@ -816,6 +825,8 @@ void FrameSequenceTracker::ReportFramePresented(
   if (ignored_frame_tokens_.contains(frame_token))
     return;
 
+  uint32_t impl_frames_produced = 0;
+  uint32_t main_frames_produced = 0;
   trace_data_.Advance(feedback.timestamp);
 
   const bool was_presented = !feedback.timestamp.is_null();
@@ -824,6 +835,7 @@ void FrameSequenceTracker::ReportFramePresented(
               impl_throughput().frames_expected)
         << TRACKER_DCHECK_MSG;
     ++impl_throughput().frames_produced;
+    ++impl_frames_produced;
 
     if (frame_token_acks_last_frame)
       last_submitted_frame_ = 0;
@@ -842,6 +854,41 @@ void FrameSequenceTracker::ReportFramePresented(
                 main_throughput().frames_expected)
           << TRACKER_DCHECK_MSG;
       ++main_throughput().frames_produced;
+      ++main_frames_produced;
+    }
+
+    if (impl_frames_produced > 0) {
+      // If there is no main frame presented, then we need to see whether or not
+      // we are expecting main frames to be presented or not.
+      if (main_frames_produced == 0) {
+        // Only need to check the first element in the deque because the
+        // elements are in order.
+        bool expecting_main_frames =
+            !expecting_main_when_submit_impl_.empty() &&
+            !viz::FrameTokenGT(expecting_main_when_submit_impl_[0],
+                               frame_token);
+        if (expecting_main_frames) {
+          // We are expecting a main frame to be processed, the main frame
+          // should either report no-damage or be submitted to GPU. Since we
+          // don't know which case it would be, we accumulate the number of impl
+          // frames produced so that we can apply that to aggregated throughput
+          // if the main frame reports no-damage later on.
+          impl_frames_produced_while_expecting_main_ += impl_frames_produced;
+        } else {
+          DCHECK_EQ(impl_frames_produced_while_expecting_main_, 0u)
+              << TRACKER_DCHECK_MSG;
+          aggregated_throughput().frames_produced += impl_frames_produced;
+          impl_frames_produced_while_expecting_main_ = 0;
+        }
+      } else {
+        aggregated_throughput().frames_produced += main_frames_produced;
+        impl_frames_produced_while_expecting_main_ = 0;
+        while (!expecting_main_when_submit_impl_.empty() &&
+               !viz::FrameTokenGT(expecting_main_when_submit_impl_.front(),
+                                  frame_token)) {
+          expecting_main_when_submit_impl_.pop_front();
+        }
+      }
     }
 
     if (checkerboarding_.last_frame_had_checkerboarding) {
@@ -950,6 +997,11 @@ void FrameSequenceTracker::ReportMainFrameCausedNoDamage(
         << TRACKER_DCHECK_MSG;
   }
   begin_main_frame_data_.previous_sequence = 0;
+
+  aggregated_throughput().frames_produced +=
+      impl_frames_produced_while_expecting_main_;
+  impl_frames_produced_while_expecting_main_ = 0;
+  expecting_main_when_submit_impl_.clear();
 }
 
 void FrameSequenceTracker::PauseFrameProduction() {

cc/metrics/frame_sequence_tracker.h

@@ -112,15 +112,20 @@ class CC_EXPORT FrameSequenceMetrics {
   bool HasDataLeftForReporting() const;
   // Report related metrics: throughput, checkboarding...
   void ReportMetrics();
+  void ComputeAggregatedThroughputForTesting() {
+    ComputeAggregatedThroughput();
+  }
 
   ThroughputData& impl_throughput() { return impl_throughput_; }
   ThroughputData& main_throughput() { return main_throughput_; }
+  ThroughputData& aggregated_throughput() { return aggregated_throughput_; }
   void add_checkerboarded_frames(int64_t frames) {
     frames_checkerboarded_ += frames;
   }
   uint32_t frames_checkerboarded() const { return frames_checkerboarded_; }
 
  private:
+  void ComputeAggregatedThroughput();
   const FrameSequenceTrackerType type_;
 
   // Pointer to the reporter owned by the FrameSequenceTrackerCollection.
@@ -128,6 +133,8 @@ class CC_EXPORT FrameSequenceMetrics {
 
   ThroughputData impl_throughput_;
   ThroughputData main_throughput_;
+  // The aggregated throughput for the main/compositor thread.
+  ThroughputData aggregated_throughput_;
 
   ThreadType scrolling_thread_ = ThreadType::kUnknown;
 
@@ -305,6 +312,9 @@ class CC_EXPORT FrameSequenceTracker {
   FrameSequenceMetrics::ThroughputData& main_throughput() {
     return metrics_->main_throughput();
   }
+  FrameSequenceMetrics::ThroughputData& aggregated_throughput() {
+    return metrics_->aggregated_throughput();
+  }
 
   void ScheduleTerminate();
 
@@ -416,6 +426,14 @@ class CC_EXPORT FrameSequenceTracker {
   uint64_t last_processed_main_sequence_ = 0;
   uint64_t last_processed_main_sequence_latency_ = 0;
 
+  // Used to compute aggregated throughput.
+  // When expecting a main frame, we accumulate the number of impl frames
+  // presented because if that main frame ends up with no-damage, then we should
+  // count the impl frames that were produced in the meantime.
+  uint32_t impl_frames_produced_while_expecting_main_ = 0;
+  // Each entry is a frame token, inserted at ReportSubmitFrame.
+  base::circular_deque<uint32_t> expecting_main_when_submit_impl_;
+
   // Handle off-screen main damage case. In this case, the sequence is typically
   // like: b(1)B(0,1)E(1)n(1)e(1)b(2)n(2)e(2)...b(10)E(2)B(10,10)n(10)e(10).
   // Note that between two 'E's, all the impl frames caused no damage, and

chrome/test/data/perf/throughput_test_cases/main-impl-animations-throughput.html

@@ -53,6 +53,12 @@ const targetMainFps =
   window.location.hash.length > 1 ? + window.location.hash.substr(1) : 0;
 const skipEvery = targetMainFps > 0 ? parseInt(60 / targetMainFps) : 0;
 
+const query = window.location.search.substr(1).split('&');
+const shouldJank = query.indexOf('jank') >= 0;
+// Delay the start of the main thread animation by 2 seconds to test the
+// aggregation of the main and compositor thread throughput.
+const shouldDelay = query.indexOf('delay') >= 0;
+
 var currentValue = 0;
 const maxValue = 150;
 const valueChange = 5 * skipEvery;
@@ -63,9 +69,11 @@ const setProp = (v) => 'translateX(' + v + 'px)';
 var animating = false;
 var incrementing = false;
 var frameCount = 0;
+var startTime;
 
 function animateStep() {
-  if (skipEvery && ++frameCount % skipEvery == 0) {
+  if (skipEvery && ++frameCount % skipEvery == 0 &&
+      (new Date() - startTime) >= 1000) {
     if (incrementing) {
       currentValue += valueChange;
       if (currentValue >= maxValue)
@@ -75,6 +83,10 @@ function animateStep() {
       if (currentValue <= minValue)
         incrementing = true;
     }
+    if (shouldJank) {
+      const now = new Date();
+      while ((new Date() - now) < 16) {}
+    }
     mainanim.style[attributeName] = setProp(currentValue);
   }
   if (animating)
@@ -83,6 +95,7 @@ function animateStep() {
 
 function startAnimation() {
   animating = true;
+  startTime = new Date();
   requestAnimationFrame(animateStep);
   cssanim.classList.remove('paused');
 }

tools/perf/benchmarks/rendering.py

@@ -38,6 +38,9 @@ RENDERING_BENCHMARK_UMA = [
     'Graphics.Smoothness.PercentDroppedFrames.CompositorThread.PinchZoom',
     'Graphics.Smoothness.PercentDroppedFrames.CompositorThread.TouchScroll',
     'Graphics.Smoothness.PercentDroppedFrames.CompositorThread.WheelScroll',
+    'Graphics.Smoothness.PercentDroppedFrames.MainThread.Universal',
+    'Graphics.Smoothness.PercentDroppedFrames.CompositorThread.Universal',
+    'Graphics.Smoothness.PercentDroppedFrames.SlowerThread.Universal',
     'Graphics.Smoothness.PercentDroppedFrames.ScrollingThread.TouchScroll',
     'Graphics.Smoothness.PercentDroppedFrames.ScrollingThread.WheelScroll',
     'Memory.GPU.PeakMemoryUsage.Scroll',

tools/perf/page_sets/rendering/throughput_test_cases.py

@@ -56,6 +56,13 @@ class MainSixtyImplSixty(ThroughputMetricStory):
          'main-impl-animations-throughput.html#60')
 
 
+class MainSixtyImplSixtyWithJankAndDelay(ThroughputMetricStory):
+  BASE_NAME = 'main_60fps_with_jank_and_delay_impl_60fps'
+  SUPPORTED_PLATFORMS = platforms.ALL_PLATFORMS
+  URL = ('file://../../../../chrome/test/data/perf/throughput_test_cases/'
+         'main-impl-animations-throughput.html?jank&delay#60')
+
+
 class MainSixtyImplSixtyNoUpdate(ThroughputMetricStory):
   BASE_NAME = 'main_60fps_impl_60fps_no_update'
   SUPPORTED_PLATFORMS = platforms.ALL_PLATFORMS