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 13 matching lines @@
       control_task_runner_(
           task_queue_manager_->TaskRunnerForQueue(CONTROL_TASK_QUEUE)),
       control_task_after_wakeup_runner_(task_queue_manager_->TaskRunnerForQueue(
           CONTROL_TASK_AFTER_WAKEUP_QUEUE)),
       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)),
+      pending_non_delayed_update_policy_(false),
       current_policy_(Policy::NORMAL),
       last_input_type_(blink::WebInputEvent::Undefined),
       input_stream_state_(InputStreamState::INACTIVE),
       policy_may_need_update_(&incoming_signals_lock_),
       weak_factory_(this) {
   weak_renderer_scheduler_ptr_ = weak_factory_.GetWeakPtr();
   update_policy_closure_ = base::Bind(&RendererSchedulerImpl::UpdatePolicy,
-                                      weak_renderer_scheduler_ptr_);
+                                      weak_renderer_scheduler_ptr_, true);
   end_idle_period_closure_.Reset(base::Bind(
       &RendererSchedulerImpl::EndIdlePeriod, weak_renderer_scheduler_ptr_));
+  delayed_update_policy_closure_.Reset(
+      base::Bind(&RendererSchedulerImpl::UpdatePolicy,
+                 weak_renderer_scheduler_ptr_, false));
   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);
 
   renderer_task_queue_selector_->SetQueuePriority(
@@ skipping 125 matching lines @@
     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());
+    PostUpdatePolicyOnControlRunnerLocked(base::TimeDelta());
   }
   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;
 }
@@ skipping 57 matching lines @@
 }
 
 RendererSchedulerImpl::Policy RendererSchedulerImpl::SchedulerPolicy() const {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   return current_policy_;
 }
 
 void RendererSchedulerImpl::MaybeUpdatePolicy() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (policy_may_need_update_.IsSet()) {
-    UpdatePolicy();
+    UpdatePolicy(false);
   }
 }
 
-void RendererSchedulerImpl::PostUpdatePolicyOnControlRunner(
+void RendererSchedulerImpl::PostUpdatePolicyOnControlRunnerLocked(
     base::TimeDelta delay) {
-  control_task_runner_->PostDelayedTask(
-      FROM_HERE, update_policy_closure_, delay);
+  incoming_signals_lock_.AssertAcquired();
+  if (delay > base::TimeDelta()) {
+    // Only allow one pending delayed UpdatePolicy task.
+    delayed_update_policy_closure_.Cancel();
+    control_task_runner_->PostDelayedTask(
+        FROM_HERE, delayed_update_policy_closure_.callback(), delay);
+  } else {
+    // Only allow one pending non-delayed UpdatePolicy task.
+    if (!pending_non_delayed_update_policy_) {
+      pending_non_delayed_update_policy_ = true;
+      control_task_runner_->PostDelayedTask(FROM_HERE, update_policy_closure_,
+                                            base::TimeDelta());
+    }
+  }
 }
 
-void RendererSchedulerImpl::UpdatePolicy() {
+void RendererSchedulerImpl::UpdatePolicy(
+    bool clear_pending_non_delayed_update_policy) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (!task_queue_manager_)
     return;
 
   base::AutoLock lock(incoming_signals_lock_);
   base::TimeTicks now;
   policy_may_need_update_.SetWhileLocked(false);
+  if (clear_pending_non_delayed_update_policy)
+    pending_non_delayed_update_policy_ = false;

[rmcilroy, 2015/03/10 16:08:39]

I don't think the pending_non_delayed_update_policy_ changes are really
required. I don't think we have a big issue with multiple non-delayed
UpdatePolicy tasks (they typically check other fields and only repost if they
really want to update the policy). Maybe we could just make
PostUpdatePolicyOnControlRunner not take a delay (only post now), and have the
UpdatePolicy function always cancel any pending delayed updatepolicy tasks and
repost a new delayed one.  I think this would end up being a fair bit simpler to
understand, WDYT?

[Sami, 2015/03/10 16:19:38]

Yeah, I'm all for simplicity too. I was thinking we could just keep track of
when the next policy update is due to run and act accordingly here, but I think
that's equivalent to what you are suggesting.

[alex clarke (OOO till 29th), 2015/03/10 16:21:41]

Currently they only get sent when the input_stream_state_ changes (i.e. you're
right they're not a problem at the moment). 

Certinly your suggestion would simplify things a bit, however we've had a number
of bugs when DoWork and now UpdatePolicy where posted more frequently than
anticipated. I would argue that in light of that defensive programming is a good
idea.

 
   base::TimeDelta new_policy_duration;
   Policy new_policy = ComputeNewPolicy(&new_policy_duration);
   if (new_policy_duration > base::TimeDelta())
-    PostUpdatePolicyOnControlRunner(new_policy_duration);
+    PostUpdatePolicyOnControlRunnerLocked(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