cc: Use less aggressive scheduling strategy for impl-side invalidations.

We currently rely both on the timing history and whether the main thread
responded within the frame deadline to decide whether to prioritize impl
side invalidations. The latter had a bug where we incorrectly assumed the
main thread is missing deadlines when impl thread is high latency/swap
throttled. Update the logic to account for the second case.

R=sunnyps@chromium.org
Bug:1086790

Change-Id: I0b163e99c2a2081518ba89b0743c9de9836ba8b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2217062
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Auto-Submit: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775824}

cc/metrics/compositor_frame_reporting_controller.h

@@ -98,6 +98,9 @@ class CC_EXPORT CompositorFrameReportingController {
   base::TimeTicks Now() const;
 
   bool HasReporterAt(PipelineStage stage) const;
+  bool next_activate_has_invalidation() const {
+    return next_activate_has_invalidation_;
+  }
 
  private:
   void AdvanceReporterStage(PipelineStage start, PipelineStage target);

cc/scheduler/scheduler.cc

@@ -516,20 +516,19 @@ void Scheduler::BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args) {
             ->BeginMainFrameQueueDurationNotCriticalEstimate();
   }
 
-  // We defer the invalidation if we expect the main thread to respond within
-  // this frame, and our prediction in the last frame was correct. That
-  // is, if we predicted the main thread to be fast and it fails to respond
-  // before the deadline in the previous frame, we don't defer the invalidation
-  // in the next frame.
-  const bool main_thread_response_expected_before_deadline =
-      bmf_sent_to_ready_to_commit_estimate - time_since_main_frame_sent <
-      bmf_to_activate_threshold;
-  const bool previous_invalidation_maybe_skipped_for_main_frame =
-      state_machine_.should_defer_invalidation_for_fast_main_frame() &&
-      state_machine_.main_thread_failed_to_respond_last_deadline();
+  bool main_thread_response_expected_before_deadline;
+  if (time_since_main_frame_sent > bmf_to_activate_threshold) {
+    // If the response to a main frame is pending past the desired duration
+    // then proactively assume that the main thread is slow instead of late
+    // correction through the frame history.
+    main_thread_response_expected_before_deadline = false;
+  } else {
+    main_thread_response_expected_before_deadline =
+        bmf_sent_to_ready_to_commit_estimate - time_since_main_frame_sent <
+        bmf_to_activate_threshold;
+  }
   state_machine_.set_should_defer_invalidation_for_fast_main_frame(
-      main_thread_response_expected_before_deadline &&
-      !previous_invalidation_maybe_skipped_for_main_frame);
+      main_thread_response_expected_before_deadline);
 
   base::TimeDelta bmf_to_activate_estimate = bmf_to_activate_estimate_critical;
   if (!begin_main_frame_args_.on_critical_path) {

cc/scheduler/scheduler_state_machine.cc

@@ -1191,8 +1191,6 @@ void SchedulerStateMachine::OnBeginImplFrameIdle() {
   // then the main thread is in a high latency mode.
   main_thread_missed_last_deadline_ =
       CommitPending() || has_pending_tree_ || active_tree_needs_first_draw_;
-  main_thread_failed_to_respond_last_deadline_ =
-      begin_main_frame_state_ == BeginMainFrameState::SENT;
 
   // If we're entering a state where we won't get BeginFrames set all the
   // funnels so that we don't perform any actions that we shouldn't.

cc/scheduler/scheduler_state_machine.h

@@ -344,10 +344,6 @@ class CC_EXPORT SchedulerStateMachine {
     return should_defer_invalidation_for_fast_main_frame_;
   }
 
-  bool main_thread_failed_to_respond_last_deadline() const {
-    return main_thread_failed_to_respond_last_deadline_;
-  }
-
  protected:
   bool BeginFrameRequiredForAction() const;
   bool BeginFrameNeededForVideo() const;
@@ -471,10 +467,6 @@ class CC_EXPORT SchedulerStateMachine {
   // tree. During this time we should not activate the pending tree.
   bool processing_paint_worklets_for_pending_tree_ = false;
 
-  // Set to true if the main thread fails to respond with a commit or abort the
-  // main frame before the draw deadline on the previous impl frame.
-  bool main_thread_failed_to_respond_last_deadline_ = false;
-
   bool previous_pending_tree_was_impl_side_ = false;
   bool current_pending_tree_is_impl_side_ = false;
 

cc/scheduler/scheduler_unittest.cc

@@ -4362,5 +4362,46 @@ TEST_F(SchedulerTest, SendEarlyDidNotProduceFrameIfIdle) {
             begin_main_frame_args.frame_id.sequence_number);
 }
 
+TEST_F(SchedulerTest,
+       HighImplLatencyModePrioritizesMainFramesOverImplInvalidation) {
+  scheduler_settings_.enable_main_latency_recovery = false;
+  scheduler_settings_.enable_impl_latency_recovery = false;
+  SetUpScheduler(EXTERNAL_BFS);
+  fake_compositor_timing_history_->SetAllEstimatesTo(kFastDuration);
+
+  // Place the impl thread in high latency mode.
+  scheduler_->SetNeedsImplSideInvalidation(true);
+  client_->Reset();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTIONS("WillBeginImplFrame",
+                 "ScheduledActionPerformImplSideInvalidation");
+
+  // Request a main frame and start the next impl frame. Since we have an impl
+  // side pending tree, we will activate and draw it. This finishes the impl
+  // frame before the main thread can respond causing the scheduler to
+  // incorrectly assume the main thread is slow.
+  client_->Reset();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTIONS("WillBeginImplFrame");
+  client_->Reset();
+  scheduler_->SetNeedsBeginMainFrame();
+  EXPECT_ACTIONS("ScheduledActionSendBeginMainFrame");
+  fake_compositor_timing_history_->SetBeginMainFrameSentTime(
+      task_runner_->NowTicks() + base::TimeDelta::FromMilliseconds(8));
+  client_->Reset();
+  scheduler_->NotifyReadyToActivate();
+  task_runner_->RunTasksWhile(client_->InsideBeginImplFrame(true));
+  EXPECT_ACTIONS("ScheduledActionActivateSyncTree",
+                 "ScheduledActionDrawIfPossible");
+
+  // Start a new frame. We should not assume the main thread is slow.
+  client_->Reset();
+  EXPECT_SCOPED(AdvanceFrame());
+  scheduler_->SetNeedsImplSideInvalidation(true);
+  // No invalidation should be performed since we are waiting for the main
+  // thread to respond and merge with the commit.
+  EXPECT_ACTIONS("WillBeginImplFrame");
+}
+
 }  // namespace
 }  // namespace cc

