[content]: Ensure TaskQueueManager AFTER_WAKEUP autopump policy wakes only on newer tasks.

content/renderer/scheduler/task_queue_manager.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/task_queue_manager.h"
 
 #include <queue>
 
 #include "base/bind.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 25 matching lines @@
                                   base::TimeDelta delay) override {
     return PostDelayedTaskImpl(from_here, task, delay, TaskType::NON_NESTABLE);
   }
 
   bool IsQueueEmpty() const;
 
   void SetPumpPolicy(TaskQueueManager::PumpPolicy pump_policy);
   void PumpQueue();
 
   bool UpdateWorkQueue(base::TimeTicks* next_pending_delayed_task,
-                       TaskQueueManager::WorkQueueUpdateEventType event_type);
+                       const base::PendingTask& previous_task);
   base::PendingTask TakeTaskFromWorkQueue();
 
   void WillDeleteTaskQueueManager();
 
   base::TaskQueue& work_queue() { return work_queue_; }
 
   void set_name(const char* name) { name_ = name; }
 
   void AsValueInto(base::trace_event::TracedValue* state) const;
 
  private:
   enum class TaskType {
     NORMAL,
     NON_NESTABLE,
   };
 
   ~TaskQueue() override;
 
   bool PostDelayedTaskImpl(const tracked_objects::Location& from_here,
                            const base::Closure& task,
                            base::TimeDelta delay,
                            TaskType task_type);
 
   // Adds a task at the end of the incoming task queue and schedules a call to
   // TaskQueueManager::DoWork() if the incoming queue was empty and automatic
   // pumping is enabled. Can be called on an arbitrary thread.
   void EnqueueTask(const base::PendingTask& pending_task);
 
   void PumpQueueLocked();
-  bool ShouldAutoPumpQueueLocked(
-      TaskQueueManager::WorkQueueUpdateEventType event_type);
+  bool TaskIsOlderThanQueuedTasks(const base::PendingTask& task);
+  bool ShouldAutoPumpQueueLocked(const base::PendingTask& previous_task);
   void EnqueueTaskLocked(const base::PendingTask& pending_task);
 
   void TraceQueueSize(bool is_locked) const;
   static const char* PumpPolicyToString(
       TaskQueueManager::PumpPolicy pump_policy);
   static void QueueAsValueInto(const base::TaskQueue& queue,
                                base::trace_event::TracedValue* state);
   static void TaskAsValueInto(const base::PendingTask& task,
                               base::trace_event::TracedValue* state);
 
@@ skipping 58 matching lines @@
 bool TaskQueue::IsQueueEmpty() const {
   if (!work_queue_.empty())
     return false;
 
   {
     base::AutoLock lock(lock_);
     return incoming_queue_.empty();
   }
 }
 
+bool TaskQueue::TaskIsOlderThanQueuedTasks(const base::PendingTask& task) {
+  lock_.AssertAcquired();
+  if (incoming_queue_.empty())
+    return false;
+
+  // Always return true if passed a NULL task.
+  if (task.sequence_num == 0)
+    return true;

[rmcilroy, 2015/03/02 11:44:13]

I realized that the previous patch set might incorrectly schedule the queue to
be pumped when the sequence number wrapped to negative, and we were passing a
NULL task (i.e., the NULL task passed to UpdateWorkQueues before running the
first task), so I've added this check.  This might cause the queue not to be
pumped for the 1 in 2^32 times the sequence number actually is validly '0', but
I think this is OK - if you disagree I can rework it to be more explicit about
when the task is actually a null task.

[Sami, 2015/03/02 12:28:56]

Note that the sequence number starts at zero so the first task will always be
considered a null task. Not pumping in this case would be fine if AFTER_WAKEUP
work is always considered to be optional work, but I don't think we should make
that assumption at the TQM level.

I guess if you just made |task| a possibly null pointer then this would work
correctly in all cases. WDYT?

[rmcilroy, 2015/03/02 15:34:51]

Yeah that would probably be better - done.

+
+  base::PendingTask oldest_queued_task = incoming_queue_.front();
+
+  // Note: the comparison is correct due to the fact that the PendingTask
+  // operator inverts its comparison operation in order to work well in a heap
+  // based priority queue.
+  return oldest_queued_task < task;

[Sami, 2015/03/02 12:28:56]

We should really go and swap that operator around :) I was trying to decide if
it makes sense to look at the delayed_run_time here or not. If we ever wanted to
support delayed post-wakeup tasks, they delay should really be computed from the
time the wakeup happens and not the posting time. It would be probably simpler
to disallow them completely.

[rmcilroy, 2015/03/02 15:34:51]

This would break the use of priority queues - we should probably just add an
IsOlder method to pending_tasks though, WDYT?
The '<' operator in pending_tasks already looks at the delayed_run_time. Are you
saying we should avoid using it and should rely on just the sequence number?

[Sami, 2015/03/02 15:55:57]

We could just define the priority queue type with the correct ordering like
here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/s...
Right, '<' already looks at the time and I was just wondering out loud if it
should do that in this case. Maybe we just add a DCHECK somewhere that prevents
delayed work for after-wakeup queues. WDYT?

[rmcilroy, 2015/03/02 16:47:52]

