Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid cloning committee #9065

Merged
merged 1 commit into from
Mar 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1630,7 +1630,7 @@ impl AuthorityState {
let cache_metrics = Arc::new(ResolverMetrics::new(&registry));
let epoch_store = AuthorityPerEpochStore::new(
name,
genesis_committee.clone(),
Arc::new(genesis_committee.clone()),
&path.join("store"),
None,
EpochMetrics::new(&registry),
Expand Down
9 changes: 4 additions & 5 deletions crates/sui-core/src/authority/authority_per_epoch_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ pub struct ExecutionComponents {
}

pub struct AuthorityPerEpochStore {
// TODO: Make this Arc<Committee> for more efficient pass-around.
committee: Committee,
committee: Arc<Committee>,
tables: AuthorityEpochTables,

protocol_config: ProtocolConfig,
Expand Down Expand Up @@ -379,7 +378,7 @@ impl AuthorityEpochTables {
impl AuthorityPerEpochStore {
pub fn new(
name: AuthorityName,
committee: Committee,
committee: Arc<Committee>,
parent_path: &Path,
db_options: Option<Options>,
metrics: Arc<EpochMetrics>,
Expand All @@ -391,7 +390,7 @@ impl AuthorityPerEpochStore {
let epoch_id = committee.epoch;
let tables = AuthorityEpochTables::open(epoch_id, parent_path, db_options.clone());
let end_of_publish =
StakeAggregator::from_iter(Arc::new(committee.clone()), tables.end_of_publish.iter());
StakeAggregator::from_iter(committee.clone(), tables.end_of_publish.iter());
let reconfig_state = tables
.load_reconfig_state()
.expect("Load reconfig state at initialization cannot fail");
Expand Down Expand Up @@ -467,7 +466,7 @@ impl AuthorityPerEpochStore {
self.record_epoch_total_duration_metric();
Self::new(
name,
new_committee,
Arc::new(new_committee),
&self.parent_path,
self.db_options.clone(),
self.metrics.clone(),
Expand Down
6 changes: 3 additions & 3 deletions crates/sui-core/src/authority/authority_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl AuthorityStore {
Self::open_inner(
genesis,
perpetual_tables,
committee,
&committee,
indirect_objects_threshold,
)
.await
Expand All @@ -114,7 +114,7 @@ impl AuthorityStore {
Self::open_inner(
genesis,
perpetual_tables,
committee.clone(),
committee,
indirect_objects_threshold,
)
.await
Expand All @@ -123,7 +123,7 @@ impl AuthorityStore {
async fn open_inner(
genesis: &Genesis,
perpetual_tables: Arc<AuthorityPerpetualTables>,
committee: Committee,
committee: &Committee,
indirect_objects_threshold: usize,
) -> SuiResult<Self> {
let epoch = committee.epoch;
Expand Down
19 changes: 11 additions & 8 deletions crates/sui-core/src/epoch/committee_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use parking_lot::RwLock;
use rocksdb::Options;
use std::collections::HashMap;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use sui_storage::default_db_options;
use sui_types::base_types::ObjectID;
use sui_types::committee::{Committee, EpochId};
Expand All @@ -19,7 +20,7 @@ use sui_macros::nondeterministic;

pub struct CommitteeStore {
tables: CommitteeStoreTables,
cache: RwLock<HashMap<EpochId, Committee>>,
cache: RwLock<HashMap<EpochId, Arc<Committee>>>,
}

#[derive(DBMapUtils)]
Expand Down Expand Up @@ -63,32 +64,33 @@ impl CommitteeStore {
pub fn init_genesis_committee(&self, genesis_committee: Committee) -> SuiResult {
assert_eq!(genesis_committee.epoch, 0);
self.tables.committee_map.insert(&0, &genesis_committee)?;
self.cache.write().insert(0, genesis_committee);
self.cache.write().insert(0, Arc::new(genesis_committee));
Ok(())
}

pub fn insert_new_committee(&self, new_committee: &Committee) -> SuiResult {
if let Some(old_committee) = self.get_committee(&new_committee.epoch)? {
// If somehow we already have this committee in the store, they must be the same.
assert_eq!(&old_committee, new_committee);
assert_eq!(&*old_committee, new_committee);
} else {
self.tables
.committee_map
.insert(&new_committee.epoch, new_committee)?;
self.cache
.write()
.insert(new_committee.epoch, new_committee.clone());
.insert(new_committee.epoch, Arc::new(new_committee.clone()));
}
Ok(())
}

pub fn get_committee(&self, epoch_id: &EpochId) -> SuiResult<Option<Committee>> {
pub fn get_committee(&self, epoch_id: &EpochId) -> SuiResult<Option<Arc<Committee>>> {
if let Some(committee) = self.cache.read().get(epoch_id) {
return Ok(Some(committee.clone())); // todo use Arc
return Ok(Some(committee.clone()));
}
let committee = self.tables.committee_map.get(epoch_id)?;
let committee = committee.map(Arc::new);
if let Some(committee) = committee.as_ref() {
self.cache.write().insert(*epoch_id, committee.clone()); // todo use Arc
self.cache.write().insert(*epoch_id, committee.clone());
}
Ok(committee)
}
Expand All @@ -111,7 +113,8 @@ impl CommitteeStore {
Ok(match epoch {
Some(epoch) => self
.get_committee(&epoch)?
.ok_or(SuiError::MissingCommitteeAtEpoch(epoch))?,
.ok_or(SuiError::MissingCommitteeAtEpoch(epoch))
.map(|c| Committee::clone(&*c))?,
None => self.get_latest_committee(),
})
}
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-core/src/safe_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ impl<C, S: SignatureVerifier> SafeClient<C, S> {
&mut self.authority_client
}

fn get_committee(&self, epoch_id: &EpochId) -> SuiResult<Committee> {
fn get_committee(&self, epoch_id: &EpochId) -> SuiResult<Arc<Committee>> {
self.committee_store
.get_committee(epoch_id)?
.ok_or(SuiError::MissingCommitteeAtEpoch(*epoch_id))
Expand Down Expand Up @@ -446,7 +446,7 @@ where
match checkpoint {
Some(c) => {
let epoch_id = c.epoch;
c.verify_with_contents(&self.get_committee(&epoch_id)?, contents.as_ref())
c.verify_with_contents(&*self.get_committee(&epoch_id)?, contents.as_ref())
}
None => Ok(()),
}
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-core/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl ReadStore for RocksDbStore {
.transpose()
}

fn get_committee(&self, epoch: EpochId) -> Result<Option<Committee>, Self::Error> {
fn get_committee(&self, epoch: EpochId) -> Result<Option<Arc<Committee>>, Self::Error> {
Ok(self.committee_store.get_committee(&epoch).unwrap())
}

Expand Down
4 changes: 2 additions & 2 deletions crates/sui-core/src/unit_tests/authority_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2722,7 +2722,7 @@ async fn test_authority_persist() {
let cache_metrics = Arc::new(ResolverMetrics::new(&registry));
let epoch_store = AuthorityPerEpochStore::new(
name,
committee,
Arc::new(committee),
&epoch_store_path,
None,
EpochMetrics::new(&registry),
Expand Down Expand Up @@ -4777,7 +4777,7 @@ async fn test_tallying_rule_score_updates() {
let cache_metrics = Arc::new(ResolverMetrics::new(&registry));
let epoch_store = AuthorityPerEpochStore::new(
auth_0_name,
committee.clone(),
Arc::new(committee.clone()),
&path,
None,
metrics.clone(),
Expand Down
13 changes: 9 additions & 4 deletions crates/sui-types/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use serde::{Deserialize, Serialize};
use serde_with::serde_as;
use std::collections::{BTreeMap, HashMap};
use std::convert::Infallible;
use std::sync::Arc;
use tap::Pipe;

#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)]
Expand Down Expand Up @@ -214,7 +215,7 @@ pub trait ReadStore {
digest: &CheckpointContentsDigest,
) -> Result<Option<FullCheckpointContents>, Self::Error>;

fn get_committee(&self, epoch: EpochId) -> Result<Option<Committee>, Self::Error>;
fn get_committee(&self, epoch: EpochId) -> Result<Option<Arc<Committee>>, Self::Error>;

fn get_transaction(
&self,
Expand Down Expand Up @@ -264,7 +265,7 @@ impl<T: ReadStore> ReadStore for &T {
ReadStore::get_full_checkpoint_contents(*self, digest)
}

fn get_committee(&self, epoch: EpochId) -> Result<Option<Committee>, Self::Error> {
fn get_committee(&self, epoch: EpochId) -> Result<Option<Arc<Committee>>, Self::Error> {
ReadStore::get_committee(*self, epoch)
}

Expand Down Expand Up @@ -546,8 +547,12 @@ impl ReadStore for SharedInMemoryStore {
.transpose()
}

fn get_committee(&self, epoch: EpochId) -> Result<Option<Committee>, Self::Error> {
self.inner().get_committee_by_epoch(epoch).cloned().pipe(Ok)
fn get_committee(&self, epoch: EpochId) -> Result<Option<Arc<Committee>>, Self::Error> {
self.inner()
.get_committee_by_epoch(epoch)
.cloned()
.map(Arc::new)
.pipe(Ok)
}

fn get_transaction(
Expand Down