From e56d4bd7f655637494826d4ace26d96f9d84f39c Mon Sep 17 00:00:00 2001 From: Paul Farcasanu Date: Tue, 28 Mar 2023 16:00:38 -0700 Subject: [PATCH] refactor setStreamPriority to accept Priority struct Summary: ## Context Context: see summary for D43854603. ## In this diff In this diff, make way for easily adding new elements to Priority struct. Reviewed By: afrind Differential Revision: D43882907 fbshipit-source-id: f74f6ec41162a970dcd666bfa5192676f968d740 --- quic/api/QuicSocket.h | 16 ++++++++++++++-- quic/api/QuicTransportBase.cpp | 12 ++++-------- quic/api/QuicTransportBase.h | 3 +-- quic/api/test/MockQuicSocket.h | 2 +- quic/api/test/QuicPacketSchedulerTest.cpp | 6 +++--- quic/api/test/QuicTransportBaseTest.cpp | 4 ++-- quic/api/test/QuicTransportTest.cpp | 6 +++--- quic/state/QuicStreamManager.cpp | 8 ++------ quic/state/QuicStreamManager.h | 2 +- quic/state/test/QuicStreamManagerTest.cpp | 16 +++++++++------- 10 files changed, 40 insertions(+), 35 deletions(-) diff --git a/quic/api/QuicSocket.h b/quic/api/QuicSocket.h index b6eba51a5..319d8931f 100644 --- a/quic/api/QuicSocket.h +++ b/quic/api/QuicSocket.h @@ -567,8 +567,20 @@ class QuicSocket { * Set stream priority. * level: can only be in [0, 7]. */ - virtual folly::Expected - setStreamPriority(StreamId id, PriorityLevel level, bool incremental) = 0; + folly::Expected + setStreamPriority(StreamId id, PriorityLevel level, bool incremental) { + return setStreamPriority(id, Priority(level, incremental)); + } + + /** + * Set stream priority. + * level: can only be in [0, 7]. + * incremental: true/false + * orderId: uint64 + */ + virtual folly::Expected setStreamPriority( + StreamId id, + Priority priority) = 0; /** * Get stream priority. diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index d15c58797..95e77f881 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -3311,14 +3311,11 @@ const TransportSettings& QuicTransportBase::getTransportSettings() const { } folly::Expected -QuicTransportBase::setStreamPriority( - StreamId id, - PriorityLevel level, - bool incremental) { +QuicTransportBase::setStreamPriority(StreamId id, Priority priority) { if (closeState_ != CloseState::OPEN) { return folly::makeUnexpected(LocalErrorCode::CONNECTION_CLOSED); } - if (level > kDefaultMaxPriority) { + if (priority.level > kDefaultMaxPriority) { return folly::makeUnexpected(LocalErrorCode::INVALID_OPERATION); } if (!conn_->streamManager->streamExists(id)) { @@ -3327,10 +3324,9 @@ QuicTransportBase::setStreamPriority( } // It's not an error to prioritize a stream after it's sent its FIN - this // can reprioritize retransmissions. - bool updated = - conn_->streamManager->setStreamPriority(id, level, incremental); + bool updated = conn_->streamManager->setStreamPriority(id, priority); if (updated && conn_->qLogger) { - conn_->qLogger->addPriorityUpdate(id, level, incremental); + conn_->qLogger->addPriorityUpdate(id, priority.level, priority.incremental); } return folly::unit; } diff --git a/quic/api/QuicTransportBase.h b/quic/api/QuicTransportBase.h index 3826bebdc..80b45c514 100644 --- a/quic/api/QuicTransportBase.h +++ b/quic/api/QuicTransportBase.h @@ -337,8 +337,7 @@ class QuicTransportBase : public QuicSocket, QuicStreamPrioritiesObserver { folly::Expected setStreamPriority( StreamId id, - PriorityLevel level, - bool incremental) override; + Priority priority) override; folly::Expected getStreamPriority( StreamId id) override; diff --git a/quic/api/test/MockQuicSocket.h b/quic/api/test/MockQuicSocket.h index c45df1d87..3894187d8 100644 --- a/quic/api/test/MockQuicSocket.h +++ b/quic/api/test/MockQuicSocket.h @@ -126,7 +126,7 @@ class MockQuicSocket : public QuicSocket { MOCK_METHOD( (folly::Expected), setStreamPriority, - (StreamId, uint8_t, bool)); + (StreamId, Priority)); MOCK_METHOD( (folly::Expected), getStreamPriority, diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 9d756d370..4a361c8e0 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -1670,12 +1670,12 @@ TEST_F(QuicPacketSchedulerTest, HighPriNewDataBeforeLowPriLossData) { lowPriStream->lossBuffer.push_back( StreamBuffer(folly::IOBuf::copyBuffer("Onegin"), 0, false)); conn.streamManager->updateWritableStreams(*lowPriStream); - conn.streamManager->setStreamPriority(lowPriStream->id, 5, false); + conn.streamManager->setStreamPriority(lowPriStream->id, Priority(5, false)); auto inBuf = buildRandomInputData(conn.udpSendPacketLen * 10); writeDataToQuicStream(*highPriStream, inBuf->clone(), false); conn.streamManager->updateWritableStreams(*highPriStream); - conn.streamManager->setStreamPriority(highPriStream->id, 0, false); + conn.streamManager->setStreamPriority(highPriStream->id, Priority(0, false)); StreamFrameScheduler scheduler(conn); ShortHeader shortHeader( @@ -1816,7 +1816,7 @@ TEST_F(QuicPacketSchedulerTest, WriteLossWithoutFlowControlSequential) { conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = 1000; auto streamId = (*conn.streamManager->createNextBidirectionalStream())->id; - conn.streamManager->setStreamPriority(streamId, 0, false); + conn.streamManager->setStreamPriority(streamId, Priority(0, false)); auto stream = conn.streamManager->findStream(streamId); auto data = buildRandomInputData(1000); writeDataToQuicStream(*stream, std::move(data), true); diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index 63d424de5..b1ce65671 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -4217,7 +4217,7 @@ TEST_P(QuicTransportImplTestBase, BackgroundModeChangeWithStreamChanges) { EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(_)) .Times(0); // Backgound params not set auto stream = manager.createNextUnidirectionalStream().value(); - manager.setStreamPriority(stream->id, 1, false); + manager.setStreamPriority(stream->id, Priority(1, false)); EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(0.5)) .Times(1); // On setting the background params @@ -4236,7 +4236,7 @@ TEST_P(QuicTransportImplTestBase, BackgroundModeChangeWithStreamChanges) { EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(1.0)) .Times(1); // On increasing the priority of one of the streams - manager.setStreamPriority(stream3id, 0, false); + manager.setStreamPriority(stream3id, Priority(0, false)); EXPECT_CALL(*rawCongestionController, setBandwidthUtilizationFactor(1.0)) .Times(1); // a new lower priority stream does not affect the utlization diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index ef8dd6306..d9bfe218f 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -1629,7 +1629,7 @@ TEST_F(QuicTransportTest, WriteSmall) { EXPECT_CALL(*socket_, write(_, _)).WillOnce(Invoke(bufLength)); transport_->writeChain(stream, buf->clone(), false); - transport_->setStreamPriority(stream, 0, false); + transport_->setStreamPriority(stream, Priority(0, false)); loopForWrites(); auto& conn = transport_->getConnectionState(); verifyCorrectness(conn, 0, stream, *buf); @@ -2761,7 +2761,7 @@ TEST_F(QuicTransportTest, NonWritableStreamAPI) { // Check that write-side APIs return an error auto res2 = transport_->notifyPendingWriteOnStream(streamId, &writeCallback_); EXPECT_EQ(LocalErrorCode::STREAM_CLOSED, res2.error()); - auto res3 = transport_->setStreamPriority(streamId, 0, false); + auto res3 = transport_->setStreamPriority(streamId, Priority(0, false)); EXPECT_FALSE(res3.hasError()); } @@ -4744,7 +4744,7 @@ TEST_F(QuicTransportTest, GetStreamPacketsTxedMultiplePackets) { TEST_F(QuicTransportTest, PrioritySetAndGet) { auto stream = transport_->createBidirectionalStream().value(); EXPECT_EQ(kDefaultPriority, transport_->getStreamPriority(stream).value()); - transport_->setStreamPriority(stream, 0, false); + transport_->setStreamPriority(stream, Priority(0, false)); EXPECT_EQ(Priority(0, false), transport_->getStreamPriority(stream).value()); auto nonExistStreamPri = transport_->getStreamPriority(stream + 4); EXPECT_TRUE(nonExistStreamPri.hasError()); diff --git a/quic/state/QuicStreamManager.cpp b/quic/state/QuicStreamManager.cpp index 247684bc1..9b64da63b 100644 --- a/quic/state/QuicStreamManager.cpp +++ b/quic/state/QuicStreamManager.cpp @@ -228,13 +228,9 @@ bool QuicStreamManager::consumeMaxLocalUnidirectionalStreamIdIncreased() { return res; } -bool QuicStreamManager::setStreamPriority( - StreamId id, - PriorityLevel level, - bool incremental) { +bool QuicStreamManager::setStreamPriority(StreamId id, Priority newPriority) { auto stream = findStream(id); if (stream) { - Priority newPriority(level, incremental); if (stream->priority == newPriority) { return false; } @@ -250,7 +246,7 @@ bool QuicStreamManager::setStreamPriority( } notifyStreamPriorityChanges(); } - // If this stream is already in the writable or loss queus, update the + // If this stream is already in the writable or loss queues, update the // priority there. writableStreams_.updateIfExist(id, stream->priority); writableDSRStreams_.updateIfExist(id, stream->priority); diff --git a/quic/state/QuicStreamManager.h b/quic/state/QuicStreamManager.h index b60409806..1f3ea9738 100644 --- a/quic/state/QuicStreamManager.h +++ b/quic/state/QuicStreamManager.h @@ -403,7 +403,7 @@ class QuicStreamManager { * passed in values are different from current priority. Return true if * stream priority is update, false otherwise. */ - bool setStreamPriority(StreamId id, PriorityLevel level, bool incremental); + bool setStreamPriority(StreamId id, Priority priority); // TODO figure out a better interface here. /* diff --git a/quic/state/test/QuicStreamManagerTest.cpp b/quic/state/test/QuicStreamManagerTest.cpp index 0e8fce0c2..33ddd6fc7 100644 --- a/quic/state/test/QuicStreamManagerTest.cpp +++ b/quic/state/test/QuicStreamManagerTest.cpp @@ -66,12 +66,14 @@ TEST_P(QuicStreamManagerTest, SkipRedundantPriorityUpdate) { Priority currentPriority = stream.value()->priority; EXPECT_TRUE(manager.setStreamPriority( streamId, - (currentPriority.level + 1) % (kDefaultMaxPriority + 1), - !currentPriority.incremental)); + Priority( + (currentPriority.level + 1) % (kDefaultMaxPriority + 1), + !currentPriority.incremental))); EXPECT_FALSE(manager.setStreamPriority( streamId, - (currentPriority.level + 1) % (kDefaultMaxPriority + 1), - !currentPriority.incremental)); + Priority( + (currentPriority.level + 1) % (kDefaultMaxPriority + 1), + !currentPriority.incremental))); } TEST_P(QuicStreamManagerTest, TestAppIdleCreateBidiStream) { @@ -672,7 +674,7 @@ TEST_P(QuicStreamManagerTest, NotifyOnStreamPriorityChanges) { auto stream = manager.createNextUnidirectionalStream().value(); EXPECT_EQ(manager.getHighestPriorityLevel(), kDefaultPriority.level); - manager.setStreamPriority(stream->id, 1, false); + manager.setStreamPriority(stream->id, Priority(1, false)); EXPECT_EQ(manager.getHighestPriorityLevel(), 1); EXPECT_CALL(mObserver, onStreamPrioritiesChange()) @@ -692,7 +694,7 @@ TEST_P(QuicStreamManagerTest, NotifyOnStreamPriorityChanges) { EXPECT_CALL(mObserver, onStreamPrioritiesChange()) .Times(1); // On increasing the priority of one of the streams - manager.setStreamPriority(stream3->id, 0, false); + manager.setStreamPriority(stream3->id, Priority(0, false)); EXPECT_EQ(manager.getHighestPriorityLevel(), 0); EXPECT_CALL(mObserver, onStreamPrioritiesChange()) @@ -732,7 +734,7 @@ TEST_P(QuicStreamManagerTest, StreamPriorityExcludesControl) { auto stream = manager.createNextUnidirectionalStream().value(); EXPECT_EQ(manager.getHighestPriorityLevel(), kDefaultPriority.level); - manager.setStreamPriority(stream->id, 1, false); + manager.setStreamPriority(stream->id, Priority(1, false)); EXPECT_EQ(manager.getHighestPriorityLevel(), 1); manager.setStreamAsControl(*stream);