Reland "cc: Throttle incoming begin frames in scheduler"

This is a reland of 6beef1e6

Removed incorrect DCHECKs that were added in the original change.

Original change's description:
> cc: Throttle incoming begin frames in scheduler
>
> Unlike PostTask from IO thread to compositor thread in Chrome IPC, mojo
> polls for messages on the compositor thread which means it can dequeue a
> large number of begin frame messages after the compositor thread has
> been busy for some time. All but the last begin frame cancels the
> previous begin frame, and is essentially a nop, but it still ticks
> animations. When a page has a large number of animations each begin
> frame can take a long time and push out other tasks such as tile manager
> callbacks stalling the pipeline.
>
> Throttling the begin frames in viz doesn't fully solve the problem
> because we have to allow at least two begin frames in flight for
> pipelining, and so the client can still process two begin frames back to
> back.
>
> Throttling in AsyncLTFS causes issues with LTFS lifetime and ordering
> with respect to other messages.
>
> Saving incoming begin frame in scheduler and posting a task works and
> ensures that only one begin frame is outstanding at any time.
>
> R=danakj
> BUG=782002
>
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I247a87ad7475d33f878a215ce87056d20482f88c
> Reviewed-on: https://chromium-review.googlesource.com/1130082
> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577046}

TBR=danakj
BUG=782002

Change-Id: Iec7bd9e421bdb372f101ecebc6cb71835dcb27bf
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1147082
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577332}

cc/scheduler/scheduler.h

@@ -193,6 +193,7 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
 
   std::unique_ptr<CompositorTimingHistory> compositor_timing_history_;
 
+  // What the latest deadline was, and when it was scheduled.
   SchedulerStateMachine::BeginImplFrameDeadlineMode
       begin_impl_frame_deadline_mode_ =
           SchedulerStateMachine::BeginImplFrameDeadlineMode::NONE;
@@ -202,9 +203,18 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
   BeginFrameTracker begin_impl_frame_tracker_;
   viz::BeginFrameArgs begin_main_frame_args_;
 
-  base::Closure begin_impl_frame_deadline_closure_;
-  base::CancelableClosure begin_impl_frame_deadline_task_;
-  base::CancelableClosure missed_begin_frame_task_;
+  // Task posted for the deadline or drawing phase of the scheduler. This task
+  // can be rescheduled e.g. when the condition for the deadline is met, it is
+  // scheduled to run immediately.
+  // NOTE: Scheduler weak ptrs are not necessary if CancelableCallback is used.
+  base::CancelableOnceClosure begin_impl_frame_deadline_task_;
+
+  // This is used for queueing begin frames while scheduler is waiting for
+  // previous frame's deadline, or if it's inside ProcessScheduledActions().
+  // Only one such task is posted at any time, but the args are updated as we
+  // get new begin frames.
+  viz::BeginFrameArgs pending_begin_frame_args_;
+  base::CancelableOnceClosure pending_begin_frame_task_;
 
   SchedulerStateMachine state_machine_;
   bool inside_process_scheduled_actions_ = false;
@@ -215,11 +225,32 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
   bool stopped_ = false;
 
  private:
+  // Posts the deadline task if needed by checking
+  // SchedulerStateMachine::CurrentBeginImplFrameDeadlineMode(). This only
+  // happens when the scheduler is processing a begin frame
+  // (BeginImplFrameState::INSIDE_BEGIN_FRAME).
   void ScheduleBeginImplFrameDeadline();
-  void ScheduleBeginImplFrameDeadlineIfNeeded();
+
+  // Starts or stops begin frames as needed by checking
+  // SchedulerStateMachine::BeginFrameNeeded(). This only happens when the
+  // scheduler is not processing a begin frame (BeginImplFrameState::IDLE).
+  void StartOrStopBeginFrames();
+
+  // This will only post a task if the args are valid and there's no existing
+  // task. That implies that we're still expecting begin frames. If begin frames
+  // aren't needed this will be a nop. This only happens when the scheduler is
+  // not processing a begin frame (BeginImplFrameState::IDLE).
+  void PostPendingBeginFrameTask();
+
+  // Use |pending_begin_frame_args_| to begin a new frame like it was received
+  // in OnBeginFrameDerivedImpl().
+  void HandlePendingBeginFrame();
+
+  // Used to drop the pending begin frame before we go idle.
+  void CancelPendingBeginFrameTask();
+
   void BeginImplFrameNotExpectedSoon();
   void BeginMainFrameNotExpectedUntil(base::TimeTicks time);
