TaskScheduler: Change PriorityQueue's internal container from std::priority_queue to IntrusiveHeap

This involves changing SequenceSortKey from high == important to low == important, because IntrusiveHeap is a min-heap (as opposed to std::priority_queue, which is a max-heap).

Bug: 889029
Change-Id: Ieeb2707d0112389b6cb7f4d187ec447bc58bef5a
Reviewed-on: https://chromium-review.googlesource.com/c/1302319
Commit-Queue: Jesse McKenna <jessemckenna@google.com>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603243}

base/task/task_scheduler/priority_queue.cc

@@ -18,6 +18,7 @@ namespace internal {
 // call.
 class PriorityQueue::SequenceAndSortKey {
  public:
+  SequenceAndSortKey() = default;
   SequenceAndSortKey(scoped_refptr<Sequence> sequence,
                      const SequenceSortKey& sort_key)
       : sequence_(std::move(sequence)), sort_key_(sort_key) {
@@ -25,10 +26,10 @@ class PriorityQueue::SequenceAndSortKey {
   }
 
   // Note: while |sequence_| should always be non-null post-move (i.e. we
-  // shouldn't be moving an invalid SequenceAndSortKey around), there can't be a
-  // DCHECK(sequence_) on moves as the Windows STL moves elements on pop instead
-  // of overwriting them: resulting in the move of a SequenceAndSortKey with a
-  // null |sequence_| in Transaction::Pop()'s implementation.
+  // shouldn't be moving an invalid SequenceAndSortKey around), there can't be
+  // a DCHECK(sequence_) on moves as IntrusiveHeap moves elements on pop
+  // instead of overwriting them: resulting in the move of a SequenceAndSortKey
+  // with a null |sequence_| in Transaction::Pop()'s implementation.
   SequenceAndSortKey(SequenceAndSortKey&& other) = default;
   SequenceAndSortKey& operator=(SequenceAndSortKey&& other) = default;
 
@@ -40,14 +41,16 @@ class PriorityQueue::SequenceAndSortKey {
   }
 
   // Compares this SequenceAndSortKey to |other| based on their respective
-  // |sort_key_|.
-  bool operator<(const SequenceAndSortKey& other) const {
-    return sort_key_ < other.sort_key_;
+  // |sort_key_|. Required by IntrusiveHeap.
+  bool operator<=(const SequenceAndSortKey& other) const {
+    return sort_key_ <= other.sort_key_;
   }
-  // Style-guide dictates to define operator> when defining operator< but it's
-  // unused in this case and this isn't a public API. Explicitly delete it so
-  // any errors point here if that ever changes.
-  bool operator>(const SequenceAndSortKey& other) const = delete;
+
+  // Required by IntrusiveHeap.
+  void SetHeapHandle(const HeapHandle& handle) {}
+
+  // Required by IntrusiveHeap.
+  void ClearHeapHandle() {}
 
   const SequenceSortKey& sort_key() const { return sort_key_; }
 
@@ -66,12 +69,13 @@ PriorityQueue::Transaction::~Transaction() = default;
 void PriorityQueue::Transaction::Push(
     scoped_refptr<Sequence> sequence,
     const SequenceSortKey& sequence_sort_key) {
-  outer_queue_->container_.emplace(std::move(sequence), sequence_sort_key);
+  outer_queue_->container_.insert(
+      SequenceAndSortKey(std::move(sequence), sequence_sort_key));
 }
 
 const SequenceSortKey& PriorityQueue::Transaction::PeekSortKey() const {
   DCHECK(!IsEmpty());
-  return outer_queue_->container_.top().sort_key();
+  return outer_queue_->container_.Min().sort_key();
 }
 
 scoped_refptr<Sequence> PriorityQueue::Transaction::PopSequence() {
@@ -79,13 +83,12 @@ scoped_refptr<Sequence> PriorityQueue::Transaction::PopSequence() {
 
   // The const_cast on top() is okay since the SequenceAndSortKey is
   // transactionally being popped from |container_| right after and taking its
-  // Sequence does not alter its sort order (a requirement for the Windows STL's
-  // consistency debug-checks for std::priority_queue::top()).
+  // Sequence does not alter its sort order.
   scoped_refptr<Sequence> sequence =
       const_cast<PriorityQueue::SequenceAndSortKey&>(
-          outer_queue_->container_.top())
+          outer_queue_->container_.Min())
           .take_sequence();
-  outer_queue_->container_.pop();
+  outer_queue_->container_.Pop();
   return sequence;
 }
 

base/task/task_scheduler/priority_queue.h

@@ -6,12 +6,11 @@
 #define BASE_TASK_TASK_SCHEDULER_PRIORITY_QUEUE_H_
 
 #include <memory>
-#include <queue>
-#include <vector>
 
 #include "base/base_export.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/task/common/intrusive_heap.h"
 #include "base/task/task_scheduler/scheduler_lock.h"
 #include "base/task/task_scheduler/sequence.h"
 #include "base/task/task_scheduler/sequence_sort_key.h"
@@ -88,7 +87,7 @@ class BASE_EXPORT PriorityQueue {
   // position in a PriorityQueue.
   class SequenceAndSortKey;
 
-  using ContainerType = std::priority_queue<SequenceAndSortKey>;
+  using ContainerType = IntrusiveHeap<SequenceAndSortKey>;
 
   // Synchronizes access to |container_|.
   SchedulerLock container_lock_;

base/task/task_scheduler/sequence_sort_key.cc

@@ -12,17 +12,17 @@ SequenceSortKey::SequenceSortKey(TaskPriority priority,
     : priority_(priority),
       next_task_sequenced_time_(next_task_sequenced_time) {}
 
-bool SequenceSortKey::operator<(const SequenceSortKey& other) const {
-  // This SequenceSortKey is considered less important than |other| if it has a
-  // lower priority or if it has the same priority but its next task was posted
-  // later than |other|'s.
+bool SequenceSortKey::operator<=(const SequenceSortKey& other) const {
+  // This SequenceSortKey is considered more important than |other| if it has a
+  // higher priority or if it has the same priority but its next task was
+  // posted sooner than |other|'s.
   const int priority_diff =
       static_cast<int>(priority_) - static_cast<int>(other.priority_);
-  if (priority_diff < 0)
-    return true;
   if (priority_diff > 0)
+    return true;
+  if (priority_diff < 0)
     return false;
-  return next_task_sequenced_time_ > other.next_task_sequenced_time_;
+  return next_task_sequenced_time_ <= other.next_task_sequenced_time_;
 }
 
 }  // namespace internal

base/task/task_scheduler/sequence_sort_key.h

@@ -15,6 +15,7 @@ namespace internal {
 // An immutable but assignable representation of the priority of a Sequence.
 class BASE_EXPORT SequenceSortKey final {
  public:
+  SequenceSortKey() = default;
   SequenceSortKey(TaskPriority priority, TimeTicks next_task_sequenced_time);
 
   TaskPriority priority() const { return priority_; }
@@ -22,8 +23,8 @@ class BASE_EXPORT SequenceSortKey final {
     return next_task_sequenced_time_;
   }
 
-  bool operator<(const SequenceSortKey& other) const;
-  bool operator>(const SequenceSortKey& other) const { return other < *this; }
+  // Lower sort key means more important.
+  bool operator<=(const SequenceSortKey& other) const;
 
   bool operator==(const SequenceSortKey& other) const {
     return priority_ == other.priority_ &&

base/task/task_scheduler/sequence_sort_key_unittest.cc

@@ -11,7 +11,7 @@
 namespace base {
 namespace internal {
 
-TEST(TaskSchedulerSequenceSortKeyTest, OperatorLessThan) {
+TEST(TaskSchedulerSequenceSortKeyTest, OperatorLessThanOrEqual) {
   SequenceSortKey key_a(TaskPriority::USER_BLOCKING,
                         TimeTicks::FromInternalValue(1000));
   SequenceSortKey key_b(TaskPriority::USER_BLOCKING,
@@ -25,104 +25,47 @@ TEST(TaskSchedulerSequenceSortKeyTest, OperatorLessThan) {
   SequenceSortKey key_f(TaskPriority::BEST_EFFORT,
                         TimeTicks::FromInternalValue(2000));
 
-  EXPECT_FALSE(key_a < key_a);
-  EXPECT_LT(key_b, key_a);
-  EXPECT_LT(key_c, key_a);
-  EXPECT_LT(key_d, key_a);
-  EXPECT_LT(key_e, key_a);
-  EXPECT_LT(key_f, key_a);
-
-  EXPECT_FALSE(key_a < key_b);
-  EXPECT_FALSE(key_b < key_b);
-  EXPECT_LT(key_c, key_b);
-  EXPECT_LT(key_d, key_b);
-  EXPECT_LT(key_e, key_b);
-  EXPECT_LT(key_f, key_b);
-
-  EXPECT_FALSE(key_a < key_c);
-  EXPECT_FALSE(key_b < key_c);
-  EXPECT_FALSE(key_c < key_c);
-  EXPECT_LT(key_d, key_c);
-  EXPECT_LT(key_e, key_c);
-  EXPECT_LT(key_f, key_c);
-
-  EXPECT_FALSE(key_a < key_d);
-  EXPECT_FALSE(key_b < key_d);
-  EXPECT_FALSE(key_c < key_d);
-  EXPECT_FALSE(key_d < key_d);
-  EXPECT_LT(key_e, key_d);
-  EXPECT_LT(key_f, key_d);
-
-  EXPECT_FALSE(key_a < key_e);
-  EXPECT_FALSE(key_b < key_e);
-  EXPECT_FALSE(key_c < key_e);
-  EXPECT_FALSE(key_d < key_e);
-  EXPECT_FALSE(key_e < key_e);
-  EXPECT_LT(key_f, key_e);
-
-  EXPECT_FALSE(key_a < key_f);
-  EXPECT_FALSE(key_b < key_f);
-  EXPECT_FALSE(key_c < key_f);
-  EXPECT_FALSE(key_d < key_f);
-  EXPECT_FALSE(key_e < key_f);
-  EXPECT_FALSE(key_f < key_f);
-}
-
-TEST(TaskSchedulerSequenceSortKeyTest, OperatorGreaterThan) {
-  SequenceSortKey key_a(TaskPriority::USER_BLOCKING,
-                        TimeTicks::FromInternalValue(1000));
-  SequenceSortKey key_b(TaskPriority::USER_BLOCKING,
-                        TimeTicks::FromInternalValue(2000));
-  SequenceSortKey key_c(TaskPriority::USER_VISIBLE,
-                        TimeTicks::FromInternalValue(1000));
-  SequenceSortKey key_d(TaskPriority::USER_VISIBLE,
-                        TimeTicks::FromInternalValue(2000));
-  SequenceSortKey key_e(TaskPriority::BEST_EFFORT,
-                        TimeTicks::FromInternalValue(1000));
-  SequenceSortKey key_f(TaskPriority::BEST_EFFORT,
-                        TimeTicks::FromInternalValue(2000));
-
-  EXPECT_FALSE(key_a > key_a);
-  EXPECT_FALSE(key_b > key_a);
-  EXPECT_FALSE(key_c > key_a);
-  EXPECT_FALSE(key_d > key_a);
-  EXPECT_FALSE(key_e > key_a);
-  EXPECT_FALSE(key_f > key_a);
-
-  EXPECT_GT(key_a, key_b);
-  EXPECT_FALSE(key_b > key_b);
-  EXPECT_FALSE(key_c > key_b);
-  EXPECT_FALSE(key_d > key_b);
-  EXPECT_FALSE(key_e > key_b);
-  EXPECT_FALSE(key_f > key_b);
-
-  EXPECT_GT(key_a, key_c);
-  EXPECT_GT(key_b, key_c);
-  EXPECT_FALSE(key_c > key_c);
-  EXPECT_FALSE(key_d > key_c);
-  EXPECT_FALSE(key_e > key_c);
-  EXPECT_FALSE(key_f > key_c);
-
-  EXPECT_GT(key_a, key_d);
-  EXPECT_GT(key_b, key_d);
-  EXPECT_GT(key_c, key_d);
-  EXPECT_FALSE(key_d > key_d);
-  EXPECT_FALSE(key_e > key_d);
-  EXPECT_FALSE(key_f > key_d);
-
-  EXPECT_GT(key_a, key_e);
-  EXPECT_GT(key_b, key_e);
-  EXPECT_GT(key_c, key_e);
-  EXPECT_GT(key_d, key_e);
-  EXPECT_FALSE(key_e > key_e);
-  EXPECT_FALSE(key_f > key_e);
-
-  EXPECT_GT(key_a, key_f);
-  EXPECT_GT(key_b, key_f);
-  EXPECT_GT(key_c, key_f);
-  EXPECT_GT(key_d, key_f);
-  EXPECT_GT(key_e, key_f);
-  EXPECT_FALSE(key_f > key_f);
+  EXPECT_LE(key_a, key_a);
+  EXPECT_FALSE(key_b <= key_a);
+  EXPECT_FALSE(key_c <= key_a);
+  EXPECT_FALSE(key_d <= key_a);
+  EXPECT_FALSE(key_e <= key_a);
+  EXPECT_FALSE(key_f <= key_a);
+
+  EXPECT_LE(key_a, key_b);
+  EXPECT_LE(key_b, key_b);
+  EXPECT_FALSE(key_c <= key_b);
+  EXPECT_FALSE(key_d <= key_b);
+  EXPECT_FALSE(key_e <= key_b);
+  EXPECT_FALSE(key_f <= key_b);
+
+  EXPECT_LE(key_a, key_c);
+  EXPECT_LE(key_b, key_c);
+  EXPECT_LE(key_c, key_c);
+  EXPECT_FALSE(key_d <= key_c);
+  EXPECT_FALSE(key_e <= key_c);
+  EXPECT_FALSE(key_f <= key_c);
+
+  EXPECT_LE(key_a, key_d);
+  EXPECT_LE(key_b, key_d);
+  EXPECT_LE(key_c, key_d);
+  EXPECT_LE(key_d, key_d);
+  EXPECT_FALSE(key_e <= key_d);
+  EXPECT_FALSE(key_f <= key_d);
+
+  EXPECT_LE(key_a, key_e);
+  EXPECT_LE(key_b, key_e);
+  EXPECT_LE(key_c, key_e);
+  EXPECT_LE(key_d, key_e);
+  EXPECT_LE(key_e, key_e);
+  EXPECT_FALSE(key_f <= key_e);
+
+  EXPECT_LE(key_a, key_f);
+  EXPECT_LE(key_b, key_f);
+  EXPECT_LE(key_c, key_f);
+  EXPECT_LE(key_d, key_f);
+  EXPECT_LE(key_e, key_f);
+  EXPECT_LE(key_f, key_f);
 }
 
 TEST(TaskSchedulerSequenceSortKeyTest, OperatorEqual) {