diff --git a/quic/priority/HTTPPriorityQueue.cpp b/quic/priority/HTTPPriorityQueue.cpp index b24b0ff20..bc362bfed 100644 --- a/quic/priority/HTTPPriorityQueue.cpp +++ b/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( diff --git a/quic/priority/HTTPPriorityQueue.h b/quic/priority/HTTPPriorityQueue.h index 6a9736695..b7ff068ff 100644 --- a/quic/priority/HTTPPriorityQueue.h +++ b/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) { - getFields().paused = true; + /* implicit */ Priority(Paused) : Priority(7, 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(); - 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()); + return toUint64() == other.toUint64(); } 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 ( - (uint64_t(fields.urgency) << 61) | (uint64_t(fields.paused) << 60) | - (uint64_t(fields.incremental) << 59) | fields.order); + return fields.uninitialized ? kDefaultUint64 : toUint64Static(fields); } [[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(); } diff --git a/quic/priority/test/HTTPPriorityQueueTest.cpp b/quic/priority/test/HTTPPriorityQueueTest.cpp index 3a3599a8e..803dae155 100644 --- a/quic/priority/test/HTTPPriorityQueueTest.cpp +++ b/quic/priority/test/HTTPPriorityQueueTest.cpp @@ -36,10 +36,10 @@ TEST_F(HTTPPriorityQueueTest, IncrementalEmptyQueue) { TEST_F(HTTPPriorityQueueTest, Compare) { std::vector pris = { - PriorityQueue::Priority(), {0, false}, {0, false, 1}, {0, true}, + PriorityQueue::Priority(), {7, false}, {7, false, std::numeric_limits::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(&baseDefaultPri); + EXPECT_TRUE( + HTTPPriorityQueue::Priority( + 3, false, std::numeric_limits::max()) < *defaultPri); + EXPECT_TRUE( + *defaultPri < HTTPPriorityQueue::Priority( + 4, false, std::numeric_limits::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