Ping the premier connection on each network with higher priority.

webrtc/p2p/base/p2ptransportchannel.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 48 matching lines @@
 }  // unnamed namespace
 
 namespace cricket {
 
 // When the socket is unwritable, we will use 10 Kbps (ignoring IP+UDP headers)
 // for pinging.  When the socket is writable, we will use only 1 Kbps because
 // we don't want to degrade the quality on a modem.  These numbers should work
 // well on a 28.8K modem, which is the slowest connection on which the voice
 // quality is reasonable at all.
 static const int PING_PACKET_SIZE = 60 * 8;
+
+// The next two ping intervals are at the channel level.
 // STRONG_PING_INTERVAL (480ms) is applied when the selected connection is both
 // writable and receiving.
 static const int STRONG_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 1000;
 // WEAK_PING_INTERVAL (48ms) is applied when the selected connection is either
 // not writable or not receiving.
 const int WEAK_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 10000;
 
-// Writable connections are pinged at a faster rate while stabilizing.
-const int STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL = 900;  // ms
-
-// Writable connections are pinged at a slower rate once stabilized.
-const int STABLE_WRITABLE_CONNECTION_PING_INTERVAL = 2500;  // ms
+// The next two ping intervals are at the connection level.
+// Writable connections are pinged at a faster rate while the connections are
+// stabilizing or the channel is weak.
+const int WEAK_OR_STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL = 900;  // ms
+// Writable connections are pinged at a slower rate once they are stabilized and
+// the channel is strongly connected.
+const int STRONG_AND_STABLE_WRITABLE_CONNECTION_PING_INTERVAL = 2500;  // ms
 
 static const int MIN_CHECK_RECEIVING_INTERVAL = 50;  // ms
 
 static const int RECEIVING_SWITCHING_DELAY = 1000;  // ms
 
 // We periodically check if any existing networks do not have any connection
 // and regather on those networks.
 static const int DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL = 5 * 60 * 1000;
 
 static constexpr int DEFAULT_BACKUP_CONNECTION_PING_INTERVAL = 25 * 1000;
@@ skipping 18 matching lines @@
       sort_dirty_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_interval_(MIN_CHECK_RECEIVING_INTERVAL * 5),
       config_(MIN_CHECK_RECEIVING_INTERVAL * 50 /* receiving_timeout */,
               DEFAULT_BACKUP_CONNECTION_PING_INTERVAL,
               GATHER_ONCE /* continual_gathering_policy */,
               false /* prioritize_most_likely_candidate_pairs */,
-              STABLE_WRITABLE_CONNECTION_PING_INTERVAL,
+              STRONG_AND_STABLE_WRITABLE_CONNECTION_PING_INTERVAL,
               true /* presume_writable_when_fully_relayed */,
               DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL,
               RECEIVING_SWITCHING_DELAY) {
   uint32_t weak_ping_interval = ::strtoul(
       webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
       nullptr, 10);
   if (weak_ping_interval) {
     weak_ping_interval_ = static_cast<int>(weak_ping_interval);
   }
 }
@@ skipping 1168 matching lines @@
   UpdateState();
 
   // Also possibly start pinging.
   // We could start pinging if:
   // * The first connection was created.
   // * ICE credentials were provided.
   // * A TCP connection became connected.
   MaybeStartPinging();
 }
 