Hmm, good point :). I'll do this as a followup change. 
I need to use delayed task for the after-wakeup queue for the long idle
implementation (see https://codereview.chromium.org/968073003/), but by the time
the task get's into the incoming queue it's delayed_run_time should already have
been set to null, so I added a DCHECK that this is always the case. 

+}
+
 bool TaskQueue::ShouldAutoPumpQueueLocked(
-    TaskQueueManager::WorkQueueUpdateEventType event_type) {
+    const base::PendingTask& previous_task) {
   lock_.AssertAcquired();
   if (pump_policy_ == TaskQueueManager::PumpPolicy::MANUAL)
     return false;
   if (pump_policy_ == TaskQueueManager::PumpPolicy::AFTER_WAKEUP &&
-      event_type != TaskQueueManager::WorkQueueUpdateEventType::AFTER_WAKEUP)
+      TaskIsOlderThanQueuedTasks(previous_task))
     return false;
   if (incoming_queue_.empty())
     return false;
   return true;
 }
 
 bool TaskQueue::UpdateWorkQueue(
     base::TimeTicks* next_pending_delayed_task,
-    TaskQueueManager::WorkQueueUpdateEventType event_type) {
+    const base::PendingTask& previous_task) {
   if (!work_queue_.empty())
     return true;
 
   {
     base::AutoLock lock(lock_);
     if (!delayed_task_run_times_.empty()) {
       *next_pending_delayed_task =
           std::min(*next_pending_delayed_task, delayed_task_run_times_.top());
     }
-    if (!ShouldAutoPumpQueueLocked(event_type))
+    if (!ShouldAutoPumpQueueLocked(previous_task))
       return false;
     work_queue_.Swap(&incoming_queue_);
     TraceQueueSize(true);
     return true;
   }
 }
 
 base::PendingTask TaskQueue::TakeTaskFromWorkQueue() {
   base::PendingTask pending_task = work_queue_.front();
   work_queue_.pop();
@@ skipping 184 matching lines @@
 }
 
 void TaskQueueManager::PumpQueue(size_t queue_index) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   internal::TaskQueue* queue = Queue(queue_index);
   queue->PumpQueue();
 }
 
 bool TaskQueueManager::UpdateWorkQueues(
     base::TimeTicks* next_pending_delayed_task,
-    WorkQueueUpdateEventType event_type) {
+    const base::PendingTask& previous_task) {
   // TODO(skyostil): This is not efficient when the number of queues grows very
   // large due to the number of locks taken. Consider optimizing when we get
   // there.
   DCHECK(main_thread_checker_.CalledOnValidThread());
   bool has_work = false;
   for (auto& queue : queues_) {
-    has_work |= queue->UpdateWorkQueue(next_pending_delayed_task, event_type);
+    has_work |= queue->UpdateWorkQueue(next_pending_delayed_task,
+                                       previous_task);
     if (!queue->work_queue().empty()) {
       // Currently we should not be getting tasks with delayed run times in any
       // of the work queues.
       DCHECK(queue->work_queue().front().delayed_run_time.is_null());
     }
   }
   return has_work;
 }
 
 void TaskQueueManager::MaybePostDoWorkOnMainRunner() {
@@ skipping 14 matching lines @@
 
 void TaskQueueManager::DoWork(bool posted_from_main_thread) {
   if (posted_from_main_thread) {
     pending_dowork_count_--;
     DCHECK_GE(pending_dowork_count_, 0);
   }
   DCHECK(main_thread_checker_.CalledOnValidThread());
 
   base::TimeTicks next_pending_delayed_task(
       base::TimeTicks::FromInternalValue(kMaxTimeTicks));
+  base::PendingTask previous_task((tracked_objects::Location()),
+                                  (base::Closure()));
 
-  if (!UpdateWorkQueues(&next_pending_delayed_task,
-                        WorkQueueUpdateEventType::BEFORE_WAKEUP))
+  if (!UpdateWorkQueues(&next_pending_delayed_task, previous_task))
     return;
 
-  base::PendingTask previous_task((tracked_objects::Location()),
-                                  (base::Closure()));
   for (int i = 0; i < work_batch_size_; i++) {
     // Interrupt the work batch if we should run the next delayed task.
     if (i > 0 && next_pending_delayed_task.ToInternalValue() != kMaxTimeTicks &&
         Now() >= next_pending_delayed_task)
       return;
 
     size_t queue_index;
     if (!SelectWorkQueueToService(&queue_index))
       return;
     // Note that this function won't post another call to DoWork if one is
     // already pending, so it is safe to call it in a loop.
     MaybePostDoWorkOnMainRunner();
     ProcessTaskFromWorkQueue(queue_index, i > 0, &previous_task);
 
-    if (!UpdateWorkQueues(&next_pending_delayed_task,
-                          WorkQueueUpdateEventType::AFTER_WAKEUP))
+    if (!UpdateWorkQueues(&next_pending_delayed_task, previous_task))
       return;
   }
 }
 
 bool TaskQueueManager::SelectWorkQueueToService(size_t* out_queue_index) {
   bool should_run = selector_->SelectWorkQueueToService(out_queue_index);
   TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(
       TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"), "TaskQueueManager", this,
       AsValueWithSelectorResult(should_run, *out_queue_index));
   return should_run;
@@ skipping 93 matching lines @@
   state->EndArray();
   state->BeginDictionary("selector");
   selector_->AsValueInto(state.get());
   state->EndDictionary();
   if (should_run)
     state->SetInteger("selected_queue", selected_queue);
   return state;
 }
 
 }  // namespace content