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

[Optimize] Cache the recently queried committees #3106

Merged
merged 5 commits into from
Mar 2, 2024
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion node/bft/ledger-service/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ edition = "2021"

[features]
default = [ ]
ledger = [ "rand", "tokio", "tracing" ]
ledger = [ "parking_lot", "rand", "tokio", "tracing" ]
ledger-write = [ ]
mock = [ "parking_lot", "tracing" ]
prover = [ ]
Expand All @@ -32,6 +32,9 @@ version = "0.1"
version = "2.1"
features = [ "serde", "rayon" ]

[dependencies.lru]
version = "0.12"

[dependencies.parking_lot]
version = "0.12"
optional = true
Expand Down
21 changes: 19 additions & 2 deletions node/bft/ledger-service/src/ledger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ use snarkvm::{
};

use indexmap::IndexMap;
use lru::LruCache;
use parking_lot::Mutex;
use std::{
fmt,
ops::Range,
Expand All @@ -35,18 +37,23 @@ use std::{
},
};

/// The capacity of the LRU holiding the recently queried committees.
const COMMITTEE_CACHE_SIZE: usize = 16;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for using 16?

This committee cache should just need the rounds near the ones being processed, but not sure what a good number would be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really; the idea was just that it would be small (for low overhead), but still large enough to cover all possible hits. It could probably be a bit smaller, but any resulting difference should be negligible.


/// A core ledger service.
pub struct CoreLedgerService<N: Network, C: ConsensusStorage<N>> {
ledger: Ledger<N, C>,
coinbase_verifying_key: Arc<CoinbaseVerifyingKey<N>>,
committee_cache: Arc<Mutex<LruCache<u64, Committee<N>>>>,
shutdown: Arc<AtomicBool>,
}

impl<N: Network, C: ConsensusStorage<N>> CoreLedgerService<N, C> {
/// Initializes a new core ledger service.
pub fn new(ledger: Ledger<N, C>, shutdown: Arc<AtomicBool>) -> Self {
let coinbase_verifying_key = Arc::new(ledger.coinbase_puzzle().coinbase_verifying_key().clone());
Self { ledger, coinbase_verifying_key, shutdown }
let committee_cache = Arc::new(Mutex::new(LruCache::new(COMMITTEE_CACHE_SIZE.try_into().unwrap())));
Self { ledger, coinbase_verifying_key, committee_cache, shutdown }
}
}

Expand Down Expand Up @@ -127,9 +134,19 @@ impl<N: Network, C: ConsensusStorage<N>> LedgerService<N> for CoreLedgerService<
/// Returns the committee for the given round.
/// If the given round is in the future, then the current committee is returned.
fn get_committee_for_round(&self, round: u64) -> Result<Committee<N>> {
// Check if the committee is already in the cache.
if let Some(committee) = self.committee_cache.lock().get(&round) {
return Ok(committee.clone());
}

match self.ledger.get_committee_for_round(round)? {
// Return the committee if it exists.
Some(committee) => Ok(committee),
Some(committee) => {
// Insert the committee into the cache.
self.committee_cache.lock().push(round, committee.clone());
// Return the committee.
Ok(committee)
}
// Return the current committee if the round is in the future.
None => {
// Retrieve the current committee.
Expand Down