dcsctp: Refactor OutstandingData

Minor refactoring of the API, to put optional arguments last. Also
changed internal structures to reflect that order, for consistency.

Also reduced size of Item from 88 to 72 bytes, by packing fields better.

Bug: webrtc:5696
Change-Id: I1b9d50831a8e9a358224682d06a782a3269b8416
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264123
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37403}

4 files changed, 91 insertions, 115 deletions

diff --git a/net/dcsctp/tx/outstanding_data.cc b/net/dcsctp/tx/outstanding_data.cc
index 91651e9a73..8e91ef9e3f 100644
--- a/net/dcsctp/tx/outstanding_data.cc
+++ b/net/dcsctp/tx/outstanding_data.cc
@@ -265,9 +265,10 @@ void OutstandingData::AbandonAllFor(const Item& item) {
                      Data::IsEnd(true), item.data().is_unordered);
     Item& added_item =
         outstanding_data_
-            .emplace(tsn,
-                     Item(std::move(message_end), MaxRetransmits::NoLimit(),
-                          TimeMs(0), TimeMs::InfiniteFuture()))
+            .emplace(std::piecewise_construct, std::forward_as_tuple(tsn),
+                     std::forward_as_tuple(std::move(message_end), TimeMs(0),
+                                           MaxRetransmits::NoLimit(),
+                                           TimeMs::InfiniteFuture()))
             .first->second;
     // The added chunk shouldn't be included in `outstanding_bytes`, so set it
     // as acked.
