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.

Sign up for GitHub

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

Removes AccountStorageReference #4931

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Removes AccountStorageReference #4931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
108 changes: 25 additions & 83 deletions accounts-db/src/account_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,7 @@ use {

pub mod meta;

#[derive(Clone, Debug)]
pub struct AccountStorageReference {
/// the single storage for a given slot
pub storage: Arc<AccountStorageEntry>,
/// id can be read from 'storage', but it is an atomic read.
/// id will never change while a storage is held, so we store it separately here for faster runtime lookup in 'get_account_storage_entry'
pub id: AccountsFileId,
}

pub type AccountStorageMap = DashMap<Slot, AccountStorageReference, BuildNoHashHasher<Slot>>;
pub type AccountStorageMap = DashMap<Slot, Arc<AccountStorageEntry>, BuildNoHashHasher<Slot>>;
roryharr marked this conversation as resolved.
Show resolved Hide resolved

#[derive(Default, Debug)]
pub struct AccountStorage {
Expand All @@ -28,7 +19,7 @@ pub struct AccountStorage {
/// while shrink is operating on a slot, there can be 2 append vecs active for that slot
/// Once the index has been updated to only refer to the new append vec, the single entry for the slot in 'map' can be updated.
/// Entries in 'shrink_in_progress_map' can be found by 'get_account_storage_entry'
shrink_in_progress_map: DashMap<Slot, Arc<AccountStorageEntry>, BuildNoHashHasher<Slot>>,
shrink_in_progress_map: AccountStorageMap,
}

impl AccountStorage {
Expand Down Expand Up @@ -56,9 +47,9 @@ impl AccountStorage {
store_id: AccountsFileId,
) -> Option<Arc<AccountStorageEntry>> {
let lookup_in_map = || {
self.map
.get(&slot)
.and_then(|r| (r.id == store_id).then_some(Arc::clone(&r.storage)))
self.map.get(&slot).and_then(|entry| {
(entry.value().id() == store_id).then_some(Arc::clone(entry.value()))
})
};

lookup_in_map()
Expand Down Expand Up @@ -92,8 +83,8 @@ impl AccountStorage {
) {
assert_eq!(storage.slot(), slot);
if let Some(mut existing_storage) = self.map.get_mut(&slot) {
assert_eq!(slot, existing_storage.value().storage.slot());
existing_storage.value_mut().storage = storage;
assert_eq!(slot, existing_storage.value().slot());
*existing_storage.value_mut() = storage;
}
}

Expand All @@ -102,7 +93,7 @@ impl AccountStorage {
&self,
slot: Slot,
) -> Option<Arc<AccountStorageEntry>> {
self.map.get(&slot).map(|entry| Arc::clone(&entry.storage))
self.map.get(&slot).map(|entry| Arc::clone(entry.value()))
}

pub(crate) fn all_slots(&self) -> Vec<Slot> {
Expand Down Expand Up @@ -135,7 +126,7 @@ impl AccountStorage {
shrink_can_be_active: bool,
) -> Option<Arc<AccountStorageEntry>> {
assert!(shrink_can_be_active || self.shrink_in_progress_map.is_empty());
self.map.remove(slot).map(|(_, entry)| entry.storage)
self.map.remove(slot).map(|(_, storage)| storage)
}

/// iterate through all (slot, append-vec)
Expand All @@ -149,16 +140,7 @@ impl AccountStorage {
self.no_shrink_in_progress(),
"self.no_shrink_in_progress(): {slot}"
);
assert!(self
.map
.insert(
slot,
AccountStorageReference {
id: store.id(),
storage: store,
}
)
.is_none());
assert!(self.map.insert(slot, store).is_none());
}

/// called when shrinking begins on a slot and append vec.
Expand All @@ -173,12 +155,10 @@ impl AccountStorage {
new_store: Arc<AccountStorageEntry>,
) -> ShrinkInProgress<'_> {
let shrinking_store = Arc::clone(
&self
.map
self.map
.get(&slot)
.expect("no pre-existing storage for shrinking slot")
.value()
.storage,
.value(),
);

// insert 'new_store' into 'shrink_in_progress_map'
Expand Down Expand Up @@ -219,7 +199,7 @@ impl AccountStorage {
.iter()
.filter_map(|entry| {
let slot = entry.key();
let storage = &entry.value().storage;
let storage = entry.value();
predicate(slot, storage).then(|| (*slot, Arc::clone(storage)))
})
.collect()
Expand All @@ -228,7 +208,7 @@ impl AccountStorage {

/// iterate contents of AccountStorage without exposing internals
pub struct AccountStorageIter<'a> {
iter: dashmap::iter::Iter<'a, Slot, AccountStorageReference, BuildNoHashHasher<Slot>>,
iter: dashmap::iter::Iter<'a, Slot, Arc<AccountStorageEntry>, BuildNoHashHasher<Slot>>,
}

impl<'a> AccountStorageIter<'a> {
Expand All @@ -246,7 +226,7 @@ impl Iterator for AccountStorageIter<'_> {
if let Some(entry) = self.iter.next() {
let slot = entry.key();
let store = entry.value();
return Some((*slot, Arc::clone(&store.storage)));
return Some((*slot, Arc::clone(store)));
}
None
}
Expand All @@ -270,14 +250,8 @@ impl Drop for ShrinkInProgress<'_> {
assert_eq!(
self.storage
.map
.insert(
self.slot,
AccountStorageReference {
storage: Arc::clone(&self.new_store),
id: self.new_store.id()
}
)
.map(|store| store.id),
.insert(self.slot, Arc::clone(&self.new_store))
.map(|store| store.id()),
Some(self.old_store.id())
);

Expand Down Expand Up @@ -349,9 +323,7 @@ pub(crate) mod tests {
store_file_size2,
AccountsFileProvider::AppendVec,
));
storage
.map
.insert(slot, AccountStorageReference { id, storage: entry });
storage.map.insert(slot, entry);

// look in map
assert_eq!(
Expand Down Expand Up @@ -471,13 +443,7 @@ pub(crate) mod tests {
// already entry in shrink_in_progress_map
let storage = AccountStorage::default();
let sample = storage.get_test_storage();
storage.map.insert(
0,
AccountStorageReference {
id: 0,
storage: sample.clone(),
},
);
storage.map.insert(0, sample.clone());
storage.shrink_in_progress_map.insert(0, sample.clone());
storage.shrinking_in_progress(0, sample);
}
Expand All @@ -489,13 +455,7 @@ pub(crate) mod tests {
let storage = AccountStorage::default();
let sample_to_shrink = storage.get_test_storage();
let sample = storage.get_test_storage();
storage.map.insert(
0,
AccountStorageReference {
id: 0,
storage: sample_to_shrink,
},
);
storage.map.insert(0, sample_to_shrink);
let _shrinking_in_progress = storage.shrinking_in_progress(0, sample.clone());
storage.shrinking_in_progress(0, sample);
}
Expand All @@ -510,16 +470,10 @@ pub(crate) mod tests {
let id_shrunk = 0;
let sample_to_shrink = storage.get_test_storage_with_id(id_to_shrink);
let sample = storage.get_test_storage();
storage.map.insert(
slot,
AccountStorageReference {
id: id_to_shrink,
storage: sample_to_shrink,
},
);
storage.map.insert(slot, sample_to_shrink);
let shrinking_in_progress = storage.shrinking_in_progress(slot, sample.clone());
assert!(storage.map.contains_key(&slot));
assert_eq!(id_to_shrink, storage.map.get(&slot).unwrap().storage.id());
assert_eq!(id_to_shrink, storage.map.get(&slot).unwrap().id());
assert_eq!(
(slot, id_shrunk),
storage
Expand All @@ -531,7 +485,7 @@ pub(crate) mod tests {
);
drop(shrinking_in_progress);
assert!(storage.map.contains_key(&slot));
assert_eq!(id_shrunk, storage.map.get(&slot).unwrap().storage.id());
assert_eq!(id_shrunk, storage.map.get(&slot).unwrap().id());
assert!(storage.shrink_in_progress_map.is_empty());
storage.shrinking_in_progress(slot, sample);
}
Expand Down Expand Up @@ -569,13 +523,7 @@ pub(crate) mod tests {
assert!(storage
.get_account_storage_entry(slot, missing_id)
.is_none());
storage.map.insert(
slot,
AccountStorageReference {
id,
storage: sample.clone(),
},
);
storage.map.insert(slot, sample.clone());
// id is found in map
assert!(storage.get_account_storage_entry(slot, id).is_some());
assert!(storage
Expand Down Expand Up @@ -613,13 +561,7 @@ pub(crate) mod tests {
5000,
AccountsFileProvider::AppendVec,
);
storage.map.insert(
slot,
AccountStorageReference {
id,
storage: entry.into(),
},
);
storage.map.insert(slot, entry.into());
}

// look 'em up
Expand Down
10 changes: 2 additions & 8 deletions runtime/src/bank/serde_snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ mod tests {
stakes::{SerdeStakesToStakeFormat, Stakes, StakesEnum},
},
solana_accounts_db::{
account_storage::{AccountStorageMap, AccountStorageReference},
account_storage::AccountStorageMap,
accounts_db::{
get_temp_accounts_paths, AccountStorageEntry, AccountsDb, AccountsDbConfig,
AtomicAccountsFileId, ACCOUNTS_DB_CONFIG_FOR_TESTING,
Expand Down Expand Up @@ -79,13 +79,7 @@ mod tests {
num_accounts,
);
next_append_vec_id = next_append_vec_id.max(new_storage_entry.id());
storage.insert(
new_storage_entry.slot(),
AccountStorageReference {
id: new_storage_entry.id(),
storage: Arc::new(new_storage_entry),
},
);
storage.insert(new_storage_entry.slot(), Arc::new(new_storage_entry));
}

Ok(StorageAndNextAccountsFileId {
Expand Down
10 changes: 2 additions & 8 deletions runtime/src/serde_snapshot/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ mod serde_snapshot_tests {
log::info,
rand::{thread_rng, Rng},
solana_accounts_db::{
account_storage::{AccountStorageMap, AccountStorageReference},
account_storage::AccountStorageMap,
accounts::Accounts,
accounts_db::{
get_temp_accounts_paths, test_utils::create_test_accounts, AccountStorageEntry,
Expand Down Expand Up @@ -159,13 +159,7 @@ mod serde_snapshot_tests {
num_accounts,
);
next_append_vec_id = next_append_vec_id.max(new_storage_entry.id());
storage.insert(
new_storage_entry.slot(),
AccountStorageReference {
id: new_storage_entry.id(),
storage: Arc::new(new_storage_entry),
},
);
storage.insert(new_storage_entry.slot(), Arc::new(new_storage_entry));
}

Ok(StorageAndNextAccountsFileId {
Expand Down
14 changes: 6 additions & 8 deletions runtime/src/snapshot_utils/snapshot_storage_rebuilder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use {
},
regex::Regex,
solana_accounts_db::{
account_storage::{AccountStorageMap, AccountStorageReference},
accounts_db::{AccountStorageEntry, AccountsFileId, AtomicAccountsFileId},
account_storage::AccountStorageMap,
accounts_db::{AccountsFileId, AtomicAccountsFileId},
accounts_file::StorageAccess,
},
solana_nohash_hasher::BuildNoHashHasher,
Expand Down Expand Up @@ -342,10 +342,9 @@ impl SnapshotStorageRebuilder {
)?,
};

Ok((storage_entry.id(), storage_entry))
Ok(storage_entry)
})
.collect::<Result<HashMap<AccountsFileId, Arc<AccountStorageEntry>>, SnapshotError>>(
)?;
.collect::<Result<Vec<_>, SnapshotError>>()?;

if slot_stores.len() != 1 {
return Err(SnapshotError::RebuildStorages(format!(
Expand All @@ -355,10 +354,9 @@ impl SnapshotStorageRebuilder {
}
// SAFETY: The check above guarantees there is one item in slot_stores,
// so `.next()` will always return `Some`
let (id, storage) = slot_stores.into_iter().next().unwrap();
let storage = slot_stores.into_iter().next().unwrap();

self.storage
.insert(slot, AccountStorageReference { id, storage });
self.storage.insert(slot, storage);
Ok(())
}

Expand Down
Loading