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 2 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.

6 changes: 4 additions & 2 deletions node/bft/ledger-service/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ edition = "2021"
default = [ ]
ledger = [ "rand", "tokio", "tracing" ]
ledger-write = [ ]
mock = [ "parking_lot", "tracing" ]
mock = [ "tracing" ]
prover = [ ]
test = [ "mock", "translucent" ]
translucent = [ "ledger" ]
Expand All @@ -32,9 +32,11 @@ version = "0.1"
version = "2.1"
features = [ "serde", "rayon" ]

[dependencies.lru]
version = "0.12"

[dependencies.parking_lot]
version = "0.12"
optional = true

[dependencies.rand]
version = "0.8"
Expand Down
27 changes: 22 additions & 5 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>>,
shutdown: Arc<AtomicBool>,
committee_cache: Arc<Mutex<LruCache<u64, Committee<N>>>>,
}

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, shutdown, committee_cache }
}
}

Expand Down Expand Up @@ -127,20 +134,30 @@ 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>> {
match self.ledger.get_committee_for_round(round)? {
// Check if the committee is already in the cache.
if let Some(committee) = self.committee_cache.lock().get(&round) {
return Ok(committee.clone());
}

let committee = match self.ledger.get_committee_for_round(round)? {
// Return the committee if it exists.
Some(committee) => Ok(committee),
Some(committee) => committee,
// Return the current committee if the round is in the future.
None => {
// Retrieve the current committee.
let current_committee = self.current_committee()?;
// Return the current committee if the round is in the future.
match current_committee.starting_round() <= round {
true => Ok(current_committee),
true => current_committee,
false => bail!("No committee found for round {round} in the ledger"),
}
}
}
};

// Insert the committee into the cache.
self.committee_cache.lock().push(round, committee.clone());
Copy link
Contributor

@howardwu howardwu Feb 20, 2024

Choose a reason for hiding this comment

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

@ljedrz this only works for the match case on Some. It does not work for the committee from None.

Can you scope the cache for only committees from Some and profile the performance again?

(If you want to go the extra mile, you can also try adding a cache for the underlying impl of self.current_committee(), but this extra overhead may not be worth the complexity. The majority of calls will route to Some I believe)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed; I ran the branch locally and all I can see are still cache hits. As for the current committee, I agree that its overhead is low; we can always go back to that idea in the future if need be.


Ok(committee)
}

/// Returns the committee lookback for the given round.
Expand Down