From 777f77678435be97ec88b14b75c66d2f62e77b7c Mon Sep 17 00:00:00 2001 From: Kyle Mirzakhanian Date: Wed, 23 Mar 2022 19:27:50 -0700 Subject: [PATCH] Ship short header padding Summary: QE result post: https://fb.workplace.com/groups/646964542156536/permalink/1989360141250296/ We are going ahead with shipping this, with a default value of 32 for paddingModulo. Leaving the transport knob settings still in place in case vips want to selectively disable the feature (not sure if that is the proper way to config that but I think so it is?) Reviewed By: mjoras Differential Revision: D34596608 fbshipit-source-id: 5603bb391113c29830f43a67e73c0f50154dcca1 --- quic/QuicConstants.h | 2 ++ quic/api/test/QuicTransportFunctionsTest.cpp | 14 +++++++- quic/api/test/QuicTransportTest.cpp | 36 ++++++++++++++------ quic/loss/test/QuicLossFunctionsTest.cpp | 17 +++++++-- quic/state/TransportSettings.h | 2 +- 5 files changed, 56 insertions(+), 15 deletions(-) diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index d95527f6b..d12cfa13d 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -652,4 +652,6 @@ enum class DataPathType : uint8_t { using PriorityLevel = uint8_t; constexpr uint8_t kDefaultMaxPriority = 7; +constexpr size_t kShortHeaderPaddingModulo = 32; + } // namespace quic diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 60178e282..bc3a3e9d6 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -87,6 +87,17 @@ void writeCryptoDataProbesToSocketForTest( version); } +RegularQuicWritePacket stripPaddingFrames(RegularQuicWritePacket packet) { + SmallVec trimmedFrames{}; + for (auto frame : packet.frames) { + if (!frame.asPaddingFrame()) { + trimmedFrames.push_back(frame); + } + } + packet.frames = trimmedFrames; + return packet; +} + auto buildEmptyPacket( QuicServerConnectionState& conn, PacketNumberSpace pnSpace, @@ -4018,7 +4029,8 @@ TEST_F(QuicTransportFunctionsTest, ProbeWriteNewFunctionalFrames) { getVersion(*conn), 1 /* limit to 1 packet */); EXPECT_EQ(2, conn->outstandings.packets.size()); - EXPECT_EQ(1, conn->outstandings.packets[1].packet.frames.size()); + auto packet = stripPaddingFrames(conn->outstandings.packets[1].packet); + EXPECT_EQ(1, packet.frames.size()); EXPECT_EQ( QuicWriteFrame::Type::MaxDataFrame, conn->outstandings.packets[1].packet.frames[0].type()); diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index 3137be937..df2aebeab 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -122,6 +123,17 @@ class QuicTransportTest : public Test { std::shared_ptr transport_; }; +RegularQuicWritePacket stripPaddingFrames(RegularQuicWritePacket packet) { + SmallVec trimmedFrames{}; + for (auto frame : packet.frames) { + if (!frame.asPaddingFrame()) { + trimmedFrames.push_back(frame); + } + } + packet.frames = trimmedFrames; + return packet; +} + size_t bufLength( const SocketAddress&, const std::unique_ptr& buf) { @@ -1704,7 +1716,7 @@ TEST_F(QuicTransportTest, StopSending) { getLastOutstandingPacket( transport_->getConnectionState(), PacketNumberSpace::AppData) ->packet; - EXPECT_EQ(1, packet.frames.size()); + EXPECT_EQ(1, stripPaddingFrames(packet).frames.size()); bool foundStopSending = false; for (auto& frame : packet.frames) { const QuicSimpleFrame* simpleFrame = frame.asQuicSimpleFrame(); @@ -1734,7 +1746,7 @@ TEST_F(QuicTransportTest, StopSendingReadCallbackDefault) { getLastOutstandingPacket( transport_->getConnectionState(), PacketNumberSpace::AppData) ->packet; - EXPECT_EQ(1, packet.frames.size()); + EXPECT_EQ(1, stripPaddingFrames(packet).frames.size()); bool foundStopSending = false; for (auto& frame : packet.frames) { const QuicSimpleFrame* simpleFrame = frame.asQuicSimpleFrame(); @@ -1765,7 +1777,7 @@ TEST_F(QuicTransportTest, StopSendingReadCallback) { getLastOutstandingPacket( transport_->getConnectionState(), PacketNumberSpace::AppData) ->packet; - EXPECT_EQ(1, packet.frames.size()); + EXPECT_EQ(1, stripPaddingFrames(packet).frames.size()); bool foundStopSending = false; for (auto& frame : packet.frames) { const QuicSimpleFrame* simpleFrame = frame.asQuicSimpleFrame(); @@ -3760,9 +3772,11 @@ TEST_F(QuicTransportTest, WriteStreamFromMiddleOfMap) { transport_->getVersion(), conn.transportSettings.writeConnectionDataPacketsLimit); EXPECT_EQ(1, conn.outstandings.packets.size()); - auto& packet2 = *getFirstOutstandingPacket(conn, PacketNumberSpace::AppData); - EXPECT_EQ(1, packet2.packet.frames.size()); - auto& frame2 = packet2.packet.frames.front(); + auto& outstandingPacket2 = + *getFirstOutstandingPacket(conn, PacketNumberSpace::AppData); + auto packet2 = stripPaddingFrames(outstandingPacket2.packet); + EXPECT_EQ(1, packet2.frames.size()); + auto& frame2 = packet2.frames.front(); const WriteStreamFrame* streamFrame2 = frame2.asWriteStreamFrame(); EXPECT_TRUE(streamFrame2); EXPECT_EQ(streamFrame2->streamId, s2); @@ -3782,10 +3796,12 @@ TEST_F(QuicTransportTest, WriteStreamFromMiddleOfMap) { transport_->getVersion(), conn.transportSettings.writeConnectionDataPacketsLimit); EXPECT_EQ(1, conn.outstandings.packets.size()); - auto& packet3 = *getFirstOutstandingPacket(conn, PacketNumberSpace::AppData); - EXPECT_EQ(2, packet3.packet.frames.size()); - auto& frame3 = packet3.packet.frames.front(); - auto& frame4 = packet3.packet.frames.back(); + auto& outstandingPacket3 = + *getFirstOutstandingPacket(conn, PacketNumberSpace::AppData); + auto packet3 = stripPaddingFrames(outstandingPacket3.packet); + EXPECT_EQ(2, packet3.frames.size()); + auto& frame3 = packet3.frames.front(); + auto& frame4 = packet3.frames.back(); const WriteStreamFrame* streamFrame3 = frame3.asWriteStreamFrame(); EXPECT_TRUE(streamFrame3); EXPECT_EQ(streamFrame3->streamId, s2); diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index de5d7217d..99d42c363 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -304,6 +304,17 @@ PacketNum QuicLossFunctionsTest::sendPacket( return conn.lossState.largestSent.value(); } +RegularQuicWritePacket stripPaddingFrames(RegularQuicWritePacket packet) { + SmallVec trimmedFrames{}; + for (auto frame : packet.frames) { + if (!frame.asPaddingFrame()) { + trimmedFrames.push_back(frame); + } + } + packet.frames = trimmedFrames; + return packet; +} + TEST_F(QuicLossFunctionsTest, AllPacketsProcessed) { auto conn = createConn(); EXPECT_CALL(*quicStats_, onPTO()).Times(0); @@ -1592,9 +1603,9 @@ TEST_F(QuicLossFunctionsTest, TestMarkPacketLossProcessedPacket) { ASSERT_TRUE(conn->outstandings.packetEvents.empty()); uint32_t streamDataCounter = 0, streamWindowUpdateCounter = 0, connWindowUpdateCounter = 0; - for (const auto& frame : - getLastOutstandingPacket(*conn, PacketNumberSpace::AppData) - ->packet.frames) { + auto strippedPacket = stripPaddingFrames( + getLastOutstandingPacket(*conn, PacketNumberSpace::AppData)->packet); + for (const auto& frame : strippedPacket.frames) { switch (frame.type()) { case QuicWriteFrame::Type::WriteStreamFrame: streamDataCounter++; diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index f2f09dc43..2d2e11d58 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -278,7 +278,7 @@ struct TransportSettings { // in a packet is always an increment of paddingModulo, hiding the actual // packet size from packet analysis. // Padding Modulo of 0 turns off padding for short header packets. - size_t paddingModulo{0}; + size_t paddingModulo{kShortHeaderPaddingModulo}; // Whether to use adaptive loss thresholds for reodering and timeout bool useAdaptiveLossThresholds{false}; // Whether to automatically increase receive conn flow control. The