Skip to content

Commit 008ad45

Browse files
jimmygchenjimmygchen
committed
Rewrite to avoid lock contention during computation.
1 parent 5fa537b commit 008ad45

File tree

6 files changed

+77
-96
lines changed

6 files changed

+77
-96
lines changed

‎Cargo.lock‎

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ maplit = "1"
159159
milhouse = "0.5"
160160
mockito = "1.5.0"
161161
num_cpus = "1"
162+
once_cell = "1.17.1"
162163
parking_lot = "0.12"
163164
paste = "1"
164165
prometheus = { version = "0.13", default-features = false }

‎beacon_node/beacon_chain/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ logging = { workspace = true }
4747
lru = { workspace = true }
4848
merkle_proof = { workspace = true }
4949
metrics = { workspace = true }
50-
once_cell = "1.17.1"
50+
once_cell = { workspace = true }
5151
oneshot_broadcast = { path = "../../common/oneshot_broadcast/" }
5252
operation_pool = { workspace = true }
5353
parking_lot = { workspace = true }

‎beacon_node/beacon_chain/src/beacon_proposer_cache.rs‎

Lines changed: 28 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
//! very simple to reason about, but it might store values that are useless due to finalization. The
99
//! values it stores are very small, so this should not be an issue.
1010
11-
use crate::block_verification::BlockBlobError;
1211
use crate::{BeaconChain, BeaconChainError, BeaconChainTypes};
1312
use fork_choice::ExecutionStatus;
1413
use lru::LruCache;
@@ -17,6 +16,7 @@ use smallvec::SmallVec;
1716
use state_processing::state_advance::partial_state_advance;
1817
use std::cmp::Ordering;
1918
use std::num::NonZeroUsize;
19+
use std::sync::Arc;
2020
use types::non_zero_usize::new_non_zero_usize;
2121
use types::{
2222
BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, Fork, Hash256, Slot, Unsigned,
@@ -41,21 +41,21 @@ pub struct Proposer {
4141
/// their signatures.
4242
pub struct EpochBlockProposers {
4343
/// The epoch to which the proposers pertain.
44-
epoch: Epoch,
44+
pub(crate) epoch: Epoch,
4545
/// The fork that should be used to verify proposer signatures.
46-
fork: Fork,
46+
pub(crate) fork: Fork,
4747
/// A list of length `T::EthSpec::slots_per_epoch()`, representing the proposers for each slot
4848
/// in that epoch.
4949
///
5050
/// E.g., if `self.epoch == 1`, then `self.proposers[0]` contains the proposer for slot `32`.
51-
proposers: SmallVec<[usize; TYPICAL_SLOTS_PER_EPOCH]>,
51+
pub(crate) proposers: SmallVec<[usize; TYPICAL_SLOTS_PER_EPOCH]>,
5252
}
5353

5454
/// A cache to store the proposers for some epoch.
5555
///
5656
/// See the module-level documentation for more information.
5757
pub struct BeaconProposerCache {
58-
cache: LruCache<(Epoch, Hash256), OnceCell<EpochBlockProposers>>,
58+
cache: LruCache<(Epoch, Hash256), Arc<OnceCell<EpochBlockProposers>>>,
5959
}
6060

6161
impl Default for BeaconProposerCache {
@@ -111,6 +111,23 @@ impl BeaconProposerCache {
111111
.and_then(|cache_once_cell| cache_once_cell.get().map(|proposers| &proposers.proposers))
112112
}
113113

114+
/// Returns the `OnceCell` for the given `(epoch, shuffling_decision_block)` key,
115+
/// inserting an empty one if it doesn't exist.
116+
///
117+
/// The returned `OnceCell` allows the caller to initialise the value externally
118+
/// using `get_or_try_init`, enabling deferred computation without holding a mutable
119+
/// reference to the cache.
120+
pub fn get_or_insert_key(
121+
&mut self,
122+
epoch: Epoch,
123+
shuffling_decision_block: Hash256,
124+
) -> Arc<OnceCell<EpochBlockProposers>> {
125+
let key = (epoch, shuffling_decision_block);
126+
self.cache
127+
.get_or_insert(key, || Arc::new(OnceCell::new()))
128+
.clone()
129+
}
130+
114131
/// Insert the proposers into the cache.
115132
///
116133
/// See `Self::get` for a description of `shuffling_decision_block`.
@@ -125,61 +142,16 @@ impl BeaconProposerCache {
125142
) -> Result<(), BeaconStateError> {
126143
let key = (epoch, shuffling_decision_block);
127144
if !self.cache.contains(&key) {
128-
self.cache.put(
129-
key,
130-
OnceCell::with_value(EpochBlockProposers {
131-
epoch,
132-
fork,
133-
proposers: proposers.into(),
134-
}),
135-
);
136-
}
137-
138-
Ok(())
139-
}
140-
141-
pub fn get_slot_or_insert_with<E: EthSpec, F, Err: BlockBlobError>(
142-
&mut self,
143-
shuffling_decision_block: Hash256,
144-
slot: Slot,
145-
compute_proposers_and_fork_fn: F,
146-
) -> Result<Option<Proposer>, Err>
147-
where
148-
F: FnOnce() -> Result<(Vec<usize>, Fork), Err>,
149-
{
150-
let epoch = slot.epoch(E::slots_per_epoch());
151-
let (proposers, fork) = self.get_epoch_or_insert_with(
152-
shuffling_decision_block,
153-
epoch,
154-
compute_proposers_and_fork_fn,
155-
)?;
156-
157-
Ok(proposers
158-
.get(slot.as_usize() % E::SlotsPerEpoch::to_usize())
159-
.map(|&index| Proposer { index, fork }))
160-
}
161-
162-
pub fn get_epoch_or_insert_with<F, Err: BlockBlobError>(
163-
&mut self,
164-
shuffling_decision_block: Hash256,
165-
epoch: Epoch,
166-
compute_proposers_and_fork_fn: F,
167-
) -> Result<(&SmallVec<[usize; TYPICAL_SLOTS_PER_EPOCH]>, Fork), Err>
168-
where
169-
F: FnOnce() -> Result<(Vec<usize>, Fork), Err>,
170-
{
171-
let key = (epoch, shuffling_decision_block);
172-
let once_cell = self.cache.get_or_insert_mut(key, OnceCell::new);
173-
let epoch_block_proposers = once_cell.get_or_try_init(|| {
174-
let (proposers, fork) = compute_proposers_and_fork_fn()?;
175-
Ok::<EpochBlockProposers, Err>(EpochBlockProposers {
145+
let epoch_proposers = EpochBlockProposers {
176146
epoch,
177147
fork,
178148
proposers: proposers.into(),
179-
})
180-
})?;
149+
};
150+
self.cache
151+
.put(key, Arc::new(OnceCell::with_value(epoch_proposers)));
152+
}
181153

182-
Ok((&epoch_block_proposers.proposers, epoch_block_proposers.fork))
154+
Ok(())
183155
}
184156
}
185157

‎beacon_node/beacon_chain/src/data_column_verification.rs‎

Lines changed: 47 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::beacon_proposer_cache::Proposer;
1+
use crate::beacon_proposer_cache::EpochBlockProposers;
22
use crate::block_verification::{
33
cheap_state_advance_to_obtain_committees, get_validator_pubkey_cache, process_block_slash_info,
44
BlockSlashInfo,
@@ -572,45 +572,57 @@ fn verify_proposer_and_signature<T: BeaconChainTypes>(
572572
parent_block.root
573573
};
574574

575-
let Proposer {
576-
index: proposer_index,
577-
fork,
578-
} = chain
575+
// We lock the cache briefly to get or insert a OnceCell, then drop the lock
576+
// before doing proposer shuffling calculation via `OnceCell::get_or_try_init`. This avoids
577+
// holding the lock during the computation, while still ensuring the result is cached and
578+
// initialised only once.
579+
//
580+
// This approach exposes the cache internals (`OnceCell` & `EpochBlockProposers`)
581+
// as a trade-off for avoiding lock contention.
582+
let epoch_proposers_cell = chain
579583
.beacon_proposer_cache
580584
.lock()
581-
.get_slot_or_insert_with::<E, _, GossipDataColumnError>(
582-
proposer_shuffling_root,
585+
.get_or_insert_key(column_epoch, proposer_shuffling_root);
586+
587+
let epoch_proposers = epoch_proposers_cell.get_or_try_init(move || {
588+
debug!(
589+
%block_root,
590+
index = %column_index,
591+
"Proposer shuffling cache miss for column verification"
592+
);
593+
let (parent_state_root, mut parent_state) = chain
594+
.store
595+
.get_advanced_hot_state(block_parent_root, column_slot, parent_block.state_root)
596+
.map_err(|e| GossipDataColumnError::BeaconChainError(e.into()))?
597+
.ok_or_else(|| {
598+
BeaconChainError::DBInconsistent(format!(
599+
"Missing state for parent block {block_parent_root:?}",
600+
))
601+
})?;
602+
603+
let state = cheap_state_advance_to_obtain_committees::<_, GossipDataColumnError>(
604+
&mut parent_state,
605+
Some(parent_state_root),
583606
column_slot,
584-
move || {
585-
debug!(
586-
%block_root,
587-
index = %column_index,
588-
"Proposer shuffling cache miss for column verification"
589-
);
590-
let (parent_state_root, mut parent_state) = chain
591-
.store
592-
.get_advanced_hot_state(block_parent_root, column_slot, parent_block.state_root)
593-
.map_err(|e| GossipDataColumnError::BeaconChainError(e.into()))?
594-
.ok_or_else(|| {
595-
BeaconChainError::DBInconsistent(format!(
596-
"Missing state for parent block {block_parent_root:?}",
597-
))
598-
})?;
599-
600-
let state = cheap_state_advance_to_obtain_committees::<_, GossipDataColumnError>(
601-
&mut parent_state,
602-
Some(parent_state_root),
603-
column_slot,
604-
&chain.spec,
605-
)?;
606-
607-
let proposers = state.get_beacon_proposer_indices(&chain.spec)?;
608-
// Prime the proposer shuffling cache with the newly-learned value.
609-
Ok((proposers, state.fork()))
610-
},
611-
)?
607+
&chain.spec,
608+
)?;
609+
610+
let proposers = state.get_beacon_proposer_indices(&chain.spec)?;
611+
// Prime the proposer shuffling cache with the newly-learned value.
612+
Ok::<_, GossipDataColumnError>(EpochBlockProposers {
613+
epoch: column_epoch,
614+
fork: state.fork(),
615+
proposers: proposers.into(),
616+
})
617+
})?;
618+
619+
let proposer_index = *epoch_proposers
620+
.proposers
621+
.get(column_slot.as_usize() % T::EthSpec::slots_per_epoch() as usize)
612622
.ok_or_else(|| BeaconChainError::NoProposerForSlot(column_slot))?;
613623

624+
let fork = epoch_proposers.fork;
625+
614626
// Signature verify the signed block header.
615627
let signature_is_valid = {
616628
let pubkey_cache = get_validator_pubkey_cache(chain)

‎common/logging/Cargo.toml‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ test_logger = [] # Print log output to stderr when running tests instead of drop
1111
chrono = { version = "0.4", default-features = false, features = ["clock", "std"] }
1212
logroller = { workspace = true }
1313
metrics = { workspace = true }
14-
once_cell = "1.17.1"
15-
parking_lot = { workspace = true }
1614
serde = { workspace = true }
1715
serde_json = { workspace = true }
1816
tokio = { workspace = true, features = [ "time" ] }

0 commit comments

Comments
 (0)