From 3c5a34ee11f5a098a18c665c73eb45be2b0e7351 Mon Sep 17 00:00:00 2001
From: Matt Joras <mjoras@meta.com>
Date: Thu, 15 Jun 2023 13:15:08 -0700
Subject: [PATCH] Put opportunistic ACKing behind TransportSetting

Summary:
This has been a default behavior for us for a long time. Basically, we will always include ACK frames in a burst if there's new packets to ACK.

This is overall probably fine and is flexible to peer behavior such that the peer can get away with basically never sending anything ACK-eliciting.

However it is not without its issues. In particular, for DSR it means that we write an excessive amount of pure ACK packets since they cannot be coalesced with a DSR burst.

Make this behavior optional, such that we only include ACK frames in non-probing schedulers when we are only writing stream data.

Reviewed By: kvtsoy

Differential Revision: D39479149

fbshipit-source-id: 6e92d45311a3fb23750c93483b94e97dd3fdce26
---
 quic/api/QuicTransportBase.cpp      |  3 +++
 quic/api/QuicTransportFunctions.cpp |  8 ++++++-
 quic/api/test/QuicTransportTest.cpp | 37 +++++++++++++++++++++++++++++
 quic/state/TransportSettings.h      |  2 ++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp
index 422c3d8d4..0fcedcfad 100644
--- a/quic/api/QuicTransportBase.cpp
+++ b/quic/api/QuicTransportBase.cpp
@@ -3618,6 +3618,9 @@ QuicSocket::WriteResult QuicTransportBase::setDSRPacketizationRequestSender(
       return folly::makeUnexpected(LocalErrorCode::STREAM_CLOSED);
     }
     stream->dsrSender = std::move(sender);
+    // Default to disabling opportunistic ACKing for DSR since it causes extra
+    // writes and spurious losses.
+    conn_->transportSettings.opportunisticAcking = false;
     // Fow now, no appLimited or appIdle update here since we are not writing
     // either BufferMetas yet. The first BufferMeta write will update it.
   } catch (const QuicTransportException& ex) {
diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp
index 312c516fc..ec9669d36 100644
--- a/quic/api/QuicTransportFunctions.cpp
+++ b/quic/api/QuicTransportFunctions.cpp
@@ -177,7 +177,6 @@ WriteQuicDataResult writeQuicDataToSocketImpl(
           PacketNumberSpace::AppData,
           exceptCryptoStream ? "FrameSchedulerWithoutCrypto" : "FrameScheduler")
           .streamFrames()
-          .ackFrames()
           .resetFrames()
           .windowUpdateFrames()
           .blockedFrames()
@@ -185,6 +184,13 @@ WriteQuicDataResult writeQuicDataToSocketImpl(
           .pingFrames()
           .datagramFrames()
           .immediateAckFrames();
+  // Only add ACK frames if we need to send an ACK, or if the write reason isn't
+  // just streams.
+  if (connection.transportSettings.opportunisticAcking ||
+      toWriteAppDataAcks(connection) ||
+      (hasNonAckDataToWrite(connection) != WriteDataReason::STREAM)) {
+    schedulerBuilder.ackFrames();
+  }
   if (!exceptCryptoStream) {
     schedulerBuilder.cryptoFrames();
   }
diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp
index a9a3b013c..8a907b73e 100644
--- a/quic/api/test/QuicTransportTest.cpp
+++ b/quic/api/test/QuicTransportTest.cpp
@@ -2025,6 +2025,43 @@ TEST_F(QuicTransportTest, WritePendingAckIfHavingData) {
   EXPECT_EQ(0, ackState.numNonRxPacketsRecvd);
 }
 
+TEST_F(QuicTransportTest, NoWritePendingAckIfHavingData) {
+  auto& conn = transport_->getConnectionState();
+  conn.transportSettings.opportunisticAcking = false;
+  auto streamId = transport_->createBidirectionalStream().value();
+  auto buf = buildRandomInputData(20);
+  PacketNum start = 10;
+  PacketNum end = 15;
+  addAckStatesWithCurrentTimestamps(conn.ackStates.appDataAckState, start, end);
+  conn.ackStates.appDataAckState.needsToSendAckImmediately = false;
+  conn.ackStates.appDataAckState.numNonRxPacketsRecvd = 3;
+  EXPECT_CALL(*socket_, write(_, _)).WillOnce(Invoke(bufLength));
+  // We should write acks if there is data pending
+  transport_->writeChain(streamId, buf->clone(), true);
+  loopForWrites();
+  EXPECT_EQ(conn.outstandings.packets.size(), 1);
+  auto& packet =
+      getFirstOutstandingPacket(conn, PacketNumberSpace::AppData)->packet;
+  EXPECT_GE(packet.frames.size(), 1);
+
+  bool ackFound = false;
+  for (auto& frame : packet.frames) {
+    auto ackFrame = frame.asWriteAckFrame();
+    if (!ackFrame) {
+      continue;
+    }
+    ackFound = true;
+  }
+  EXPECT_FALSE(ackFound);
+  EXPECT_EQ(conn.ackStates.appDataAckState.largestAckScheduled, folly::none);
+
+  auto pnSpace = packet.header.getPacketNumberSpace();
+  auto ackState = getAckState(conn, pnSpace);
+  EXPECT_EQ(ackState.largestAckScheduled, folly::none);
+  EXPECT_FALSE(ackState.needsToSendAckImmediately);
+  EXPECT_EQ(3, ackState.numNonRxPacketsRecvd);
+}
+
 TEST_F(QuicTransportTest, RstStream) {
   auto streamId = transport_->createBidirectionalStream().value();
   EXPECT_CALL(*socket_, write(_, _)).WillOnce(Invoke(bufLength));
diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h
index 4d3c72aff..aacb798d8 100644
--- a/quic/state/TransportSettings.h
+++ b/quic/state/TransportSettings.h
@@ -274,6 +274,8 @@ struct TransportSettings {
   // How many times we will a schedule a stream to packets before moving onto
   // the next one in the queue. Only relevant for incremental priority.
   uint64_t priorityQueueWritesPerStream{1};
+  // Whether to include ACKs whenever we have data to write and packets to ACK.
+  bool opportunisticAcking{true};
 
   // Local configuration for ACK receive timestamps.
   //