diff --git a/quic/congestion_control/Bbr.cpp b/quic/congestion_control/Bbr.cpp index 811e76a1c..e3a452b5f 100644 --- a/quic/congestion_control/Bbr.cpp +++ b/quic/congestion_control/Bbr.cpp @@ -268,13 +268,27 @@ void BbrCongestionController::onPacketAcked( auto updatedMaxAckDelay = std::chrono::duration_cast(clampMaxAckDelay( conn_, minRtt() / ackFrequencyConfig->minRttDivisor)); - if (!lastMaxAckDelay_ || lastMaxAckDelay_.value() != updatedMaxAckDelay) { + // If we are either in STARTUP or haven't sent enough packets, based on + // config. + bool shouldUseInitThreshold = + (ackFrequencyConfig->useSmallThresholdDuringStartup && + state_ == BbrState::Startup) || + (!ackFrequencyConfig->useSmallThresholdDuringStartup && + (conn_.lossState.totalAckElicitingPacketsSent <= + conn_.transportSettings.rxPacketsBeforeAckInitThreshold)); + // Default to 2 as the init threshold, which has been shown to be + // a good choice empirically. + uint32_t ackThreshold = + shouldUseInitThreshold ? 2 : ackFrequencyConfig->ackElicitingThreshold; + if ((!lastMaxAckDelay_ || lastMaxAckDelay_.value() != updatedMaxAckDelay) || + (!lastAckThreshold_ || lastAckThreshold_.value() != ackThreshold)) { requestPeerAckFrequencyChange( conn_, - ackFrequencyConfig->ackElicitingThreshold, + ackThreshold, updatedMaxAckDelay, ackFrequencyConfig->reorderingThreshold); lastMaxAckDelay_ = updatedMaxAckDelay; + lastAckThreshold_ = ackThreshold; } } updateCwnd(ack.ackedBytes, excessiveBytes); diff --git a/quic/congestion_control/Bbr.h b/quic/congestion_control/Bbr.h index a5b83a924..ed1e3ee64 100644 --- a/quic/congestion_control/Bbr.h +++ b/quic/congestion_control/Bbr.h @@ -311,6 +311,7 @@ class BbrCongestionController : public CongestionController { // The last max ACK delay requested, so we don't end up sending // them too frequently. folly::Optional lastMaxAckDelay_; + folly::Optional lastAckThreshold_; friend std::ostream& operator<<( std::ostream& os, diff --git a/quic/congestion_control/test/BbrTest.cpp b/quic/congestion_control/test/BbrTest.cpp index 2125f792e..a1d38ff8c 100644 --- a/quic/congestion_control/test/BbrTest.cpp +++ b/quic/congestion_control/test/BbrTest.cpp @@ -138,7 +138,7 @@ TEST_F(BbrTest, Recovery) { EXPECT_FALSE(bbr.inRecovery()); // Only one update should have been issued. ASSERT_EQ(conn.pendingEvents.frames.size(), 1); - AckFrequencyFrame expectedFrame{0, 10, 5000, 3}; + AckFrequencyFrame expectedFrame{0, 2, 5000, 3}; auto ackFrequencyFrame = conn.pendingEvents.frames.front().asAckFrequencyFrame(); EXPECT_EQ(expectedFrame, *ackFrequencyFrame); diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 23a8306f7..e8b0a7db6 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -1048,7 +1048,8 @@ void QuicServerTransport::registerAllTransportKnobParamHandlers() { val, ackFrequencyConfig.ackElicitingThreshold, ackFrequencyConfig.reorderingThreshold, - ackFrequencyConfig.minRttDivisor); + ackFrequencyConfig.minRttDivisor, + ackFrequencyConfig.useSmallThresholdDuringStartup); // Sanity check the values. parseSuccess = parseSuccess && ackFrequencyConfig.ackElicitingThreshold > 1 && @@ -1063,9 +1064,11 @@ void QuicServerTransport::registerAllTransportKnobParamHandlers() { "ackElicitingThreshold={}, " "reorderingThreshold={}, " "minRttDivisor={}", + "useSmallThresholdDuringStartup={}", ackFrequencyConfig.ackElicitingThreshold, ackFrequencyConfig.reorderingThreshold, - ackFrequencyConfig.minRttDivisor); + ackFrequencyConfig.minRttDivisor, + ackFrequencyConfig.useSmallThresholdDuringStartup); serverTransport->conn_->transportSettings.bbrConfig .ackFrequencyConfig = ackFrequencyConfig; serverTransport->conn_->transportSettings.autoAdaptiveForDsr = false; diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index d3b52dbc6..b09ac585a 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -4805,11 +4805,11 @@ TEST_F(QuicServerTransportTest, TestAckFrequencyPolicyKnobHandler) { EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "1,1,1"}}); + "1,1,1,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "10,3,1"}}); + "10,3,1,1"}}); ASSERT_TRUE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); EXPECT_EQ( server->getTransportSettings() @@ -4823,31 +4823,35 @@ TEST_F(QuicServerTransportTest, TestAckFrequencyPolicyKnobHandler) { server->getTransportSettings() .bbrConfig.ackFrequencyConfig->minRttDivisor, 1); + EXPECT_EQ( + server->getTransportSettings() + .bbrConfig.ackFrequencyConfig->useSmallThresholdDuringStartup, + true); server->getNonConstConn() .transportSettings.bbrConfig.ackFrequencyConfig.reset(); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "10,3,-1"}}); + "10,3,-1,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "10,-1,1"}}); + "10,-1,1,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "-1,3,1"}}); + "-1,3,1,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "10,3,0"}}); + "10,3,0,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "10,1,1"}}); + "10,1,1,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), - "1,3,1"}}); + "1,3,1,1"}}); EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); } diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 614af40d8..7590a192d 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -48,6 +48,8 @@ struct BbrConfig { uint64_t ackElicitingThreshold{kDefaultRxPacketsBeforeAckAfterInit}; uint64_t reorderingThreshold{kReorderingThreshold}; uint32_t minRttDivisor{2}; + // Threshold to use early in the connection. + bool useSmallThresholdDuringStartup{false}; }; folly::Optional ackFrequencyConfig; };