From 3408de81518f5125e38042b2bc9e33cfbd026242 Mon Sep 17 00:00:00 2001 From: blacktemplar Date: Wed, 18 Nov 2020 23:31:37 +0000 Subject: [PATCH] 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 | 29 ++++---- beacon_node/eth2_libp2p/Cargo.toml | 2 +- .../eth2_libp2p/src/peer_manager/client.rs | 22 +++++- beacon_node/eth2_libp2p/src/types/topics.rs | 20 ++++-- beacon_node/network/src/service.rs | 71 ++++++------------- consensus/types/Cargo.toml | 1 + consensus/types/src/lib.rs | 2 + consensus/types/src/subnet_id.rs | 29 ++++++++ 8 files changed, 107 insertions(+), 69 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b3b0fdcc9..442aba829 100644 --- a/Cargo.lock +++ b/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", diff --git a/beacon_node/eth2_libp2p/Cargo.toml b/beacon_node/eth2_libp2p/Cargo.toml index cf539512a..c78fc879b 100644 --- a/beacon_node/eth2_libp2p/Cargo.toml +++ b/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"] diff --git a/beacon_node/eth2_libp2p/src/peer_manager/client.rs b/beacon_node/eth2_libp2p/src/peer_manager/client.rs index f23f32be6..51fa1cb11 100644 --- a/beacon_node/eth2_libp2p/src/peer_manager/client.rs +++ b/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 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()) } } diff --git a/beacon_node/eth2_libp2p/src/types/topics.rs b/beacon_node/eth2_libp2p/src/types/topics.rs index 3f120b3ec..7409607d6 100644 --- a/beacon_node/eth2_libp2p/src/types/topics.rs +++ b/beacon_node/eth2_libp2p/src/types/topics.rs @@ -52,15 +52,25 @@ pub enum GossipKind { AttesterSlashing, } +impl AsRef 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"), - GossipKind::ProposerSlashing => write!(f, "proposer_slashing"), - GossipKind::AttesterSlashing => write!(f, "attester_slashing"), + x => f.write_str(x.as_ref()), } } } diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index d0c9e4159..d517d4f10 100644 --- a/beacon_node/network/src/service.rs +++ b/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( update_gossip_metrics::( &service.libp2p.swarm.gs(), &service.network_globals, - &service.log ); } _ = service.gossipsub_parameter_update.next() => { @@ -588,7 +588,7 @@ fn expose_publish_metrics(messages: &[PubsubMessage]) { 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(message: &PubsubMessage) { } } -/// A work-around to reduce temporary allocation when updating gossip metrics. -pub struct ToStringCache(HashMap); - -impl ToStringCache { - 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( gossipsub: &Gossipsub, network_globals: &Arc>, - logger: &slog::Logger, ) { // Clear the metrics let _ = metrics::PEERS_PER_PROTOCOL @@ -683,28 +667,23 @@ fn update_gossip_metrics( .as_ref() .map(|gauge| gauge.reset()); - let mut subnet_ids: ToStringCache = - ToStringCache::with_capacity(T::default_spec().attestation_subnet_count as usize); - let mut gossip_kinds: ToStringCache = - 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( 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( 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( 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( 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( // 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( // 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( 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( // 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( } // protocol peers - let mut peers_per_protocol: HashMap = 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) = - metrics::get_int_gauge(&metrics::PEERS_PER_PROTOCOL, &[&protocol.to_string()]) - { + if let Some(v) = metrics::get_int_gauge(&metrics::PEERS_PER_PROTOCOL, &[protocol]) { v.set(*peers) }; } let mut peer_to_client = HashMap::new(); - let mut scores_per_client: HashMap> = HashMap::new(); + let mut scores_per_client: HashMap<&'static str, Vec> = 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()) - .unwrap_or_else(|| "Unknown".to_string()); + .map(|peer_info| peer_info.client.kind.as_static_ref()) + .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( } 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; diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index 8984beeda..397ee7e16 100644 --- a/consensus/types/Cargo.toml +++ b/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" diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 3ee69c726..b9b1950ae 100644 --- a/consensus/types/src/lib.rs +++ b/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; diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index f03506230..9b54af169 100644 --- a/consensus/types/src/subnet_id.rs +++ b/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 = { + 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 for &SubnetId { self.0 } } + +impl AsRef for SubnetId { + fn as_ref(&self) -> &str { + subnet_id_to_string(self.0) + } +}