diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index da706679c..63e50ccbb 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -458,8 +458,9 @@ void QuicTransportBase::closeImpl( // Don't need outstanding packets. conn_->outstandings.packets.clear(); - conn_->outstandings.packetCount = {}; - conn_->outstandings.clonedPacketCount = {}; + conn_->outstandings.initialPacketsCount = 0; + conn_->outstandings.handshakePacketsCount = 0; + conn_->outstandings.clonedPacketsCount = 0; // We don't need no congestion control. conn_->congestionController = nullptr; diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 3addef61a..b8358a20d 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -109,9 +109,7 @@ uint64_t writeQuicDataToSocketImpl( // add a flag to the Scheduler to control the priority between them and see // which way is better. uint64_t written = 0; - auto& numProbePackets = - connection.pendingEvents.numProbePackets[PacketNumberSpace::AppData]; - if (numProbePackets) { + if (connection.pendingEvents.numProbePackets) { auto probeSchedulerBuilder = FrameScheduler::Builder( connection, @@ -137,12 +135,13 @@ uint64_t writeQuicDataToSocketImpl( EncryptionLevel::AppData, PacketNumberSpace::AppData, probeScheduler, - std::min(packetLimit, numProbePackets), + std::min( + packetLimit, connection.pendingEvents.numProbePackets), aead, headerCipher, version); - CHECK_GE(numProbePackets, written); - numProbePackets -= written; + CHECK_GE(connection.pendingEvents.numProbePackets, written); + connection.pendingEvents.numProbePackets -= written; } auto schedulerBuilder = FrameScheduler::Builder( @@ -827,13 +826,26 @@ void updateConnection( (conn.pendingEvents.pathChallenge || conn.outstandingPathValidation)) { conn.pathValidationLimiter->onPacketSent(pkt.metadata.encodedSize); } + if (pkt.metadata.isHandshake) { + if (!pkt.associatedEvent) { + if (packetNumberSpace == PacketNumberSpace::Initial) { + ++conn.outstandings.initialPacketsCount; + } else { + CHECK_EQ(packetNumberSpace, PacketNumberSpace::Handshake); + ++conn.outstandings.handshakePacketsCount; + } + } + } conn.lossState.lastRetransmittablePacketSentTime = pkt.metadata.time; if (pkt.associatedEvent) { - ++conn.outstandings.clonedPacketCount[packetNumberSpace]; + ++conn.outstandings.clonedPacketsCount; ++conn.lossState.timeoutBasedRtxCount; - } else { - ++conn.outstandings.packetCount[packetNumberSpace]; } + + auto opCount = conn.outstandings.numOutstanding(); + DCHECK_GE(opCount, conn.outstandings.initialPacketsCount); + DCHECK_GE(opCount, conn.outstandings.handshakePacketsCount); + DCHECK_GE(opCount, conn.outstandings.clonedPacketsCount); } uint64_t congestionControlWritableBytes(const QuicConnectionStateBase& conn) { @@ -924,10 +936,7 @@ uint64_t writeCryptoAndAckDataToSocket( uint64_t written = 0; auto& cryptoStream = *getCryptoStream(*connection.cryptoState, encryptionLevel); - auto& numProbePackets = - connection.pendingEvents - .numProbePackets[LongHeader::typeToPacketNumberSpace(packetType)]; - if (numProbePackets && + if (connection.pendingEvents.numProbePackets && (cryptoStream.retransmissionBuffer.size() || scheduler.hasData())) { written = writeProbingDataToSocket( sock, @@ -938,13 +947,14 @@ uint64_t writeCryptoAndAckDataToSocket( encryptionLevel, LongHeader::typeToPacketNumberSpace(packetType), scheduler, - std::min(packetLimit, numProbePackets), + std::min( + packetLimit, connection.pendingEvents.numProbePackets), cleartextCipher, headerCipher, version, token); - CHECK_GE(numProbePackets, written); - numProbePackets -= written; + CHECK_GE(connection.pendingEvents.numProbePackets, written); + connection.pendingEvents.numProbePackets -= written; } // Crypto data is written without aead protection. written += writeConnectionDataToSocket( @@ -1467,7 +1477,7 @@ uint64_t writeD6DProbeToSocket( } WriteDataReason shouldWriteData(const QuicConnectionStateBase& conn) { - if (conn.pendingEvents.anyProbePackets()) { + if (conn.pendingEvents.numProbePackets) { VLOG(10) << nodeToString(conn.nodeType) << " needs write because of PTO" << conn; return WriteDataReason::PROBES; diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 4280e61dd..2d2e29485 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -29,7 +29,8 @@ enum PacketBuilderType { Regular, Inplace }; namespace { PacketNum addInitialOutstandingPacket(QuicConnectionStateBase& conn) { - PacketNum nextPacketNum = getNextPacketNum(conn, PacketNumberSpace::Initial); + PacketNum nextPacketNum = + getNextPacketNum(conn, PacketNumberSpace::Handshake); std::vector zeroConnIdData(quic::kDefaultConnectionIdSize, 0); ConnectionId srcConnId(zeroConnIdData); LongHeader header( @@ -41,8 +42,8 @@ PacketNum addInitialOutstandingPacket(QuicConnectionStateBase& conn) { RegularQuicWritePacket packet(std::move(header)); conn.outstandings.packets.emplace_back( packet, Clock::now(), 0, true, 0, 0, 0, LossState(), 0); - conn.outstandings.packetCount[PacketNumberSpace::Initial]++; - increaseNextPacketNum(conn, PacketNumberSpace::Initial); + conn.outstandings.handshakePacketsCount++; + increaseNextPacketNum(conn, PacketNumberSpace::Handshake); return nextPacketNum; } @@ -60,7 +61,7 @@ PacketNum addHandshakeOutstandingPacket(QuicConnectionStateBase& conn) { RegularQuicWritePacket packet(std::move(header)); conn.outstandings.packets.emplace_back( packet, Clock::now(), 0, true, 0, 0, 0, LossState(), 0); - conn.outstandings.packetCount[PacketNumberSpace::Handshake]++; + conn.outstandings.handshakePacketsCount++; increaseNextPacketNum(conn, PacketNumberSpace::Handshake); return nextPacketNum; } diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index d83acd2e2..a50ec4b42 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -1309,13 +1309,9 @@ TEST_F(QuicTransportImplTest, CloseStreamAfterReadFin) { } TEST_F(QuicTransportImplTest, CloseTransportCleansupOutstandingCounters) { - transport->transportConn->outstandings - .packetCount[PacketNumberSpace::Handshake] = 200; + transport->transportConn->outstandings.handshakePacketsCount = 200; transport->closeNow(folly::none); - EXPECT_EQ( - 0, - transport->transportConn->outstandings - .packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, transport->transportConn->outstandings.handshakePacketsCount); } TEST_F(QuicTransportImplTest, DeliveryCallbackUnsetAll) { diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 271d85643..6e4f286d7 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -628,7 +628,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionPureAckCounter) { conn->qLogger = std::make_shared(VantagePoint::Client); auto stream = conn->streamManager->createNextBidirectionalStream().value(); writeDataToQuicStream(*stream, nullptr, true); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); auto packet = buildEmptyPacket(*conn, PacketNumberSpace::Handshake); auto packetEncodedSize = @@ -738,8 +738,8 @@ TEST_F(QuicTransportFunctionsTest, TestImplicitAck) { TimePoint(), getEncodedSize(packet), false /* isDSRPacket */); - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Initial]); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn->outstandings.initialPacketsCount); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); EXPECT_EQ(1, conn->outstandings.packets.size()); EXPECT_EQ(1, initialStream->retransmissionBuffer.size()); @@ -757,8 +757,8 @@ TEST_F(QuicTransportFunctionsTest, TestImplicitAck) { TimePoint(), getEncodedSize(packet), false /* isDSRPacket */); - EXPECT_EQ(2, conn->outstandings.packetCount[PacketNumberSpace::Initial]); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(2, conn->outstandings.initialPacketsCount); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); EXPECT_EQ(2, conn->outstandings.packets.size()); EXPECT_EQ(3, initialStream->retransmissionBuffer.size()); EXPECT_TRUE(initialStream->writeBuffer.empty()); @@ -770,7 +770,7 @@ TEST_F(QuicTransportFunctionsTest, TestImplicitAck) { initialStream->retransmissionBuffer.erase(0); initialStream->lossBuffer.emplace_back(std::move(firstBuf), 0, false); conn->outstandings.packets.pop_front(); - conn->outstandings.packetCount[PacketNumberSpace::Initial]--; + conn->outstandings.initialPacketsCount--; auto handshakeStream = getCryptoStream(*conn->cryptoState, EncryptionLevel::Handshake); @@ -787,8 +787,8 @@ TEST_F(QuicTransportFunctionsTest, TestImplicitAck) { TimePoint(), getEncodedSize(packet), false /* isDSRPacket */); - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Initial]); - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn->outstandings.initialPacketsCount); + EXPECT_EQ(1, conn->outstandings.handshakePacketsCount); EXPECT_EQ(2, conn->outstandings.packets.size()); EXPECT_EQ(1, handshakeStream->retransmissionBuffer.size()); @@ -803,8 +803,8 @@ TEST_F(QuicTransportFunctionsTest, TestImplicitAck) { TimePoint(), getEncodedSize(packet), false /* isDSRPacket */); - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Initial]); - EXPECT_EQ(2, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn->outstandings.initialPacketsCount); + EXPECT_EQ(2, conn->outstandings.handshakePacketsCount); EXPECT_EQ(3, conn->outstandings.packets.size()); EXPECT_EQ(2, handshakeStream->retransmissionBuffer.size()); EXPECT_TRUE(handshakeStream->writeBuffer.empty()); @@ -820,19 +820,19 @@ TEST_F(QuicTransportFunctionsTest, TestImplicitAck) { auto frame = op.packet.frames[0].asWriteCryptoFrame(); EXPECT_EQ(frame->offset, 0); conn->outstandings.packets.pop_front(); - conn->outstandings.packetCount[PacketNumberSpace::Handshake]--; + conn->outstandings.handshakePacketsCount--; implicitAckCryptoStream(*conn, EncryptionLevel::Initial); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Initial]); - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.initialPacketsCount); + EXPECT_EQ(1, conn->outstandings.handshakePacketsCount); EXPECT_EQ(1, conn->outstandings.packets.size()); EXPECT_TRUE(initialStream->retransmissionBuffer.empty()); EXPECT_TRUE(initialStream->writeBuffer.empty()); EXPECT_TRUE(initialStream->lossBuffer.empty()); implicitAckCryptoStream(*conn, EncryptionLevel::Handshake); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Initial]); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.initialPacketsCount); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); EXPECT_TRUE(conn->outstandings.packets.empty()); EXPECT_TRUE(handshakeStream->retransmissionBuffer.empty()); EXPECT_TRUE(handshakeStream->writeBuffer.empty()); @@ -844,7 +844,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionHandshakeCounter) { conn->qLogger = std::make_shared(VantagePoint::Client); auto stream = conn->streamManager->createNextBidirectionalStream().value(); writeDataToQuicStream(*stream, nullptr, true); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); auto packet = buildEmptyPacket(*conn, PacketNumberSpace::Handshake); auto packetEncodedSize = @@ -859,7 +859,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionHandshakeCounter) { TimePoint(), getEncodedSize(packet), false /* isDSRPacket */); - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn->outstandings.handshakePacketsCount); auto nonHandshake = buildEmptyPacket(*conn, PacketNumberSpace::AppData); packetEncodedSize = @@ -907,8 +907,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionHandshakeCounter) { EXPECT_EQ(gotFrame->offset, 0); EXPECT_EQ(gotFrame->len, 0); EXPECT_TRUE(gotFrame->fin); - EXPECT_EQ( - 1, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn->outstandings.handshakePacketsCount); } } } @@ -918,7 +917,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionForOneRttCryptoData) { conn->qLogger = std::make_shared(VantagePoint::Client); auto stream = conn->streamManager->createNextBidirectionalStream().value(); writeDataToQuicStream(*stream, nullptr, true); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); // Packet with CryptoFrame in AppData pn space auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData, true); @@ -935,7 +934,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionForOneRttCryptoData) { getEncodedSize(packet), false /* isDSRPacket */); - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); EXPECT_EQ(1, conn->outstandings.packets.size()); auto nonHandshake = buildEmptyPacket(*conn, PacketNumberSpace::AppData); @@ -987,7 +986,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionForOneRttCryptoData) { } } - EXPECT_EQ(0, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(0, conn->outstandings.handshakePacketsCount); EXPECT_EQ(2, conn->outstandings.packets.size()); } @@ -1371,9 +1370,7 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { conn->transportSettings.writeConnectionDataPacketsLimit)); // Normal limit - conn->pendingEvents.numProbePackets[PacketNumberSpace::Initial] = 0; - conn->pendingEvents.numProbePackets[PacketNumberSpace::Handshake] = 0; - conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = 0; + conn->pendingEvents.numProbePackets = 0; conn->transportSettings.writeConnectionDataPacketsLimit = kDefaultWriteConnectionDataPacketLimit; EXPECT_CALL(*rawSocket, write(_, _)) @@ -1398,7 +1395,7 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { conn->transportSettings.writeConnectionDataPacketsLimit)); // Probing can be limited by packet limit too - conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = + conn->pendingEvents.numProbePackets = kDefaultWriteConnectionDataPacketLimit * 2; writeDataToQuicStream(*stream1, buf->clone(), true); writableBytes = 10000; @@ -1873,9 +1870,7 @@ TEST_F(QuicTransportFunctionsTest, NoCryptoProbeWriteIfNoProbeCredit) { ASSERT_EQ(1, cryptoStream->retransmissionBuffer.size()); ASSERT_TRUE(cryptoStream->writeBuffer.empty()); - conn->pendingEvents.numProbePackets[PacketNumberSpace::Initial] = 0; - conn->pendingEvents.numProbePackets[PacketNumberSpace::Handshake] = 0; - conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = 0; + conn->pendingEvents.numProbePackets = 0; EXPECT_EQ( 0, writeCryptoAndAckDataToSocket( @@ -2180,10 +2175,9 @@ TEST_F(QuicTransportFunctionsTest, HasAppDataToWrite) { EXPECT_EQ(WriteDataReason::STREAM, hasNonAckDataToWrite(*conn)); } -TEST_F(QuicTransportFunctionsTest, UpdateConnectionCloneCounterAppData) { +TEST_F(QuicTransportFunctionsTest, UpdateConnectionCloneCounter) { auto conn = createConn(); - ASSERT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::AppData]); + ASSERT_EQ(0, conn->outstandings.clonedPacketsCount); auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData); auto connWindowUpdate = MaxDataFrame(conn->flowControlState.advertisedMaxOffset); @@ -2198,64 +2192,7 @@ TEST_F(QuicTransportFunctionsTest, UpdateConnectionCloneCounterAppData) { TimePoint(), 123, false /* isDSRPacket */); - EXPECT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::Initial]); - EXPECT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::Handshake]); - EXPECT_EQ( - 1, conn->outstandings.clonedPacketCount[PacketNumberSpace::AppData]); -} - -TEST_F(QuicTransportFunctionsTest, UpdateConnectionCloneCounterHandshake) { - auto conn = createConn(); - ASSERT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::Handshake]); - auto packet = buildEmptyPacket(*conn, PacketNumberSpace::Handshake); - auto connWindowUpdate = - MaxDataFrame(conn->flowControlState.advertisedMaxOffset); - conn->pendingEvents.connWindowUpdate = true; - packet.packet.frames.emplace_back(connWindowUpdate); - PacketEvent packetEvent(PacketNumberSpace::AppData, 100); - conn->outstandings.packetEvents.insert(packetEvent); - updateConnection( - *conn, - packetEvent, - packet.packet, - TimePoint(), - 123, - false /* isDSRPacket */); - EXPECT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::Initial]); - EXPECT_EQ( - 1, conn->outstandings.clonedPacketCount[PacketNumberSpace::Handshake]); - EXPECT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::AppData]); -} - -TEST_F(QuicTransportFunctionsTest, UpdateConnectionCloneCounterInitial) { - auto conn = createConn(); - ASSERT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::Initial]); - auto packet = buildEmptyPacket(*conn, PacketNumberSpace::Initial); - auto connWindowUpdate = - MaxDataFrame(conn->flowControlState.advertisedMaxOffset); - conn->pendingEvents.connWindowUpdate = true; - packet.packet.frames.emplace_back(connWindowUpdate); - PacketEvent packetEvent(PacketNumberSpace::AppData, 100); - conn->outstandings.packetEvents.insert(packetEvent); - updateConnection( - *conn, - packetEvent, - packet.packet, - TimePoint(), - 123, - false /* isDSRPacket */); - EXPECT_EQ( - 1, conn->outstandings.clonedPacketCount[PacketNumberSpace::Initial]); - EXPECT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::Handshake]); - EXPECT_EQ( - 0, conn->outstandings.clonedPacketCount[PacketNumberSpace::AppData]); + EXPECT_EQ(1, conn->outstandings.clonedPacketsCount); } TEST_F(QuicTransportFunctionsTest, ClearBlockedFromPendingEvents) { @@ -2274,7 +2211,7 @@ TEST_F(QuicTransportFunctionsTest, ClearBlockedFromPendingEvents) { false /* isDSRPacket */); EXPECT_FALSE(conn->streamManager->hasBlocked()); EXPECT_FALSE(conn->outstandings.packets.empty()); - EXPECT_EQ(0, conn->outstandings.numClonedPackets()); + EXPECT_EQ(0, conn->outstandings.clonedPacketsCount); } TEST_F(QuicTransportFunctionsTest, ClonedBlocked) { @@ -2296,8 +2233,7 @@ TEST_F(QuicTransportFunctionsTest, ClonedBlocked) { getEncodedSize(packet), false /* isDSRPacket */); EXPECT_FALSE(conn->outstandings.packets.empty()); - EXPECT_EQ( - 1, conn->outstandings.clonedPacketCount[PacketNumberSpace::AppData]); + EXPECT_EQ(1, conn->outstandings.clonedPacketsCount); } TEST_F(QuicTransportFunctionsTest, TwoConnWindowUpdateWillCrash) { @@ -2354,7 +2290,7 @@ TEST_F(QuicTransportFunctionsTest, ClearRstFromPendingEvents) { false /* isDSRPacket */); EXPECT_TRUE(conn->pendingEvents.resets.empty()); EXPECT_FALSE(conn->outstandings.packets.empty()); - EXPECT_EQ(0, conn->outstandings.numClonedPackets()); + EXPECT_EQ(0, conn->outstandings.clonedPacketsCount); } TEST_F(QuicTransportFunctionsTest, ClonedRst) { @@ -2377,7 +2313,7 @@ TEST_F(QuicTransportFunctionsTest, ClonedRst) { getEncodedSize(packet), false /* isDSRPacket */); EXPECT_FALSE(conn->outstandings.packets.empty()); - EXPECT_EQ(1, conn->outstandings.numClonedPackets()); + EXPECT_EQ(1, conn->outstandings.clonedPacketsCount); } TEST_F(QuicTransportFunctionsTest, TotalBytesSentUpdate) { @@ -2582,7 +2518,7 @@ TEST_F(QuicTransportFunctionsTest, ProbeWriteNewFunctionalFrames) { conn->transportSettings.writeConnectionDataPacketsLimit); ASSERT_EQ(1, stream->retransmissionBuffer.size()); - conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = 1; + conn->pendingEvents.numProbePackets = 1; conn->flowControlState.windowSize *= 2; conn->flowControlState.timeOfLastFlowControlUpdate = Clock::now() - 20s; maybeSendConnWindowUpdate(*conn, Clock::now()); diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index a6b8dec3b..b416f80eb 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -229,7 +229,7 @@ TEST_F(QuicTransportTest, WriteDataWithProbing) { auto streamId = transport_->createBidirectionalStream().value(); auto buf = buildRandomInputData(kDefaultUDPSendPacketLen * 2); - conn.pendingEvents.numProbePackets[PacketNumberSpace::AppData] = 1; + conn.pendingEvents.numProbePackets = 1; // Probing won't ask about getWritableBytes. Then regular write may ask // multiple times: int getWritableBytesCounter = 0; @@ -255,7 +255,7 @@ TEST_F(QuicTransportTest, WriteDataWithProbing) { transport_->writeChain(streamId, buf->clone(), true); loopForWrites(); // Pending numProbePackets is cleared: - EXPECT_EQ(0, conn.pendingEvents.numProbePackets[PacketNumberSpace::AppData]); + EXPECT_EQ(0, conn.pendingEvents.numProbePackets); transport_->close(folly::none); } @@ -1114,7 +1114,7 @@ TEST_F(QuicTransportTest, SendPathValidationWhileThereIsOutstandingOne) { TEST_F(QuicTransportTest, ClonePathChallenge) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out - conn.outstandings.packetCount[PacketNumberSpace::Handshake] = 0; + conn.outstandings.handshakePacketsCount = 0; conn.outstandings.packets.clear(); for (auto& t : conn.lossState.lossTimes) { t.reset(); @@ -1148,7 +1148,7 @@ TEST_F(QuicTransportTest, ClonePathChallenge) { TEST_F(QuicTransportTest, OnlyClonePathValidationIfOutstanding) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out - conn.outstandings.packetCount[PacketNumberSpace::Handshake] = 0; + conn.outstandings.handshakePacketsCount = 0; conn.outstandings.packets.clear(); for (auto& t : conn.lossState.lossTimes) { t.reset(); @@ -1290,7 +1290,7 @@ TEST_F(QuicTransportTest, CloneAfterRecvReset) { TEST_F(QuicTransportTest, ClonePathResponse) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out - conn.outstandings.packetCount[PacketNumberSpace::Handshake] = 0; + conn.outstandings.handshakePacketsCount = 0; conn.outstandings.packets.clear(); for (auto& t : conn.lossState.lossTimes) { t.reset(); @@ -1373,8 +1373,8 @@ TEST_F(QuicTransportTest, SendNewConnectionIdFrame) { TEST_F(QuicTransportTest, CloneNewConnectionIdFrame) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out - conn.outstandings.packetCount[PacketNumberSpace::Initial] = 0; - conn.outstandings.packetCount[PacketNumberSpace::Handshake] = 0; + conn.outstandings.initialPacketsCount = 0; + conn.outstandings.handshakePacketsCount = 0; conn.outstandings.packets.clear(); for (auto& t : conn.lossState.lossTimes) { t.reset(); @@ -1513,8 +1513,8 @@ TEST_F(QuicTransportTest, SendRetireConnectionIdFrame) { TEST_F(QuicTransportTest, CloneRetireConnectionIdFrame) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out - conn.outstandings.packetCount[PacketNumberSpace::Initial] = 0; - conn.outstandings.packetCount[PacketNumberSpace::Handshake] = 0; + conn.outstandings.initialPacketsCount = 0; + conn.outstandings.handshakePacketsCount = 0; conn.outstandings.packets.clear(); for (auto& t : conn.lossState.lossTimes) { t.reset(); diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 058b28bbd..9ba6c4bb7 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -770,11 +770,10 @@ void QuicClientTransport::writeData() { auto& initialCryptoStream = *getCryptoStream(*conn_->cryptoState, EncryptionLevel::Initial); CryptoStreamScheduler initialScheduler(*conn_, initialCryptoStream); - auto& numProbePackets = - conn_->pendingEvents.numProbePackets[PacketNumberSpace::Initial]; + if ((initialCryptoStream.retransmissionBuffer.size() && - conn_->outstandings.packetCount[PacketNumberSpace::Initial] && - numProbePackets) || + conn_->outstandings.initialPacketsCount && + conn_->pendingEvents.numProbePackets) || initialScheduler.hasData() || (conn_->ackStates.initialAckState.needsToSendAckImmediately && hasAcksToSchedule(conn_->ackStates.initialAckState))) { @@ -791,7 +790,7 @@ void QuicClientTransport::writeData() { packetLimit, clientConn_->retryToken); } - if (!packetLimit && !conn_->pendingEvents.anyProbePackets()) { + if (!packetLimit && !conn_->pendingEvents.numProbePackets) { return; } } @@ -799,11 +798,9 @@ void QuicClientTransport::writeData() { auto& handshakeCryptoStream = *getCryptoStream(*conn_->cryptoState, EncryptionLevel::Handshake); CryptoStreamScheduler handshakeScheduler(*conn_, handshakeCryptoStream); - auto& numProbePackets = - conn_->pendingEvents.numProbePackets[PacketNumberSpace::Handshake]; - if ((conn_->outstandings.packetCount[PacketNumberSpace::Handshake] && + if ((conn_->outstandings.handshakePacketsCount && handshakeCryptoStream.retransmissionBuffer.size() && - numProbePackets) || + conn_->pendingEvents.numProbePackets) || handshakeScheduler.hasData() || (conn_->ackStates.handshakeAckState.needsToSendAckImmediately && hasAcksToSchedule(conn_->ackStates.handshakeAckState))) { @@ -819,7 +816,7 @@ void QuicClientTransport::writeData() { version, packetLimit); } - if (!packetLimit && !conn_->pendingEvents.anyProbePackets()) { + if (!packetLimit && !conn_->pendingEvents.numProbePackets) { return; } } @@ -835,7 +832,7 @@ void QuicClientTransport::writeData() { version, packetLimit); } - if (!packetLimit && !conn_->pendingEvents.anyProbePackets()) { + if (!packetLimit && !conn_->pendingEvents.numProbePackets) { return; } if (conn_->oneRttWriteCipher) { diff --git a/quic/codec/QuicPacketRebuilder.cpp b/quic/codec/QuicPacketRebuilder.cpp index 93ac1f290..f6ff06852 100644 --- a/quic/codec/QuicPacketRebuilder.cpp +++ b/quic/codec/QuicPacketRebuilder.cpp @@ -36,8 +36,7 @@ PacketEvent PacketRebuilder::cloneOutstandingPacket(OutstandingPacket& packet) { DCHECK(!conn_.outstandings.packetEvents.count(event)); packet.associatedEvent = event; conn_.outstandings.packetEvents.insert(event); - ++conn_.outstandings - .clonedPacketCount[packet.packet.header.getPacketNumberSpace()]; + ++conn_.outstandings.clonedPacketsCount; } return *packet.associatedEvent; } diff --git a/quic/codec/test/QuicPacketRebuilderTest.cpp b/quic/codec/test/QuicPacketRebuilderTest.cpp index f7ddcc317..66a6a5b5b 100644 --- a/quic/codec/test/QuicPacketRebuilderTest.cpp +++ b/quic/codec/test/QuicPacketRebuilderTest.cpp @@ -461,7 +461,7 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) { PacketRebuilder rebuilder(regularBuilder2, conn); rebuilder.rebuildFromPacket(outstandingPacket); EXPECT_TRUE(outstandingPacket.associatedEvent.has_value()); - EXPECT_EQ(1, conn.outstandings.numClonedPackets()); + EXPECT_EQ(1, conn.outstandings.clonedPacketsCount); } TEST_F(QuicPacketRebuilderTest, PurePingWontRebuild) { @@ -485,7 +485,7 @@ TEST_F(QuicPacketRebuilderTest, PurePingWontRebuild) { PacketRebuilder rebuilder(regularBuilder2, conn); EXPECT_EQ(folly::none, rebuilder.rebuildFromPacket(outstandingPacket)); EXPECT_FALSE(outstandingPacket.associatedEvent.has_value()); - EXPECT_EQ(0, conn.outstandings.numClonedPackets()); + EXPECT_EQ(0, conn.outstandings.clonedPacketsCount); } TEST_F(QuicPacketRebuilderTest, LastStreamFrameSkipLen) { diff --git a/quic/fizz/client/test/QuicClientTransportTest.cpp b/quic/fizz/client/test/QuicClientTransportTest.cpp index c9c7acf48..29bf8cb85 100644 --- a/quic/fizz/client/test/QuicClientTransportTest.cpp +++ b/quic/fizz/client/test/QuicClientTransportTest.cpp @@ -3884,8 +3884,8 @@ TEST_F(QuicClientTransportAfterStartTest, IdleTimerResetNoOutstandingPackets) { // Clear out all the outstanding packets to simulate quiescent state. client->getNonConstConn().receivedNewPacketBeforeWrite = false; client->getNonConstConn().outstandings.packets.clear(); - client->getNonConstConn().outstandings.packetCount = {}; - client->getNonConstConn().outstandings.clonedPacketCount = {}; + client->getNonConstConn().outstandings.handshakePacketsCount = 0; + client->getNonConstConn().outstandings.clonedPacketsCount = 0; client->idleTimeout().cancelTimeout(); auto streamId = client->createBidirectionalStream().value(); auto expected = folly::IOBuf::copyBuffer("hello"); diff --git a/quic/loss/QuicLossFunctions.cpp b/quic/loss/QuicLossFunctions.cpp index 52e088b97..0d5799ad9 100644 --- a/quic/loss/QuicLossFunctions.cpp +++ b/quic/loss/QuicLossFunctions.cpp @@ -7,7 +7,6 @@ */ #include "quic/loss/QuicLossFunctions.h" -#include "quic/QuicConstants.h" #include "quic/state/QuicStreamFunctions.h" namespace quic { @@ -54,21 +53,11 @@ void onPTOAlarm(QuicConnectionStateBase& conn) { throw QuicInternalException("Exceeded max PTO", LocalErrorCode::NO_ERROR); } - uint64_t numInitial = - conn.outstandings.packetCount[PacketNumberSpace::Initial] + - conn.outstandings.clonedPacketCount[PacketNumberSpace::Initial]; - uint64_t numHandshake = - conn.outstandings.packetCount[PacketNumberSpace::Handshake] + - conn.outstandings.clonedPacketCount[PacketNumberSpace::Handshake]; - uint64_t numAppData = - conn.outstandings.packetCount[PacketNumberSpace::AppData] + - conn.outstandings.clonedPacketCount[PacketNumberSpace::AppData]; - conn.pendingEvents.numProbePackets[PacketNumberSpace::Initial] = - std::min(numInitial, kPacketToSendForPTO); - conn.pendingEvents.numProbePackets[PacketNumberSpace::Handshake] = - std::min(numHandshake, kPacketToSendForPTO); - conn.pendingEvents.numProbePackets[PacketNumberSpace::AppData] = - std::min(numAppData, kPacketToSendForPTO); + // If there is only one packet outstanding, no point to clone it twice in the + // same write loop. + conn.pendingEvents.numProbePackets = + std::min( + conn.outstandings.numOutstanding(), kPacketToSendForPTO); } void markPacketLoss( diff --git a/quic/loss/QuicLossFunctions.h b/quic/loss/QuicLossFunctions.h index d67e58f24..09d2a9127 100644 --- a/quic/loss/QuicLossFunctions.h +++ b/quic/loss/QuicLossFunctions.h @@ -139,11 +139,10 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { */ if (!hasDataToWrite && conn.outstandings.packetEvents.empty() && (totalPacketsOutstanding - totalD6DProbesOutstanding) == - conn.outstandings.numClonedPackets()) { + conn.outstandings.clonedPacketsCount) { VLOG(10) << __func__ << " unset alarm pure ack or processed packets only" << " outstanding=" << totalPacketsOutstanding - << " handshakePackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Handshake] + << " handshakePackets=" << conn.outstandings.handshakePacketsCount << " " << conn; conn.pendingEvents.setLossDetectionAlarm = false; timeout.cancelLossTimeout(); @@ -165,11 +164,10 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { if (!conn.pendingEvents.setLossDetectionAlarm) { VLOG_IF(10, !timeout.isLossTimeoutScheduled()) << __func__ << " alarm not scheduled" - << " outstanding=" << totalPacketsOutstanding << " initialPackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Initial] - << " handshakePackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Handshake] << " " - << nodeToString(conn.nodeType) << " " << conn; + << " outstanding=" << totalPacketsOutstanding + << " initialPackets=" << conn.outstandings.initialPacketsCount + << " handshakePackets=" << conn.outstandings.handshakePacketsCount + << " " << nodeToString(conn.nodeType) << " " << conn; return; } timeout.cancelLossTimeout(); @@ -180,13 +178,11 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { << " method=" << conn.lossState.currentAlarmMethod << " haDataToWrite=" << hasDataToWrite << " outstanding=" << totalPacketsOutstanding - << " outstanding clone=" << conn.outstandings.numClonedPackets() + << " outstanding clone=" << conn.outstandings.clonedPacketsCount << " packetEvents=" << conn.outstandings.packetEvents.size() - << " initialPackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Initial] - << " handshakePackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Handshake] << " " - << nodeToString(conn.nodeType) << " " << conn; + << " initialPackets=" << conn.outstandings.initialPacketsCount + << " handshakePackets=" << conn.outstandings.handshakePacketsCount + << " " << nodeToString(conn.nodeType) << " " << conn; timeout.scheduleLossTimeout(alarmDuration.first); conn.pendingEvents.setLossDetectionAlarm = false; } @@ -263,8 +259,8 @@ folly::Optional detectLossPackets( observerLossEvent.addLostPacket(lostByTimeout, lostByReorder, pkt); if (pkt.associatedEvent) { - DCHECK_GT(conn.outstandings.numClonedPackets(), 0); - --conn.outstandings.clonedPacketCount[pnSpace]; + DCHECK_GT(conn.outstandings.clonedPacketsCount, 0); + --conn.outstandings.clonedPacketsCount; } // Invoke LossVisitor if the packet doesn't have a associated PacketEvent; // or if the PacketEvent is present in conn.outstandings.packetEvents. @@ -277,12 +273,12 @@ folly::Optional detectLossPackets( } if (pkt.metadata.isHandshake && !processed) { if (currentPacketNumberSpace == PacketNumberSpace::Initial) { - CHECK(conn.outstandings.packetCount[PacketNumberSpace::Initial]); - --conn.outstandings.packetCount[PacketNumberSpace::Initial]; + CHECK(conn.outstandings.initialPacketsCount); + --conn.outstandings.initialPacketsCount; } else { CHECK_EQ(PacketNumberSpace::Handshake, currentPacketNumberSpace); - CHECK(conn.outstandings.packetCount[PacketNumberSpace::Handshake]); - --conn.outstandings.packetCount[PacketNumberSpace::Handshake]; + CHECK(conn.outstandings.handshakePacketsCount); + --conn.outstandings.handshakePacketsCount; } } VLOG(10) << __func__ << " lost packetNum=" << currentPacketNum @@ -396,11 +392,9 @@ void onLossDetectionAlarm( VLOG(10) << __func__ << " setLossDetectionAlarm=" << conn.pendingEvents.setLossDetectionAlarm << " outstanding=" << conn.outstandings.numOutstanding() - << " initialPackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Initial] - << " handshakePackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Handshake] << " " - << conn; + << " initialPackets=" << conn.outstandings.initialPacketsCount + << " handshakePackets=" << conn.outstandings.handshakePacketsCount + << " " << conn; } /* @@ -441,11 +435,9 @@ folly::Optional handleAckForLoss( << " setLossDetectionAlarm=" << conn.pendingEvents.setLossDetectionAlarm << " outstanding=" << conn.outstandings.numOutstanding() - << " initialPackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Initial] - << " handshakePackets=" - << conn.outstandings.packetCount[PacketNumberSpace::Handshake] << " " - << conn; + << " initialPackets=" << conn.outstandings.initialPacketsCount + << " handshakePackets=" << conn.outstandings.handshakePacketsCount + << " " << conn; return lossEvent; } @@ -472,8 +464,8 @@ void markZeroRttPacketsLost( // Remove the PacketEvent from the outstandings.packetEvents set if (pkt.associatedEvent) { conn.outstandings.packetEvents.erase(*pkt.associatedEvent); - DCHECK_GT(conn.outstandings.numClonedPackets(), 0); - --conn.outstandings.clonedPacketCount[PacketNumberSpace::AppData]; + DCHECK_GT(conn.outstandings.clonedPacketsCount, 0); + --conn.outstandings.clonedPacketsCount; } lossEvent.addLostPacket(pkt); iter = conn.outstandings.packets.erase(iter); diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index ceeefd8ee..429d64a5c 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -187,7 +187,7 @@ PacketNum QuicLossFunctionsTest::sendPacket( *conn.serverConnectionId, conn.ackStates.initialAckState.nextPacketNum, *conn.version); - conn.outstandings.packetCount[PacketNumberSpace::Initial]++; + conn.outstandings.initialPacketsCount++; isHandshake = true; break; case PacketType::Handshake: @@ -197,7 +197,7 @@ PacketNum QuicLossFunctionsTest::sendPacket( *conn.serverConnectionId, conn.ackStates.handshakeAckState.nextPacketNum, *conn.version); - conn.outstandings.packetCount[PacketNumberSpace::Handshake]++; + conn.outstandings.handshakePacketsCount++; isHandshake = true; break; case PacketType::ZeroRtt: @@ -207,14 +207,12 @@ PacketNum QuicLossFunctionsTest::sendPacket( *conn.serverConnectionId, conn.ackStates.appDataAckState.nextPacketNum, *conn.version); - conn.outstandings.packetCount[PacketNumberSpace::AppData]++; break; case PacketType::OneRtt: header = ShortHeader( ProtectionType::KeyPhaseZero, *conn.serverConnectionId, conn.ackStates.appDataAckState.nextPacketNum); - conn.outstandings.packetCount[PacketNumberSpace::AppData]++; break; } PacketNumberSpace packetNumberSpace; @@ -260,7 +258,7 @@ PacketNum QuicLossFunctionsTest::sendPacket( conn.congestionController->onPacketSent(outstandingPacket); } if (associatedEvent) { - conn.outstandings.clonedPacketCount[packetNumberSpace]++; + conn.outstandings.clonedPacketsCount++; // Simulates what the real writer does. auto it = std::find_if( conn.outstandings.packets.begin(), @@ -274,7 +272,7 @@ PacketNum QuicLossFunctionsTest::sendPacket( if (it != conn.outstandings.packets.end()) { if (!it->associatedEvent) { conn.outstandings.packetEvents.emplace(*associatedEvent); - conn.outstandings.clonedPacketCount[packetNumberSpace]++; + conn.outstandings.clonedPacketsCount++; it->associatedEvent = *associatedEvent; } } @@ -1012,7 +1010,7 @@ TEST_F(QuicLossFunctionsTest, TestReorderingThreshold) { for (int i = 0; i < 6; ++i) { sendPacket(*conn, Clock::now(), folly::none, PacketType::Handshake); } - EXPECT_EQ(6, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(6, conn->outstandings.handshakePacketsCount); // Assume some packets are already acked for (auto iter = getFirstOutstandingPacket(*conn, PacketNumberSpace::Handshake) + 2; @@ -1020,7 +1018,7 @@ TEST_F(QuicLossFunctionsTest, TestReorderingThreshold) { getFirstOutstandingPacket(*conn, PacketNumberSpace::Handshake) + 5; iter++) { if (iter->metadata.isHandshake) { - conn->outstandings.packetCount[PacketNumberSpace::Handshake]--; + conn->outstandings.handshakePacketsCount--; } } auto firstHandshakeOpIter = @@ -1042,7 +1040,7 @@ TEST_F(QuicLossFunctionsTest, TestReorderingThreshold) { EXPECT_EQ(lostPacket.back(), 2); // Packet 6 is the only thing remaining inflight, it is a handshake pkt - EXPECT_EQ(1, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn->outstandings.handshakePacketsCount); // Packet 6 should remain in packet as the delta is less than threshold auto numDeclaredLost = std::count_if( @@ -1408,12 +1406,7 @@ TEST_F(QuicLossFunctionsTest, PTOWithHandshakePackets) { EXPECT_EQ(0, lostPackets.size()); EXPECT_EQ(1, conn->lossState.ptoCount); EXPECT_EQ(0, conn->lossState.timeoutBasedRtxCount); - EXPECT_EQ( - conn->pendingEvents.numProbePackets[PacketNumberSpace::Handshake], - kPacketToSendForPTO); - EXPECT_EQ( - conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData], - kPacketToSendForPTO); + EXPECT_EQ(conn->pendingEvents.numProbePackets, kPacketToSendForPTO); EXPECT_EQ(0, conn->lossState.rtxCount); } @@ -1589,7 +1582,7 @@ TEST_F(QuicLossFunctionsTest, DetectPacketLossClonedPacketsCounter) { noopLossMarker, Clock::now(), PacketNumberSpace::AppData); - EXPECT_EQ(0, conn->outstandings.numClonedPackets()); + EXPECT_EQ(0, conn->outstandings.clonedPacketsCount); } TEST_F(QuicLossFunctionsTest, TestMarkPacketLossProcessedPacket) { @@ -1765,7 +1758,7 @@ TEST_F(QuicLossFunctionsTest, TestZeroRttRejectedWithClones) { } EXPECT_EQ(6, conn->outstandings.packets.size()); - ASSERT_EQ(conn->outstandings.numClonedPackets(), 6); + ASSERT_EQ(conn->outstandings.clonedPacketsCount, 6); ASSERT_EQ(conn->outstandings.packetEvents.size(), 2); std::vector lostPackets; @@ -1777,7 +1770,7 @@ TEST_F(QuicLossFunctionsTest, TestZeroRttRejectedWithClones) { ASSERT_EQ(conn->outstandings.packetEvents.size(), 0); EXPECT_EQ(3, conn->outstandings.packets.size()); EXPECT_EQ(lostPackets.size(), 3); - ASSERT_EQ(conn->outstandings.numClonedPackets(), 3); + ASSERT_EQ(conn->outstandings.clonedPacketsCount, 3); size_t numProcessed = 0; for (auto lostPacket : lostPackets) { numProcessed += lostPacket; @@ -1835,7 +1828,7 @@ TEST_F(QuicLossFunctionsTest, OutstandingInitialCounting) { largestSent = sendPacket(*conn, Clock::now(), folly::none, PacketType::Initial); } - EXPECT_EQ(10, conn->outstandings.packetCount[PacketNumberSpace::Initial]); + EXPECT_EQ(10, conn->outstandings.initialPacketsCount); auto noopLossVisitor = [&](auto& /* conn */, auto& /* packet */, bool /* processed */ ) {}; @@ -1846,7 +1839,7 @@ TEST_F(QuicLossFunctionsTest, OutstandingInitialCounting) { TimePoint(100ms), PacketNumberSpace::Initial); // [1, 6] are removed, [7, 10] are still in OP list - EXPECT_EQ(4, conn->outstandings.packetCount[PacketNumberSpace::Initial]); + EXPECT_EQ(4, conn->outstandings.initialPacketsCount); } TEST_F(QuicLossFunctionsTest, OutstandingHandshakeCounting) { @@ -1858,7 +1851,7 @@ TEST_F(QuicLossFunctionsTest, OutstandingHandshakeCounting) { largestSent = sendPacket(*conn, Clock::now(), folly::none, PacketType::Handshake); } - EXPECT_EQ(10, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(10, conn->outstandings.handshakePacketsCount); auto noopLossVisitor = [&](auto& /* conn */, auto& /* packet */, bool /* processed */ ) {}; @@ -1869,12 +1862,12 @@ TEST_F(QuicLossFunctionsTest, OutstandingHandshakeCounting) { TimePoint(100ms), PacketNumberSpace::Handshake); // [1, 6] are removed, [7, 10] are still in OP list - EXPECT_EQ(4, conn->outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(4, conn->outstandings.handshakePacketsCount); } TEST_P(QuicLossFunctionsTest, CappedShiftNoCrash) { auto conn = createConn(); - conn->outstandings.packetCount[PacketNumberSpace::Handshake] = 0; + conn->outstandings.handshakePacketsCount = 0; conn->outstandings.packets.clear(); conn->lossState.ptoCount = std::numeric_limitslossState.ptoCount)>::max(); diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 655812c89..cb1714e77 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -211,10 +211,9 @@ void QuicServerTransport::writeData() { auto& initialCryptoStream = *getCryptoStream(*conn_->cryptoState, EncryptionLevel::Initial); CryptoStreamScheduler initialScheduler(*conn_, initialCryptoStream); - auto& numProbePackets = - conn_->pendingEvents.numProbePackets[PacketNumberSpace::Initial]; - if ((numProbePackets && initialCryptoStream.retransmissionBuffer.size() && - conn_->outstandings.packetCount[PacketNumberSpace::Initial]) || + if ((conn_->pendingEvents.numProbePackets && + initialCryptoStream.retransmissionBuffer.size() && + conn_->outstandings.initialPacketsCount) || initialScheduler.hasData() || (conn_->ackStates.initialAckState.needsToSendAckImmediately && hasAcksToSchedule(conn_->ackStates.initialAckState))) { @@ -231,7 +230,7 @@ void QuicServerTransport::writeData() { version, packetLimit); } - if (!packetLimit && !conn_->pendingEvents.anyProbePackets()) { + if (!packetLimit && !conn_->pendingEvents.numProbePackets) { return; } } @@ -239,11 +238,9 @@ void QuicServerTransport::writeData() { auto& handshakeCryptoStream = *getCryptoStream(*conn_->cryptoState, EncryptionLevel::Handshake); CryptoStreamScheduler handshakeScheduler(*conn_, handshakeCryptoStream); - auto& numProbePackets = - conn_->pendingEvents.numProbePackets[PacketNumberSpace::Handshake]; - if ((conn_->outstandings.packetCount[PacketNumberSpace::Handshake] && + if ((conn_->outstandings.handshakePacketsCount && handshakeCryptoStream.retransmissionBuffer.size() && - numProbePackets) || + conn_->pendingEvents.numProbePackets) || handshakeScheduler.hasData() || (conn_->ackStates.handshakeAckState.needsToSendAckImmediately && hasAcksToSchedule(conn_->ackStates.handshakeAckState))) { @@ -260,7 +257,7 @@ void QuicServerTransport::writeData() { version, packetLimit); } - if (!packetLimit && !conn_->pendingEvents.anyProbePackets()) { + if (!packetLimit && !conn_->pendingEvents.numProbePackets) { return; } } diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 8a32558e5..404a78b80 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -1212,7 +1212,8 @@ TEST_F(QuicServerTransportTest, TestOpenAckStreamFrame) { // Remove any packets that might have been queued. server->getNonConstConn().outstandings.packets.clear(); - server->getNonConstConn().outstandings.packetCount = {}; + server->getNonConstConn().outstandings.initialPacketsCount = 0; + server->getNonConstConn().outstandings.handshakePacketsCount = 0; server->writeChain(streamId, data->clone(), false); loopForWrites(); server->writeChain(streamId, data->clone(), false); @@ -1806,7 +1807,8 @@ TEST_F(QuicServerTransportTest, TestCloneStopSending) { server->getNonConstConn().qLogger = qLogger; server->getNonConstConn().streamManager->getStream(streamId); // knock every handshake outstanding packets out - server->getNonConstConn().outstandings.packetCount = {}; + server->getNonConstConn().outstandings.initialPacketsCount = 0; + server->getNonConstConn().outstandings.handshakePacketsCount = 0; server->getNonConstConn().outstandings.packets.clear(); for (auto& t : server->getNonConstConn().lossState.lossTimes) { t.reset(); diff --git a/quic/state/AckHandlers.cpp b/quic/state/AckHandlers.cpp index 326b6dc3c..6899cdbf8 100644 --- a/quic/state/AckHandlers.cpp +++ b/quic/state/AckHandlers.cpp @@ -232,26 +232,20 @@ void processAckFrame( if (lastAckedPacketSentTime) { conn.lossState.lastAckedPacketSentTime = *lastAckedPacketSentTime; } - CHECK_GE( - conn.outstandings.packetCount[PacketNumberSpace::Initial], - initialPacketAcked); - conn.outstandings.packetCount[PacketNumberSpace::Initial] -= - initialPacketAcked; - CHECK_GE( - conn.outstandings.packetCount[PacketNumberSpace::Handshake], - handshakePacketAcked); - conn.outstandings.packetCount[PacketNumberSpace::Handshake] -= - handshakePacketAcked; - CHECK_GE(conn.outstandings.numClonedPackets(), clonedPacketsAcked); - conn.outstandings.clonedPacketCount[pnSpace] -= clonedPacketsAcked; + CHECK_GE(conn.outstandings.initialPacketsCount, initialPacketAcked); + conn.outstandings.initialPacketsCount -= initialPacketAcked; + CHECK_GE(conn.outstandings.handshakePacketsCount, handshakePacketAcked); + conn.outstandings.handshakePacketsCount -= handshakePacketAcked; + CHECK_GE(conn.outstandings.clonedPacketsCount, clonedPacketsAcked); + conn.outstandings.clonedPacketsCount -= clonedPacketsAcked; CHECK_GE( conn.outstandings.packets.size(), conn.outstandings.declaredLostCount); auto updatedOustandingPacketsCount = conn.outstandings.numOutstanding(); CHECK_GE( updatedOustandingPacketsCount, - conn.outstandings.packetCount[PacketNumberSpace::Handshake] + - conn.outstandings.packetCount[PacketNumberSpace::Initial]); - CHECK_GE(updatedOustandingPacketsCount, conn.outstandings.numClonedPackets()); + conn.outstandings.handshakePacketsCount + + conn.outstandings.initialPacketsCount); + CHECK_GE(updatedOustandingPacketsCount, conn.outstandings.clonedPacketsCount); auto lossEvent = handleAckForLoss(conn, lossVisitor, ack, pnSpace); if (conn.congestionController && (ack.largestAckedPacket.has_value() || lossEvent)) { diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 2f1058ee8..9e28289d4 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -118,11 +118,16 @@ struct OutstandingsInfo { // TODO: Enforce only AppTraffic packets to be clonable folly::F14FastSet packetEvents; - // Number of outstanding packets not including cloned - EnumArray packetCount{}; + // Number of outstanding packets in Initial space, not including cloned + // Initial packets. + uint64_t initialPacketsCount{0}; + + // Number of outstanding packets in Handshake space, not including cloned + // Handshake packets. + uint64_t handshakePacketsCount{0}; // Number of packets are clones or cloned. - EnumArray clonedPacketCount{}; + uint64_t clonedPacketsCount{0}; // Number of packets currently declared lost. uint64_t declaredLostCount{0}; @@ -131,13 +136,6 @@ struct OutstandingsInfo { uint64_t numOutstanding() { return packets.size() - declaredLostCount; } - - // Total number of cloned packets. - uint64_t numClonedPackets() { - return clonedPacketCount[PacketNumberSpace::Initial] + - clonedPacketCount[PacketNumberSpace::Handshake] + - clonedPacketCount[PacketNumberSpace::AppData]; - } }; struct Pacer { @@ -568,13 +566,7 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { std::vector knobs; // Number of probing packets to send after PTO - EnumArray numProbePackets{}; - - bool anyProbePackets() const { - return numProbePackets[PacketNumberSpace::Initial] + - numProbePackets[PacketNumberSpace::Handshake] + - numProbePackets[PacketNumberSpace::AppData]; - } + uint8_t numProbePackets{0}; // true: schedule timeout if not scheduled // false: cancel scheduled timeout diff --git a/quic/state/test/AckHandlersTest.cpp b/quic/state/test/AckHandlersTest.cpp index 2990f8430..370a1a0a8 100644 --- a/quic/state/test/AckHandlersTest.cpp +++ b/quic/state/test/AckHandlersTest.cpp @@ -672,11 +672,9 @@ TEST_P(AckHandlersTest, TestHandshakeCounterUpdate) { 0, LossState()); if (GetParam() == PacketNumberSpace::Initial) { - conn.outstandings.packetCount[PacketNumberSpace::Initial] += - packetNum % 2; + conn.outstandings.initialPacketsCount += packetNum % 2; } else if (GetParam() == PacketNumberSpace::Handshake) { - conn.outstandings.packetCount[PacketNumberSpace::Handshake] += - packetNum % 2; + conn.outstandings.handshakePacketsCount += packetNum % 2; } } @@ -701,13 +699,13 @@ TEST_P(AckHandlersTest, TestHandshakeCounterUpdate) { EXPECT_EQ(numDeclaredLost, conn.outstandings.declaredLostCount); if (GetParam() == PacketNumberSpace::Initial) { EXPECT_EQ(numDeclaredLost, 1); - EXPECT_EQ(1, conn.outstandings.packetCount[PacketNumberSpace::Initial]); + EXPECT_EQ(1, conn.outstandings.initialPacketsCount); // AppData packets won't be acked by an ack in Initial space: // So 0, 2, 4, 6, 8 and 9 are left in OP list EXPECT_EQ(numDeclaredLost + 6, conn.outstandings.packets.size()); } else if (GetParam() == PacketNumberSpace::Handshake) { EXPECT_EQ(numDeclaredLost, 1); - EXPECT_EQ(1, conn.outstandings.packetCount[PacketNumberSpace::Handshake]); + EXPECT_EQ(1, conn.outstandings.handshakePacketsCount); // AppData packets won't be acked by an ack in Handshake space: // So 0, 2, 4, 6, 8 and 9 are left in OP list EXPECT_EQ(numDeclaredLost + 6, conn.outstandings.packets.size()); @@ -802,7 +800,7 @@ TEST_P(AckHandlersTest, SkipAckVisitor) { // outstandings.packetEvents outstandingPacket.associatedEvent.emplace(PacketNumberSpace::AppData, 0); conn.outstandings.packets.push_back(std::move(outstandingPacket)); - conn.outstandings.clonedPacketCount[PacketNumberSpace::AppData]++; + conn.outstandings.clonedPacketsCount++; ReadAckFrame ackFrame; ackFrame.largestAcked = 0; @@ -850,7 +848,7 @@ TEST_P(AckHandlersTest, NoDoubleProcess) { conn.outstandings.packets.push_back(std::move(outstandingPacket1)); conn.outstandings.packets.push_back(std::move(outstandingPacket2)); - conn.outstandings.clonedPacketCount[PacketNumberSpace::AppData] += 2; + conn.outstandings.clonedPacketsCount += 2; conn.outstandings.packetEvents.insert( PacketEvent(PacketNumberSpace::AppData, packetNum1)); @@ -913,7 +911,7 @@ TEST_P(AckHandlersTest, ClonedPacketsCounter) { conn.outstandings.packets.push_back(std::move(outstandingPacket1)); conn.outstandings.packets.push_back(std::move(outstandingPacket2)); - conn.outstandings.clonedPacketCount[PacketNumberSpace::AppData] = 1; + conn.outstandings.clonedPacketsCount = 1; conn.outstandings.packetEvents.emplace( PacketNumberSpace::AppData, packetNum1); @@ -936,7 +934,7 @@ TEST_P(AckHandlersTest, ClonedPacketsCounter) { ) { /* no-op */ }, Clock::now()); EXPECT_EQ(2, ackVisitorCounter); - EXPECT_EQ(0, conn.outstandings.numClonedPackets()); + EXPECT_EQ(0, conn.outstandings.clonedPacketsCount); } TEST_P(AckHandlersTest, UpdateMaxAckDelay) { @@ -1009,7 +1007,7 @@ TEST_P(AckHandlersTest, AckNotOutstandingButLoss) { 0, LossState()); conn.outstandings.packets.push_back(std::move(outstandingPacket)); - conn.outstandings.clonedPacketCount[PacketNumberSpace::AppData]++; + conn.outstandings.clonedPacketsCount++; EXPECT_CALL(*mockQLogger, addPacketsLost(1, 1, 1));