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 <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36396}
diff --git a/net/dcsctp/tx/rr_send_queue.cc b/net/dcsctp/tx/rr_send_queue.cc
index 7f42f3d..b3e695b 100644
--- a/net/dcsctp/tx/rr_send_queue.cc
+++ b/net/dcsctp/tx/rr_send_queue.cc
@@ -136,19 +136,13 @@
RTC_DCHECK(IsConsistent());
}
-absl::optional<SendQueue::DataToSend> 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 @@
RTC_DCHECK(stream_it != streams_.end());
}
- absl::optional<DataToSend> 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 fecb6e0..6da585d 100644
--- a/net/dcsctp/tx/rr_send_queue.h
+++ b/net/dcsctp/tx/rr_send_queue.h
@@ -40,9 +40,6 @@
// 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<void(StreamID)> on_buffered_amount_low,
@@ -126,8 +123,9 @@
TimeMs expires_at,
const SendOptions& send_options);
- // Possibly produces a data chunk to send.
- absl::optional<DataToSend> 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 78f7616..a93c4a3 100644
--- a/net/dcsctp/tx/rr_send_queue_test.cc
+++ b/net/dcsctp/tx/rr_send_queue_test.cc
@@ -154,35 +154,6 @@
EXPECT_TRUE(buf_.IsEmpty());
}
-TEST_F(RRSendQueueTest, WillNotSendTooSmallPacket) {
- std::vector<uint8_t> 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<SendQueue::DataToSend> 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<SendQueue::DataToSend> 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<uint8_t> payload(20);
@@ -774,40 +745,5 @@
EXPECT_FALSE(buf_.Produce(kNow, kOneFragmentPacketSize).has_value());
}
-
-TEST_F(RRSendQueueTest, WillStayInStreamWhenOnlySmallFragmentRemaining) {
- buf_.Add(kNow,
- DcSctpMessage(StreamID(5), kPPID,
- std::vector<uint8_t>(kOneFragmentPacketSize * 2)));
- buf_.Add(kNow, DcSctpMessage(StreamID(6), kPPID, std::vector<uint8_t>(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
