Prevent multiple pending UpdatePolicy tasks.

content/renderer/scheduler/renderer_scheduler_impl.cc

 // 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.
 
 #include "content/renderer/scheduler/renderer_scheduler_impl.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
@@ skipping 17 matching lines @@
       default_task_runner_(
           task_queue_manager_->TaskRunnerForQueue(DEFAULT_TASK_QUEUE)),
       compositor_task_runner_(
           task_queue_manager_->TaskRunnerForQueue(COMPOSITOR_TASK_QUEUE)),
       loading_task_runner_(
           task_queue_manager_->TaskRunnerForQueue(LOADING_TASK_QUEUE)),
       current_policy_(Policy::NORMAL),
       last_input_type_(blink::WebInputEvent::Undefined),
       input_stream_state_(InputStreamState::INACTIVE),
       policy_may_need_update_(&incoming_signals_lock_),
+      pending_delayed_policy_update_(false),
       weak_factory_(this) {
   weak_renderer_scheduler_ptr_ = weak_factory_.GetWeakPtr();
   update_policy_closure_ = base::Bind(&RendererSchedulerImpl::UpdatePolicy,
                                       weak_renderer_scheduler_ptr_);
+  delayed_update_policy_closure_ =
+      base::Bind(&RendererSchedulerImpl::DelayedPolicyUpdateTask,
+                 weak_renderer_scheduler_ptr_);
   end_idle_period_closure_.Reset(base::Bind(
       &RendererSchedulerImpl::EndIdlePeriod, weak_renderer_scheduler_ptr_));
   idle_task_runner_ = make_scoped_refptr(new SingleThreadIdleTaskRunner(
       task_queue_manager_->TaskRunnerForQueue(IDLE_TASK_QUEUE),
       control_task_after_wakeup_runner_,
       base::Bind(&RendererSchedulerImpl::CurrentIdleTaskDeadlineCallback,
                  weak_renderer_scheduler_ptr_)));
 
   renderer_task_queue_selector_->SetQueuePriority(
       CONTROL_TASK_QUEUE, RendererTaskQueueSelector::CONTROL_PRIORITY);
@@ skipping 126 matching lines @@
 void RendererSchedulerImpl::UpdateForInputEvent(
     blink::WebInputEvent::Type type) {
   base::AutoLock lock(incoming_signals_lock_);
 
   InputStreamState new_input_stream_state =
       ComputeNewInputStreamState(input_stream_state_, type, last_input_type_);
 
   if (input_stream_state_ != new_input_stream_state) {
     // Update scheduler policy if we should start a new policy mode.
     input_stream_state_ = new_input_stream_state;
-    policy_may_need_update_.SetWhileLocked(true);
-    PostUpdatePolicyOnControlRunner(base::TimeDelta());
+    ScheduleUrgentPolicyUpdate(FROM_HERE);
   }
   last_input_receipt_time_on_compositor_ = Now();
   // Clear the last known input processing time so that we know an input event
   // is still queued up. This timestamp will be updated the next time the
   // compositor commits or becomes quiescent. Note that this function is always
   // called before the input event is processed either on the compositor or
   // main threads.
   last_input_process_time_on_main_ = base::TimeTicks();
   last_input_type_ = type;
 }
 
 void RendererSchedulerImpl::DidProcessInputEvent(
     base::TimeTicks begin_frame_time) {
   base::AutoLock lock(incoming_signals_lock_);
   if (input_stream_state_ == InputStreamState::INACTIVE)
     return;
   // Avoid marking input that arrived after the BeginFrame as processed.
   if (!begin_frame_time.is_null() &&
       begin_frame_time < last_input_receipt_time_on_compositor_)
     return;
   last_input_process_time_on_main_ = Now();
-  policy_may_need_update_.SetWhileLocked(true);
+  UpdatePolicyLocked();

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

Could you add a DCHECK that this function is only called on the main thread?

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

Done.

 }
 
 bool RendererSchedulerImpl::IsHighPriorityWorkAnticipated() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (!task_queue_manager_)
     return false;
 
   MaybeUpdatePolicy();
   // The touchstart and compositor policies indicate a strong likelihood of
   // high-priority work in the near future.
@@ skipping 39 matching lines @@
   return current_policy_;
 }
 
 void RendererSchedulerImpl::MaybeUpdatePolicy() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (policy_may_need_update_.IsSet()) {
     UpdatePolicy();
   }
 }
 