+std::map<rtc::Network*, Connection*>
+P2PTransportChannel::GetBestConnectionsByNetwork() const {
+  // |connections_| has been sorted, so the first one in the list on a given
+  // network is the best connection on the network, except that the selected
+  // connection is always the best connection on the network.
+  std::map<rtc::Network*, Connection*> best_connections_by_network;
+  if (selected_connection_) {
+    best_connections_by_network[selected_connection_->port()->Network()] =
+        selected_connection_;
+  }
+  for (Connection* conn : connections_) {
+    rtc::Network* network = conn->port()->Network();
+    // This only inserts when the network does not exist in the map.
+    best_connections_by_network.insert(std::make_pair(network, conn));
+  }
+  return best_connections_by_network;

[pthatcher1, 2016/10/25 17:00:06]

best_connection_by_network (makes it clear there is one per network).

[honghaiz3, 2016/10/25 18:01:09]

Done.

+}
+
 void P2PTransportChannel::PruneConnections() {
   // We can prune any connection for which there is a connected, writable
   // connection on the same network with better or equal priority.  We leave
   // those with better priority just in case they become writable later (at
   // which point, we would prune out the current selected connection).  We leave
   // connections on other networks because they may not be using the same
   // resources and they may represent very distinct paths over which we can
-  // switch. If the |premier| connection is not connected, we may be
-  // reconnecting a TCP connection and temporarily do not prune connections in
-  // this network. See the big comment in CompareConnectionStates.
-
-  // Get a list of the networks that we are using.
-  std::set<rtc::Network*> networks;
-  for (const Connection* conn : connections_) {
-    networks.insert(conn->port()->Network());
-  }
-  for (rtc::Network* network : networks) {
-    Connection* premier = GetBestConnectionOnNetwork(network);
-    // Do not prune connections if the current selected connection is weak on
-    // this network. Otherwise, it may delete connections prematurely.
-    if (!premier || premier->weak()) {
-      continue;
-    }
-
-    for (Connection* conn : connections_) {
-      if ((conn != premier) && (conn->port()->Network() == network) &&
-          (CompareConnectionCandidates(premier, conn) >= 0)) {
-        conn->Prune();
-      }
+  // switch. If |best_conn_on_network| is not connected, we may be reconnecting
+  // a TCP connection and should not prune connections in this network.
+  // See the big comment in CompareConnectionStates.
+  auto best_connections_by_network = GetBestConnectionsByNetwork();

[pthatcher1, 2016/10/25 17:00:06]

here too

[honghaiz3, 2016/10/25 18:01:09]

Done.

+  for (Connection* conn : connections_) {
+    // Do not prune connections if the current best connection on this network
+    // is weak. Otherwise, it may delete connections prematurely.
+    Connection* best_conn_on_network =
+        best_connections_by_network[conn->port()->Network()];
+    if (best_conn_on_network && conn != best_conn_on_network &&
+        !best_conn_on_network->weak() &&
+        CompareConnectionCandidates(best_conn_on_network, conn) >= 0) {
+      conn->Prune();
     }
   }
 }
 
 // Change the selected connection, and let listeners know.
 void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) {
   // Note: if conn is NULL, the previous |selected_connection_| has been
   // destroyed, so don't use it.
   Connection* old_selected_connection = selected_connection_;
   selected_connection_ = conn;
@@ skipping 117 matching lines @@
 bool P2PTransportChannel::ReadyToSend(Connection* connection) const {
   // Note that we allow sending on an unreliable connection, because it's
   // possible that it became unreliable simply due to bad chance.
   // So this shouldn't prevent attempting to send media.
   return connection != nullptr &&
          (connection->writable() ||
           connection->write_state() == Connection::STATE_WRITE_UNRELIABLE ||
           PresumedWritable(connection));
 }
 
-// If we have a selected connection, return it, otherwise return top one in the
-// list (later we will mark it best).
-Connection* P2PTransportChannel::GetBestConnectionOnNetwork(
-    rtc::Network* network) const {
-  // If the selected connection is on this network, then it wins.
-  if (selected_connection_ &&
-      (selected_connection_->port()->Network() == network)) {
-    return selected_connection_;
-  }
-
-  // Otherwise, we return the top-most in sorted order.
-  for (size_t i = 0; i < connections_.size(); ++i) {
-    if (connections_[i]->port()->Network() == network) {
-      return connections_[i];
-    }
-  }
-
-  return NULL;
-}
-
 // Handle any queued up requests
 void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
   switch (pmsg->message_id) {
     case MSG_SORT_AND_UPDATE_STATE:
       SortConnectionsAndUpdateState();
       break;
     case MSG_CHECK_AND_PING:
       OnCheckAndPing();
       break;
     case MSG_REGATHER_ON_FAILED_NETWORKS:
@@ skipping 85 matching lines @@
   if (!conn->writable()) {
     return true;
   }
 
   // Ping writable, active connections if it's been long enough since the last
   // ping.
   int ping_interval = CalculateActiveWritablePingInterval(conn, now);
   return (now >= conn->last_ping_sent() + ping_interval);
 }
 
-bool P2PTransportChannel::IsSelectedConnectionPingable(int64_t now) {
-  if (!selected_connection_ || !selected_connection_->connected() ||
-      !selected_connection_->writable()) {
+bool P2PTransportChannel::IsPerNetworkBestConnectionPingable(
+    Connection* per_network_best_connection,
+    int64_t now) {
+  if (!per_network_best_connection ||
+      !per_network_best_connection->connected() ||
+      !per_network_best_connection->writable()) {
     return false;
   }
 
-  int interval = CalculateActiveWritablePingInterval(selected_connection_, now);
-  return selected_connection_->last_ping_sent() + interval <= now;
+  int interval =
+      CalculateActiveWritablePingInterval(per_network_best_connection, now);
+  return per_network_best_connection->last_ping_sent() + interval <= now;

[pthatcher1, 2016/10/25 17:00:06]

I think you could just call this variable "connection".

[honghaiz3, 2016/10/25 18:01:09]

Done.

 }
 
 int P2PTransportChannel::CalculateActiveWritablePingInterval(
     const Connection* conn,
     int64_t now) const {
   // Ping each connection at a higher rate at least
   // MIN_PINGS_AT_WEAK_PING_INTERVAL times.
   if (conn->num_pings_sent() < MIN_PINGS_AT_WEAK_PING_INTERVAL) {
     return weak_ping_interval_;
   }
 
   int stable_interval = config_.stable_writable_connection_ping_interval;
-  int stablizing_interval =
-      std::min(stable_interval, STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL);
-
-  return conn->stable(now) ? stable_interval : stablizing_interval;
+  int weak_or_stablizing_interval = std::min(
+      stable_interval, WEAK_OR_STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL);
+  // If the channel is weak or the connection is not stable yet, use the
+  // weak_or_stablizing_interval.
+  return (!weak() && conn->stable(now)) ? stable_interval
+                                        : weak_or_stablizing_interval;
 }
 
 // Returns the next pingable connection to ping.  This will be the oldest
 // pingable connection unless we have a connected, writable connection that is
 // past the writable ping interval. When reconnecting a TCP
 // connection, the selected connection is disconnected, although still WRITABLE
 // while reconnecting. The newly created connection should be selected as the
 // ping target to become writable instead. See the big comment in
 // CompareConnectionStates.
+// TODO(honghaiz): Re-thinking the way of prioritizing connections for ping
+// requests.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   int64_t now = rtc::TimeMillis();
-  Connection* conn_to_ping = nullptr;
-  if (IsSelectedConnectionPingable(now)) {
-    conn_to_ping = selected_connection_;
-  } else {
-    conn_to_ping = FindConnectionToPing(now);
+
+  // If the selected connection is pingable, select it to ping.
+  if (IsPerNetworkBestConnectionPingable(selected_connection_, now)) {
+    return selected_connection_;
   }
-  return conn_to_ping;
+
+  // If the selected connection is strongly connected, don't bother pinging

[pthatcher1, 2016/10/25 17:00:06]

Now that you have this as if (weak()), you should probably make the comment say
"if not strongly connected, ping the best connection of each network at a higher
priority".  Either that or use an early return.

[honghaiz3, 2016/10/25 18:01:09]

Done.

+  // best connections on other networks at a higher priority. This prevents
+  // a few best connections from saturating all transmission slots for ping
+  // and also prevents the backup connections from being pinged frequently.
+  // Otherwise, find the per-network best connection that has not been pinged
+  // for the longest time.
+  if (weak()) {
+    auto best_connections_by_network = GetBestConnectionsByNetwork();
+    Connection* oldest_pingable_best_connection = nullptr;
+    for (auto kv : best_connections_by_network) {
+      Connection* conn = kv.second;
+      if (conn == nullptr || conn == selected_connection_) {
+        continue;
+      }
+      if (IsPerNetworkBestConnectionPingable(conn, now) &&
+          (!oldest_pingable_best_connection ||
+           conn->last_ping_sent() <
+               oldest_pingable_best_connection->last_ping_sent())) {
+        oldest_pingable_best_connection = conn;
+      }

[pthatcher1, 2016/10/25 17:00:06]

Can we do something like this?

auto oldest_best_connection_by_network =
std::min(best_connection_by_network.values(), 
  [](Connection* conn1, Connection* conn2) {
    // Push unpingable connections to the max end to make pingable connections
be at the min end.
    return !IsConnectionPingable(conn2) || conn1->last_ping_sent() <
conn2->last_ping_sent();
  });
if (IsConnectionPingable(oldest_best_connection_by_network)) {
  return oldest_best_connection_by_network;
}

[honghaiz3, 2016/10/25 18:01:09]

It will need to be something like this:
auto oldest_best_connection_by_network =
std::min(best_connection_by_network.values(), 
  [](Connection* conn1, Connection* conn2) {
    // Push unpingable connections to the max end to make pingable connections
be at the min end.
    return IsPerNetworkBestConnectionPingable(conn1) <
IsPerNetworkBestConnectionPingable(conn2) || conn1->last_ping_sent() <
conn2->last_ping_sent();
  });
if (IsPerNetworkBestConnectionPingable(oldest_best_connection_by_network)) {
  return oldest_best_connection_by_network;
}
I do not feel this is much more readable. It needs to check
IsPerNetworkBestConnectionPingable more times. Plus, The comparing logic could
easily get wrong. So I would prefer the current one in the code. It might be a
few more lines, but it is hard to get wrong. 

[pthatcher1, 2016/10/26 22:37:50]

You don't need to call IsPerNetworkBestConnectionPingable twice.  Once should be
enough.  We're just taking the min, but not sorting.  But you bring up a good
point that it would be more clear to do something like this:

auto oldest_pingable_best_connection =
std::min(std::filter(best_connection_by_network.values(),
ShouldPingWithHigherPriority), [](Connection* conn1, Connection* conn2) {
conn1->last_ping_sent() < conn2->last_ping_sent();}));

I don't know if there is such a std::filter, though.  If there is, I think that
would be more readable.

But maybe it doesn't matter, because if we cleanup GetNextPingableConnection
from the top, we might not need this after all.

[honghaiz3, 2016/10/27 19:20:58]

We can do that. Will defer this until the other, main comment is resolved. 

[honghaiz3, 2016/10/27 21:45:09]

copy_if is the c++ filter but unfortunately, c++ map does not have the value
method to get all values in this map (i.e., connections). 
I do not feel it would be worth iterating the map to find the list of
connections, apply filter and then apply std::min to get the minimum. 

+    }
+    if (oldest_pingable_best_connection) {
+      return oldest_pingable_best_connection;
+    }
+  }
+
+  return FindConnectionToPing(now);
 }
 
 void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
   if (conn && pinged_connections_.insert(conn).second) {
     unpinged_connections_.erase(conn);
   }
 }
 
 // Apart from sending ping from |conn| this method also updates
 // |use_candidate_attr| and |nomination| flags. One of the flags is set to
@@ skipping 91 matching lines @@
   // use it.
 
   // Remove this connection from the list.
   std::vector<Connection*>::iterator iter =
       std::find(connections_.begin(), connections_.end(), connection);
   ASSERT(iter != connections_.end());
   pinged_connections_.erase(*iter);
   unpinged_connections_.erase(*iter);
   connections_.erase(iter);
 
-  LOG_J(LS_INFO, this) << "Removed connection ("
-    << static_cast<int>(connections_.size()) << " remaining)";
+  LOG_J(LS_INFO, this) << "Removed connection " << std::hex << connection
+                       << std::dec << " (" << connections_.size()
+                       << " remaining)";
 
   // If this is currently the selected connection, then we need to pick a new
   // one. The call to SortConnectionsAndUpdateState will pick a new one. It
   // looks at the current selected connection in order to avoid switching
   // between fairly similar ones. Since this connection is no longer an option,
   // we can just set selected to nullptr and re-choose a best assuming that
   // there was no selected connection.
   if (selected_connection_ == connection) {
     LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one.";
     SwitchSelectedConnection(nullptr);
@@ skipping 107 matching lines @@
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == selected_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 // Find "triggered checks".  We ping first those connections that have
 // received a ping but have not sent a ping since receiving it
-// (last_received_ping > last_sent_ping).  But we shouldn't do
+// (last_ping_received > last_ping_sent).  But we shouldn't do
 // triggered checks if the connection is already writable.
 Connection* P2PTransportChannel::FindOldestConnectionNeedingTriggeredCheck(
     int64_t now) {
   Connection* oldest_needing_triggered_check = nullptr;
   for (auto conn : connections_) {
     if (!IsPingable(conn, now)) {
       continue;
     }
     bool needs_triggered_check =
         (!conn->writable() &&
@@ skipping 95 matching lines @@
 
   // During the initial state when nothing has been pinged yet, return the first
   // one in the ordered |connections_|.
   return *(std::find_if(connections_.begin(), connections_.end(),
                         [conn1, conn2](Connection* conn) {
                           return conn == conn1 || conn == conn2;
                         }));
 }
 
 }  // namespace cricket