From f47b75795ce6a90c7bb7dc8915c2f9c69eefb634 Mon Sep 17 00:00:00 2001 From: Tim Vermeulen Date: Tue, 12 May 2020 16:47:28 +0200 Subject: [PATCH] SyncBucket cleanup (#407) * Minor bucket improvements * Remove unused Ord implementations of several structures * Derive PartialEq and Hash for Cid in order to avoid allocating * Remove unnecessary mut --- blockchain/blocks/src/header.rs | 13 ---- blockchain/blocks/src/tipset.rs | 4 +- blockchain/chain_sync/src/bucket.rs | 66 +++++++++----------- blockchain/chain_sync/src/network_context.rs | 4 +- blockchain/chain_sync/src/sync.rs | 2 +- ipld/cid/src/codec.rs | 2 +- ipld/cid/src/lib.rs | 27 +------- ipld/cid/src/version.rs | 2 +- 8 files changed, 37 insertions(+), 83 deletions(-) diff --git a/blockchain/blocks/src/header.rs b/blockchain/blocks/src/header.rs index 8e4ae368b72a..5c51426637f1 100644 --- a/blockchain/blocks/src/header.rs +++ b/blockchain/blocks/src/header.rs @@ -18,7 +18,6 @@ use num_bigint::{ BigUint, }; use sha2::Digest; -use std::cmp::Ordering; use std::fmt; use std::time::{SystemTime, UNIX_EPOCH}; use vm::PoStProof; @@ -202,18 +201,6 @@ impl<'de> Deserialize<'de> for BlockHeader { } } -impl PartialOrd for BlockHeader { - fn partial_cmp(&self, other: &Self) -> Option { - self.cached_bytes.partial_cmp(&other.cached_bytes) - } -} - -impl Ord for BlockHeader { - fn cmp(&self, other: &Self) -> Ordering { - self.cached_bytes.cmp(&other.cached_bytes) - } -} - impl BlockHeader { /// Generates a BlockHeader builder as a constructor pub fn builder() -> BlockHeaderBuilder { diff --git a/blockchain/blocks/src/tipset.rs b/blockchain/blocks/src/tipset.rs index 7827102c5747..aeafc301860a 100644 --- a/blockchain/blocks/src/tipset.rs +++ b/blockchain/blocks/src/tipset.rs @@ -17,7 +17,7 @@ use serde::Deserialize; /// A set of CIDs forming a unique key for a Tipset. /// Equal keys will have equivalent iteration order, but note that the CIDs are *not* maintained in /// the same order as the canonical iteration order of blocks in a tipset (which is by ticket) -#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, Ord, PartialOrd)] +#[derive(Clone, Debug, PartialEq, Eq, Hash, Default)] pub struct TipsetKeys { pub cids: Vec, } @@ -56,7 +56,7 @@ impl Cbor for TipsetKeys {} /// An immutable set of blocks at the same height with the same parent set. /// Blocks in a tipset are canonically ordered by ticket size. -#[derive(Clone, PartialEq, Debug, PartialOrd, Ord, Eq)] +#[derive(Clone, PartialEq, Debug, Eq)] pub struct Tipset { blocks: Vec, key: TipsetKeys, diff --git a/blockchain/chain_sync/src/bucket.rs b/blockchain/chain_sync/src/bucket.rs index 596defa47fa6..3957bf246fbe 100644 --- a/blockchain/chain_sync/src/bucket.rs +++ b/blockchain/chain_sync/src/bucket.rs @@ -2,10 +2,11 @@ // SPDX-License-Identifier: Apache-2.0, MIT use blocks::Tipset; +use num_bigint::BigUint; use std::sync::Arc; /// SyncBucket defines a bucket of tipsets to sync -#[derive(Clone, Default, PartialEq, PartialOrd, Ord, Eq)] +#[derive(Clone, Default, PartialEq, Eq)] pub struct SyncBucket { tips: Vec>, } @@ -15,25 +16,20 @@ impl SyncBucket { fn new(tips: Vec>) -> SyncBucket { Self { tips } } + /// Returns the weight of the heaviest tipset + fn max_weight(&self) -> Option<&BigUint> { + self.tips.iter().map(|ts| ts.weight()).max() + } /// Returns the tipset with the max weight pub fn heaviest_tipset(&self) -> Option> { - if self.tips.is_empty() { - return None; - } - - // return max value pointer self.tips.iter().max_by_key(|a| a.weight()).cloned() } /// Returns true if tipset is from same chain - pub fn same_chain_as(&mut self, ts: &Tipset) -> bool { - for t in self.tips.iter_mut() { - // TODO Confirm that comparing keys will be sufficient on full tipset impl - if ts.key() == t.key() || ts.key() == t.parents() || ts.parents() == t.key() { - return true; - } - } - - false + pub fn is_same_chain_as(&self, ts: &Tipset) -> bool { + // TODO Confirm that comparing keys will be sufficient on full tipset impl + self.tips + .iter() + .any(|t| ts.key() == t.key() || ts.key() == t.parents() || ts.parents() == t.key()) } /// Adds tipset to vector to be included in the bucket pub fn add(&mut self, ts: Arc) { @@ -56,39 +52,35 @@ pub(crate) struct SyncBucketSet { impl SyncBucketSet { /// Inserts a tipset into a bucket pub(crate) fn insert(&mut self, tipset: Arc) { - for b in self.buckets.iter_mut() { - if b.same_chain_as(&tipset) { - b.add(tipset); - return; - } + if let Some(b) = self + .buckets + .iter_mut() + .find(|b| b.is_same_chain_as(&tipset)) + { + b.add(tipset); + } else { + self.buckets.push(SyncBucket::new(vec![tipset])) } - self.buckets.push(SyncBucket::new(vec![tipset])) } /// Removes the SyncBucket with heaviest weighted Tipset from SyncBucketSet pub(crate) fn pop(&mut self) -> Option { - if let Some((i, _)) = self + let (i, _) = self .buckets() .iter() .enumerate() - .max_by_key(|(_, b)| b.heaviest_tipset()) - { - let ts = self.buckets.remove(i); - Some(ts) - } else { - None - } + .map(|(i, b)| (i, b.max_weight())) + .max_by(|(_, w1), (_, w2)| w1.cmp(w2))?; + // we can't use `max_by_key` here because the weight is a reference, + // see https://github.com/rust-lang/rust/issues/34162 + + Some(self.buckets.swap_remove(i)) } /// Returns heaviest tipset from bucket set pub(crate) fn heaviest(&self) -> Option> { - // Transform max values from each bucket into a Vec - let vals: Vec> = self - .buckets + self.buckets .iter() - .filter_map(|b| b.heaviest_tipset()) - .collect(); - - // Return the heaviest tipset bucket - vals.iter().max_by_key(|b| b.weight()).cloned() + .filter_map(SyncBucket::heaviest_tipset) + .max_by(|ts1, ts2| ts1.weight().cmp(ts2.weight())) } /// Returns a vector of SyncBuckets pub(crate) fn buckets(&self) -> &[SyncBucket] { diff --git a/blockchain/chain_sync/src/network_context.rs b/blockchain/chain_sync/src/network_context.rs index 6d3af6ed3c98..ea7d02a61b30 100644 --- a/blockchain/chain_sync/src/network_context.rs +++ b/blockchain/chain_sync/src/network_context.rs @@ -113,7 +113,7 @@ impl SyncNetworkContext { } /// Send a hello request to the network (does not await response) - pub async fn hello_request(&mut self, peer_id: PeerId, request: HelloMessage) { + pub async fn hello_request(&self, peer_id: PeerId, request: HelloMessage) { trace!("Sending Hello Message {:?}", request); // TODO update to await response when we want to handle the latency self.send_rpc_event(peer_id, RPCEvent::Request(0, RPCRequest::Hello(request))) @@ -146,7 +146,7 @@ impl SyncNetworkContext { } /// Handles sending the base event to the network service - async fn send_rpc_event(&mut self, peer_id: PeerId, event: RPCEvent) { + async fn send_rpc_event(&self, peer_id: PeerId, event: RPCEvent) { self.network_send .send(NetworkMessage::RPC { peer_id, event }) .await diff --git a/blockchain/chain_sync/src/sync.rs b/blockchain/chain_sync/src/sync.rs index 8174e0556252..002ed4002720 100644 --- a/blockchain/chain_sync/src/sync.rs +++ b/blockchain/chain_sync/src/sync.rs @@ -414,7 +414,7 @@ where // TODO check for related tipsets // if next_sync_target is from same chain as incoming tipset add it to be synced next - if !self.next_sync_target.is_empty() && self.next_sync_target.same_chain_as(&tipset) { + if self.next_sync_target.is_same_chain_as(&tipset) { self.next_sync_target.add(tipset); } else { // add incoming tipset to queue to by synced later diff --git a/ipld/cid/src/codec.rs b/ipld/cid/src/codec.rs index 29f0030cd17d..cbb73f0c3b44 100644 --- a/ipld/cid/src/codec.rs +++ b/ipld/cid/src/codec.rs @@ -5,7 +5,7 @@ use crate::Error; macro_rules! build_codec_enum { {$( $val:expr => $var:ident, )*} => { - #[derive(PartialEq, Eq, Clone, Copy, Debug)] + #[derive(PartialEq, Eq, Hash, Clone, Copy, Debug)] pub enum Codec { $( $var, )* } diff --git a/ipld/cid/src/lib.rs b/ipld/cid/src/lib.rs index 7593598f2381..9a5625f6001b 100644 --- a/ipld/cid/src/lib.rs +++ b/ipld/cid/src/lib.rs @@ -12,7 +12,6 @@ pub use self::version::Version; use integer_encoding::{VarIntReader, VarIntWriter}; pub use multihash; use multihash::{Code, Identity, Multihash, MultihashDigest}; -use std::cmp::Ordering; use std::convert::TryInto; use std::fmt; use std::io::Cursor; @@ -43,7 +42,7 @@ pub struct Prefix { } /// Representation of a IPLD CID. -#[derive(Eq, Clone)] +#[derive(PartialEq, Eq, Hash, Clone)] pub struct Cid { pub version: Version, pub codec: Codec, @@ -203,30 +202,6 @@ impl Cid { } } -impl std::hash::Hash for Cid { - fn hash(&self, state: &mut H) { - self.to_bytes().hash(state); - } -} - -impl PartialEq for Cid { - fn eq(&self, other: &Self) -> bool { - self.to_bytes() == other.to_bytes() - } -} - -impl PartialOrd for Cid { - fn partial_cmp(&self, other: &Self) -> Option { - self.to_bytes().partial_cmp(&other.to_bytes()) - } -} - -impl Ord for Cid { - fn cmp(&self, other: &Self) -> Ordering { - self.to_bytes().cmp(&other.to_bytes()) - } -} - impl fmt::Display for Cid { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let encoded = match self.version { diff --git a/ipld/cid/src/version.rs b/ipld/cid/src/version.rs index 75bd6e9168ff..057887d80a1c 100644 --- a/ipld/cid/src/version.rs +++ b/ipld/cid/src/version.rs @@ -4,7 +4,7 @@ use crate::Error; /// Cid protocol version -#[derive(PartialEq, Eq, Clone, Copy, Debug)] +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug)] pub enum Version { V0, V1,