Correct comparison with default

Summary:
Comparing (operator<) for default PriorityQueue::Priority vs HTTPPriorityQueue::Priority was not correctly using the default HTTP priority - instead it would interpret 0xffffffff as u=7, i=1, which is the lowest priority.  Also constructing or assigning HTTPPriorityQueue::Priority from PriorityQueue::Priority was suboptimal.

1) Make order a uint32_t.  From HTTPPriorityFunctions, it looks like we could only set this to a max of 2^31-1 anyways.
2) Add an 'uninitialized' bit to HTTPPriority.  Since the default is 1 initialized in the base class, only fully initialized HTTPPriority will return true for `isInitializedFast`
3) Implement new constructor and assignment operator
4) Return a static default uint64 for uninitialized priority in `toUnit64`, this fixes `operator<`
5) Streamline `operator==`

Reviewed By: hanidamlaj

Differential Revision: D73685997

fbshipit-source-id: 5c6cd2404d6506f661aa29983c5ded3cf913758c

quic/priority/HTTPPriorityQueue.cpp

@@ -17,17 +17,27 @@ namespace quic {
 /*implicit*/ HTTPPriorityQueue::Priority::Priority(
     const PriorityQueue::Priority& basePriority)
     : PriorityQueue::Priority(basePriority) {
-  if (!isInitialized()) {
+  if (!isInitializedFast()) {
     getFields() = kDefaultPriority;
   }
 }
 
+HTTPPriorityQueue::Priority& HTTPPriorityQueue::Priority::operator=(
+    const PriorityQueue::Priority& basePriority) {
+  PriorityQueue::Priority::operator=(basePriority);
+  if (!isInitializedFast()) {
+    getFields() = kDefaultPriority;
+  }
+  return *this;
+}
+
 HTTPPriorityQueue::Priority::Priority(uint8_t u, bool i, OrderId o) {
   auto& fields = getFields();
   fields.urgency = u;
   fields.incremental = i;
   fields.order = (i ? 0 : o);
   fields.paused = false;
+  fields.uninitialized = false;
 }
 
 PriorityQueue::PriorityLogFields HTTPPriorityQueue::toLogFields(

quic/priority/HTTPPriorityQueue.h

@@ -30,26 +30,37 @@ class HTTPPriorityQueue : public quic::PriorityQueue {
  public:
   class Priority : public quic::PriorityQueue::Priority {
    public:
-    using OrderId = uint64_t;
+    using OrderId = uint32_t;
 
     struct HTTPPriority {
       uint8_t urgency : 3;
       bool paused : 1;
       bool incremental : 1;
-      OrderId order : 59;
+      OrderId order : 32;
+      bool uninitialized : 1;
     };
 
     // TODO: change default priority to (3, false, false, 0) to match spec
-    static constexpr HTTPPriority kDefaultPriority{3, false, true, 0};
+#if __cplusplus >= 202002L
-
+    static constexpr HTTPPriority kDefaultPriority{
+        .urgency = 3,
+        .paused = false,
+        .incremental = true,
+        .order = 0,
+        .uninitialized = false};
+#else
+    static constexpr HTTPPriority kDefaultPriority = {3, false, true, 0, false};
+#endif
     /*implicit*/ Priority(const PriorityQueue::Priority& basePriority);
 
     Priority(uint8_t u, bool i, OrderId o = 0);
+    Priority& operator=(const PriorityQueue::Priority& basePriority);
 
     enum Paused { PAUSED };
 
-    /* implicit */ Priority(Paused) : Priority(0, false, 0) {
+    /* implicit */ Priority(Paused) : Priority(7, true) {
-      getFields().paused = true;
+      auto& fields = getFields();
+      fields.paused = true;
     }
 
     Priority(const Priority&) = default;
@@ -63,20 +74,7 @@ class HTTPPriorityQueue : public quic::PriorityQueue {
     }
 
     bool operator==(const Priority& other) const {
-      auto asUint64 = toUint64();
+      return toUint64() == other.toUint64();
-      auto otherAsUint64 = other.toUint64();
-      if (asUint64 == otherAsUint64) {
-        return true;
-      }
-      // The only other way to be equal is if one is initialized and the other
-      // is default
-      const static uint64_t kDefaultUint64 = Priority(
-                                                 kDefaultPriority.urgency,
-                                                 kDefaultPriority.incremental,
-                                                 kDefaultPriority.order)
-                                                 .toUint64();
-      return (kDefaultUint64 == otherAsUint64 && !isInitialized()) ||
-          (asUint64 == kDefaultUint64 && !other.isInitialized());
     }
 
     bool operator<(const Priority& other) const {
@@ -84,10 +82,9 @@ class HTTPPriorityQueue : public quic::PriorityQueue {
     }
 
     [[nodiscard]] uint64_t toUint64() const {
+      const static uint64_t kDefaultUint64 = toUint64Static(kDefaultPriority);
       const auto& fields = getFields();
-      return (
+      return fields.uninitialized ? kDefaultUint64 : toUint64Static(fields);
-          (uint64_t(fields.urgency) << 61) | (uint64_t(fields.paused) << 60) |
-          (uint64_t(fields.incremental) << 59) | fields.order);
     }
 
     [[nodiscard]] const HTTPPriority& getFields() const {
@@ -95,6 +92,17 @@ class HTTPPriorityQueue : public quic::PriorityQueue {
     }
 
    private:
+    [[nodiscard]] bool isInitializedFast() const {
+      return !getFields().uninitialized;
+    }
+
+    static uint64_t toUint64Static(const HTTPPriority& fields) {
+      return (
+          (uint64_t(fields.urgency) << 61) | (uint64_t(fields.paused) << 60) |
+          (uint64_t(fields.incremental) << 59) |
+          (uint64_t(fields.order) << 27));
+    }
+
     HTTPPriority& getFields() {
       return getPriority<HTTPPriority>();
     }

quic/priority/test/HTTPPriorityQueueTest.cpp

@@ -36,10 +36,10 @@ TEST_F(HTTPPriorityQueueTest, IncrementalEmptyQueue) {
 
 TEST_F(HTTPPriorityQueueTest, Compare) {
   std::vector<HTTPPriorityQueue::Priority> pris = {
-      PriorityQueue::Priority(),
       {0, false},
       {0, false, 1},
       {0, true},
+      PriorityQueue::Priority(),
       {7, false},
       {7, false, std::numeric_limits<uint32_t>::max()},
       {7, true},
@@ -51,10 +51,19 @@ TEST_F(HTTPPriorityQueueTest, Compare) {
       EXPECT_TRUE(
           (i == j && queue_.equalPriority(pris[i], pris[j])) ||
           (i != j && !queue_.equalPriority(pris[i], pris[j])));
+      EXPECT_TRUE(i >= j || (i < j && pris[i] < pris[j])) << i << " " << j;
     }
   }
   // TODO: will change when default changes
-  EXPECT_EQ(pris[0], HTTPPriorityQueue::Priority(3, true));
+  EXPECT_EQ(pris[3], HTTPPriorityQueue::Priority(3, true));
+  PriorityQueue::Priority baseDefaultPri;
+  auto* defaultPri = static_cast<HTTPPriorityQueue::Priority*>(&baseDefaultPri);
+  EXPECT_TRUE(
+      HTTPPriorityQueue::Priority(
+          3, false, std::numeric_limits<uint32_t>::max()) < *defaultPri);
+  EXPECT_TRUE(
+      *defaultPri < HTTPPriorityQueue::Priority(
+                        4, false, std::numeric_limits<uint32_t>::max()));
 }
 
 TEST_F(HTTPPriorityQueueTest, InsertSingleElement) {
@@ -352,4 +361,35 @@ TEST_F(HTTPPriorityQueueTest, ToLogFields) {
   EXPECT_EQ(lookup(fieldsRegular2, "incremental"), "false");
   EXPECT_EQ(lookup(fieldsRegular2, "order"), "5");
 }
+
+TEST_F(HTTPPriorityQueueTest, ImplicitPriorityConstructor) {
+  // Test with uninitialized basePriority
+  PriorityQueue::Priority uninitializedPriority;
+  HTTPPriorityQueue::Priority priorityWithDefault(uninitializedPriority);
+
+  EXPECT_EQ(
+      priorityWithDefault->urgency,
+      HTTPPriorityQueue::Priority::kDefaultPriority.urgency);
+  EXPECT_EQ(
+      priorityWithDefault->incremental,
+      HTTPPriorityQueue::Priority::kDefaultPriority.incremental);
+  EXPECT_EQ(
+      priorityWithDefault->order,
+      HTTPPriorityQueue::Priority::kDefaultPriority.order);
+}
+
+TEST_F(HTTPPriorityQueueTest, PriorityAssignmentOperator) {
+  PriorityQueue::Priority uninitializedPriority;
+  HTTPPriorityQueue::Priority priority(0, false);
+  priority = uninitializedPriority;
+
+  EXPECT_EQ(
+      priority->urgency, HTTPPriorityQueue::Priority::kDefaultPriority.urgency);
+  EXPECT_EQ(
+      priority->incremental,
+      HTTPPriorityQueue::Priority::kDefaultPriority.incremental);
+  EXPECT_EQ(
+      priority->order, HTTPPriorityQueue::Priority::kDefaultPriority.order);
+}
+
 } // namespace