Modify write_index_ advancement to incorporate coalescing stream frames. gfe-relnote: protected by gfe2_restart_flag_quic_coalesce_stream_frames_2 PiperOrigin-RevId: 279803164 Change-Id: Idc809c831725b92401b2fd5864379398ddec1bf0
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index f07394d..8c17421 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -1475,10 +1475,10 @@ return false; } - if (GetQuicReloadableFlag(quic_coalesce_stream_frames) && + if (GetQuicRestartFlag(quic_coalesce_stream_frames_2) && frame.type == STREAM_FRAME && MaybeCoalesceStreamFrame(frame.stream_frame)) { - QUIC_RELOADABLE_FLAG_COUNT(quic_coalesce_stream_frames); + QUIC_RESTART_FLAG_COUNT_N(quic_coalesce_stream_frames_2, 1, 3); return true; }
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc index 1b7deda..da6b5ea 100644 --- a/quic/core/quic_packet_creator_test.cc +++ b/quic/core/quic_packet_creator_test.cc
@@ -1999,7 +1999,7 @@ if (!GetParam().version_serialization) { creator_.StopSendingVersion(); } - SetQuicReloadableFlag(quic_coalesce_stream_frames, true); + SetQuicRestartFlag(quic_coalesce_stream_frames_2, true); const size_t max_plaintext_size = client_framer_.GetMaxPlaintextSize(creator_.max_packet_length()); EXPECT_FALSE(creator_.HasPendingFrames());
diff --git a/quic/core/quic_stream_send_buffer.cc b/quic/core/quic_stream_send_buffer.cc index ead192d..158afd8 100644 --- a/quic/core/quic_stream_send_buffer.cc +++ b/quic/core/quic_stream_send_buffer.cc
@@ -9,6 +9,7 @@ #include "net/third_party/quiche/src/quic/core/quic_stream_send_buffer.h" #include "net/third_party/quiche/src/quic/core/quic_utils.h" #include "net/third_party/quiche/src/quic/platform/api/quic_bug_tracker.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_containers.h" #include "net/third_party/quiche/src/quic/platform/api/quic_flag_utils.h" #include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" @@ -95,6 +96,8 @@ bool QuicStreamSendBuffer::WriteStreamData(QuicStreamOffset offset, QuicByteCount data_length, QuicDataWriter* writer) { + // TODO(renjietang): Remove this variable once quic_coalesce_stream_frames_2 + // is deprecated. bool write_index_hit = false; QuicDeque<BufferedSlice>::iterator slice_it = write_index_ == -1 @@ -134,15 +137,33 @@ offset += copy_length; data_length -= copy_length; - if (write_index_hit && copy_length == available_bytes_in_slice) { + if (GetQuicRestartFlag(quic_coalesce_stream_frames_2)) { + QUIC_RESTART_FLAG_COUNT_N(quic_coalesce_stream_frames_2, 2, 3); + if (write_index_ != -1) { + QuicDeque<BufferedSlice>::const_iterator index_slice = + buffered_slices_.begin() + write_index_; + if (index_slice->offset == slice_it->offset && + copy_length == available_bytes_in_slice) { + // The slice pointed by write_index has been fully written, advance + // write index. + ++write_index_; + } + } + } else if (write_index_hit && copy_length == available_bytes_in_slice) { // Finished writing all data in current slice, advance write index for // next write. ++write_index_; } } - if (write_index_hit && - static_cast<size_t>(write_index_) == buffered_slices_.size()) { + if (GetQuicRestartFlag(quic_coalesce_stream_frames_2)) { + QUIC_RESTART_FLAG_COUNT_N(quic_coalesce_stream_frames_2, 3, 3); + if (write_index_ != -1 && + static_cast<size_t>(write_index_) == buffered_slices_.size()) { + write_index_ = -1; + } + } else if (write_index_hit && + static_cast<size_t>(write_index_) == buffered_slices_.size()) { // Already write to the end off buffer. QUIC_DVLOG(2) << "Finish writing out all buffered data."; write_index_ = -1;
diff --git a/quic/core/quic_stream_send_buffer_test.cc b/quic/core/quic_stream_send_buffer_test.cc index 3a5efb5..a6a7ad2 100644 --- a/quic/core/quic_stream_send_buffer_test.cc +++ b/quic/core/quic_stream_send_buffer_test.cc
@@ -115,6 +115,41 @@ EXPECT_EQ(3840u, send_buffer_.stream_bytes_outstanding()); } +// Regression test for b/143491027. +TEST_F(QuicStreamSendBufferTest, + WriteStreamDataContainsBothRetransmissionAndNewData) { + std::string copy1(1024, 'a'); + std::string copy2 = + std::string(512, 'a') + std::string(256, 'b') + std::string(256, 'c'); + std::string copy3 = std::string(1024, 'c') + std::string(100, 'd'); + char buf[6000]; + QuicDataWriter writer(6000, buf, HOST_BYTE_ORDER); + // Write more than one slice. + EXPECT_EQ(0, QuicStreamSendBufferPeer::write_index(&send_buffer_)); + ASSERT_TRUE(send_buffer_.WriteStreamData(0, 1024, &writer)); + EXPECT_EQ(copy1, QuicStringPiece(buf, 1024)); + EXPECT_EQ(1, QuicStreamSendBufferPeer::write_index(&send_buffer_)); + + // Retransmit the first frame and also send new data. + ASSERT_TRUE(send_buffer_.WriteStreamData(0, 2048, &writer)); + EXPECT_EQ(copy1 + copy2, QuicStringPiece(buf + 1024, 2048)); + + // Write new data. + if (!GetQuicRestartFlag(quic_coalesce_stream_frames_2)) { + EXPECT_EQ(1, QuicStreamSendBufferPeer::write_index(&send_buffer_)); + EXPECT_DEBUG_DEATH(send_buffer_.WriteStreamData(2048, 50, &writer), + "Tried to write data out of sequence."); + } else { + EXPECT_EQ(2, QuicStreamSendBufferPeer::write_index(&send_buffer_)); + ASSERT_TRUE(send_buffer_.WriteStreamData(2048, 50, &writer)); + EXPECT_EQ(std::string(50, 'c'), QuicStringPiece(buf + 1024 + 2048, 50)); + EXPECT_EQ(2, QuicStreamSendBufferPeer::write_index(&send_buffer_)); + ASSERT_TRUE(send_buffer_.WriteStreamData(2048, 1124, &writer)); + EXPECT_EQ(copy3, QuicStringPiece(buf + 1024 + 2048 + 50, 1124)); + EXPECT_EQ(3, QuicStreamSendBufferPeer::write_index(&send_buffer_)); + } +} + TEST_F(QuicStreamSendBufferTest, RemoveStreamFrame) { WriteAllData();
diff --git a/quic/test_tools/quic_stream_send_buffer_peer.h b/quic/test_tools/quic_stream_send_buffer_peer.h index f61cb00..3adb173 100644 --- a/quic/test_tools/quic_stream_send_buffer_peer.h +++ b/quic/test_tools/quic_stream_send_buffer_peer.h
@@ -20,6 +20,10 @@ QuicStreamSendBuffer* send_buffer); static QuicByteCount TotalLength(QuicStreamSendBuffer* send_buffer); + + static int32_t write_index(QuicStreamSendBuffer* send_buffer) { + return send_buffer->write_index_; + } }; } // namespace test