-  void SetupNextBeginFrameIfNeeded();
   void DrawIfPossible();
   void DrawForced();
   void ProcessScheduledActions();
@@ -235,6 +266,7 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
       base::TimeTicks now) const;
   void AdvanceCommitStateIfPossible();
   bool IsBeginMainFrameSentOrStarted() const;
+
   void BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args);
   void BeginImplFrameSynchronous(const viz::BeginFrameArgs& args);
   void BeginImplFrame(const viz::BeginFrameArgs& args, base::TimeTicks now);
@@ -250,8 +282,6 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
     return inside_action_ == action;
   }
 
-  base::WeakPtrFactory<Scheduler> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(Scheduler);
 };
 

cc/scheduler/scheduler_state_machine.cc

@@ -1076,12 +1076,18 @@ void SchedulerStateMachine::OnBeginImplFrameIdle() {
 
 SchedulerStateMachine::BeginImplFrameDeadlineMode
 SchedulerStateMachine::CurrentBeginImplFrameDeadlineMode() const {
-  if (settings_.using_synchronous_renderer_compositor) {
-    // No deadline for synchronous compositor.
+  const bool outside_begin_frame =
+      begin_impl_frame_state_ != BeginImplFrameState::INSIDE_BEGIN_FRAME;
+  if (settings_.using_synchronous_renderer_compositor || outside_begin_frame) {
+    // No deadline for synchronous compositor, or when outside the begin frame.
     return BeginImplFrameDeadlineMode::NONE;
   } else if (ShouldBlockDeadlineIndefinitely()) {
+    // We do not want to wait for a deadline because we're waiting for full
+    // pipeline to be flushed for headless.
     return BeginImplFrameDeadlineMode::BLOCKED;
   } else if (ShouldTriggerBeginImplFrameDeadlineImmediately()) {
+    // We are ready to draw a new active tree immediately because there's no
+    // commit expected or we're prioritizing active tree latency.
     return BeginImplFrameDeadlineMode::IMMEDIATE;
   } else if (needs_redraw_) {
     // We have an animation or fast input path on the impl thread that wants
@@ -1089,7 +1095,7 @@ SchedulerStateMachine::CurrentBeginImplFrameDeadlineMode() const {
     return BeginImplFrameDeadlineMode::REGULAR;
   } else {
     // The impl thread doesn't have anything it wants to draw and we are just
-    // waiting for a new active tree. In short we are blocked.
+    // waiting for a new active tree.
     return BeginImplFrameDeadlineMode::LATE;
   }
 }

cc/scheduler/scheduler_state_machine.h

@@ -69,12 +69,18 @@ class CC_EXPORT SchedulerStateMachine {
   };
   static const char* BeginImplFrameStateToString(BeginImplFrameState state);
 
+  // The scheduler uses a deadline to wait for main thread updates before
+  // submitting a compositor frame. BeginImplFrameDeadlineMode specifies when
+  // the deadline should run.
   enum class BeginImplFrameDeadlineMode {
-    NONE,
-    IMMEDIATE,
-    REGULAR,
-    LATE,
-    BLOCKED,
+    NONE,  // No deadline should be scheduled e.g. for synchronous compositor.
+    IMMEDIATE,  // Deadline should be scheduled to run immediately.
+    REGULAR,  // Deadline should be scheduled to run at the deadline provided by
+              // in the BeginFrameArgs.
+    LATE,  // Deadline should be scheduled run when the next frame is expected
+           // to arrive.
+    BLOCKED,  // Deadline should be blocked indefinitely until the next frame
+              // arrives.
   };
   static const char* BeginImplFrameDeadlineModeToString(
       BeginImplFrameDeadlineMode mode);
@@ -168,6 +174,8 @@ class CC_EXPORT SchedulerStateMachine {
   BeginImplFrameState begin_impl_frame_state() const {
     return begin_impl_frame_state_;
   }
+
+  // Returns BeginImplFrameDeadlineMode computed based on current state.
   BeginImplFrameDeadlineMode CurrentBeginImplFrameDeadlineMode() const;
 
   // If the main thread didn't manage to produce a new frame in time for the
@@ -320,13 +328,20 @@ class CC_EXPORT SchedulerStateMachine {
   bool BeginFrameNeededForVideo() const;
   bool ProactiveBeginFrameWanted() const;
 
+  // Indicates if we should post the deadline to draw immediately. This is true
+  // when we aren't expecting a commit or activation, or we're prioritizing
+  // active tree draw (see ImplLatencyTakesPriority()).
+  bool ShouldTriggerBeginImplFrameDeadlineImmediately() const;
+
+  // Indicates if we shouldn't schedule a deadline. Used to defer drawing until
+  // the entire pipeline is flushed and active tree is ready to draw for
+  // headless.
+  bool ShouldBlockDeadlineIndefinitely() const;
+
   bool ShouldPerformImplSideInvalidation() const;
   bool CouldCreatePendingTree() const;
   bool ShouldDeferInvalidatingForMainFrame() const;
 
-  bool ShouldTriggerBeginImplFrameDeadlineImmediately() const;
-  bool ShouldBlockDeadlineIndefinitely() const;
-
   bool ShouldAbortCurrentFrame() const;
 
   bool ShouldBeginLayerTreeFrameSinkCreation() const;

cc/scheduler/scheduler_unittest.cc

@@ -511,6 +511,9 @@ class SchedulerTest : public testing::Test {
     if (scheduler_->begin_frame_source() ==
         fake_external_begin_frame_source_.get()) {
       EXPECT_TRUE(scheduler_->begin_frames_expected());
+      // Run the deadline first if we're inside the previous frame.
+      if (client_->IsInsideBeginImplFrame())
+        task_runner_->RunPendingTasks();
       SendNextBeginFrame(animate_only);
     } else {
       task_runner_->RunTasksWhile(client_->FrameHasNotAdvancedCallback());
@@ -3678,10 +3681,11 @@ TEST_F(SchedulerTest, BeginFrameAckForBeginFrameBeforeLastDeadline) {
   client_->Reset();
 
   // Send the next BeginFrame before the previous one's deadline was executed.
-  // This should trigger the previous BeginFrame's deadline synchronously,
-  // during which tiles will be prepared. As a result of that, no further
-  // BeginFrames will be needed, and the new BeginFrame should be dropped.
+  // This should post the previous BeginFrame's deadline, during which tiles
+  // will be prepared. As a result of that, no further BeginFrames will be
+  // needed, and the new BeginFrame should be dropped.
   viz::BeginFrameArgs args = SendNextBeginFrame();
+  task_runner_->RunPendingTasks();  // Run posted deadline.
   EXPECT_ACTIONS("ScheduledActionPrepareTiles", "RemoveObserver(this)");
   EXPECT_FALSE(client_->IsInsideBeginImplFrame());
   EXPECT_FALSE(scheduler_->begin_frames_expected());
@@ -3748,6 +3752,7 @@ TEST_F(SchedulerTest, BeginFrameAckForLateMissedBeginFrame) {
   task_runner_->AdvanceMockTickClock(viz::BeginFrameArgs::DefaultInterval());
   EXPECT_GT(task_runner_->NowTicks(), args.deadline);
   fake_external_begin_frame_source_->TestOnBeginFrame(args);
+  task_runner_->RunPendingTasks();
 
   EXPECT_NO_ACTION();
   EXPECT_FALSE(client_->IsInsideBeginImplFrame());