@@ -381,8 +382,8 @@ UnwrappedTSN OutstandingData::highest_outstanding_tsn() const {
 
 absl::optional<UnwrappedTSN> OutstandingData::Insert(
     const Data& data,
-    MaxRetransmits max_retransmissions,
     TimeMs time_sent,
+    MaxRetransmits max_retransmissions,
     TimeMs expires_at) {
   UnwrappedTSN tsn = next_tsn_;
   next_tsn_.Increment();
@@ -392,8 +393,9 @@ absl::optional<UnwrappedTSN> OutstandingData::Insert(
   outstanding_bytes_ += chunk_size;
   ++outstanding_items_;
   auto it = outstanding_data_
-                .emplace(tsn, Item(data.Clone(), max_retransmissions, time_sent,
-                                   expires_at))
+                .emplace(std::piecewise_construct, std::forward_as_tuple(tsn),
+                         std::forward_as_tuple(data.Clone(), time_sent,
+                                               max_retransmissions, expires_at))
                 .first;
 
   if (it->second.has_expired(time_sent)) {
diff --git a/net/dcsctp/tx/outstanding_data.h b/net/dcsctp/tx/outstanding_data.h
index 5c638680b7..0332fdd38e 100644
--- a/net/dcsctp/tx/outstanding_data.h
+++ b/net/dcsctp/tx/outstanding_data.h
@@ -21,6 +21,7 @@
 #include "net/dcsctp/packet/chunk/iforward_tsn_chunk.h"
 #include "net/dcsctp/packet/chunk/sack_chunk.h"
 #include "net/dcsctp/packet/data.h"
+#include "net/dcsctp/public/types.h"
 
 namespace dcsctp {
 
@@ -120,10 +121,11 @@ class OutstandingData {
   // Schedules `data` to be sent, with the provided partial reliability
   // parameters. Returns the TSN if the item was actually added and scheduled to
   // be sent, and absl::nullopt if it shouldn't be sent.
-  absl::optional<UnwrappedTSN> Insert(const Data& data,
-                                      MaxRetransmits max_retransmissions,
-                                      TimeMs time_sent,
-                                      TimeMs expires_at);
+  absl::optional<UnwrappedTSN> Insert(
+      const Data& data,
+      TimeMs time_sent,
+      MaxRetransmits max_retransmissions = MaxRetransmits::NoLimit(),
+      TimeMs expires_at = TimeMs::InfiniteFuture());
 
   // Nacks all outstanding data.
   void NackAll();
@@ -162,15 +164,18 @@ class OutstandingData {
       kAbandon,
     };
 
-    explicit Item(Data data,
-                  MaxRetransmits max_retransmissions,
-                  TimeMs time_sent,
-                  TimeMs expires_at)
-        : max_retransmissions_(max_retransmissions),
-          time_sent_(time_sent),
+    Item(Data data,
+         TimeMs time_sent,
+         MaxRetransmits max_retransmissions,
+         TimeMs expires_at)
+        : time_sent_(time_sent),
+          max_retransmissions_(max_retransmissions),
           expires_at_(expires_at),
           data_(std::move(data)) {}
 
+    Item(const Item&) = delete;
+    Item& operator=(const Item&) = delete;
+
     TimeMs time_sent() const { return time_sent_; }
 
     const Data& data() const { return data_; }
@@ -208,7 +213,7 @@ class OutstandingData {
     bool has_expired(TimeMs now) const;
 
    private:
-    enum class Lifecycle {
+    enum class Lifecycle : uint8_t {
       // The chunk is alive (sent, received, etc)
       kActive,
       // The chunk is scheduled to be retransmitted, and will then transition to
@@ -217,7 +222,7 @@ class OutstandingData {
       // The chunk has been abandoned. This is a terminal state.
       kAbandoned
     };
-    enum class AckState {
+    enum class AckState : uint8_t {
       // The chunk is in-flight.
       kUnacked,
       // The chunk has been received and acknowledged.
@@ -225,6 +230,19 @@ class OutstandingData {
       // The chunk has been nacked and is possibly lost.
       kNacked
     };
+
+    // NOTE: This data structure has been optimized for size, by ordering fields
+    // to avoid unnecessary padding.
+
+    // When the packet was sent, and placed in this queue.
+    const TimeMs time_sent_;
+    // If the message was sent with a maximum number of retransmissions, this is
+    // set to that number. The value zero (0) means that it will never be
+    // retransmitted.
+    const MaxRetransmits max_retransmissions_;
+    // At this exact millisecond, the item is considered expired. If the message
+    // is not to be expired, this is set to the infinite future.
+
     // Indicates the life cycle status of this chunk.
     Lifecycle lifecycle_ = Lifecycle::kActive;
     // Indicates the presence of this chunk, if it's in flight (Unacked), has
@@ -236,17 +254,11 @@ class OutstandingData {
     uint8_t nack_count_ = 0;
     // The number of times the DATA chunk has been retransmitted.
     uint16_t num_retransmissions_ = 0;
-    // If the message was sent with a maximum number of retransmissions, this is
-    // set to that number. The value zero (0) means that it will never be
-    // retransmitted.
-    const MaxRetransmits max_retransmissions_;
-    // When the packet was sent, and placed in this queue.
-    const TimeMs time_sent_;
-    // At this exact millisecond, the item is considered expired. If the message
-    // is not to be expired, this is set to the infinite future.
+
     const TimeMs expires_at_;
+
     // The actual data to send/retransmit.
-    Data data_;
+    const Data data_;
   };
 
   // Returns how large a chunk will be, serialized, carrying the data
diff --git a/net/dcsctp/tx/outstanding_data_test.cc b/net/dcsctp/tx/outstanding_data_test.cc
index 113f5f44e4..70f1f27eb0 100644
--- a/net/dcsctp/tx/outstanding_data_test.cc
+++ b/net/dcsctp/tx/outstanding_data_test.cc
@@ -64,10 +64,8 @@ TEST_F(OutstandingDataTest, HasInitialState) {
 }
 
 TEST_F(OutstandingDataTest, InsertChunk) {
-  ASSERT_HAS_VALUE_AND_ASSIGN(
-      UnwrappedTSN tsn,
-      buf_.Insert(gen_.Ordered({1}, "BE"), MaxRetransmits::NoLimit(), kNow,
-                  TimeMs::InfiniteFuture()));
+  ASSERT_HAS_VALUE_AND_ASSIGN(UnwrappedTSN tsn,
+                              buf_.Insert(gen_.Ordered({1}, "BE"), kNow));
 
   EXPECT_EQ(tsn.Wrap(), TSN(10));
 
@@ -83,8 +81,7 @@ TEST_F(OutstandingDataTest, InsertChunk) {
 }
 
 TEST_F(OutstandingDataTest, AcksSingleChunk) {
-  buf_.Insert(gen_.Ordered({1}, "BE"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "BE"), kNow);
   OutstandingData::AckInfo ack =
       buf_.HandleSack(unwrapper_.Unwrap(TSN(10)), {}, false);
 
@@ -103,8 +100,7 @@ TEST_F(OutstandingDataTest, AcksSingleChunk) {
 }
 
 TEST_F(OutstandingDataTest, AcksPreviousChunkDoesntUpdate) {
-  buf_.Insert(gen_.Ordered({1}, "BE"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "BE"), kNow);
   buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), {}, false);
 
   EXPECT_EQ(buf_.outstanding_bytes(), DataChunk::kHeaderSize + RoundUpTo4(1));
@@ -119,10 +115,8 @@ TEST_F(OutstandingDataTest, AcksPreviousChunkDoesntUpdate) {
 }
 
 TEST_F(OutstandingDataTest, AcksAndNacksWithGapAckBlocks) {
-  buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow);
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow);
 
   std::vector<SackChunk::GapAckBlock> gab = {SackChunk::GapAckBlock(2, 2)};
   OutstandingData::AckInfo ack =
@@ -144,10 +138,8 @@ TEST_F(OutstandingDataTest, AcksAndNacksWithGapAckBlocks) {
 }
 
 TEST_F(OutstandingDataTest, NacksThreeTimesWithSameTsnDoesntRetransmit) {
-  buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow);
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow);
 
   std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(2, 2)};
   EXPECT_FALSE(
@@ -169,14 +161,10 @@ TEST_F(OutstandingDataTest, NacksThreeTimesWithSameTsnDoesntRetransmit) {
 }
 
 TEST_F(OutstandingDataTest, NacksThreeTimesResultsInRetransmission) {
-  buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow);
 
   std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(2, 2)};
   EXPECT_FALSE(
@@ -211,14 +199,10 @@ TEST_F(OutstandingDataTest, NacksThreeTimesResultsInRetransmission) {
 
 TEST_F(OutstandingDataTest, NacksThreeTimesResultsInAbandoning) {
   static constexpr MaxRetransmits kMaxRetransmissions(0);
-  buf_.Insert(gen_.Ordered({1}, "B"), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "E"), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow, kMaxRetransmissions);
 
   std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(2, 2)};
   EXPECT_FALSE(
@@ -251,14 +235,10 @@ TEST_F(OutstandingDataTest, NacksThreeTimesResultsInAbandoning) {
 
 TEST_F(OutstandingDataTest, NacksThreeTimesResultsInAbandoningWithPlaceholder) {
   static constexpr MaxRetransmits kMaxRetransmissions(0);
-  buf_.Insert(gen_.Ordered({1}, "B"), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, kMaxRetransmissions);
 
   std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(2, 2)};
   EXPECT_FALSE(
@@ -292,17 +272,17 @@ TEST_F(OutstandingDataTest, NacksThreeTimesResultsInAbandoningWithPlaceholder) {
 
 TEST_F(OutstandingDataTest, ExpiresChunkBeforeItIsInserted) {
   static constexpr TimeMs kExpiresAt = kNow + DurationMs(1);
-  EXPECT_TRUE(buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(),
-                          kNow, kExpiresAt)
+  EXPECT_TRUE(buf_.Insert(gen_.Ordered({1}, "B"), kNow,
+                          MaxRetransmits::NoLimit(), kExpiresAt)
                   .has_value());
-  EXPECT_TRUE(buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(),
-                          kNow + DurationMs(0), kExpiresAt)
+  EXPECT_TRUE(buf_.Insert(gen_.Ordered({1}, ""), kNow + DurationMs(0),
+                          MaxRetransmits::NoLimit(), kExpiresAt)
                   .has_value());
 
   EXPECT_CALL(on_discard_, Call(IsUnordered(false), StreamID(1), MID(42)))
       .WillOnce(Return(false));
-  EXPECT_FALSE(buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(),
-                           kNow + DurationMs(1), kExpiresAt)
+  EXPECT_FALSE(buf_.Insert(gen_.Ordered({1}, "E"), kNow + DurationMs(1),
+                           MaxRetransmits::NoLimit(), kExpiresAt)
                    .has_value());
 
   EXPECT_FALSE(buf_.has_data_to_be_retransmitted());
@@ -318,12 +298,9 @@ TEST_F(OutstandingDataTest, ExpiresChunkBeforeItIsInserted) {
 
 TEST_F(OutstandingDataTest, CanGenerateForwardTsn) {
   static constexpr MaxRetransmits kMaxRetransmissions(0);
-  buf_.Insert(gen_.Ordered({1}, "B"), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "E"), kMaxRetransmissions, kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, kMaxRetransmissions);
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow, kMaxRetransmissions);
 
   EXPECT_CALL(on_discard_, Call(IsUnordered(false), StreamID(1), MID(42)))
       .WillOnce(Return(false));
@@ -342,22 +319,14 @@ TEST_F(OutstandingDataTest, CanGenerateForwardTsn) {
 }
 
 TEST_F(OutstandingDataTest, AckWithGapBlocksFromRFC4960Section334) {
-  buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow);
 
   EXPECT_THAT(buf_.GetChunkStatesForTesting(),
               testing::ElementsAre(Pair(TSN(9), State::kAcked),      //
@@ -384,12 +353,9 @@ TEST_F(OutstandingDataTest, AckWithGapBlocksFromRFC4960Section334) {
 }
 
 TEST_F(OutstandingDataTest, MeasureRTT) {
-  buf_.Insert(gen_.Ordered({1}, "BE"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "BE"), MaxRetransmits::NoLimit(),
-              kNow + DurationMs(1), TimeMs::InfiniteFuture());
-  buf_.Insert(gen_.Ordered({1}, "BE"), MaxRetransmits::NoLimit(),
-              kNow + DurationMs(2), TimeMs::InfiniteFuture());
+  buf_.Insert(gen_.Ordered({1}, "BE"), kNow);
+  buf_.Insert(gen_.Ordered({1}, "BE"), kNow + DurationMs(1));
+  buf_.Insert(gen_.Ordered({1}, "BE"), kNow + DurationMs(2));
 
   static constexpr DurationMs kDuration(123);
   ASSERT_HAS_VALUE_AND_ASSIGN(
@@ -406,8 +372,10 @@ TEST_F(OutstandingDataTest, MustRetransmitBeforeGettingNackedAgain) {
 
   static constexpr MaxRetransmits kOneRetransmission(1);
   for (int tsn = 10; tsn <= 20; ++tsn) {
-    buf_.Insert(gen_.Ordered({1}, tsn == 10 ? "B" : tsn == 20 ? "E" : ""),
-                kOneRetransmission, kNow, TimeMs::InfiniteFuture());
+    buf_.Insert(gen_.Ordered({1}, tsn == 10   ? "B"
+                                  : tsn == 20 ? "E"
+                                              : ""),
+                kNow, kOneRetransmission);
   }
 
   std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(2, 2)};
@@ -484,16 +452,11 @@ TEST_F(OutstandingDataTest, CanAbandonChunksMarkedForFastRetransmit) {
   // 11 are NACKed three times, chunk 10 will be marked for retransmission, but
   // chunk 11 will be abandoned, which also abandons chunk 10, as it's part of
   // the same message.
-  buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());  // 10
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits(0), kNow,
-              TimeMs::InfiniteFuture());  // 11
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());  // 12
-  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());  // 13
-  buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(), kNow,
-              TimeMs::InfiniteFuture());  // 13
+  buf_.Insert(gen_.Ordered({1}, "B"), kNow);                    // 10
+  buf_.Insert(gen_.Ordered({1}, ""), kNow, MaxRetransmits(0));  // 11
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);                     // 12
+  buf_.Insert(gen_.Ordered({1}, ""), kNow);                     // 13
+  buf_.Insert(gen_.Ordered({1}, "E"), kNow);                    // 14
 
   // ACK 9, 12
   std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(3, 3)};
diff --git a/net/dcsctp/tx/retransmission_queue.cc b/net/dcsctp/tx/retransmission_queue.cc
index 0ca02b0b88..958879387e 100644
--- a/net/dcsctp/tx/retransmission_queue.cc
+++ b/net/dcsctp/tx/retransmission_queue.cc
@@ -464,10 +464,9 @@ std::vector<std::pair<TSN, Data>> RetransmissionQueue::GetChunksToSend(
     rwnd_ -= chunk_size;
 
     absl::optional<UnwrappedTSN> tsn = outstanding_data_.Insert(
-        chunk_opt->data,
+        chunk_opt->data, now,
         partial_reliability_ ? chunk_opt->max_retransmissions
                              : MaxRetransmits::NoLimit(),
-        now,
         partial_reliability_ ? chunk_opt->expires_at
                              : TimeMs::InfiniteFuture());