Cache the negotiated config for ACKs once the transport parameters are received

Summary: Cache the negotiated config for what ACK type to write and which fields to use once the peer transport parameters are available. This avoids computing the config with every ack frame being written.

Reviewed By: sharmafb

Differential Revision: D70004436

fbshipit-source-id: 79354f5137c77353c3a97d4c41782a700622e986

Author: jbeshay

quic/api/QuicAckScheduler.cpp

@@ -63,32 +63,13 @@ Optional<PacketNum> AckScheduler::writeNextAcks(
 
   Optional<WriteAckFrameResult> ackWriteResult;
 
-  bool isAckReceiveTimestampsSupported =
-      conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer &&
-      conn_.maybePeerAckReceiveTimestampsConfig;
-
   uint64_t peerRequestedTimestampsCount =
       conn_.maybePeerAckReceiveTimestampsConfig.has_value()
       ? conn_.maybePeerAckReceiveTimestampsConfig.value()
             .maxReceiveTimestampsPerAck
       : 0;
 
-  uint64_t extendedAckSupportedAndEnabled =
-      conn_.peerAdvertisedExtendedAckFeatures &
-      conn_.transportSettings.enableExtendedAckFeatures;
-  // Disable the ECN fields if we are not reading them
-  if (!conn_.transportSettings.readEcnOnIngress) {
-    extendedAckSupportedAndEnabled &= ~static_cast<ExtendedAckFeatureMaskType>(
-        ExtendedAckFeatureMask::ECN_COUNTS);
-  }
-  // Disable the receive timestamps fields if we have not regoatiated receive
-  // timestamps support
-  if (!isAckReceiveTimestampsSupported || (peerRequestedTimestampsCount == 0)) {
-    extendedAckSupportedAndEnabled &= ~static_cast<ExtendedAckFeatureMaskType>(
-        ExtendedAckFeatureMask::RECEIVE_TIMESTAMPS);
-  }
-
-  if (extendedAckSupportedAndEnabled > 0) {
+  if (conn_.negotiatedExtendedAckFeatures > 0) {
     // The peer supports extended ACKs and we have them enabled.
     ackWriteResult = writeAckFrame(
         meta,
@@ -97,7 +78,7 @@ Optional<PacketNum> AckScheduler::writeNextAcks(
         conn_.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer
             .value_or(AckReceiveTimestampsConfig()),
         peerRequestedTimestampsCount,
-        extendedAckSupportedAndEnabled);
+        conn_.negotiatedExtendedAckFeatures);
   } else if (
       conn_.transportSettings.readEcnOnIngress &&
       (meta.ackState.ecnECT0CountReceived ||
@@ -107,8 +88,7 @@ Optional<PacketNum> AckScheduler::writeNextAcks(
     // frame. In this case, we give ACK_ECN precedence over
     // ACK_RECEIVE_TIMESTAMPS.
     ackWriteResult = writeAckFrame(meta, builder, FrameType::ACK_ECN);
-  } else if (
-      isAckReceiveTimestampsSupported && (peerRequestedTimestampsCount > 0)) {
+  } else if (conn_.negotiatedAckReceiveTimestampSupport) {
     // Use ACK_RECEIVE_TIMESTAMPS if its enabled on both endpoints AND the
     // peer requests at least 1 timestamp
     ackWriteResult = writeAckFrame(

quic/api/QuicTransportFunctions.cpp

@@ -2131,4 +2131,35 @@ void maybeScheduleAckForCongestionFeedback(
   }
 }
 
+void updateNegotiatedAckFeatures(QuicConnectionStateBase& conn) {
+  bool isAckReceiveTimestampsSupported =
+      conn.transportSettings.maybeAckReceiveTimestampsConfigSentToPeer &&
+      conn.maybePeerAckReceiveTimestampsConfig;
+
+  uint64_t peerRequestedTimestampsCount =
+      conn.maybePeerAckReceiveTimestampsConfig.has_value()
+      ? conn.maybePeerAckReceiveTimestampsConfig.value()
+            .maxReceiveTimestampsPerAck
+      : 0;
+
+  conn.negotiatedAckReceiveTimestampSupport =
+      isAckReceiveTimestampsSupported && (peerRequestedTimestampsCount > 0);
+
+  conn.negotiatedExtendedAckFeatures = conn.peerAdvertisedExtendedAckFeatures &
+      conn.transportSettings.enableExtendedAckFeatures;
+  // Disable the ECN fields if we are not reading them
+  if (!conn.transportSettings.readEcnOnIngress) {
+    conn.negotiatedExtendedAckFeatures &=
+        ~static_cast<ExtendedAckFeatureMaskType>(
+            ExtendedAckFeatureMask::ECN_COUNTS);
+  }
+  // Disable the receive timestamps fields if we have not regoatiated receive
+  // timestamps support
+  if (!conn.negotiatedAckReceiveTimestampSupport) {
+    conn.negotiatedExtendedAckFeatures &=
+        ~static_cast<ExtendedAckFeatureMaskType>(
+            ExtendedAckFeatureMask::RECEIVE_TIMESTAMPS);
+  }
+}
+
 } // namespace quic

quic/api/QuicTransportFunctions.h

@@ -345,4 +345,5 @@ void maybeAddPacketMark(
 void maybeScheduleAckForCongestionFeedback(
     const ReceivedUdpPacket& receivedPacket,
     AckState& ackState);
+void updateNegotiatedAckFeatures(QuicConnectionStateBase& conn);
 } // namespace quic

quic/api/test/QuicPacketSchedulerTest.cpp

@@ -2816,6 +2816,7 @@ TEST_F(QuicAckSchedulerTest, WriteAckReceiveTimestampsWhenEnabled) {
   conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer =
       AckReceiveTimestampsConfig();
 
+  updateNegotiatedAckFeatures(*conn_);
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
 
@@ -2874,6 +2875,7 @@ TEST_F(QuicAckSchedulerTest, AckExtendedNotSentIfNotSupported) {
   conn_->transportSettings.enableExtendedAckFeatures =
       3; // ECN + ReceiveTimestamps
   conn_->peerAdvertisedExtendedAckFeatures = 0;
+  updateNegotiatedAckFeatures(*conn_);
 
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
@@ -2902,6 +2904,7 @@ TEST_F(QuicAckSchedulerTest, AckExtendedNotSentIfNotEnabled) {
 
   conn_->transportSettings.enableExtendedAckFeatures = 0;
   conn_->peerAdvertisedExtendedAckFeatures = 3; // ECN + ReceiveTimestamps;
+  updateNegotiatedAckFeatures(*conn_);
 
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
@@ -2938,6 +2941,7 @@ TEST_F(
   conn_->maybePeerAckReceiveTimestampsConfig = AckReceiveTimestampsConfig();
   // We don't have an ART config (i.e. we can't sent ART)
   conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer = none;
+  updateNegotiatedAckFeatures(*conn_);
 
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
@@ -2972,6 +2976,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedNotSentIfECNFeatureNotSupported) {
   conn_->transportSettings.maybeAckReceiveTimestampsConfigSentToPeer =
       AckReceiveTimestampsConfig();
 
+  updateNegotiatedAckFeatures(*conn_);
+
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
 
@@ -3009,6 +3015,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedWithAllFeatures) {
   // We can read ECN
   conn_->transportSettings.readEcnOnIngress = true;
 
+  updateNegotiatedAckFeatures(*conn_);
+
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
 
@@ -3046,6 +3054,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedTakesPrecedenceOverECN) {
   // We can read ECN
   conn_->transportSettings.readEcnOnIngress = true;
 
+  updateNegotiatedAckFeatures(*conn_);
+
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
 
@@ -3083,6 +3093,8 @@ TEST_F(QuicAckSchedulerTest, AckExtendedTakesPrecedenceOverReceiveTimestamps) {
   // We can read ECN
   conn_->transportSettings.readEcnOnIngress = true;
 
+  updateNegotiatedAckFeatures(*conn_);
+
   AckScheduler ackScheduler(*conn_, ackState_);
   ASSERT_TRUE(ackScheduler.hasPendingAcks());
 

quic/client/QuicClientTransportLite.cpp

@@ -798,6 +798,8 @@ void QuicClientTransportLite::processUdpPacketData(
           }
         }
       }
+      updateNegotiatedAckFeatures(*conn_);
+
       // TODO This sucks, but manually update the max packet size until we fix
       // 0-rtt transport parameters.
       if (conn_->transportSettings.canIgnorePathMTU &&

quic/server/state/ServerStateMachine.cpp

@@ -445,6 +445,7 @@ void updateHandshakeState(QuicServerConnectionState& conn) {
           TransportErrorCode::TRANSPORT_PARAMETER_ERROR);
     }
     processClientInitialParams(conn, std::move(*clientParams));
+    updateNegotiatedAckFeatures(conn);
   }
   if (oneRttReadCipher) {
     if (conn.qLogger) {

quic/state/StateData.h

@@ -705,6 +705,12 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction {
   bool peerAdvertisedKnobFrameSupport{false};
 
   ExtendedAckFeatureMaskType peerAdvertisedExtendedAckFeatures{0};
+
+  // Negotiated ACK related config. These don't change throughout the connection
+  // so cache them once we've receive the relevant transport parameters.
+  bool negotiatedAckReceiveTimestampSupport{false};
+  ExtendedAckFeatureMaskType negotiatedExtendedAckFeatures{0};
+
   // Retransmission policies map.
   folly::F14FastMap<StreamGroupId, QuicStreamGroupRetransmissionPolicy>
       retransmissionPolicies;