-void RendererSchedulerImpl::PostUpdatePolicyOnControlRunner(
+void RendererSchedulerImpl::ScheduleUrgentPolicyUpdate(

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

nit on naming - how about PostUpdatePolicyToMainThread or similar to identify
that this should only really be used when you are not guaranteed to be on the
main thread?

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

I like the OnMainThread, but it doesn't always post so I prefer
ScheduleUrgentPolicyUpdateOnMainThread.

+    const tracked_objects::Location& from_here) {
+  incoming_signals_lock_.AssertAcquired();

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

Maybe add a DCHECK() that this is never called on the main thread, because there
we should just do the update immediately.

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

It feels like this would make it easy to introduce bugs where you accidentally
prevent posting a more recent policy update if there was already a pending
update policy task with a long deadline - I don't think there is any code in
UpdatePolicy which would prevent us from doing this.  As with my comment above,
I think it would be safer to use a CancellableCallback to ensure that we always
do the update at the most up-to-date time.

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

In principle this is a good idea, but it makes testing tricky. We've discussed a
bit of a testing refactor, lets fix this at the same time.

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

The de-duping for ScheduleDelayedPolicyUpdate is separate from
ScheduleUrgentPolicyUpdate.   So we don't have a problem if there's a
UpdatePolicy posted for 100ms time but the compositor thread wants to post an
UpdatePolicy with no delay.

Hopefully the DeadlineTaskRunner addresses your other concerns.

+  if (!policy_may_need_update_.IsSet()) {
+    policy_may_need_update_.SetWhileLocked(true);
+    control_task_runner_->PostTask(from_here, update_policy_closure_);
+  }
+}
+
+void RendererSchedulerImpl::ScheduleDelayedPolicyUpdate(
+    const tracked_objects::Location& from_here,
     base::TimeDelta delay) {
-  control_task_runner_->PostDelayedTask(
-      FROM_HERE, update_policy_closure_, delay);
+  incoming_signals_lock_.AssertAcquired();

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

I think this is probably flexible enough for now, but to provide some insurance
in the future, could we change pending_delayed_policy_update_ to a timestamp and
DCHECK here that we're not trying to schedule a policy update sooner than that?

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

I ended up adding a DeadlineTaskRunner that should be future proof.

+  if (!pending_delayed_policy_update_) {
+    pending_delayed_policy_update_ = true;
+    control_task_runner_->PostDelayedTask(
+        from_here, delayed_update_policy_closure_, delay);
+  }
+}
+
+void RendererSchedulerImpl::DelayedPolicyUpdateTask() {
+  base::AutoLock lock(incoming_signals_lock_);
+  pending_delayed_policy_update_ = false;
+  UpdatePolicyLocked();
 }
 
 void RendererSchedulerImpl::UpdatePolicy() {
+  base::AutoLock lock(incoming_signals_lock_);
+  UpdatePolicyLocked();
+}
+
+void RendererSchedulerImpl::UpdatePolicyLocked() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
+  incoming_signals_lock_.AssertAcquired();
   if (!task_queue_manager_)
     return;
 
-  base::AutoLock lock(incoming_signals_lock_);
   base::TimeTicks now;
   policy_may_need_update_.SetWhileLocked(false);
 
   base::TimeDelta new_policy_duration;
   Policy new_policy = ComputeNewPolicy(&new_policy_duration);
   if (new_policy_duration > base::TimeDelta())
-    PostUpdatePolicyOnControlRunner(new_policy_duration);
+    ScheduleDelayedPolicyUpdate(FROM_HERE, new_policy_duration);
 
   if (new_policy == current_policy_)
     return;
 
   switch (new_policy) {
     case Policy::COMPOSITOR_PRIORITY:
       renderer_task_queue_selector_->SetQueuePriority(
           COMPOSITOR_TASK_QUEUE, RendererTaskQueueSelector::HIGH_PRIORITY);
       // TODO(scheduler-dev): Add a task priority between HIGH and BEST_EFFORT
       // that still has some guarantee of running.
@@ skipping 255 matching lines @@
 }
 
 void RendererSchedulerImpl::RemoveTaskObserver(
     base::MessageLoop::TaskObserver* task_observer) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (task_queue_manager_)
     task_queue_manager_->RemoveTaskObserver(task_observer);
 }
 
 }  // namespace content