cc/test/fake_compositor_frame_reporting_controller.cc

@@ -52,13 +52,19 @@ void FakeCompositorFrameReportingController::DidCommit() {
 }
 
 void FakeCompositorFrameReportingController::WillActivate() {
-  if (!HasReporterAt(PipelineStage::kCommit))
+  // Pending trees for impl-side invalidations are created without a prior
+  // commit.
+  if (!HasReporterAt(PipelineStage::kCommit) &&
+      !next_activate_has_invalidation())
     DidCommit();
   CompositorFrameReportingController::WillActivate();
 }
 
 void FakeCompositorFrameReportingController::DidActivate() {
-  if (!HasReporterAt(PipelineStage::kCommit))
+  // Pending trees for impl-side invalidations are created without a prior
+  // commit.
+  if (!HasReporterAt(PipelineStage::kCommit) &&
+      !next_activate_has_invalidation())
     WillActivate();
   CompositorFrameReportingController::DidActivate();
 }

cc/test/scheduler_test_common.cc

@@ -96,6 +96,11 @@ void FakeCompositorTimingHistory::SetDrawDurationEstimate(
   draw_duration_ = duration;
 }
 
+void FakeCompositorTimingHistory::SetBeginMainFrameSentTime(
+    base::TimeTicks time) {
+  begin_main_frame_sent_time_ = time;
+}
+
 base::TimeDelta
 FakeCompositorTimingHistory::BeginMainFrameQueueDurationCriticalEstimate()
     const {

cc/test/scheduler_test_common.h

@@ -45,6 +45,7 @@ class FakeCompositorTimingHistory : public CompositorTimingHistory {
   void SetPrepareTilesDurationEstimate(base::TimeDelta duration);
   void SetActivateDurationEstimate(base::TimeDelta duration);
   void SetDrawDurationEstimate(base::TimeDelta duration);
+  void SetBeginMainFrameSentTime(base::TimeTicks time);
 
   base::TimeDelta BeginMainFrameQueueDurationCriticalEstimate() const override;
   base::TimeDelta BeginMainFrameQueueDurationNotCriticalEstimate()