[scheduler] Do not activate high-resolution timer for low priority task queues.

With this CL, high-resolution timer is never activated for a delayed
task that lives in a task queue with a low priority. Since such a task
queue must already support late scheduling, it shouldn't care about
precise execution delays.

Bug: 967777
Change-Id: I6c8403075dc15368acaee74aef9c39b0b9b7c926
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470322
Commit-Queue: Chris Davis <chrdavis@microsoft.com>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818144}

base/task/sequence_manager/sequence_manager_impl_unittest.cc

@@ -4040,6 +4040,73 @@ TEST_P(SequenceManagerTest, HasPendingHighResolutionTasks) {
   EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
 }
 
+TEST_P(SequenceManagerTest, HasPendingHighResolutionTasksLowPriority) {
+  auto queue = CreateTaskQueue();
+  queue->SetQueuePriority(TaskQueue::QueuePriority::kLowPriority);
+  bool supports_high_res = false;
+#if defined(OS_WIN)
+  supports_high_res = true;
+#endif
+
+  // No task should be considered high resolution in a low priority queue.
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+  queue->task_runner()->PostTask(FROM_HERE, BindOnce(&NopTask));
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+  queue->task_runner()->PostDelayedTask(FROM_HERE, BindOnce(&NopTask),
+                                        TimeDelta::FromMilliseconds(100));
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+  queue->task_runner()->PostDelayedTask(FROM_HERE, BindOnce(&NopTask),
+                                        TimeDelta::FromMilliseconds(10));
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+
+  // Increasing queue priority should enable high resolution timer.
+  queue->SetQueuePriority(TaskQueue::QueuePriority::kNormalPriority);
+  EXPECT_EQ(sequence_manager()->HasPendingHighResolutionTasks(),
+            supports_high_res);
+  queue->SetQueuePriority(TaskQueue::QueuePriority::kLowPriority);
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+
+  // Running immediate tasks doesn't affect pending high resolution tasks.
+  RunLoop().RunUntilIdle();
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+
+  // Advancing to just before a pending low resolution task doesn't mean that we
+  // have pending high resolution work.
+  AdvanceMockTickClock(TimeDelta::FromMilliseconds(99));
+  RunLoop().RunUntilIdle();
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+
+  AdvanceMockTickClock(TimeDelta::FromMilliseconds(100));
+  RunLoop().RunUntilIdle();
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+}
+
+TEST_P(SequenceManagerTest,
+       HasPendingHighResolutionTasksLowAndNormalPriorityQueues) {
+  auto queueLow = CreateTaskQueue();
+  queueLow->SetQueuePriority(TaskQueue::QueuePriority::kLowPriority);
+  auto queueNormal = CreateTaskQueue();
+  queueNormal->SetQueuePriority(TaskQueue::QueuePriority::kNormalPriority);
+  bool supports_high_res = false;
+#if defined(OS_WIN)
+  supports_high_res = true;
+#endif
+
+  // No task should be considered high resolution in a low priority queue.
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+  queueLow->task_runner()->PostDelayedTask(FROM_HERE, BindOnce(&NopTask),
+                                           TimeDelta::FromMilliseconds(10));
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+  queueNormal->task_runner()->PostDelayedTask(FROM_HERE, BindOnce(&NopTask),
+                                              TimeDelta::FromMilliseconds(100));
+  EXPECT_FALSE(sequence_manager()->HasPendingHighResolutionTasks());
+
+  // Increasing queue priority should enable high resolution timer.
+  queueLow->SetQueuePriority(TaskQueue::QueuePriority::kNormalPriority);
+  EXPECT_EQ(sequence_manager()->HasPendingHighResolutionTasks(),
+            supports_high_res);
+}
+
 namespace {
 
 class PostTaskWhenDeleted;

base/task/sequence_manager/task_queue_impl.cc

@@ -547,7 +547,18 @@ Optional<DelayedWakeUp> TaskQueueImpl::GetNextScheduledWakeUpImpl() {
   if (main_thread_only().delayed_incoming_queue.empty() || !IsQueueEnabled())
     return nullopt;
 
-  return main_thread_only().delayed_incoming_queue.top().delayed_wake_up();
+  // High resolution is needed if the queue contains high resolution tasks and
+  // has a priority index <= kNormalPriority (precise execution time is
+  // unnecessary for a low priority queue).
+  WakeUpResolution resolution =
+      has_pending_high_resolution_tasks() &&
+              GetQueuePriority() <= TaskQueue::QueuePriority::kNormalPriority
+          ? WakeUpResolution::kHigh
+          : WakeUpResolution::kLow;
+
+  const auto& top_task = main_thread_only().delayed_incoming_queue.top();
+  return DelayedWakeUp{top_task.delayed_run_time, top_task.sequence_num,
+                       resolution};
 }
 
 Optional<TimeTicks> TaskQueueImpl::GetNextScheduledWakeUp() {
@@ -621,6 +632,12 @@ void TaskQueueImpl::SetQueuePriority(TaskQueue::QueuePriority priority) {
   sequence_manager_->main_thread_only().selector.SetQueuePriority(this,
                                                                   priority);
 
+#if defined(OS_WIN)
+  // Updating queue priority can change whether high resolution timer is needed.
+  LazyNow lazy_now = main_thread_only().time_domain->CreateLazyNow();
+  UpdateDelayedWakeUp(&lazy_now);
+#endif
+
   static_assert(TaskQueue::QueuePriority::kLowPriority >
                     TaskQueue::QueuePriority::kNormalPriority,
                 "Priorities are not ordered as expected");
@@ -1118,11 +1135,8 @@ void TaskQueueImpl::UpdateDelayedWakeUpImpl(LazyNow* lazy_now,
         wake_up->time);
   }
 
-  WakeUpResolution resolution = has_pending_high_resolution_tasks()
-                                    ? WakeUpResolution::kHigh
-                                    : WakeUpResolution::kLow;
   main_thread_only().time_domain->SetNextWakeUpForQueue(this, wake_up,
-                                                        resolution, lazy_now);
+                                                        lazy_now);
 }
 
 void TaskQueueImpl::SetDelayedWakeUpForTesting(

base/task/sequence_manager/task_queue_impl.h

@@ -112,7 +112,6 @@ class BASE_EXPORT TaskQueueImpl {
   size_t GetNumberOfPendingTasks() const;
   bool HasTaskToRunImmediately() const;
   Optional<TimeTicks> GetNextScheduledWakeUp();
-  Optional<DelayedWakeUp> GetNextScheduledWakeUpImpl();
   void SetQueuePriority(TaskQueue::QueuePriority priority);
   TaskQueue::QueuePriority GetQueuePriority() const;
   void AddTaskObserver(TaskObserver* task_observer);
@@ -414,6 +413,8 @@ class BASE_EXPORT TaskQueueImpl {
   // threads.
   void PushOntoDelayedIncomingQueue(Task pending_task);
 
+  Optional<DelayedWakeUp> GetNextScheduledWakeUpImpl();
+
   void ScheduleDelayedWorkTask(Task pending_task);
 
   void MoveReadyImmediateTasksToImmediateWorkQueueLocked()

base/task/sequence_manager/tasks.h

@@ -50,9 +50,11 @@ struct BASE_EXPORT PostedTask {
 struct DelayedWakeUp {
   TimeTicks time;
   int sequence_num;
+  WakeUpResolution resolution;
 
   bool operator!=(const DelayedWakeUp& other) const {
-    return time != other.time || other.sequence_num != sequence_num;
+    return time != other.time || other.sequence_num != sequence_num ||
+           resolution != other.resolution;
   }
 
   bool operator==(const DelayedWakeUp& other) const {
@@ -61,11 +63,19 @@ struct DelayedWakeUp {
 
   bool operator<=(const DelayedWakeUp& other) const {
     if (time == other.time) {
-      // Debug gcc builds can compare an element against itself.
-      DCHECK(sequence_num != other.sequence_num || this == &other);
+      if (sequence_num == other.sequence_num) {
+        if (resolution == other.resolution) {
+          // Debug gcc builds can compare an element against itself.
+          DCHECK_EQ(this, &other);
+          return true;
+        }
+
+        return resolution < other.resolution;
+      }
+
       // |sequence_num| is int and might wrap around to a negative number when
       // casted from EnqueueOrder. This way of comparison handles that properly.
-      return (sequence_num - other.sequence_num) <= 0;
+      return (sequence_num - other.sequence_num) < 0;
     }
     return time < other.time;
   }
@@ -85,10 +95,6 @@ struct BASE_EXPORT Task : public PendingTask {
   ~Task();
   Task& operator=(Task&& other);
 
-  internal::DelayedWakeUp delayed_wake_up() const {
-    return internal::DelayedWakeUp{delayed_run_time, sequence_num};
-  }
-
   // SequenceManager is particularly sensitive to enqueue order,
   // so we have accessors for safety.
   EnqueueOrder enqueue_order() const {

base/task/sequence_manager/time_domain.cc

@@ -50,14 +50,12 @@ void TimeDomain::UnregisterQueue(internal::TaskQueueImpl* queue) {
   DCHECK_CALLED_ON_VALID_THREAD(associated_thread_->thread_checker);
   DCHECK_EQ(queue->GetTimeDomain(), this);
   LazyNow lazy_now(CreateLazyNow());
-  SetNextWakeUpForQueue(queue, nullopt, internal::WakeUpResolution::kLow,
-                        &lazy_now);
+  SetNextWakeUpForQueue(queue, nullopt, &lazy_now);
 }
 
 void TimeDomain::SetNextWakeUpForQueue(
     internal::TaskQueueImpl* queue,
     Optional<internal::DelayedWakeUp> wake_up,
-    internal::WakeUpResolution resolution,
     LazyNow* lazy_now) {
   DCHECK_CALLED_ON_VALID_THREAD(associated_thread_->thread_checker);
   DCHECK_EQ(queue->GetTimeDomain(), this);
@@ -69,7 +67,7 @@ void TimeDomain::SetNextWakeUpForQueue(
     previous_wake_up = delayed_wake_up_queue_.Min().wake_up.time;
   if (queue->heap_handle().IsValid()) {
     previous_queue_resolution =
-        delayed_wake_up_queue_.at(queue->heap_handle()).resolution;
+        delayed_wake_up_queue_.at(queue->heap_handle()).wake_up.resolution;
   }
 
   if (wake_up) {
@@ -77,10 +75,10 @@ void TimeDomain::SetNextWakeUpForQueue(
     if (queue->heap_handle().IsValid()) {
       // O(log n)
       delayed_wake_up_queue_.ChangeKey(queue->heap_handle(),
-                                       {wake_up.value(), resolution, queue});
+                                       {wake_up.value(), queue});
     } else {
       // O(log n)
-      delayed_wake_up_queue_.insert({wake_up.value(), resolution, queue});
+      delayed_wake_up_queue_.insert({wake_up.value(), queue});
     }
   } else {
     // Remove a wake-up from heap if present.
@@ -96,7 +94,7 @@ void TimeDomain::SetNextWakeUpForQueue(
       *previous_queue_resolution == internal::WakeUpResolution::kHigh) {
     pending_high_res_wake_up_count_--;
   }
-  if (wake_up && resolution == internal::WakeUpResolution::kHigh)
+  if (wake_up && wake_up->resolution == internal::WakeUpResolution::kHigh)
     pending_high_res_wake_up_count_++;
   DCHECK_GE(pending_high_res_wake_up_count_, 0);
 

base/task/sequence_manager/time_domain.h

@@ -112,7 +112,6 @@ class BASE_EXPORT TimeDomain {
   // NOTE: |lazy_now| is provided in TimeDomain's time.
   void SetNextWakeUpForQueue(internal::TaskQueueImpl* queue,
                              Optional<internal::DelayedWakeUp> wake_up,
-                             internal::WakeUpResolution resolution,
                              LazyNow* lazy_now);
 
   // Remove the TaskQueue from any internal data sctructures.
@@ -124,14 +123,9 @@ class BASE_EXPORT TimeDomain {
 
   struct ScheduledDelayedWakeUp {
     internal::DelayedWakeUp wake_up;
-    internal::WakeUpResolution resolution;
     internal::TaskQueueImpl* queue;
 
     bool operator<=(const ScheduledDelayedWakeUp& other) const {
-      if (wake_up == other.wake_up) {
-        return static_cast<int>(resolution) <=
-               static_cast<int>(other.resolution);
-      }
       return wake_up <= other.wake_up;
     }
 

base/task/sequence_manager/time_domain_unittest.cc

@@ -345,45 +345,46 @@ TEST_F(TimeDomainTest, HighResolutionWakeUps) {
   // Add two high resolution wake-ups.
   EXPECT_FALSE(time_domain_->has_pending_high_resolution_tasks());
   time_domain_->SetNextWakeUpForQueue(
-      &q1, internal::DelayedWakeUp{run_time1, 0},
-      internal::WakeUpResolution::kHigh, &lazy_now);
+      &q1,
+      internal::DelayedWakeUp{run_time1, 0, internal::WakeUpResolution::kHigh},
+      &lazy_now);
   EXPECT_TRUE(time_domain_->has_pending_high_resolution_tasks());
   time_domain_->SetNextWakeUpForQueue(
-      &q2, internal::DelayedWakeUp{run_time2, 0},
-      internal::WakeUpResolution::kHigh, &lazy_now);
+      &q2,
+      internal::DelayedWakeUp{run_time2, 0, internal::WakeUpResolution::kHigh},
+      &lazy_now);
   EXPECT_TRUE(time_domain_->has_pending_high_resolution_tasks());
 
   // Remove one of the wake-ups.
-  time_domain_->SetNextWakeUpForQueue(
-      &q1, nullopt, internal::WakeUpResolution::kLow, &lazy_now);
+  time_domain_->SetNextWakeUpForQueue(&q1, nullopt, &lazy_now);
   EXPECT_TRUE(time_domain_->has_pending_high_resolution_tasks());
 
   // Remove the second one too.
-  time_domain_->SetNextWakeUpForQueue(
-      &q2, nullopt, internal::WakeUpResolution::kLow, &lazy_now);
+  time_domain_->SetNextWakeUpForQueue(&q2, nullopt, &lazy_now);
   EXPECT_FALSE(time_domain_->has_pending_high_resolution_tasks());
 
   // Change a low resolution wake-up to a high resolution one.
   time_domain_->SetNextWakeUpForQueue(
-      &q1, internal::DelayedWakeUp{run_time1, 0},
-      internal::WakeUpResolution::kLow, &lazy_now);
+      &q1,
+      internal::DelayedWakeUp{run_time1, 0, internal::WakeUpResolution::kLow},
+      &lazy_now);
   EXPECT_FALSE(time_domain_->has_pending_high_resolution_tasks());
   time_domain_->SetNextWakeUpForQueue(
-      &q1, internal::DelayedWakeUp{run_time1, 0},
-      internal::WakeUpResolution::kHigh, &lazy_now);
+      &q1,
+      internal::DelayedWakeUp{run_time1, 0, internal::WakeUpResolution::kHigh},
+      &lazy_now);
   EXPECT_TRUE(time_domain_->has_pending_high_resolution_tasks());
 
   // Move a high resolution wake-up in time.
   time_domain_->SetNextWakeUpForQueue(
-      &q1, internal::DelayedWakeUp{run_time2, 0},
-      internal::WakeUpResolution::kHigh, &lazy_now);
+      &q1,
+      internal::DelayedWakeUp{run_time2, 0, internal::WakeUpResolution::kHigh},
+      &lazy_now);
   EXPECT_TRUE(time_domain_->has_pending_high_resolution_tasks());
 
   // Cancel the wake-up twice.
-  time_domain_->SetNextWakeUpForQueue(
-      &q1, nullopt, internal::WakeUpResolution::kLow, &lazy_now);
-  time_domain_->SetNextWakeUpForQueue(
-      &q1, nullopt, internal::WakeUpResolution::kLow, &lazy_now);
+  time_domain_->SetNextWakeUpForQueue(&q1, nullopt, &lazy_now);
+  time_domain_->SetNextWakeUpForQueue(&q1, nullopt, &lazy_now);
   EXPECT_FALSE(time_domain_->has_pending_high_resolution_tasks());
 
   // Tidy up.