From f5ca7ef590805e020461617f697261cb73b4d69c Mon Sep 17 00:00:00 2001 From: Yang Chi Date: Tue, 24 Sep 2019 15:14:05 -0700 Subject: [PATCH] Add new retransmission buffer directly at the end of buffer list Summary: The retransmission buffer list is sorted by offset. New written always has higher offset than existing ones. Thus the binary search isn't necessary. Reviewed By: mjoras Differential Revision: D17477354 fbshipit-source-id: d413a5c84c3831b257d5c1e6375bec56a763926b --- quic/api/QuicTransportFunctions.cpp | 15 ++---- quic/api/test/QuicTransportFunctionsTest.cpp | 35 +++++++++++++ quic/loss/test/QuicLossFunctionsTest.cpp | 40 +++++++++++++++ .../stream/test/StreamStateMachineTest.cpp | 49 +++++++++++++++++++ 4 files changed, 129 insertions(+), 10 deletions(-) diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index eb1c951bf..f65cebc87 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -98,16 +98,11 @@ void handleNewStreamDataWritten( stream.currentWriteOffset += frameLen; auto bufWritten = stream.writeBuffer.split(folly::to(frameLen)); stream.currentWriteOffset += frameFin ? 1 : 0; - auto insertIt = std::upper_bound( - stream.retransmissionBuffer.begin(), - stream.retransmissionBuffer.end(), - originalOffset, - [](const auto& offset, const auto& compare) { - // TODO: huh? why isn't this a >= ? - return compare.offset > offset; - }); - stream.retransmissionBuffer.emplace( - insertIt, std::move(bufWritten), originalOffset, frameFin); + CHECK( + stream.retransmissionBuffer.empty() || + stream.retransmissionBuffer.back().offset < originalOffset); + stream.retransmissionBuffer.emplace_back( + std::move(bufWritten), originalOffset, frameFin); } void handleRetransmissionWritten( diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index c2fbee6e6..28597bc96 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -1155,6 +1155,41 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketWithNoBytesForHeader) { EXPECT_TRUE(conn->outstandingPackets.empty()); } +TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketRetxBufferSorted) { + EventBase evb; + folly::test::MockAsyncUDPSocket socket(&evb); + auto conn = createConn(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + auto buf1 = IOBuf::copyBuffer("Whatsapp"); + writeDataToQuicStream(*stream, std::move(buf1), false); + writeQuicDataToSocket( + socket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + conn->transportSettings.writeConnectionDataPacketsLimit); + EXPECT_EQ(1, stream->retransmissionBuffer.size()); + + auto buf2 = IOBuf::copyBuffer("Google Buzz"); + writeDataToQuicStream(*stream, std::move(buf2), false); + writeQuicDataToSocket( + socket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + conn->transportSettings.writeConnectionDataPacketsLimit); + EXPECT_EQ(2, stream->retransmissionBuffer.size()); + EXPECT_GT( + stream->retransmissionBuffer.back().offset, + stream->retransmissionBuffer.front().offset); +} + TEST_F(QuicTransportFunctionsTest, NothingWritten) { auto conn = createConn(); auto mockCongestionController = std::make_unique(); diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index 8dc8f1af0..cb63ce001 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -389,6 +389,46 @@ TEST_F(QuicLossFunctionsTest, TestMarkPacketLoss) { EXPECT_TRUE(eq(buf, buffer.data.move())); } +TEST_F(QuicLossFunctionsTest, RetxBufferSortedAfterLoss) { + folly::EventBase evb; + MockAsyncUDPSocket socket(&evb); + auto conn = createConn(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + auto buf1 = IOBuf::copyBuffer("Worse case scenario"); + auto buf2 = IOBuf::copyBuffer("The hard problem"); + auto buf3 = IOBuf::copyBuffer("And then we had a flash of insight..."); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + socket, + *stream, + *buf1); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + socket, + *stream, + *buf2); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + socket, + *stream, + *buf3); + EXPECT_EQ(3, stream->retransmissionBuffer.size()); + EXPECT_EQ(3, conn->outstandingPackets.size()); + auto packet = conn->outstandingPackets[folly::Random::rand32() % 3]; + markPacketLoss( + *conn, packet.packet, false, packet.packet.header.getPacketSequenceNum()); + EXPECT_EQ(2, stream->retransmissionBuffer.size()); + EXPECT_GT( + stream->retransmissionBuffer.back().offset, + stream->retransmissionBuffer.front().offset); +} + TEST_F(QuicLossFunctionsTest, TestMarkCryptoLostAfterCancelRetransmission) { folly::EventBase evb; MockAsyncUDPSocket socket(&evb); diff --git a/quic/state/stream/test/StreamStateMachineTest.cpp b/quic/state/stream/test/StreamStateMachineTest.cpp index 884b710ae..94728fb90 100644 --- a/quic/state/stream/test/StreamStateMachineTest.cpp +++ b/quic/state/stream/test/StreamStateMachineTest.cpp @@ -217,6 +217,55 @@ TEST_F(QuicOpenStateTest, AckStream) { ASSERT_TRUE(isState(stream->send)); } +TEST_F(QuicOpenStateTest, RetxBufferSortedAfterAck) { + auto conn = createConn(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + EventBase evb; + folly::test::MockAsyncUDPSocket socket(&evb); + folly::Optional serverChosenConnId = *conn->clientConnectionId; + serverChosenConnId.value().data()[0] ^= 0x01; + + auto buf1 = IOBuf::copyBuffer("Alice"); + auto buf2 = IOBuf::copyBuffer("Bob"); + auto buf3 = IOBuf::copyBuffer("NSA"); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *serverChosenConnId, + socket, + *stream, + *buf1, + false); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *serverChosenConnId, + socket, + *stream, + *buf2, + false); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *serverChosenConnId, + socket, + *stream, + *buf3, + false); + + EXPECT_EQ(3, stream->retransmissionBuffer.size()); + EXPECT_EQ(3, conn->outstandingPackets.size()); + auto packet = conn->outstandingPackets[folly::Random::rand32() % 3]; + auto streamFrame = boost::get( + conn->outstandingPackets[std::rand() % 3].packet.frames.front()); + StreamEvents::AckStreamFrame ack(streamFrame); + invokeHandler(stream->send, ack, *stream); + EXPECT_EQ(2, stream->retransmissionBuffer.size()); + EXPECT_GT( + stream->retransmissionBuffer.back().offset, + stream->retransmissionBuffer.front().offset); +} + TEST_F(QuicOpenStateTest, AckStreamAfterSkip) { auto conn = createConn(); conn->partialReliabilityEnabled = true;