Revert "Exclude no-update frames for jank detection"

This reverts commit bfeec8b7.

Reason for revert: MSan failures; see https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20MSAN/7726

Original change's description:
> Exclude no-update frames for jank detection
>
>
> This CL improves the jank metrics by excluding the delays caused by
> frames without intended updates and prevents certain outlier cases
> when a high jank percent is reported on a page with very low percent
> of dropped frames.
>
> Bug: 1133058
> Change-Id: If32576e8b528e22b184a0b549161751fcfd976bf
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2417211
> Reviewed-by: Jonathan Ross <jonross@chromium.org>
> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
> Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#812988}

TBR=sadrul@chromium.org,jonross@chromium.org,mjzhang@chromium.org

Change-Id: I3f79f32c75d7e0638f782913b12a4c4be10260d3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1133058
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2443732Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813102}

cc/metrics/frame_sequence_metrics.cc

@@ -354,37 +354,13 @@ void FrameSequenceMetrics::ReportMetrics() {
 
 void FrameSequenceMetrics::ComputeJank(
     FrameSequenceMetrics::ThreadType thread_type,
-    uint32_t frame_token,
     base::TimeTicks presentation_time,
     base::TimeDelta frame_interval) {
   if (!jank_reporter_)
     return;
 
   if (thread_type == jank_reporter_->thread_type())
-    jank_reporter_->AddPresentedFrame(frame_token, presentation_time,
-                                      frame_interval);
-}
-
-void FrameSequenceMetrics::NotifySubmitForJankReporter(
-    FrameSequenceMetrics::ThreadType thread_type,
-    uint32_t frame_token,
-    uint32_t sequence_number) {
-  if (!jank_reporter_)
-    return;
-
-  if (thread_type == jank_reporter_->thread_type())
-    jank_reporter_->AddSubmitFrame(frame_token, sequence_number);
-}
-
-void FrameSequenceMetrics::NotifyNoUpdateForJankReporter(
-    FrameSequenceMetrics::ThreadType thread_type,
-    uint32_t sequence_number,
-    base::TimeDelta frame_interval) {
-  if (!jank_reporter_)
-    return;
-
-  if (thread_type == jank_reporter_->thread_type())
-    jank_reporter_->AddFrameWithNoUpdate(sequence_number, frame_interval);
+    jank_reporter_->AddPresentedFrame(presentation_time, frame_interval);
 }
 
 bool FrameSequenceMetrics::ThroughputData::CanReportHistogram(

cc/metrics/frame_sequence_metrics.h

@@ -150,19 +150,9 @@ class CC_EXPORT FrameSequenceMetrics {
   void AdvanceTrace(base::TimeTicks timestamp);
 
   void ComputeJank(FrameSequenceMetrics::ThreadType thread_type,
-                   uint32_t frame_token,
                    base::TimeTicks presentation_time,
                    base::TimeDelta frame_interval);
 
-  void NotifySubmitForJankReporter(FrameSequenceMetrics::ThreadType thread_type,
-                                   uint32_t frame_token,
-                                   uint32_t sequence_number);
-
-  void NotifyNoUpdateForJankReporter(
-      FrameSequenceMetrics::ThreadType thread_type,
-      uint32_t sequence_number,
-      base::TimeDelta frame_interval);
-
  private:
   const FrameSequenceTrackerType type_;
 

cc/metrics/frame_sequence_tracker.cc

@@ -276,9 +276,6 @@ void FrameSequenceTracker::ReportSubmitFrame(
 
   TRACKER_TRACE_STREAM << "s(" << frame_token % kDebugStrMod << ")";
   had_impl_frame_submitted_between_commits_ = true;
-  metrics()->NotifySubmitForJankReporter(
-      FrameSequenceMetrics::ThreadType::kCompositor, frame_token,
-      ack.frame_id.sequence_number);
 
   const bool main_changes_after_sequence_started =
       first_received_main_sequence_ &&
@@ -301,9 +298,6 @@ void FrameSequenceTracker::ReportSubmitFrame(
                            << origin_args.frame_id.sequence_number %
                                   kDebugStrMod
                            << ")";
-      metrics()->NotifySubmitForJankReporter(
-          FrameSequenceMetrics::ThreadType::kMain, frame_token,
-          origin_args.frame_id.sequence_number);
 
       last_submitted_main_sequence_ = origin_args.frame_id.sequence_number;
       main_frames_.push_back(frame_token);
@@ -364,9 +358,6 @@ void FrameSequenceTracker::ReportFrameEnd(
               impl_throughput().frames_ontime)
         << TRACKER_DCHECK_MSG;
     --impl_throughput().frames_expected;
-    metrics()->NotifyNoUpdateForJankReporter(
-        FrameSequenceMetrics::ThreadType::kCompositor,
-        args.frame_id.sequence_number, args.interval);
 #if DCHECK_IS_ON()
     ++impl_throughput().frames_processed;
     // If these two are the same, it means that each impl frame is either
@@ -470,7 +461,7 @@ void FrameSequenceTracker::ReportFramePresented(
     }
 
     metrics()->ComputeJank(FrameSequenceMetrics::ThreadType::kCompositor,
-                           frame_token, feedback.timestamp, feedback.interval);
+                           feedback.timestamp, feedback.interval);
   }
 
   if (was_presented) {
@@ -492,8 +483,7 @@ void FrameSequenceTracker::ReportFramePresented(
       }
 
       metrics()->ComputeJank(FrameSequenceMetrics::ThreadType::kMain,
-                             frame_token, feedback.timestamp,
-                             feedback.interval);
+                             feedback.timestamp, feedback.interval);
     }
     if (main_frames_.size() < size_before_erase) {
       if (!last_frame_presentation_timestamp_.is_null() &&
@@ -608,10 +598,6 @@ void FrameSequenceTracker::ReportMainFrameCausedNoDamage(
       << TRACKER_DCHECK_MSG;
   last_no_main_damage_sequence_ = args.frame_id.sequence_number;
   --main_throughput().frames_expected;
-  metrics()->NotifyNoUpdateForJankReporter(
-      FrameSequenceMetrics::ThreadType::kMain, args.frame_id.sequence_number,
-      args.interval);
-
   DCHECK_GE(main_throughput().frames_expected, main_frames_.size())
       << TRACKER_DCHECK_MSG;
 

cc/metrics/jank_metrics.cc

@@ -67,90 +67,19 @@ JankMetrics::JankMetrics(FrameSequenceTrackerType tracker_type,
 }
 JankMetrics::~JankMetrics() = default;
 
-void JankMetrics::AddSubmitFrame(uint32_t frame_token,
-                                 uint32_t sequence_number) {
-  // When a frame is submitted, record its |frame_token| and its associated
-  // |sequence_number|. This pushed item will be removed when this frame is
-  // presented.
-  queue_frame_token_and_id_.push({frame_token, sequence_number});
-}
-
-void JankMetrics::AddFrameWithNoUpdate(uint32_t sequence_number,
-                                       base::TimeDelta frame_interval) {
-  // 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});
-}
-
 void JankMetrics::AddPresentedFrame(
-    uint32_t presented_frame_token,
     base::TimeTicks current_presentation_timestamp,
     base::TimeDelta frame_interval) {
-  uint32_t presented_frame_id = 0;
-
-  // Find the main_sequence_number of the presented_frame_token
-  while (!queue_frame_token_and_id_.empty()) {
-    auto token_and_id = queue_frame_token_and_id_.front();
-
-    if (token_and_id.first > presented_frame_token) {
-      // The submitting of this presented frame was not recorded (e.g. the
-      // submitting might have occurred before JankMetrics starts recording).
-      // In that case, do not use this frame presentation for jank detection.
-      return;
-    }
-    queue_frame_token_and_id_.pop();
-
-    if (token_and_id.first == presented_frame_token) {
-      // Found information about the submit of this presented frame;
-      // retrieve the frame's sequence number.
-      presented_frame_id = token_and_id.second;
-      break;
-    }
-  }
-  // If for any reason the sequence number associated with the
-  // presented_frame_token cannot be identified, then ignore this frame
-  // presentation.
-  if (presented_frame_id == 0)
-    return;
-
-  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.
-  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();
-    if (id_and_interval.first >= last_presentation_frame_id_) {
-      // Only count no-update frames that began SINCE (i.e. have a greater [or
-      // equal] sequence number than) the beginning of previous presented frame.
-      // If, in rare cases, there are still no-update frames that began BEFORE
-      // the beginning of previous presented frame left in the queue, those
-      // frames will simply be discarded and not counted into |no_update_time|.
-      no_update_time += id_and_interval.second;
-    }
-    queue_frame_id_and_interval_.pop();
-  }
+  base::TimeDelta current_frame_delta =
+      current_presentation_timestamp - last_presentation_timestamp_;
 
-  // 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;
-  if (current_frame_delta < base::TimeDelta::FromMilliseconds(0))
-    current_frame_delta = base::TimeDelta::FromMilliseconds(0);
-
-  // Only start tracking jank if this function has already been
-  // called at least once (so that |last_presentation_timestamp_|
-  // and |prev_frame_delta_| have been set).
+  // Only start tracking jank if this function has been called (so that
+  // |last_presentation_timestamp_| and |prev_frame_delta_| have been set).
   //
-  // The presentation interval is typically a multiple of VSync
-  // intervals (i.e. 16.67ms, 33.33ms, 50ms ... on a 60Hz display)
-  // with small fluctuations. The 0.5 * |frame_interval| criterion
-  // is chosen so that the jank detection is robust to those
-  // fluctuations.
+  // The presentation interval is typically a multiple of VSync intervals (i.e.
+  // 16.67ms, 33.33ms, 50ms ... on a 60Hz display) with small fluctuations. The
+  // 0.5 * |frame_interval| criterion is chosen so that the jank detection is
+  // robust to those fluctuations.
   if (!last_presentation_timestamp_.is_null() && !prev_frame_delta_.is_zero() &&
       current_frame_delta > prev_frame_delta_ + 0.5 * frame_interval) {
     jank_count_++;
@@ -165,7 +94,7 @@ void JankMetrics::AddPresentedFrame(
         FrameSequenceTracker::GetFrameSequenceTrackerTypeName(tracker_type_));
   }
   last_presentation_timestamp_ = current_presentation_timestamp;
-  last_presentation_frame_id_ = presented_frame_id;
+
   prev_frame_delta_ = current_frame_delta;
 }
 

cc/metrics/jank_metrics.h

@@ -6,8 +6,6 @@
 #define CC_METRICS_JANK_METRICS_H_
 
 #include <memory>
-#include <queue>
-#include <utility>
 
 #include "cc/metrics/frame_sequence_metrics.h"
 
@@ -23,8 +21,7 @@ class CC_EXPORT JankMetrics {
 
   // Check if a jank occurs based on the timestamps of recent presentations.
   // If there is a jank, increment |jank_count_| and log a trace event.
-  void AddPresentedFrame(uint32_t presented_frame_token,
-                         base::TimeTicks current_presentation_timestamp,
+  void AddPresentedFrame(base::TimeTicks current_presentation_timestamp,
                          base::TimeDelta frame_interval);
 
   // Report the occurrence rate of janks as a UMA metric.
@@ -33,10 +30,6 @@ class CC_EXPORT JankMetrics {
   // Merge the current jank count with a previously unreported jank metrics.
   void Merge(std::unique_ptr<JankMetrics> jank_metrics);
 
-  void AddSubmitFrame(uint32_t frame_token, uint32_t sequence_number);
-
-  void AddFrameWithNoUpdate(uint32_t sequence_number,
-                            base::TimeDelta frame_interval);
   FrameSequenceMetrics::ThreadType thread_type() const {
     return effective_thread_;
   }
@@ -55,19 +48,8 @@ class CC_EXPORT JankMetrics {
   // The time when the last presentation occurs
   base::TimeTicks last_presentation_timestamp_;
 
-  // The sequence number associated with the last presented frame
-  uint32_t last_presentation_frame_id_;
-
   // The interval before the previous frame presentation.
   base::TimeDelta prev_frame_delta_;
-
-  // A queue storing {frame token, sequence number} for all submitted
-  // frames, in ascending order of frame token.
-  std::queue<std::pair<uint32_t, uint32_t>> queue_frame_token_and_id_;
-
-  // A queue storing {sequence number, frame interval} of unprocessed no-update
-  // frames, in ascending order of sequence number.
-  std::queue<std::pair<uint32_t, base::TimeDelta>> queue_frame_id_and_interval_;
 };
 
 }  // namespace cc