Prevent redundant DoWorks due to canceled delayed tasks

To achieve this we make a few changes:

1. We only register the next wakeup with the TimeDomain, rather than all
of them.
2. MoveReadyDelayedTasksToDelayedWorkQueue now registers the next
wakeup (if any).  Since it removes all canceled delayed tasks from the
front of the priority queue this has the effect of not scheduling
wakeups for cancelled tasks.
3. Tweaking the TaskQueueManager level delayed DoWork de-duplication
logic to only post a delayed DoWork if the task is meant to run before
any previously registered delayed DoWorks.

BUG=638542, 605718

Review-Url: https://codereview.chromium.org/2320403003
Cr-Commit-Position: refs/heads/master@{#418556}

third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc

@@ -217,11 +217,25 @@ void TaskQueueImpl::PushOntoDelayedIncomingQueueFromMainThread(
     Task pending_task, base::TimeTicks now) {
   main_thread_only().task_queue_manager->DidQueueTask(pending_task);
 
-  // Schedule a later call to MoveReadyDelayedTasksToDelayedWorkQueue.
   base::TimeTicks delayed_run_time = pending_task.delayed_run_time;
+  bool queue_was_empty = main_thread_only().delayed_incoming_queue.empty();
+  base::TimeTicks previous_next_run_time;
+  if (!queue_was_empty) {
+    previous_next_run_time =
+        main_thread_only().delayed_incoming_queue.top().delayed_run_time;
+  }
   main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
-  main_thread_only().time_domain->ScheduleDelayedWork(this, delayed_run_time,
-                                                      now);
+
+  // Make sure the next wakeup (and only the next wakeup) is registered with the
+  // TimeDomain.
+  if (queue_was_empty || delayed_run_time < previous_next_run_time) {
+    main_thread_only().time_domain->ScheduleDelayedWork(this, delayed_run_time,
+                                                        now);
+    if (!queue_was_empty) {
+      main_thread_only().time_domain->CancelDelayedWork(this,
+                                                        previous_next_run_time);
+    }
+  }
   TraceQueueSize(false);
 }
 
@@ -240,11 +254,11 @@ void TaskQueueImpl::PushOntoDelayedIncomingQueueLocked(Task pending_task) {
 }
 
 void TaskQueueImpl::PushOntoImmediateIncomingQueueLocked(
-         const tracked_objects::Location& posted_from,
-         const base::Closure& task,
-         base::TimeTicks desired_run_time,
-         EnqueueOrder sequence_number,
-         bool nestable) {
+    const tracked_objects::Location& posted_from,
+    const base::Closure& task,
+    base::TimeTicks desired_run_time,
+    EnqueueOrder sequence_number,
+    bool nestable) {
   if (any_thread().immediate_incoming_queue.empty())
     any_thread().time_domain->RegisterAsUpdatableTaskQueue(this);
   // If the |immediate_incoming_queue| is empty we need a DoWork posted to make
@@ -269,17 +283,19 @@ void TaskQueueImpl::ScheduleDelayedWorkTask(Task pending_task) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   base::TimeTicks delayed_run_time = pending_task.delayed_run_time;
   base::TimeTicks time_domain_now = main_thread_only().time_domain->Now();
-  // Make sure |delayed_run_time| isn't in the past.
   if (delayed_run_time < time_domain_now) {
+    // If |delayed_run_time| is in the past then push it onto the work queue
+    // immediately. To ensure the right task ordering we need to temporarily
+    // push it onto the |delayed_incoming_queue|.
     delayed_run_time = time_domain_now;
     pending_task.delayed_run_time = time_domain_now;
     main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
     LazyNow lazy_now(time_domain_now);
-    MoveReadyDelayedTasksToDelayedWorkQueue(&lazy_now);
+    WakeUpForDelayedWork(&lazy_now);
   } else {
-    main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
-    main_thread_only().time_domain->ScheduleDelayedWork(
-        this, delayed_run_time, main_thread_only().time_domain->Now());
+    // If |delayed_run_time| is in the future we can queue it as normal.
+    PushOntoDelayedIncomingQueueFromMainThread(std::move(pending_task),
+                                               time_domain_now);
   }
   TraceQueueSize(false);
 }
@@ -334,7 +350,7 @@ bool TaskQueueImpl::HasPendingImmediateWork() const {
   return !any_thread().immediate_incoming_queue.empty();
 }
 
-void TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue(LazyNow* lazy_now) {
+void TaskQueueImpl::WakeUpForDelayedWork(LazyNow* lazy_now) {
   // Enqueue all delayed tasks that should be running now, skipping any that
   // have been canceled.
   while (!main_thread_only().delayed_incoming_queue.empty()) {
@@ -352,6 +368,13 @@ void TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue(LazyNow* lazy_now) {
     main_thread_only().delayed_work_queue->Push(std::move(task));
     main_thread_only().delayed_incoming_queue.pop();
   }
+
+  // Make sure the next wake up is scheduled.
+  if (!main_thread_only().delayed_incoming_queue.empty()) {
+    main_thread_only().time_domain->ScheduleDelayedWork(
+        this, main_thread_only().delayed_incoming_queue.top().delayed_run_time,
+        lazy_now->Now());
+  }
 }
 
 bool TaskQueueImpl::MaybeUpdateImmediateWorkQueues() {
@@ -452,6 +475,8 @@ void TaskQueueImpl::AsValueInto(base::trace_event::TracedValue* state) const {
     state->SetDouble("delay_to_next_task_ms",
                      delay_to_next_task.InMillisecondsF());
   }
+  if (main_thread_only().current_fence)
+    state->SetInteger("current_fence", main_thread_only().current_fence);
   if (verbose_tracing_enabled) {
     state->BeginArray("immediate_incoming_queue");
     QueueAsValueInto(any_thread().immediate_incoming_queue, state);

third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h

@@ -172,8 +172,9 @@ class BLINK_PLATFORM_EXPORT TaskQueueImpl final : public TaskQueue {
   }
 
   // Enqueues any delayed tasks which should be run now on the
-  // |delayed_work_queue|.  Must be called from the main thread.
-  void MoveReadyDelayedTasksToDelayedWorkQueue(LazyNow* lazy_now);
+  // |delayed_work_queue|. It also schedules the next wake up with the
+  // TimeDomain. Must be called from the main thread.
+  void WakeUpForDelayedWork(LazyNow* lazy_now);
 
  private:
   friend class WorkQueue;

third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc

@@ -190,10 +190,14 @@ void TaskQueueManager::MaybeScheduleDelayedWork(
     return;
   }
 
-  // De-duplicate DoWork posts.
+  // Only request the wake up if, either there are no wake ups scheduled, or if
+  // its sooner than any previously requested wake up.
   base::TimeTicks run_time = now + delay;
-  if (!main_thread_pending_wakeups_.insert(run_time).second)
+  if (!main_thread_pending_wakeups_.empty() &&
+      *main_thread_pending_wakeups_.begin() <= run_time) {
     return;
+  }
+  main_thread_pending_wakeups_.insert(run_time);
   delegate_->PostDelayedTask(
       from_here, base::Bind(&TaskQueueManager::DoWork,
                             weak_factory_.GetWeakPtr(), run_time, true),

third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc

@@ -118,6 +118,26 @@ class TaskQueueManagerTest : public testing::Test {
     manager_->UpdateWorkQueues(lazy_now);
   }
 
+  // Runs all immediate tasks until there is no more work to do and advances
+  // time if there is a pending delayed task. |per_run_time_callback| is called
+  // when the clock advances.
+  void RunUntilIdle(base::Closure per_run_time_callback) {
+    for (;;) {
+      // Advance time if we've run out of immediate work to do.
+      if (manager_->selector_.EnabledWorkQueuesEmpty()) {
+        base::TimeTicks run_time;
+        if (manager_->real_time_domain()->NextScheduledRunTime(&run_time)) {
+          now_src_->SetNowTicks(run_time);
+          per_run_time_callback.Run();
+        } else {
+          break;
+        }
+      }
+
+      test_task_runner_->RunPendingTasks();
+    }
+  }
+
   std::unique_ptr<base::MessageLoop> message_loop_;
   std::unique_ptr<base::SimpleTestTickClock> now_src_;
   scoped_refptr<TaskQueueManagerDelegateForTest> main_task_runner_;
@@ -1843,5 +1863,150 @@ TEST_F(TaskQueueManagerTestWithTracing, BlameContextAttribution) {
   EXPECT_EQ(2u, events.size());
 }
 
+class CancelableTask {
+ public:
+  CancelableTask() : weak_factory_(this) {}
+
+  void NopTask() {}
+
+  base::WeakPtrFactory<CancelableTask> weak_factory_;
+};
+
+TEST_F(TaskQueueManagerTest, NoWakeUpsForCanceledDelayedTasks) {
+  Initialize(1u);
+
+  base::TimeTicks start_time = manager_->delegate()->NowTicks();
+
+  CancelableTask task1;
+  CancelableTask task2;
+  CancelableTask task3;
+  CancelableTask task4;
+  base::TimeDelta delay1(base::TimeDelta::FromSeconds(5));
+  base::TimeDelta delay2(base::TimeDelta::FromSeconds(10));
+  base::TimeDelta delay3(base::TimeDelta::FromSeconds(15));
+  base::TimeDelta delay4(base::TimeDelta::FromSeconds(30));
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task1.weak_factory_.GetWeakPtr()),
+      delay1);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task2.weak_factory_.GetWeakPtr()),
+      delay2);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task3.weak_factory_.GetWeakPtr()),
+      delay3);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task4.weak_factory_.GetWeakPtr()),
+      delay4);
+
+  task2.weak_factory_.InvalidateWeakPtrs();
+  task3.weak_factory_.InvalidateWeakPtrs();
+
+  std::set<base::TimeTicks> run_times;
+
+  RunUntilIdle(base::Bind([](std::set<base::TimeTicks>* run_times,
+                             base::SimpleTestTickClock* clock) {
+    run_times->insert(clock->NowTicks());
+  }, &run_times, now_src_.get()));
+
+  EXPECT_THAT(run_times, ElementsAre(start_time + delay1, start_time + delay4));
+}
+
+TEST_F(TaskQueueManagerTest, NoWakeUpsForCanceledDelayedTasksReversePostOrder) {
+  Initialize(1u);
+
+  base::TimeTicks start_time = manager_->delegate()->NowTicks();
+
+  CancelableTask task1;
+  CancelableTask task2;
+  CancelableTask task3;
+  CancelableTask task4;
+  base::TimeDelta delay1(base::TimeDelta::FromSeconds(5));
+  base::TimeDelta delay2(base::TimeDelta::FromSeconds(10));
+  base::TimeDelta delay3(base::TimeDelta::FromSeconds(15));
+  base::TimeDelta delay4(base::TimeDelta::FromSeconds(30));
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task4.weak_factory_.GetWeakPtr()),
+      delay4);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task3.weak_factory_.GetWeakPtr()),
+      delay3);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task2.weak_factory_.GetWeakPtr()),
+      delay2);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task1.weak_factory_.GetWeakPtr()),
+      delay1);
+
+  task2.weak_factory_.InvalidateWeakPtrs();
+  task3.weak_factory_.InvalidateWeakPtrs();
+
+  std::set<base::TimeTicks> run_times;
+
+  RunUntilIdle(base::Bind([](std::set<base::TimeTicks>* run_times,
+                             base::SimpleTestTickClock* clock) {
+    run_times->insert(clock->NowTicks());
+  }, &run_times, now_src_.get()));
+
+  EXPECT_THAT(run_times, ElementsAre(start_time + delay1, start_time + delay4));
+}
+
+TEST_F(TaskQueueManagerTest, TimeDomainWakeUpOnlyCancelledIfAllUsesCancelled) {
+  Initialize(1u);
+
+  base::TimeTicks start_time = manager_->delegate()->NowTicks();
+
+  CancelableTask task1;
+  CancelableTask task2;
+  CancelableTask task3;
+  CancelableTask task4;
+  base::TimeDelta delay1(base::TimeDelta::FromSeconds(5));
+  base::TimeDelta delay2(base::TimeDelta::FromSeconds(10));
+  base::TimeDelta delay3(base::TimeDelta::FromSeconds(15));
+  base::TimeDelta delay4(base::TimeDelta::FromSeconds(30));
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task1.weak_factory_.GetWeakPtr()),
+      delay1);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task2.weak_factory_.GetWeakPtr()),
+      delay2);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task3.weak_factory_.GetWeakPtr()),
+      delay3);
+  runners_[0]->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CancelableTask::NopTask, task4.weak_factory_.GetWeakPtr()),
+      delay4);
+
+  // Post a non-canceled task with |delay3|. So we should still get a wakeup at
+  // |delay3| even though we cancel |task3|.
+  runners_[0]->PostDelayedTask(
+      FROM_HERE, base::Bind(&CancelableTask::NopTask, base::Unretained(&task3)),
+      delay3);
+
+  task2.weak_factory_.InvalidateWeakPtrs();
+  task3.weak_factory_.InvalidateWeakPtrs();
+
+  std::set<base::TimeTicks> run_times;
+
+  RunUntilIdle(base::Bind([](std::set<base::TimeTicks>* run_times,
+                             base::SimpleTestTickClock* clock) {
+    run_times->insert(clock->NowTicks());
+  }, &run_times, now_src_.get()));
+
+  EXPECT_THAT(run_times, ElementsAre(start_time + delay1, start_time + delay3,
+                                     start_time + delay4));
+}
+
 }  // namespace scheduler
 }  // namespace blink

