Prevent multiple pending UpdatePolicy tasks.

content/renderer/scheduler/renderer_scheduler_impl.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_IMPL_H_
 #define CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_IMPL_H_
 
 #include "base/atomicops.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
@@ skipping 35 matching lines @@
   void AddTaskObserver(base::MessageLoop::TaskObserver* task_observer) override;
   void RemoveTaskObserver(
       base::MessageLoop::TaskObserver* task_observer) override;
   void Shutdown() override;
 
   void SetTimeSourceForTesting(scoped_refptr<cc::TestNowSource> time_source);
   void SetWorkBatchSizeForTesting(size_t work_batch_size);
 
  private:
   friend class RendererSchedulerImplTest;
+  friend class MockRendererSchedulerImpl;
 
   // Keep RendererSchedulerImpl::TaskQueueIdToString in sync with this enum.
   enum QueueId {
     DEFAULT_TASK_QUEUE,
     COMPOSITOR_TASK_QUEUE,
     LOADING_TASK_QUEUE,
     IDLE_TASK_QUEUE,
     CONTROL_TASK_QUEUE,
     CONTROL_TASK_AFTER_WAKEUP_QUEUE,
     // Must be the last entry.
@@ skipping 44 matching lines @@
 
   // The time we should stay in a priority-escalated mode after an input event.
   static const int kPriorityEscalationAfterInputMillis = 100;
 
   // IdleTaskDeadlineSupplier Implementation:
   void CurrentIdleTaskDeadlineCallback(base::TimeTicks* deadline_out) const;
 
   // Returns the current scheduler policy. Must be called from the main thread.
   Policy SchedulerPolicy() const;
 
-  // Posts a call to UpdatePolicy on the control runner to be run after |delay|
-  void PostUpdatePolicyOnControlRunner(base::TimeDelta delay);
+  // Schedules an immediate PolicyUpdate, if there isn't one already pending and
+  // sets |policy_may_need_update_|. Note |incoming_signals_lock_| must be
+  // locked.
+  void ScheduleUrgentPolicyUpdate(const tracked_objects::Location& from_here);
+
+  // Schedules a delayed PolicyUpdate, if there isn't one already pending.
+  // Note |incoming_signals_lock_| must be locked.
+  void ScheduleDelayedPolicyUpdate(const tracked_objects::Location& from_here,
+                                   base::TimeDelta delay);
 
   // Update the policy if a new signal has arrived. Must be called from the main
   // thread.
   void MaybeUpdatePolicy();
 
-  // Updates the scheduler policy. Must be called from the main thread.
+  // Locks |incoming_signals_lock_| and calls UpdatePolicyLocked.
   void UpdatePolicy();

[Sami, 2015/03/11 16:58:25]

Maybe move this next to UpdatePolicyLocked()? The difference between the
functions is pretty obvious from the naming so we don't really need to explain
what this does IMO.

[alex clarke (OOO till 29th), 2015/03/12 13:41:32]

Done.

 
+  // Locks |incoming_signals_lock_|, clears |pending_delayed_policy_update_| and
+  // calls UpdatePolicyLocked.
+  void DelayedPolicyUpdateTask();

[Sami, 2015/03/11 16:58:25]

nit: Name this with a verb instead a noun to match the other functions here. The
comment might also be a little unnecessary if this is called something like
PerformDelayedPolicyUpdate()

Should we also allow this function only on the main thread, since we really only
expect it to be triggered from within the policy update (maybe we could assert
for that too)?

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

This function is gone.

+
+  // Updates the scheduler policy. Must be called from the main thread and
+  // |incoming_signals_lock_| must be locked.
+  virtual void UpdatePolicyLocked();
+
   // Helper for computing the new policy. |new_policy_duration| will be filled
   // with the amount of time after which the policy should be updated again. If
   // the duration is zero, a new policy update will not be scheduled. Must be
   // called with |incoming_signals_lock_| held.
   Policy ComputeNewPolicy(base::TimeDelta* new_policy_duration);
 
   // An input event of some sort happened, the policy may need updating.
   void UpdateForInputEvent(blink::WebInputEvent::Type type);
 
   // Called when a previously queued input event was processed.
@@ skipping 11 matching lines @@
   scoped_ptr<RendererTaskQueueSelector> renderer_task_queue_selector_;
   scoped_ptr<TaskQueueManager> task_queue_manager_;
   scoped_refptr<base::SingleThreadTaskRunner> control_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> control_task_after_wakeup_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> default_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> loading_task_runner_;
   scoped_refptr<SingleThreadIdleTaskRunner> idle_task_runner_;
 
   base::Closure update_policy_closure_;
+  base::Closure delayed_update_policy_closure_;

[rmcilroy, 2015/03/11 17:15:07]

I really think this should be a CancelableClosureHolder - the we wouldn't need
to worry about the pending_delayed_policy_update_ and wouldn't need to add the
DCHECK Sami is suggesting (since we would always be posting a update for the
soonest policy update).  Is there a reason we can't do this?

[Sami, 2015/03/11 17:18:47]

I think a good reason for avoiding cancellable closures is that they give you a
false sense of safety: the underlying task will still run even if it's
cancelled, adding invisible overhead. I think this is critical enough for
battery life that we should try an be explicit about it.

   CancelableClosureHolder end_idle_period_closure_;
 
   // Don't access current_policy_ directly, instead use SchedulerPolicy().
   Policy current_policy_;
 
   base::TimeTicks estimated_next_frame_begin_;
 
   // The incoming_signals_lock_ mutex protects access to all variables in the
   // (contiguous) block below.
   base::Lock incoming_signals_lock_;
   base::TimeTicks last_input_receipt_time_on_compositor_;
   base::TimeTicks last_input_process_time_on_main_;
   blink::WebInputEvent::Type last_input_type_;
   InputStreamState input_stream_state_;
   PollableNeedsUpdateFlag policy_may_need_update_;
+  bool pending_delayed_policy_update_;
 
   scoped_refptr<cc::TestNowSource> time_source_;
 
   base::WeakPtr<RendererSchedulerImpl> weak_renderer_scheduler_ptr_;
   base::WeakPtrFactory<RendererSchedulerImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RendererSchedulerImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_IMPL_H_