Avoid string initialization in network metrics and replace by &str where possible (#1898)

## Issue Addressed

NA

## Proposed Changes

Removes most of the temporary string initializations in network metrics and replaces them by directly using `&str`. This further improves on PR https://github.com/sigp/lighthouse/pull/1895.

For the subnet id handling the current approach uses a build script to create a static map. This has the disadvantage that the build script hardcodes the number of subnets. If we want to use more than 64 subnets we need to adjust this in the build script.

## Additional Info

We still have some string initializations for the enum `PeerKind`. To also replace that by `&str` I created a PR in the libp2p dependency: https://github.com/sigp/rust-libp2p/pull/91. Either we wait with merging until this dependency PR is merged (and all conflicts with the newest libp2p version are resolved) or we just merge as is and I will create another PR when the dependency is ready.

Cargo.lock

@@ -2999,7 +2999,7 @@ checksum = "c7d73b3f436185384286bd8098d17ec07c9a7d2388a6599f824d8502b529702a"
 [[package]]
 name = "libp2p"
 version = "0.30.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "atomic",
  "bytes 0.5.6",
@@ -3061,7 +3061,7 @@ dependencies = [
 [[package]]
 name = "libp2p-core"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "asn1_der",
  "bs58 0.3.1",
@@ -3094,7 +3094,7 @@ dependencies = [
 [[package]]
 name = "libp2p-core-derive"
 version = "0.20.2"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "quote",
  "syn",
@@ -3103,7 +3103,7 @@ dependencies = [
 [[package]]
 name = "libp2p-dns"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "futures 0.3.8",
  "libp2p-core 0.24.0",
@@ -3113,7 +3113,7 @@ dependencies = [
 [[package]]
 name = "libp2p-gossipsub"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "base64 0.13.0",
  "byteorder",
@@ -3137,7 +3137,7 @@ dependencies = [
 [[package]]
 name = "libp2p-identify"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "futures 0.3.8",
  "libp2p-core 0.24.0",
@@ -3152,7 +3152,7 @@ dependencies = [
 [[package]]
 name = "libp2p-mplex"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "bytes 0.5.6",
  "futures 0.3.8",
@@ -3169,7 +3169,7 @@ dependencies = [
 [[package]]
 name = "libp2p-noise"
 version = "0.26.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "bytes 0.5.6",
  "curve25519-dalek",
@@ -3190,7 +3190,7 @@ dependencies = [
 [[package]]
 name = "libp2p-swarm"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "either",
  "futures 0.3.8",
@@ -3205,7 +3205,7 @@ dependencies = [
 [[package]]
 name = "libp2p-tcp"
 version = "0.24.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "futures 0.3.8",
  "futures-timer",
@@ -3220,7 +3220,7 @@ dependencies = [
 [[package]]
 name = "libp2p-websocket"
 version = "0.25.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "async-tls",
  "either",
@@ -3239,7 +3239,7 @@ dependencies = [
 [[package]]
 name = "libp2p-yamux"
 version = "0.27.0"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "futures 0.3.8",
  "libp2p-core 0.24.0",
@@ -3658,7 +3658,7 @@ dependencies = [
 [[package]]
 name = "multistream-select"
 version = "0.8.4"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "bytes 0.5.6",
  "futures 0.3.8",
@@ -3971,7 +3971,7 @@ dependencies = [
 [[package]]
 name = "parity-multiaddr"
 version = "0.9.3"
-source = "git+https://github.com/sigp/rust-libp2p?rev=b6278e1ba7b6bcfad1eef300f72148705da5d8d2#b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+source = "git+https://github.com/sigp/rust-libp2p?rev=f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c#f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 dependencies = [
  "arrayref",
  "bs58 0.3.1",
@@ -6519,6 +6519,7 @@ dependencies = [
  "ethereum-types",
  "hex",
  "int_to_bytes",
+ "lazy_static",
  "log 0.4.11",
  "merkle_proof",
  "rand 0.7.3",

beacon_node/eth2_libp2p/Cargo.toml

@@ -42,7 +42,7 @@ regex = "1.3.9"
 [dependencies.libp2p]
 #version = "0.23.0"
 git = "https://github.com/sigp/rust-libp2p"
-rev = "b6278e1ba7b6bcfad1eef300f72148705da5d8d2"
+rev = "f53d02bc873fef2bf52cd31e3d5ce366a41d8a8c"
 default-features = false
 features = ["websocket", "identify", "mplex", "yamux", "noise", "gossipsub", "dns", "tcp-tokio"]
 

beacon_node/eth2_libp2p/src/peer_manager/client.rs

@@ -98,9 +98,29 @@ impl std::fmt::Display for Client {
     }
 }
 
+impl ClientKind {
+    pub fn as_static_ref(&self) -> &'static str {
+        use ClientKind::*;
+        match self {
+            Lighthouse => "Lighthouse",
+            Nimbus => "Nimbus",
+            Teku => "Teku",
+            Prysm => "Prysm",
+            Lodestar => "Lodestar",
+            Unknown => "Unknown",
+        }
+    }
+}
+
+impl AsRef<str> for ClientKind {
+    fn as_ref(&self) -> &str {
+        self.as_static_ref()
+    }
+}
+
 impl std::fmt::Display for ClientKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{:?}", self)
+        f.write_str(self.as_ref())
     }
 }
 

beacon_node/eth2_libp2p/src/types/topics.rs

@@ -52,15 +52,25 @@ pub enum GossipKind {
     AttesterSlashing,
 }
 
+impl AsRef<str> for GossipKind {
+    fn as_ref(&self) -> &str {
+        use GossipKind::*;
+        match self {
+            BeaconBlock => "beacon_block",
+            BeaconAggregateAndProof => "beacon_aggregate_and_proof",
+            Attestation(_) => "beacon_attestation",
+            VoluntaryExit => "voluntary_exit",
+            ProposerSlashing => "proposer_slashing",
+            AttesterSlashing => "attester_slashing",
+        }
+    }
+}
+
 impl std::fmt::Display for GossipKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            GossipKind::BeaconBlock => write!(f, "beacon_block"),
-            GossipKind::BeaconAggregateAndProof => write!(f, "beacon_aggregate_and_proof"),
             GossipKind::Attestation(subnet_id) => write!(f, "beacon_attestation_{}", **subnet_id),
-            GossipKind::VoluntaryExit => write!(f, "voluntary_exit"),
+            x => f.write_str(x.as_ref()),
-            GossipKind::ProposerSlashing => write!(f, "proposer_slashing"),
-            GossipKind::AttesterSlashing => write!(f, "attester_slashing"),
         }
     }
 }

beacon_node/network/src/service.rs

@@ -20,6 +20,7 @@ use std::{collections::HashMap, net::SocketAddr, sync::Arc, time::Duration};
 use store::HotColdDB;
 use tokio::sync::mpsc;
 use tokio::time::Delay;
+use types::subnet_id::subnet_id_to_string;
 use types::{EthSpec, RelativeEpoch, SubnetId, Unsigned, ValidatorSubscription};
 
 mod tests;
@@ -275,7 +276,6 @@ fn spawn_service<T: BeaconChainTypes>(
                     update_gossip_metrics::<T::EthSpec>(
                         &service.libp2p.swarm.gs(),
                         &service.network_globals,
-                        &service.log
                     );
                 }
                 _ = service.gossipsub_parameter_update.next() => {
@@ -588,7 +588,7 @@ fn expose_publish_metrics<T: EthSpec>(messages: &[PubsubMessage<T>]) {
             PubsubMessage::Attestation(subnet_id) => {
                 metrics::inc_counter_vec(
                     &metrics::ATTESTATIONS_PUBLISHED_PER_SUBNET_PER_SLOT,
-                    &[&subnet_id.0.to_string()],
+                    &[&subnet_id.0.as_ref()],
                 );
                 metrics::inc_counter(&metrics::GOSSIP_UNAGGREGATED_ATTESTATIONS_TX)
             }
@@ -614,25 +614,9 @@ fn expose_receive_metrics<T: EthSpec>(message: &PubsubMessage<T>) {
     }
 }
 
-/// A work-around to reduce temporary allocation when updating gossip metrics.
-pub struct ToStringCache<T>(HashMap<T, String>);
-
-impl<T: Clone + std::hash::Hash + std::fmt::Display + Eq> ToStringCache<T> {
-    pub fn with_capacity(c: usize) -> Self {
-        Self(HashMap::with_capacity(c))
-    }
-
-    pub fn get(&mut self, item: T) -> &str {
-        self.0
-            .entry(item.clone())
-            .or_insert_with(|| item.to_string())
-    }
-}
-
 fn update_gossip_metrics<T: EthSpec>(
     gossipsub: &Gossipsub,
     network_globals: &Arc<NetworkGlobals<T>>,
-    logger: &slog::Logger,
 ) {
     // Clear the metrics
     let _ = metrics::PEERS_PER_PROTOCOL
@@ -683,28 +667,23 @@ fn update_gossip_metrics<T: EthSpec>(
         .as_ref()
         .map(|gauge| gauge.reset());
 
-    let mut subnet_ids: ToStringCache<u64> =
-        ToStringCache::with_capacity(T::default_spec().attestation_subnet_count as usize);
-    let mut gossip_kinds: ToStringCache<GossipKind> =
-        ToStringCache::with_capacity(T::default_spec().attestation_subnet_count as usize);
-
     // reset the mesh peers, showing all subnets
     for subnet_id in 0..T::default_spec().attestation_subnet_count {
         let _ = metrics::get_int_gauge(
             &metrics::MESH_PEERS_PER_SUBNET_TOPIC,
-            &[subnet_ids.get(subnet_id)],
+            &[subnet_id_to_string(subnet_id)],
         )
         .map(|v| v.set(0));
 
         let _ = metrics::get_int_gauge(
             &metrics::GOSSIPSUB_SUBSCRIBED_SUBNET_TOPIC,
-            &[subnet_ids.get(subnet_id)],
+            &[subnet_id_to_string(subnet_id)],
         )
         .map(|v| v.set(0));
 
         let _ = metrics::get_int_gauge(
             &metrics::GOSSIPSUB_SUBSCRIBED_PEERS_SUBNET_TOPIC,
-            &[subnet_ids.get(subnet_id)],
+            &[subnet_id_to_string(subnet_id)],
         )
         .map(|v| v.set(0));
     }
@@ -715,7 +694,7 @@ fn update_gossip_metrics<T: EthSpec>(
             if let GossipKind::Attestation(subnet_id) = topic.kind() {
                 let _ = metrics::get_int_gauge(
                     &metrics::GOSSIPSUB_SUBSCRIBED_SUBNET_TOPIC,
-                    &[subnet_ids.get(subnet_id.into())],
+                    &[subnet_id_to_string(subnet_id.into())],
                 )
                 .map(|v| v.set(1));
             }
@@ -733,7 +712,7 @@ fn update_gossip_metrics<T: EthSpec>(
                     GossipKind::Attestation(subnet_id) => {
                         if let Some(v) = metrics::get_int_gauge(
                             &metrics::GOSSIPSUB_SUBSCRIBED_PEERS_SUBNET_TOPIC,
-                            &[subnet_ids.get(subnet_id.into())],
+                            &[subnet_id_to_string(subnet_id.into())],
                         ) {
                             v.inc()
                         };
@@ -742,7 +721,7 @@ fn update_gossip_metrics<T: EthSpec>(
                         if let Some(score) = gossipsub.peer_score(peer_id) {
                             if let Some(v) = metrics::get_gauge(
                                 &metrics::AVG_GOSSIPSUB_PEER_SCORE_PER_SUBNET_TOPIC,
-                                &[subnet_ids.get(subnet_id.into())],
+                                &[subnet_id_to_string(subnet_id.into())],
                             ) {
                                 v.add(score)
                             };
@@ -753,7 +732,7 @@ fn update_gossip_metrics<T: EthSpec>(
                         if let Some(score) = gossipsub.peer_score(peer_id) {
                             if let Some(v) = metrics::get_gauge(
                                 &metrics::AVG_GOSSIPSUB_PEER_SCORE_PER_MAIN_TOPIC,
-                                &[gossip_kinds.get(kind.clone())],
+                                &[kind.as_ref()],
                             ) {
                                 v.add(score)
                             };
@@ -771,7 +750,7 @@ fn update_gossip_metrics<T: EthSpec>(
                     // average peer scores
                     if let Some(v) = metrics::get_gauge(
                         &metrics::AVG_GOSSIPSUB_PEER_SCORE_PER_SUBNET_TOPIC,
-                        &[subnet_ids.get(subnet_id.into())],
+                        &[subnet_id_to_string(subnet_id.into())],
                     ) {
                         v.set(v.get() / (*peers as f64))
                     };
@@ -780,7 +759,7 @@ fn update_gossip_metrics<T: EthSpec>(
                     // main topics
                     if let Some(v) = metrics::get_gauge(
                         &metrics::AVG_GOSSIPSUB_PEER_SCORE_PER_MAIN_TOPIC,
-                        &[&format!("{:?}", kind)],
+                        &[kind.as_ref()],
                     ) {
                         v.set(v.get() / (*peers as f64))
                     };
@@ -797,7 +776,7 @@ fn update_gossip_metrics<T: EthSpec>(
                 GossipKind::Attestation(subnet_id) => {
                     if let Some(v) = metrics::get_int_gauge(
                         &metrics::MESH_PEERS_PER_SUBNET_TOPIC,
-                        &[subnet_ids.get(subnet_id.into())],
+                        &[subnet_id_to_string(subnet_id.into())],
                     ) {
                         v.set(peers as i64)
                     };
@@ -806,7 +785,7 @@ fn update_gossip_metrics<T: EthSpec>(
                     // main topics
                     if let Some(v) = metrics::get_int_gauge(
                         &metrics::MESH_PEERS_PER_MAIN_TOPIC,
-                        &[gossip_kinds.get(kind.clone())],
+                        &[kind.as_ref()],
                     ) {
                         v.set(peers as i64)
                     };
@@ -816,35 +795,31 @@ fn update_gossip_metrics<T: EthSpec>(
     }
 
     // protocol peers
-    let mut peers_per_protocol: HashMap<String, i64> = HashMap::new();
+    let mut peers_per_protocol: HashMap<&'static str, i64> = HashMap::new();
     for (_peer, protocol) in gossipsub.peer_protocol() {
-        *peers_per_protocol.entry(protocol.to_string()).or_default() += 1;
+        *peers_per_protocol
+            .entry(protocol.as_static_ref())
+            .or_default() += 1;
     }
 
     for (protocol, peers) in peers_per_protocol.iter() {
-        if let Some(v) =
+        if let Some(v) = metrics::get_int_gauge(&metrics::PEERS_PER_PROTOCOL, &[protocol]) {
-            metrics::get_int_gauge(&metrics::PEERS_PER_PROTOCOL, &[&protocol.to_string()])
-        {
             v.set(*peers)
         };
     }
 
     let mut peer_to_client = HashMap::new();
-    let mut scores_per_client: HashMap<String, Vec<f64>> = HashMap::new();
+    let mut scores_per_client: HashMap<&'static str, Vec<f64>> = HashMap::new();
     {
         let peers = network_globals.peers.read();
         for (peer_id, _) in gossipsub.all_peers() {
             let client = peers
                 .peer_info(peer_id)
-                .map(|peer_info| peer_info.client.kind.to_string())
+                .map(|peer_info| peer_info.client.kind.as_static_ref())
-                .unwrap_or_else(|| "Unknown".to_string());
+                .unwrap_or_else(|| "Unknown");
 
-            peer_to_client.insert(peer_id, client.clone());
+            peer_to_client.insert(peer_id, client);
             let score = gossipsub.peer_score(peer_id).unwrap_or(0.0);
-            if (client == "Prysm" || client == "Lighthouse") && score < 0.0 {
-                trace!(logger, "Peer has negative score"; "peer" => format!("{:?}", peer_id),
-                       "client" => &client, "score" => score);
-            }
             scores_per_client.entry(client).or_default().push(score);
         }
     }
@@ -883,7 +858,7 @@ fn update_gossip_metrics<T: EthSpec>(
     }
 
     for (client, scores) in scores_per_client.into_iter() {
-        let c = &[client.as_ref()];
+        let c = &[client];
         let len = scores.len();
         if len > 0 {
             let mut below0 = 0;

consensus/types/Cargo.toml

@@ -41,6 +41,7 @@ rusqlite = { version = "0.24.0", features = ["bundled"], optional = true }
 arbitrary = { version = "0.4.6", features = ["derive"], optional = true }
 serde_utils = { path = "../serde_utils" }
 regex = "1.3.9"
+lazy_static = "1.4.0"
 
 [dev-dependencies]
 serde_json = "1.0.58"

consensus/types/src/lib.rs

@@ -5,6 +5,8 @@
 // Clippy lint set up
 #![deny(clippy::integer_arithmetic)]
 
+#[macro_use]
+extern crate lazy_static;
 #[macro_use]
 pub mod test_utils;
 

consensus/types/src/subnet_id.rs

@@ -4,11 +4,34 @@ use safe_arith::{ArithError, SafeArith};
 use serde_derive::{Deserialize, Serialize};
 use std::ops::{Deref, DerefMut};
 
+const MAX_SUBNET_ID: usize = 64;
+
+lazy_static! {
+    static ref SUBNET_ID_TO_STRING: Vec<String> = {
+        let mut v = Vec::with_capacity(MAX_SUBNET_ID);
+
+        for i in 0..MAX_SUBNET_ID {
+            v.push(i.to_string());
+        }
+        v
+    };
+}
+
 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
 #[serde(transparent)]
 pub struct SubnetId(#[serde(with = "serde_utils::quoted_u64")] u64);
 
+pub fn subnet_id_to_string(i: u64) -> &'static str {
+    if i < MAX_SUBNET_ID as u64 {
+        &SUBNET_ID_TO_STRING
+            .get(i as usize)
+            .expect("index below MAX_SUBNET_ID")
+    } else {
+        "subnet id out of range"
+    }
+}
+
 impl SubnetId {
     pub fn new(id: u64) -> Self {
         id.into()
@@ -81,3 +104,9 @@ impl Into<u64> for &SubnetId {
         self.0
     }
 }
+
+impl AsRef<str> for SubnetId {
+    fn as_ref(&self) -> &str {
+        subnet_id_to_string(self.0)
+    }
+}