From 3277f2389d68eb7d189e74d9078bfdd794dc6f7c Mon Sep 17 00:00:00 2001
From: Aman Sharma <amsharma@meta.com>
Date: Thu, 15 Aug 2024 20:13:01 -0700
Subject: [PATCH] More modularity in AckHandlers.cpp [1/n]

Summary: `processAckFrame` is very bloated. It may be due for a larger refactor, but as of now, there are some fairly simple things we can do to make it cleaner.

Reviewed By: mjoras

Differential Revision: D61290801

fbshipit-source-id: e13789b82f6ca86f7959c940573aa887abdda818
---
 quic/state/AckHandlers.cpp | 79 ++++++++++++++++++++------------------
 quic/state/AckHandlers.h   |  8 ++++
 2 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/quic/state/AckHandlers.cpp b/quic/state/AckHandlers.cpp
index 7ae039a11..cf5600a84 100644
--- a/quic/state/AckHandlers.cpp
+++ b/quic/state/AckHandlers.cpp
@@ -229,45 +229,8 @@ AckEvent processAckFrame(
       // If we hit a packet which has been lost we need to count the spurious
       // loss and ignore all other processing.
       if (rPacketIt->declaredLost) {
-        CHECK_GT(conn.outstandings.declaredLostCount, 0);
-        conn.lossState.totalPacketsSpuriouslyMarkedLost++;
-        if (conn.transportSettings.useAdaptiveLossReorderingThresholds) {
-          if (rPacketIt->metadata.lossReorderDistance.has_value() &&
-              rPacketIt->metadata.lossReorderDistance.value() >
-                  conn.lossState.reorderingThreshold) {
-            conn.lossState.reorderingThreshold =
-                rPacketIt->metadata.lossReorderDistance.value();
-          }
-        }
-        if (conn.transportSettings.useAdaptiveLossTimeThresholds) {
-          if (rPacketIt->metadata.lossTimeoutDividend.has_value() &&
-              rPacketIt->metadata.lossTimeoutDividend.value() >
-                  conn.transportSettings.timeReorderingThreshDividend) {
-            conn.transportSettings.timeReorderingThreshDividend =
-                rPacketIt->metadata.lossTimeoutDividend.value();
-          }
-        }
-        if (conn.transportSettings.removeFromLossBufferOnSpurious) {
-          for (auto& f : rPacketIt->packet.frames) {
-            auto streamFrame = f.asWriteStreamFrame();
-            if (streamFrame) {
-              auto stream =
-                  conn.streamManager->findStream(streamFrame->streamId);
-              if (stream) {
-                stream->removeFromLossBuffer(
-                    streamFrame->offset, streamFrame->len, streamFrame->fin);
-                stream->updateAckedIntervals(
-                    streamFrame->offset, streamFrame->len, streamFrame->fin);
-                conn.streamManager->updateWritableStreams(*stream);
-              }
-            }
-          }
-        }
+        modifyStateForSpuriousLoss(conn, *rPacketIt);
         QUIC_STATS(conn.statsCallback, onPacketSpuriousLoss);
-        // Decrement the counter, trust that we will erase this as part of
-        // the bulk erase.
-        CHECK_GT(conn.outstandings.declaredLostCount, 0);
-        conn.outstandings.declaredLostCount--;
         if (spuriousLossEvent) {
           spuriousLossEvent->addSpuriousPacket(
               rPacketIt->metadata,
@@ -794,4 +757,44 @@ void updateEcnCountEchoed(
   ackState.ecnCECountEchoed =
       std::max(ackState.ecnCECountEchoed, readAckFrame.ecnCECount);
 }
+
+void modifyStateForSpuriousLoss(
+    QuicConnectionStateBase& conn,
+    OutstandingPacketWrapper& spuriouslyLostPacket) {
+  CHECK_GT(conn.outstandings.declaredLostCount, 0);
+  conn.lossState.totalPacketsSpuriouslyMarkedLost++;
+  if (conn.transportSettings.useAdaptiveLossReorderingThresholds) {
+    if (spuriouslyLostPacket.metadata.lossReorderDistance.has_value() &&
+        spuriouslyLostPacket.metadata.lossReorderDistance.value() >
+            conn.lossState.reorderingThreshold) {
+      conn.lossState.reorderingThreshold =
+          spuriouslyLostPacket.metadata.lossReorderDistance.value();
+    }
+  }
+  if (conn.transportSettings.useAdaptiveLossTimeThresholds) {
+    if (spuriouslyLostPacket.metadata.lossTimeoutDividend.has_value() &&
+        spuriouslyLostPacket.metadata.lossTimeoutDividend.value() >
+            conn.transportSettings.timeReorderingThreshDividend) {
+      conn.transportSettings.timeReorderingThreshDividend =
+          spuriouslyLostPacket.metadata.lossTimeoutDividend.value();
+    }
+  }
+  if (conn.transportSettings.removeFromLossBufferOnSpurious) {
+    for (auto& f : spuriouslyLostPacket.packet.frames) {
+      auto streamFrame = f.asWriteStreamFrame();
+      if (streamFrame) {
+        auto stream = conn.streamManager->findStream(streamFrame->streamId);
+        if (stream) {
+          stream->removeFromLossBuffer(
+              streamFrame->offset, streamFrame->len, streamFrame->fin);
+          stream->updateAckedIntervals(
+              streamFrame->offset, streamFrame->len, streamFrame->fin);
+          conn.streamManager->updateWritableStreams(*stream);
+        }
+      }
+    }
+  }
+  CHECK_GT(conn.outstandings.declaredLostCount, 0);
+  conn.outstandings.declaredLostCount--;
+}
 } // namespace quic
diff --git a/quic/state/AckHandlers.h b/quic/state/AckHandlers.h
index 1ba509a42..99dc70360 100644
--- a/quic/state/AckHandlers.h
+++ b/quic/state/AckHandlers.h
@@ -104,4 +104,12 @@ void updateEcnCountEchoed(
     QuicConnectionStateBase& conn,
     PacketNumberSpace pnSpace,
     const ReadAckFrame& readAckFrame);
+
+/**
+ * Modifies the state in the QuicConnectionStateBase when a packet that
+ * was marked as lost is acked.
+ */
+void modifyStateForSpuriousLoss(
+    QuicConnectionStateBase& conn,
+    OutstandingPacketWrapper& spuriouslyLostPacket);
 } // namespace quic