third_party/WebKit/Source/platform/scheduler/base/time_domain.cc

@@ -198,7 +198,7 @@ void TimeDomain::WakeupReadyDelayedQueues(LazyNow* lazy_now) {
     // in which EnqueueTaskLocks is called is respected when choosing which
     // queue to execute a task from.
     if (dedup_set.insert(next_wakeup->second).second) {
-      next_wakeup->second->MoveReadyDelayedTasksToDelayedWorkQueue(lazy_now);
+      next_wakeup->second->WakeUpForDelayedWork(lazy_now);
     }
     delayed_wakeup_multimap_.erase(next_wakeup);
   }

third_party/WebKit/Source/platform/scheduler/base/time_domain.h

@@ -24,13 +24,17 @@ class TaskQueueImpl;
 class TaskQueueManager;
 class TaskQueueManagerDelegate;
 
-// The TimeDomain's job is to keep track of moments when delayed tasks have been
-// scheduled to fire and to notify their TaskQueues via UpdateDelayedWorkQueue.
+// The TimeDomain's job is to wake task queues up when their next delayed tasks
+// are due to fire. TaskQueues request a wake up via ScheduleDelayedWork, when
+// the WakeUp is due the TimeDomain calls TaskQueue::WakeUpForDelayedWork which
+// schedules the next non-canceled wakeup.
 //
