Optimize the peerset a bit (#8416)

* Only allocate slots for the relevant peer set

* Do a pre-check before calling has_free_outgoing_slot

* Oops, fix infinite loop

substrate/client/peerset/src/lib.rs

@@ -296,7 +296,10 @@ impl Peerset {
 			}
 		}
 
-		peerset.alloc_slots();
+		for set_index in 0..peerset.data.num_sets() {
+			peerset.alloc_slots(SetId(set_index));
+		}
+
 		(peerset, handle)
 	}
 
@@ -307,7 +310,7 @@ impl Peerset {
 		}
 
 		self.data.add_no_slot_node(set_id.0, peer_id);
-		self.alloc_slots();
+		self.alloc_slots(set_id);
 	}
 
 	fn on_remove_reserved_peer(&mut self, set_id: SetId, peer_id: PeerId) {
@@ -372,7 +375,7 @@ impl Peerset {
 			}
 
 		} else {
-			self.alloc_slots();
+			self.alloc_slots(set_id);
 		}
 	}
 
@@ -383,7 +386,7 @@ impl Peerset {
 	pub fn add_to_peers_set(&mut self, set_id: SetId, peer_id: PeerId) {
 		if let peersstate::Peer::Unknown(entry) = self.data.peer(set_id.0, &peer_id) {
 			entry.discover();
-			self.alloc_slots();
+			self.alloc_slots(set_id);
 		}
 	}
 
@@ -500,59 +503,68 @@ impl Peerset {
 		}
 	}
 
-	/// Try to fill available out slots with nodes.
-	fn alloc_slots(&mut self) {
+	/// Try to fill available out slots with nodes for the given set.
+	fn alloc_slots(&mut self, set_id: SetId) {
 		self.update_time();
 
 		// Try to connect to all the reserved nodes that we are not connected to.
-		for set_index in 0..self.data.num_sets() {
-			for reserved_node in &self.reserved_nodes[set_index].0 {
-				let entry = match self.data.peer(set_index, reserved_node) {
-					peersstate::Peer::Unknown(n) => n.discover(),
-					peersstate::Peer::NotConnected(n) => n,
-					peersstate::Peer::Connected(_) => continue,
-				};
+		for reserved_node in &self.reserved_nodes[set_id.0].0 {
+			let entry = match self.data.peer(set_id.0, reserved_node) {
+				peersstate::Peer::Unknown(n) => n.discover(),
+				peersstate::Peer::NotConnected(n) => n,
+				peersstate::Peer::Connected(_) => continue,
+			};
 
-				match entry.try_outgoing() {
-					Ok(conn) => self.message_queue.push_back(Message::Connect {
-						set_id: SetId(set_index),
-						peer_id: conn.into_peer_id()
-					}),
-					Err(_) => {
-						// An error is returned only if no slot is available. Reserved nodes are
-						// marked in the state machine with a flag saying "doesn't occupy a slot",
-						// and as such this should never happen.
-						debug_assert!(false);
-						log::error!(
-							target: "peerset",
-							"Not enough slots to connect to reserved node"
-						);
-					}
+			match entry.try_outgoing() {
+				Ok(conn) => self.message_queue.push_back(Message::Connect {
+					set_id,
+					peer_id: conn.into_peer_id()
+				}),
+				Err(_) => {
+					// An error is returned only if no slot is available. Reserved nodes are
+					// marked in the state machine with a flag saying "doesn't occupy a slot",
+					// and as such this should never happen.
+					debug_assert!(false);
+					log::error!(
+						target: "peerset",
+						"Not enough slots to connect to reserved node"
+					);
 				}
 			}
 		}
 
 		// Now, we try to connect to other nodes.
-		for set_index in 0..self.data.num_sets() {
-			// Nothing more to do if we're in reserved mode.
-			if self.reserved_nodes[set_index].1 {
-				continue;
+
+		// Nothing more to do if we're in reserved mode.
+		if self.reserved_nodes[set_id.0].1 {
+			return;
+		}
+
+		// Try to grab the next node to attempt to connect to.
+		// Since `highest_not_connected_peer` is rather expensive to call, check beforehand
+		// whether we have an available slot.
+		while self.data.has_free_outgoing_slot(set_id.0) {
+			let next = match self.data.highest_not_connected_peer(set_id.0) {
+				Some(n) => n,
+				None => break
+			};
+
+			// Don't connect to nodes with an abysmal reputation.
+			if next.reputation() < BANNED_THRESHOLD {
+				break;
 			}
 
-			// Try to grab the next node to attempt to connect to.
-			while let Some(next) = self.data.highest_not_connected_peer(set_index) {
-				// Don't connect to nodes with an abysmal reputation.
-				if next.reputation() < BANNED_THRESHOLD {
+			match next.try_outgoing() {
+				Ok(conn) => self.message_queue.push_back(Message::Connect {
+					set_id,
+					peer_id: conn.into_peer_id()
+				}),
+				Err(_) => {
+					// This branch can only be entered if there is no free slot, which is
+					// checked above.
+					debug_assert!(false);
 					break;
 				}
-
-				match next.try_outgoing() {
-					Ok(conn) => self.message_queue.push_back(Message::Connect {
-						set_id: SetId(set_index),
-						peer_id: conn.into_peer_id()
-					}),
-					Err(_) => break,	// No more slots available.
-				}
 			}
 		}
 	}
@@ -624,7 +636,7 @@ impl Peerset {
 			self.on_remove_from_peers_set(set_id, peer_id);
 		}
 
-		self.alloc_slots();
+		self.alloc_slots(set_id);
 	}
 
 	/// Reports an adjustment to the reputation of the given peer.

substrate/client/peerset/src/peersstate.rs

@@ -283,6 +283,11 @@ impl PeersState {
 		}
 	}
 
+	/// Returns `true` if there is a free outgoing slot available related to this set.
+	pub fn has_free_outgoing_slot(&self, set: usize) -> bool {
+		self.sets[set].num_out < self.sets[set].max_out
+	}
+
 	/// Add a node to the list of nodes that don't occupy slots.
 	///
 	/// Has no effect if the node was already in the group.
@@ -506,9 +511,7 @@ impl<'a> NotConnectedPeer<'a> {
 
 		// Note that it is possible for num_out to be strictly superior to the max, in case we were
 		// connected to reserved node then marked them as not reserved.
-		if self.state.sets[self.set].num_out >= self.state.sets[self.set].max_out
-			&& !is_no_slot_occupy
-		{
+		if !self.state.has_free_outgoing_slot(self.set) && !is_no_slot_occupy {
 			return Err(self);
 		}