Clean up LinkedSet (LRU) code.

* More canonical and efficient 'move to front'.
 * Don't use 'new' when value semantic is fine.
 * Simplify flow (remove One-off private method).
 * Remove dead code.

Bug: webrtc:9575
Change-Id: Ie6a3c4e3d5e2342e77e54fd59fffa05f6e5f9ebe
Reviewed-on: https://webrtc-review.googlesource.com/92802
Commit-Queue: Yves Gerey <yvesg@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24215}

modules/remote_bitrate_estimator/test/bwe.cc

@@ -186,16 +186,16 @@ float BweReceiver::RecentPacketLossRatio() {
   PacketNodeIt node_it = received_packets_.begin();  // Latest.
 
   // Lowest timestamp limit, oldest one that should be checked.
-  int64_t time_limit_ms = (*node_it)->arrival_time_ms - kPacketLossTimeWindowMs;
+  int64_t time_limit_ms = node_it->arrival_time_ms - kPacketLossTimeWindowMs;
   // Oldest and newest values found within the given time window.
-  int64_t oldest_seq_num = (*node_it)->unwrapped_sequence_number;
+  int64_t oldest_seq_num = node_it->unwrapped_sequence_number;
   int64_t newest_seq_num = oldest_seq_num;
 
   while (node_it != received_packets_.end()) {
-    if ((*node_it)->arrival_time_ms < time_limit_ms) {
+    if (node_it->arrival_time_ms < time_limit_ms) {
       break;
     }
-    int64_t seq_num = (*node_it)->unwrapped_sequence_number;
+    int64_t seq_num = node_it->unwrapped_sequence_number;
     newest_seq_num = std::max(newest_seq_num, seq_num);
     oldest_seq_num = std::min(oldest_seq_num, seq_num);
     ++node_it;
@@ -210,8 +210,6 @@ float BweReceiver::RecentPacketLossRatio() {
 LinkedSet::LinkedSet(int capacity) : capacity_(capacity) {}
 
 LinkedSet::~LinkedSet() {
-  while (!empty())
-    RemoveTail();
 }
 
 void LinkedSet::Insert(uint16_t sequence_number,
@@ -223,42 +221,28 @@ void LinkedSet::Insert(uint16_t sequence_number,
   // Handle duplicate unwrapped sequence number.
   if (it != map_.end()) {
     PacketNodeIt node_it = it->second;
-    PacketIdentifierNode* node = *node_it;
-    node->arrival_time_ms = arrival_time_ms;
-    if (node_it != list_.begin()) {
-      list_.erase(node_it);
-      list_.push_front(node);
-      map_[unwrapped_sequence_number] = list_.begin();
-    }
+    PacketIdentifierNode& node = *node_it;
+    node.arrival_time_ms = arrival_time_ms;
+    // Move to front, without invalidating iterators.
+    list_.splice(list_.begin(), list_, node_it);
   } else {
     if (size() == capacity_) {
       RemoveTail();
     }
-    UpdateHead(new PacketIdentifierNode(unwrapped_sequence_number, send_time_ms,
-                                        arrival_time_ms, payload_size));
+    // Push new element to the front of the list and insert it in the map.
+    PacketIdentifierNode new_head(unwrapped_sequence_number, send_time_ms,
+                                  arrival_time_ms, payload_size);
+    list_.push_front(new_head);
+    map_[unwrapped_sequence_number] = list_.begin();
   }
 }
 
-void LinkedSet::Insert(PacketIdentifierNode packet_identifier) {
-  Insert(packet_identifier.unwrapped_sequence_number,
-         packet_identifier.send_time_ms, packet_identifier.arrival_time_ms,
-         packet_identifier.payload_size);
-}
-
 void LinkedSet::RemoveTail() {
-  map_.erase(list_.back()->unwrapped_sequence_number);
-  delete list_.back();
-  list_.pop_back();
-}
-
-void LinkedSet::UpdateHead(PacketIdentifierNode* new_head) {
-  list_.push_front(new_head);
-  map_[new_head->unwrapped_sequence_number] = list_.begin();
+  Erase(--list_.end());
 }
 
 void LinkedSet::Erase(PacketNodeIt node_it) {
-  map_.erase((*node_it)->unwrapped_sequence_number);
-  delete (*node_it);
+  map_.erase(node_it->unwrapped_sequence_number);
   list_.erase(node_it);
 }
 

modules/remote_bitrate_estimator/test/bwe.h

@@ -57,7 +57,7 @@ struct PacketIdentifierNode {
   size_t payload_size;
 };
 
-typedef std::list<PacketIdentifierNode*>::iterator PacketNodeIt;
+typedef std::list<PacketIdentifierNode>::iterator PacketNodeIt;
 
 // FIFO implementation for a limited capacity set.
 // Used for keeping the latest arrived packets while avoiding duplicates.
@@ -68,16 +68,14 @@ class LinkedSet {
   ~LinkedSet();
 
   // If the arriving packet (identified by its sequence number) is already
-  // in the LinkedSet, move its Node to the head of the list. Else, create
-  // a PacketIdentifierNode n_ and then UpdateHead(n_), calling RemoveTail()
-  // if the LinkedSet reached its maximum capacity.
+  // in the LinkedSet, move its Node to the head of the list.
+  // Else, add a PacketIdentifierNode n_ at the head of the list,
+  // calling RemoveTail() if the LinkedSet reached its maximum capacity.
   void Insert(uint16_t sequence_number,
               int64_t send_time_ms,
               int64_t arrival_time_ms,
               size_t payload_size);
 
-  void Insert(PacketIdentifierNode packet_identifier);
-
   PacketNodeIt begin() { return list_.begin(); }
   PacketNodeIt end() { return list_.end(); }
 
@@ -96,14 +94,12 @@ class LinkedSet {
  private:
   // Pop oldest element from the back of the list and remove it from the map.
   void RemoveTail();
-  // Add new element to the front of the list and insert it in the map.
-  void UpdateHead(PacketIdentifierNode* new_head);
   size_t capacity_;
   // We want to keep track of the current oldest and newest sequence_numbers.
   // To get strict weak ordering, we unwrap uint16_t into an int64_t.
   SeqNumUnwrapper<uint16_t> unwrapper_;
   std::map<int64_t, PacketNodeIt> map_;
-  std::list<PacketIdentifierNode*> list_;
+  std::list<PacketIdentifierNode> list_;
 };
 
 const int kMinBitrateKbps = 10;

modules/remote_bitrate_estimator/test/estimators/bbr.cc

@@ -567,9 +567,9 @@ FeedbackPacket* BbrBweReceiver::GetFeedback(int64_t now_ms) {
   last_feedback_ms_ = now_ms;
   int64_t corrected_send_time_ms = 0L;
   if (!received_packets_.empty()) {
-    PacketIdentifierNode* latest = *(received_packets_.begin());
+    PacketIdentifierNode& latest = *(received_packets_.begin());
     corrected_send_time_ms =
-        latest->send_time_ms + now_ms - latest->arrival_time_ms;
+        latest.send_time_ms + now_ms - latest.arrival_time_ms;
   }
   FeedbackPacket* fb = new BbrBweFeedback(
       flow_id_, now_ms * 1000, corrected_send_time_ms, packet_feedbacks_);

modules/remote_bitrate_estimator/test/estimators/nada.cc

@@ -117,9 +117,9 @@ FeedbackPacket* NadaBweReceiver::GetFeedback(int64_t now_ms) {
   int64_t corrected_send_time_ms = 0L;
 
   if (!received_packets_.empty()) {
-    PacketIdentifierNode* latest = *(received_packets_.begin());
+    PacketIdentifierNode& latest = *(received_packets_.begin());
     corrected_send_time_ms =
-        latest->send_time_ms + now_ms - latest->arrival_time_ms;
+        latest.send_time_ms + now_ms - latest.arrival_time_ms;
   }
 
   // Sends a tuple containing latest values of <d_hat_n, d_tilde_n, x_n, x'_n,