[Jank] Improve the processing of long idle frames

This CL introduces the following changes in JankMetrics:
1. Limit the maximum length of the queue of no-update frames so as to
prevent unbounded memory growth on an idle page on some platforms.
2. Exclude from jank/stale calculation the excessively long idle frames
that have little impact on perceived smoothness, since there is no
longer enough information in the length-limited no-update queue to
accurately estimate frame delta.

Bug: 1231893
Change-Id: I5033450c377e1b0c58e2e76a11613b035751f8a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3087639
Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912628}

cc/metrics/jank_metrics.cc

@@ -19,6 +19,7 @@ namespace cc {
 
 namespace {
 
+constexpr uint64_t kMaxNoUpdateFrameQueueLength = 100;
 constexpr int kBuiltinSequenceNum =
     static_cast<int>(FrameSequenceTrackerType::kMaxType) + 1;
 constexpr int kMaximumJankHistogramIndex = 2 * kBuiltinSequenceNum;
@@ -101,10 +102,17 @@ void JankMetrics::AddSubmitFrame(uint32_t frame_token,
 
 void JankMetrics::AddFrameWithNoUpdate(uint32_t sequence_number,
                                        base::TimeDelta frame_interval) {
+  DCHECK_LE(queue_frame_id_and_interval_.size(), kMaxNoUpdateFrameQueueLength);
+
   // If a frame does not cause an increase in expected frames, it will be
   // recorded here and later subtracted from the presentation interval that
   // includes this frame.
   queue_frame_id_and_interval_.push({sequence_number, frame_interval});
+
+  // This prevents the no-update frame queue from growing infinitely on an idle
+  // page.
+  if (queue_frame_id_and_interval_.size() > kMaxNoUpdateFrameQueueLength)
+    queue_frame_id_and_interval_.pop();
 }
 
 void JankMetrics::AddPresentedFrame(
@@ -141,9 +149,18 @@ void JankMetrics::AddPresentedFrame(
   base::TimeDelta no_update_time;  // The frame time spanned by the frames that
                                    // have no updates
 
-  // Compute the presentation delay contributed by no-update frames that began
-  // BEFORE (i.e. have smaller sequence number than) the current presented
-  // frame.
+  // If |queue_frame_id_and_interval_| contains an excessive amount of no-update
+  // frames, it indicates that the current presented frame is most likely the
+  // first presentation after a long idle period. Such frames are excluded from
+  // jank/stale calculation because they usually have little impact on
+  // smoothness perception, and |queue_frame_id_and_interval_| does not hold
+  // enough data to accurately estimate the effective frame delta.
+  bool will_ignore_current_frame =
+      queue_frame_id_and_interval_.size() == kMaxNoUpdateFrameQueueLength;
+
+  // Compute the presentation delay contributed by no-update frames that
+  // began BEFORE (i.e. have smaller sequence number than) the current
+  // presented frame.
   while (!queue_frame_id_and_interval_.empty() &&
          queue_frame_id_and_interval_.front().first < presented_frame_id) {
     auto id_and_interval = queue_frame_id_and_interval_.front();
@@ -160,11 +177,16 @@ void JankMetrics::AddPresentedFrame(
 
   // Exclude the presentation delay introduced by no-update frames. If this
   // exclusion results in negative frame delta, treat the frame delta as 0.
-  base::TimeDelta current_frame_delta = current_presentation_timestamp -
-                                        last_presentation_timestamp_ -
-                                        no_update_time;
   const base::TimeDelta zero_delta = base::TimeDelta::FromMilliseconds(0);
 
+  // Setting the current_frame_delta to zero conveniently excludes the current
+  // frame to be ignored from jank/stale calculation.
+  base::TimeDelta current_frame_delta = (will_ignore_current_frame)
+                                            ? zero_delta
+                                            : current_presentation_timestamp -
+                                                  last_presentation_timestamp_ -
+                                                  no_update_time;
+
   // Guard against the situation when the physical presentation interval is
   // shorter than |no_update_time|. For example, consider two BeginFrames A and
   // B separated by 5 vsync cycles of no-updates (i.e. |no_update_time| = 5

cc/metrics/jank_metrics_unittest.cc

@@ -559,4 +559,136 @@ TEST_F(JankMetricsTest, CustomNotReported) {
   histogram_tester.ExpectTotalCount("Graphics.Smoothness.MaxStale.Custom", 0u);
 }
 
+// Test a frame sequence with a long idle period >= 100 frames.
+// The presentation interval containing the idle period is excluded from
+// jank/stale calculation since the length of the idle period reaches a
+// predefined cap.
+TEST_F(JankMetricsTest, CompositorAnimationOneJankWithLongIdlePeriod) {
+  base::HistogramTester histogram_tester;
+  FrameSequenceTrackerType tracker_type =
+      FrameSequenceTrackerType::kCompositorAnimation;
+  FrameSequenceMetrics::ThreadType thread_type =
+      FrameSequenceMetrics::ThreadType::kCompositor;
+  JankMetrics jank_reporter{tracker_type, thread_type};
+
+  // One jank at E. The long delay of 100 frames between b and c is considered
+  // a long idle period and therefore does not participate in jank/stale
+  // calculation.
+  SimulateFrameSequence(&jank_reporter,
+                        {
+                            /*submit   */ std::string("a-b") +
+                                std::string(100, '-') + std::string("c--d---E"),
+                            /*noupdate */ std::string("---") +
+                                std::string(100, '*') + std::string("--------"),
+                            /*present  */ std::string("a-b") +
+                                std::string(100, '-') + std::string("c--d---E"),
+                        },
+                        {});
+  jank_reporter.ReportJankMetrics(100u);
+
+  // One sample of 1 janks reported for "Compositor".
+  const char* metric =
+      "Graphics.Smoothness.Jank.Compositor.CompositorAnimation";
+  const char* invalid_metric =
+      "Graphics.Smoothness.Jank.Main.CompositorAnimation";
+
+  histogram_tester.ExpectTotalCount(metric, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(metric),
+              testing::ElementsAre(base::Bucket(1, 1)));
+
+  // Test all-sequence metrics.
+  histogram_tester.ExpectTotalCount(kAllSequencesMetricName, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(kAllSequencesMetricName),
+              testing::ElementsAre(base::Bucket(1, 1)));
+
+  histogram_tester.ExpectTotalCount(kAllAnimationsMetricName, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(kAllSequencesMetricName),
+              testing::ElementsAre(base::Bucket(1, 1)));
+
+  histogram_tester.ExpectTotalCount(kAllInteractionsMetricName, 0u);
+
+  // Stale-frame metrics
+  const char* stale_metric = "Graphics.Smoothness.Stale.CompositorAnimation";
+  const char* maxstale_metric =
+      "Graphics.Smoothness.MaxStale.CompositorAnimation";
+
+  histogram_tester.ExpectTotalCount(stale_metric, 4u);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(stale_metric),
+      testing::ElementsAre(base::Bucket(0, 1),  /* The long frame from b to c*/
+                           base::Bucket(16, 1), /* a-b */
+                           base::Bucket(33, 1), /* c--d */
+                           base::Bucket(50, 1)) /* d---E */);
+  histogram_tester.ExpectTotalCount(maxstale_metric, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(maxstale_metric),
+              testing::ElementsAre(base::Bucket(50, 1)));
+
+  // No reporting for "Main".
+  histogram_tester.ExpectTotalCount(invalid_metric, 0u);
+}
+
+// Test a frame sequence with an idle period < 100 frames.
+// The jank and stale are still calculated normally in this case.
+TEST_F(JankMetricsTest, CompositorAnimationTwoJanksWithModerateIdlePeriod) {
+  base::HistogramTester histogram_tester;
+  FrameSequenceTrackerType tracker_type =
+      FrameSequenceTrackerType::kCompositorAnimation;
+  FrameSequenceMetrics::ThreadType thread_type =
+      FrameSequenceMetrics::ThreadType::kCompositor;
+  JankMetrics jank_reporter{tracker_type, thread_type};
+
+  // Two janks at D and E. The long delay of 99 no-update frames does not
+  // exceed the capacity of the no-update frame queue and therefore is not
+  // excluded from jank/stale calculation.
+  SimulateFrameSequence(&jank_reporter,
+                        {
+                            /*submit   */ std::string("a-b-") +
+                                std::string(99, '-') + std::string("c--D---E"),
+                            /*noupdate */ std::string("----") +
+                                std::string(99, '*') + std::string("--------"),
+                            /*present  */ std::string("a-b-") +
+                                std::string(99, '-') + std::string("c--D---E"),
+                        },
+                        {});
+  jank_reporter.ReportJankMetrics(100u);
+
+  // One sample of 2 janks reported for "Compositor".
+  const char* metric =
+      "Graphics.Smoothness.Jank.Compositor.CompositorAnimation";
+  const char* invalid_metric =
+      "Graphics.Smoothness.Jank.Main.CompositorAnimation";
+
+  histogram_tester.ExpectTotalCount(metric, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(metric),
+              testing::ElementsAre(base::Bucket(2, 1)));
+
+  // Test all-sequence metrics.
+  histogram_tester.ExpectTotalCount(kAllSequencesMetricName, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(kAllSequencesMetricName),
+              testing::ElementsAre(base::Bucket(2, 1)));
+
+  histogram_tester.ExpectTotalCount(kAllAnimationsMetricName, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(kAllSequencesMetricName),
+              testing::ElementsAre(base::Bucket(2, 1)));
+
+  histogram_tester.ExpectTotalCount(kAllInteractionsMetricName, 0u);
+
+  // Stale-frame metrics
+  const char* stale_metric = "Graphics.Smoothness.Stale.CompositorAnimation";
+  const char* maxstale_metric =
+      "Graphics.Smoothness.MaxStale.CompositorAnimation";
+
+  histogram_tester.ExpectTotalCount(stale_metric, 4u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(stale_metric),
+              testing::ElementsAre(base::Bucket(16, 2), /* a-b & b-c */
+                                   base::Bucket(33, 1), /* c--d */
+                                   base::Bucket(50, 1)) /* d---E */);
+  histogram_tester.ExpectTotalCount(maxstale_metric, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(maxstale_metric),
+              testing::ElementsAre(base::Bucket(50, 1)));
+
+  // No reporting for "Main".
+  histogram_tester.ExpectTotalCount(invalid_metric, 0u);
+}
+
 }  // namespace cc