Differentiate ACK_FREQUENCY policy early in connection.

Summary: This changes the experimental behavior of BBR sending ACK_FREQUENCY frames. Now instead of using the static ack eliciting threshold, early in the connection BBR will set the threshold to 2 which has been empirically shown to be a good "initial" value.

Reviewed By: jbeshay

Differential Revision: D40438721

fbshipit-source-id: 184b8025581b6152eea97f9b557b6343d980322d

quic/congestion_control/Bbr.cpp

@@ -268,13 +268,27 @@ void BbrCongestionController::onPacketAcked(
     auto updatedMaxAckDelay =
         std::chrono::duration_cast<std::chrono::milliseconds>(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);

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<std::chrono::milliseconds> lastMaxAckDelay_;
+  folly::Optional<uint32_t> lastAckThreshold_;
 
   friend std::ostream& operator<<(
       std::ostream& os,

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);

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;

quic/server/test/QuicServerTransportTest.cpp

@@ -4805,11 +4805,11 @@ TEST_F(QuicServerTransportTest, TestAckFrequencyPolicyKnobHandler) {
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "1,1,1"}});
+        "1,1,1,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(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<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "10,3,-1"}});
+        "10,3,-1,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "10,-1,1"}});
+        "10,-1,1,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "-1,3,1"}});
+        "-1,3,1,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "10,3,0"}});
+        "10,3,0,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "10,1,1"}});
+        "10,1,1,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
   server->handleKnobParams(
       {{static_cast<uint64_t>(TransportKnobParamId::ACK_FREQUENCY_POLICY),
-        "1,3,1"}});
+        "1,3,1,1"}});
   EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig);
 }
 

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> ackFrequencyConfig;
 };