From 8beccd5c4784b5db8757336eb5be45ccf172b5ee Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 18 Mar 2022 16:40:12 +0100 Subject: [PATCH] dcsctp: Remove limit of message fragmentation This was available in the beginning, as a way to increase the chance of a message sent with partial reliability to be delivered, by avoiding it to be fragmented in too small fragments. This however added a few downsides: * Packet efficiency goes down, as the entire MTU isn't always used * Complexity increases when adding message interleaving, since if one stream refuses to produce messages, but there is another stream with a very small message that could fit in its place, it should be used instead. Removing this feature altogether is much easier. It's hard to defend. Bug: webrtc:5696 Change-Id: Ie2f296e052f4a32a281497d379c0d528a2df3308 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/257168 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#36396} --- net/dcsctp/tx/rr_send_queue.cc | 38 +++++++---------- net/dcsctp/tx/rr_send_queue.h | 8 ++-- net/dcsctp/tx/rr_send_queue_test.cc | 64 ----------------------------- 3 files changed, 18 insertions(+), 92 deletions(-) diff --git a/net/dcsctp/tx/rr_send_queue.cc b/net/dcsctp/tx/rr_send_queue.cc index 7f42f3d9b1..b3e695bc5c 100644 --- a/net/dcsctp/tx/rr_send_queue.cc +++ b/net/dcsctp/tx/rr_send_queue.cc @@ -136,19 +136,13 @@ void RRSendQueue::OutgoingStream::Add(DcSctpMessage message, RTC_DCHECK(IsConsistent()); } -absl::optional RRSendQueue::OutgoingStream::Produce( - TimeMs now, - size_t max_size) { +SendQueue::DataToSend RRSendQueue::OutgoingStream::Produce(TimeMs now, + size_t max_size) { RTC_DCHECK(!items_.empty()); Item* item = &items_.front(); DcSctpMessage& message = item->message; - if (item->remaining_size > max_size && max_size < kMinimumFragmentedPayload) { - RTC_DCHECK(IsConsistent()); - return absl::nullopt; - } - // Allocate Message ID and SSN when the first fragment is sent. if (!item->message_id.has_value()) { MID& mid = @@ -354,22 +348,20 @@ absl::optional RRSendQueue::Produce(TimeMs now, RTC_DCHECK(stream_it != streams_.end()); } - absl::optional data = stream_it->second.Produce(now, max_size); - if (data.has_value()) { - RTC_DLOG(LS_VERBOSE) << log_prefix_ << "Producing DATA, type=" - << (data->data.is_unordered ? "unordered" : "ordered") - << "::" - << (*data->data.is_beginning && *data->data.is_end - ? "complete" - : *data->data.is_beginning - ? "first" - : *data->data.is_end ? "last" : "middle") - << ", stream_id=" << *stream_it->first - << ", ppid=" << *data->data.ppid - << ", length=" << data->data.payload.size(); + DataToSend data = stream_it->second.Produce(now, max_size); + RTC_DLOG(LS_VERBOSE) << log_prefix_ << "Producing DATA, type=" + << (data.data.is_unordered ? "unordered" : "ordered") + << "::" + << (*data.data.is_beginning && *data.data.is_end + ? "complete" + : *data.data.is_beginning + ? "first" + : *data.data.is_end ? "last" : "middle") + << ", stream_id=" << *stream_it->first + << ", ppid=" << *data.data.ppid + << ", length=" << data.data.payload.size(); - previous_message_has_ended_ = *data->data.is_end; - } + previous_message_has_ended_ = *data.data.is_end; RTC_DCHECK(IsConsistent()); return data; diff --git a/net/dcsctp/tx/rr_send_queue.h b/net/dcsctp/tx/rr_send_queue.h index fecb6e0f2e..6da585df85 100644 --- a/net/dcsctp/tx/rr_send_queue.h +++ b/net/dcsctp/tx/rr_send_queue.h @@ -40,9 +40,6 @@ namespace dcsctp { // established, this send queue is always present - even for closed connections. class RRSendQueue : public SendQueue { public: - // How small a data chunk's payload may be, if having to fragment a message. - static constexpr size_t kMinimumFragmentedPayload = 10; - RRSendQueue(absl::string_view log_prefix, size_t buffer_size, std::function on_buffered_amount_low, @@ -126,8 +123,9 @@ class RRSendQueue : public SendQueue { TimeMs expires_at, const SendOptions& send_options); - // Possibly produces a data chunk to send. - absl::optional Produce(TimeMs now, size_t max_size); + // Produces a data chunk to send. This is only called on streams that have + // data available. + DataToSend Produce(TimeMs now, size_t max_size); const ThresholdWatcher& buffered_amount() const { return buffered_amount_; } ThresholdWatcher& buffered_amount() { return buffered_amount_; } diff --git a/net/dcsctp/tx/rr_send_queue_test.cc b/net/dcsctp/tx/rr_send_queue_test.cc index 78f7616766..a93c4a3d1c 100644 --- a/net/dcsctp/tx/rr_send_queue_test.cc +++ b/net/dcsctp/tx/rr_send_queue_test.cc @@ -154,35 +154,6 @@ TEST_F(RRSendQueueTest, BufferBecomesFullAndEmptied) { EXPECT_TRUE(buf_.IsEmpty()); } -TEST_F(RRSendQueueTest, WillNotSendTooSmallPacket) { - std::vector payload(RRSendQueue::kMinimumFragmentedPayload + 1); - buf_.Add(kNow, DcSctpMessage(kStreamID, kPPID, payload)); - - // Wouldn't fit enough payload (wouldn't want to fragment) - EXPECT_FALSE( - buf_.Produce(kNow, - /*max_size=*/RRSendQueue::kMinimumFragmentedPayload - 1) - .has_value()); - - // Minimum fragment - absl::optional chunk_one = - buf_.Produce(kNow, - /*max_size=*/RRSendQueue::kMinimumFragmentedPayload); - ASSERT_TRUE(chunk_one.has_value()); - EXPECT_EQ(chunk_one->data.stream_id, kStreamID); - EXPECT_EQ(chunk_one->data.ppid, kPPID); - - // There is only one byte remaining - it can be fetched as it doesn't require - // additional fragmentation. - absl::optional chunk_two = - buf_.Produce(kNow, /*max_size=*/1); - ASSERT_TRUE(chunk_two.has_value()); - EXPECT_EQ(chunk_two->data.stream_id, kStreamID); - EXPECT_EQ(chunk_two->data.ppid, kPPID); - - EXPECT_TRUE(buf_.IsEmpty()); -} - TEST_F(RRSendQueueTest, DefaultsToOrderedSend) { std::vector payload(20); @@ -774,40 +745,5 @@ TEST_F(RRSendQueueTest, WillStayInAStreamAsLongAsThatMessageIsSending) { EXPECT_FALSE(buf_.Produce(kNow, kOneFragmentPacketSize).has_value()); } - -TEST_F(RRSendQueueTest, WillStayInStreamWhenOnlySmallFragmentRemaining) { - buf_.Add(kNow, - DcSctpMessage(StreamID(5), kPPID, - std::vector(kOneFragmentPacketSize * 2))); - buf_.Add(kNow, DcSctpMessage(StreamID(6), kPPID, std::vector(1))); - - ASSERT_HAS_VALUE_AND_ASSIGN(SendQueue::DataToSend chunk1, - buf_.Produce(kNow, kOneFragmentPacketSize)); - EXPECT_EQ(chunk1.data.stream_id, StreamID(5)); - EXPECT_THAT(chunk1.data.payload, SizeIs(kOneFragmentPacketSize)); - - // Now assume that there will be a lot of previous chunks that need to be - // retransmitted, which fills up the next packet and there is little space - // left in the packet for new chunks. What it should NOT do right now is to - // try to send a message from StreamID 6. And it should not try to send a very - // small fragment from StreamID 5 either. So just skip this one. - EXPECT_FALSE(buf_.Produce(kNow, 8).has_value()); - - // When the next produce request comes with a large buffer to fill, continue - // sending from StreamID 5. - - ASSERT_HAS_VALUE_AND_ASSIGN(SendQueue::DataToSend chunk2, - buf_.Produce(kNow, kOneFragmentPacketSize)); - EXPECT_EQ(chunk2.data.stream_id, StreamID(5)); - EXPECT_THAT(chunk2.data.payload, SizeIs(kOneFragmentPacketSize)); - - // Lastly, produce a message on StreamID 6. - ASSERT_HAS_VALUE_AND_ASSIGN(SendQueue::DataToSend chunk3, - buf_.Produce(kNow, kOneFragmentPacketSize)); - EXPECT_EQ(chunk3.data.stream_id, StreamID(6)); - EXPECT_THAT(chunk3.data.payload, SizeIs(1)); - - EXPECT_FALSE(buf_.Produce(kNow, 8).has_value()); -} } // namespace } // namespace dcsctp