More modularity in ACK handling code [1/n]

Summary: Many functions in the ACK handling code are very verbose, and readability would improve from greater modularity. Reviewed By: mjoras Differential Revision: D67044814 fbshipit-source-id: 05f8f238625da7f056aa24f6fdff7397ece9e25c
facebook · Dec 13, 2024 · b4b4a14 · b4b4a14
1 parent 9fe7f83
commit b4b4a14
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 52 deletions.
diff --git a/quic/state/AckHandlers.cpp b/quic/state/AckHandlers.cpp
@@ -65,8 +65,6 @@ AckEvent processAckFrame(
     const AckedFrameVisitor& ackedFrameVisitor,
     const LossVisitor& lossVisitor,
     const TimePoint& ackReceiveTime) {
-  const auto nowTime = Clock::now();
-
   updateEcnCountEchoed(conn, pnSpace, frame);
 
   // TODO: send error if we get an ack for a packet we've not sent t18721184
@@ -159,57 +157,10 @@ AckEvent processAckFrame(
       ++dsrPacketsAcked;
     }
 
-    // Update RTT if current packet is the largestAcked in the frame
-    //
-    // An RTT sample is generated using only the largest acknowledged packet
-    // in the received ACK frame. To avoid generating multiple RTT samples
-    // for a single packet, an ACK frame SHOULD NOT be used to update RTT
-    // estimates if it does not newly acknowledge the largest acknowledged
-    // packet (RFC9002). This includes for minRTT estimates.
     if (!ack.implicit && currentPacketNum == frame.largestAcked) {
-      auto ackReceiveTimeOrNow =
-          ackReceiveTime > ackedPacketIterator->metadata.time ? ackReceiveTime
-                                                              : nowTime;
-
-      // Use ceil to round up to next microsecond during conversion.
-      //
-      // While unlikely, it's still technically possible for the RTT to be
-      // zero; ignore if this is the case.
-      auto rttSample = std::chrono::ceil<std::chrono::microseconds>(
-          ackReceiveTimeOrNow - ackedPacketIterator->metadata.time);
-      if (rttSample != rttSample.zero()) {
-        // notify observers
-        {
-          const auto socketObserverContainer =
-              conn.getSocketObserverContainer();
-          if (socketObserverContainer &&
-              socketObserverContainer->hasObserversForEvent<
-                  SocketObserverInterface::Events::rttSamples>()) {
-            socketObserverContainer->invokeInterfaceMethod<
-                SocketObserverInterface::Events::rttSamples>(
-                [event = SocketObserverInterface::PacketRTT(
-                     ackReceiveTimeOrNow,
-                     rttSample,
-                     frame.ackDelay,
-                     *ackedPacketIterator)](auto observer, auto observed) {
-                  observer->rttSampleGenerated(observed, event);
-                });
-          }
-        }
-
-        // update AckEvent RTTs, which are used by CCA and other processing
-        CHECK(!ack.rttSample.has_value());
-        CHECK(!ack.rttSampleNoAckDelay.has_value());
-        ack.rttSample = rttSample;
-        ack.rttSampleNoAckDelay = (rttSample >= frame.ackDelay)
-            ? OptionalMicros(std::chrono::ceil<std::chrono::microseconds>(
-                  rttSample - frame.ackDelay))
-            : std::nullopt;
-
-        // update transport RTT
-        updateRtt(conn, rttSample, frame.ackDelay);
-      } // if (rttSample != rttSample.zero())
-    } // if (!ack.implicit && currentPacketNum == frame.largestAcked)
+      updateRttForLargestAckedPacket(
+          ack, conn, *ackedPacketIterator, frame, ackReceiveTime);
+    }
 
     // Remove this ClonedPacketIdentifier from the
     // outstandings.clonedPacketIdentifiers set
@@ -633,6 +584,58 @@ void commonAckVisitorForAckFrame(
   }
 }
 
+void updateRttForLargestAckedPacket(
+    AckEvent& ackEvent,
+    QuicConnectionStateBase& conn,
+    OutstandingPacketWrapper& packet,
+    const ReadAckFrame& frame,
+    const TimePoint& ackReceiveTime) {
+  CHECK_EQ(packet.packet.header.getPacketSequenceNum(), frame.largestAcked)
+      << "An RTT sample is generated using only the largest acknowledged packet "
+      << "in the received ACK frame.";
+  CHECK(!ackEvent.implicit)
+      << "An RTT sample cannot be generated for an implicit ACK.";
+
+  auto ackReceiveTimeOrNow =
+      ackReceiveTime > packet.metadata.time ? ackReceiveTime : Clock::now();
+
+  // Use ceil to round up to next microsecond during conversion.
+  //
+  // While unlikely, it's still technically possible for the RTT to be
+  // zero; ignore if this is the case.
+  auto rttSample = std::chrono::ceil<std::chrono::microseconds>(
+      ackReceiveTimeOrNow - packet.metadata.time);
+  if (rttSample != rttSample.zero()) {
+    // notify observers
+    {
+      const auto socketObserverContainer = conn.getSocketObserverContainer();
+      if (socketObserverContainer &&
+          socketObserverContainer->hasObserversForEvent<
+              SocketObserverInterface::Events::rttSamples>()) {
+        socketObserverContainer->invokeInterfaceMethod<
+            SocketObserverInterface::Events::rttSamples>(
+            [event = SocketObserverInterface::PacketRTT(
+                 ackReceiveTimeOrNow, rttSample, frame.ackDelay, packet)](
+                auto observer, auto observed) {
+              observer->rttSampleGenerated(observed, event);
+            });
+      }
+    }
+
+    // update AckEvent RTTs, which are used by CCA and other processing
+    CHECK(!ackEvent.rttSample.has_value());
+    CHECK(!ackEvent.rttSampleNoAckDelay.has_value());
+    ackEvent.rttSample = rttSample;
+    ackEvent.rttSampleNoAckDelay = (rttSample >= frame.ackDelay)
+        ? OptionalMicros(std::chrono::ceil<std::chrono::microseconds>(
+              rttSample - frame.ackDelay))
+        : std::nullopt;
+
+    // update transport RTT
+    updateRtt(conn, rttSample, frame.ackDelay);
+  } // if (rttSample != rttSample.zero())
+}
+
 void incrementEcnCountForAckedPacket(
     QuicConnectionStateBase& conn,
     PacketNumberSpace pnSpace) {

diff --git a/quic/state/AckHandlers.h b/quic/state/AckHandlers.h
@@ -77,6 +77,20 @@ void parseAckReceiveTimestamps(
     folly::F14FastMap<PacketNum, uint64_t>& packetReceiveTimeStamps,
     Optional<PacketNum> firstPacketNum);
 
+/**
+ * An RTT sample is generated using only the largest acknowledged packet
+ * in the received ACK frame. To avoid generating multiple RTT samples
+ * for a single packet, an ACK frame SHOULD NOT be used to update RTT
+ * estimates if it does not newly acknowledge the largest acknowledged
+ * packet (RFC9002). This includes for minRTT estimates.
+ */
+void updateRttForLargestAckedPacket(
+    AckEvent& ackEvent,
+    QuicConnectionStateBase& conn,
+    OutstandingPacketWrapper& packet,
+    const ReadAckFrame& frame,
+    const TimePoint& ackReceiveTime);
+
 /**
  * Update the outgoing ECN marking count for an outstanding packet that has been
  * acked. If a packet is acked and the connection is using ECN/L4S, this