-// The time domain keeps track of the next wakeup required to pump delayed tasks
-// and issues |RequestWakeup| calls to the subclass as needed.  Where possible
-// it tried to de-dupe these wakeups. Ideally it would be possible to cancel
-// them, but that's not currently supported by the base message loop.
+// To prevent spurious wake-ups for canceled tasks the TaskQueue should only
+// have a single wake up registered with its TimeDomain.  If should call
+// CancelDelayedWork as needed to ensure this. The TimeDomain communicates with
+// the TaskQueueManager to actually schedule the wake-ups on the underlying
+// base::MessageLoop. Various levels of de-duping are employed to prevent
+// unnecessary posting of TaskQueueManager::DoWork.
 //
 // The clock itself is provided by subclasses of the TimeDomain and it may be
 // the real wall clock or a synthetic (virtual) time base.
@@ -89,13 +93,13 @@ class BLINK_PLATFORM_EXPORT TimeDomain {
   // UpdateWorkQueue on.
   void RegisterAsUpdatableTaskQueue(internal::TaskQueueImpl* queue);
 
-  // Schedules a call to TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue
+  // Schedules a call to TaskQueueImpl::WakeUpForDelayedWork
   // when this TimeDomain reaches |delayed_run_time|.
   void ScheduleDelayedWork(internal::TaskQueueImpl* queue,
                            base::TimeTicks delayed_run_time,
                            base::TimeTicks now);
 
-  // Cancels a call to TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue
+  // Cancels a call to TaskQueueImpl::WakeUpForDelayedWork
   // previously requested with ScheduleDelayedWork.  Note this only works if
   // delayed_run_time is _not_ the next scheduled run time.
   void CancelDelayedWork(internal::TaskQueueImpl* queue,