Purge old Rx timestamps based on ACKs of ACKs

Summary:
Prune stored Rx timestamps of packets if those packets were ACKed previously (`WriteAckFrame`) and we received an ACK of that ACK from the peer. `commonAckVisitorForAckFrame` already purges these packet numbers from AckBlocks in AckState. If a packet doesn't exist in the AckState, the timestamp shouldn't be sent as it was confirmed to be received.

TODO: this isn't true for old packets that are pruned in the AckState so some old timestamps might be pruned, which is probably OK!

This is being done to avoid sending duplicate timestamps and reduce network and memory overhead.

Reviewed By: bschlinker

Differential Revision: D49216621

fbshipit-source-id: a5b94ba974edac224c111cd82d7b7099312b52ac

quic/state/AckHandlers.cpp

@@ -663,17 +663,50 @@ void parseAckReceiveTimestamps(
 void commonAckVisitorForAckFrame(
     AckState& ackState,
     const WriteAckFrame& frame) {
-  // Remove ack interval from ackState if an outstandingPacket with a AckFrame
-  // is acked.
-  // We may remove the current largest acked packet here, but keep its receive
-  // time behind. But then right after this updateLargestReceivedPacketNum will
-  // update that time stamp. Please note that this assume the peer isn't buggy
-  // in the sense that packet numbers it issues are only increasing.
+  /**
+   * Purge old timestamps to avoid sending duplicate timestamps in the next ACK.
+   * We use ACKs received for the WriteAck we sent earlier to the peer to enable
+   * this purge.
+   */
+  auto purgeAckReceiveTimestamps = [&](AckState& ackState) {
+    if (ackState.recvdPacketInfos.empty()) {
+      return;
+    }
+    // No ACKs tracked locally, which means all were confirmed to be received.
+    // Clear all the timestamps.
+    if (ackState.acks.empty()) {
+      ackState.recvdPacketInfos.clear();
+      return;
+    }
+
+    for (auto recvdPacketInfoIt = ackState.recvdPacketInfos.begin();
+         recvdPacketInfoIt != ackState.recvdPacketInfos.end();) {
+      if (!ackState.acks.contains(
+              recvdPacketInfoIt->pktNum, recvdPacketInfoIt->pktNum)) {
+        recvdPacketInfoIt = ackState.recvdPacketInfos.erase(recvdPacketInfoIt);
+      } else {
+        ++recvdPacketInfoIt;
+      }
+    }
+  };
+
+  // Remove ack interval from ackState if an outstandingPacket with a
+  // AckFrame is acked. We may remove the current largest acked packet
+  // here, but keep its receive time behind. But then right after this
+  // updateLargestReceivedPacketNum will update that time stamp. Please
+  // note that this assume the peer isn't buggy in the sense that packet
+  // numbers it issues are only increasing.
   auto iter = frame.ackBlocks.crbegin();
   while (iter != frame.ackBlocks.crend()) {
     ackState.acks.withdraw(*iter);
     iter++;
   }
+  // Purge all received timestamps sent in ACKs that have been received by
+  // the peer. We don't want to purge using kAckPurgingThresh as the latest
+  // timestamps may not have been received by the peer yet. Also max
+  // timestamps is limited already by its own config.
+  purgeAckReceiveTimestamps(ackState);
+
   if (!frame.ackBlocks.empty()) {
     auto largestAcked = frame.ackBlocks.front().end;
     if (largestAcked > kAckPurgingThresh) {

quic/state/test/AckHandlersTest.cpp

@@ -1605,6 +1605,112 @@ TEST_P(AckHandlersTest, PurgeAcks) {
       expectedTime, *conn.ackStates.initialAckState->largestRecvdPacketTime);
 }
 
+TEST_P(AckHandlersTest, purgeAckReceiveTimestamps) {
+  // Case 1: No timestamps
+  {
+    QuicServerConnectionState conn(
+        FizzServerQuicHandshakeContext::Builder().build());
+    WriteAckFrame ackFrame;
+    ackFrame.ackBlocks.emplace_back(15, 40);
+    conn.ackStates.initialAckState->acks.insert(15, 40);
+
+    auto expectedTime = Clock::now();
+    conn.ackStates.initialAckState->largestRecvdPacketTime = expectedTime;
+
+    commonAckVisitorForAckFrame(*conn.ackStates.initialAckState, ackFrame);
+    EXPECT_EQ(conn.ackStates.initialAckState->acks.size(), 0);
+    EXPECT_EQ(conn.ackStates.initialAckState->recvdPacketInfos.size(), 0);
+  }
+
+  // Case 2: purge all receive timestamps
+  {
+    QuicServerConnectionState conn(
+        FizzServerQuicHandshakeContext::Builder().build());
+    WriteAckFrame ackFrame;
+    ackFrame.ackBlocks.emplace_back(15, 40);
+    conn.ackStates.initialAckState->acks.insert(15, 40);
+
+    auto expectedTime = Clock::now();
+    conn.ackStates.initialAckState->largestRecvdPacketTime = expectedTime;
+    // Fill up the last 25 timestamps ending at PN 40.
+    for (PacketNum pktNum = 15; pktNum <= 40; ++pktNum) {
+      conn.ackStates.initialAckState->recvdPacketInfos.emplace_back(
+          RecvdPacketInfo{pktNum, expectedTime});
+    }
+
+    commonAckVisitorForAckFrame(*conn.ackStates.initialAckState, ackFrame);
+    EXPECT_EQ(conn.ackStates.initialAckState->acks.size(), 0);
+    EXPECT_EQ(conn.ackStates.initialAckState->recvdPacketInfos.size(), 0);
+  }
+  // Case 3: Purge only some old timestamps in the front.
+  {
+    QuicServerConnectionState conn(
+        FizzServerQuicHandshakeContext::Builder().build());
+    WriteAckFrame ackFrame;
+    // Local ACK state has ACKs for {1, 20}, {25, 40}
+    conn.ackStates.initialAckState->acks.insert(25, 40);
+    conn.ackStates.initialAckState->acks.insert(1, 20);
+    auto expectedTime = Clock::now();
+    conn.ackStates.initialAckState->largestRecvdPacketTime = expectedTime;
+
+    // Local ACK state has timestamps for {15, 40}
+    for (PacketNum pktNum = 15; pktNum <= 40; ++pktNum) {
+      conn.ackStates.initialAckState->recvdPacketInfos.emplace_back(
+          RecvdPacketInfo{pktNum, expectedTime});
+    }
+    // ACK frame in the ACKed packet has ACKs for {10, 20}, {25, 35}
+    ackFrame.ackBlocks.emplace_back(10, 20);
+    ackFrame.ackBlocks.emplace_back(25, 35);
+
+    commonAckVisitorForAckFrame(*conn.ackStates.initialAckState, ackFrame);
+    // We should have purged old packets in ack state
+    ASSERT_EQ(conn.ackStates.initialAckState->acks.size(), 1);
+    EXPECT_EQ(conn.ackStates.initialAckState->acks.front().start, 36);
+    EXPECT_EQ(conn.ackStates.initialAckState->acks.front().end, 40);
+    // Should have purged all timestamps that are purged in ackState and only
+    // (36,40) remain.
+    ASSERT_EQ(conn.ackStates.initialAckState->recvdPacketInfos.size(), 5);
+    EXPECT_EQ(
+        conn.ackStates.initialAckState->recvdPacketInfos.front().pktNum, 36);
+    EXPECT_EQ(
+        conn.ackStates.initialAckState->recvdPacketInfos.back().pktNum, 40);
+  }
+
+  // Case 4: Purge some timestamps in the middle.
+  {
+    QuicServerConnectionState conn(
+        FizzServerQuicHandshakeContext::Builder().build());
+    WriteAckFrame ackFrame;
+    // Local ACK state has ACKs for {1, 20}, {25, 40}
+    conn.ackStates.initialAckState->acks.insert(25, 40);
+    conn.ackStates.initialAckState->acks.insert(10, 20);
+
+    auto expectedTime = Clock::now();
+    conn.ackStates.initialAckState->largestRecvdPacketTime = expectedTime;
+
+    // Local ACK state has timestamps for {15, 40}
+    for (PacketNum pktNum = 15; pktNum <= 40; ++pktNum) {
+      conn.ackStates.initialAckState->recvdPacketInfos.emplace_back(
+          RecvdPacketInfo{pktNum, expectedTime});
+    }
+    // Selectively ACK some packets in the middle - {18, 20}, {25, 35}
+    ackFrame.ackBlocks.emplace_back(25, 35);
+    ackFrame.ackBlocks.emplace_back(18, 20);
+
+    commonAckVisitorForAckFrame(*conn.ackStates.initialAckState, ackFrame);
+    // We should have purged old packets in ack state
+    ASSERT_EQ(conn.ackStates.initialAckState->acks.size(), 1);
+    EXPECT_EQ(conn.ackStates.initialAckState->acks.front().start, 36);
+    EXPECT_EQ(conn.ackStates.initialAckState->acks.front().end, 40);
+    // Should have purged some timestamps in the middle of recvdPacketInfos.
+    ASSERT_EQ(conn.ackStates.initialAckState->recvdPacketInfos.size(), 8);
+    EXPECT_EQ(
+        conn.ackStates.initialAckState->recvdPacketInfos.front().pktNum, 15);
+    EXPECT_EQ(
+        conn.ackStates.initialAckState->recvdPacketInfos.back().pktNum, 40);
+  }
+}
+
 TEST_P(AckHandlersTest, NoSkipAckVisitor) {
   QuicServerConnectionState conn(
       FizzServerQuicHandshakeContext::Builder().build());