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

[TieredStorage] Write owners block for HotAccountStorage #34927

Merged
merged 3 commits into from
Jan 26, 2024
Merged

[TieredStorage] Write owners block for HotAccountStorage #34927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d02f02
[TieredStorage] HotStorageReader::write_accounts() -- owners block
yhchiang-sol Jan 19, 2024
6b585a8
Introduce const OWNER_NO_OWNER. Extend the test to cover zero-lampor…
yhchiang-sol Jan 26, 2024
20110b7
Improve comment block for write_accounts.
yhchiang-sol Jan 26, 2024
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
72 changes: 53 additions & 19 deletions accounts-db/src/tiered_storage/hot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use {
index::{AccountIndexWriterEntry, AccountOffset, IndexBlockFormat, IndexOffset},
meta::{AccountMetaFlags, AccountMetaOptionalFields, TieredAccountMeta},
mmap_utils::{get_pod, get_slice},
owners::{OwnerOffset, OwnersBlockFormat},
owners::{OwnerOffset, OwnersBlockFormat, OwnersTable, OWNER_NO_OWNER},
readable::TieredReadableAccount,
StorableAccounts, StorableAccountsWithHashesAndWriteVersions, TieredStorageError,
TieredStorageFormat, TieredStorageResult,
Expand Down Expand Up @@ -495,6 +495,7 @@ impl HotStorageWriter {
fn write_account(
&self,
lamports: u64,
owner_offset: OwnerOffset,
account_data: &[u8],
executable: bool,
rent_epoch: Option<Epoch>,
Expand All @@ -511,6 +512,7 @@ impl HotStorageWriter {
let padding_len = padding_bytes(account_data.len());
let meta = HotAccountMeta::new()
.with_lamports(lamports)
.with_owner_offset(owner_offset)
.with_account_data_size(account_data.len() as u64)
.with_account_data_padding(padding_len)
.with_flags(&flags);
Expand All @@ -527,8 +529,9 @@ impl HotStorageWriter {
Ok(stored_size)
}

/// A work-in-progress function that will eventually implements
/// AccountsFile::appends_account()
/// Persists `accounts` into the underlying hot accounts file associated
/// with this HotStorageWriter. The first `skip` number of accounts are
/// *not* persisted.
pub fn write_accounts<
'a,
'b,
Expand All @@ -542,6 +545,7 @@ impl HotStorageWriter {
) -> TieredStorageResult<()> {
let mut footer = new_hot_footer();
let mut index = vec![];
let mut owners_table = OwnersTable::default();
let mut cursor = 0;

// writing accounts blocks
Expand All @@ -555,20 +559,28 @@ impl HotStorageWriter {

// Obtain necessary fields from the account, or default fields
// for a zero-lamport account in the None case.
let (lamports, data, executable, rent_epoch, account_hash) = account
let (lamports, owner, data, executable, rent_epoch, account_hash) = account
.map(|acc| {
(
acc.lamports(),
acc.owner(),
acc.data(),
acc.executable(),
// only persist rent_epoch for those non-rent-exempt accounts
(acc.rent_epoch() != Epoch::MAX).then_some(acc.rent_epoch()),
Some(*account_hash),
)
})
.unwrap_or((0, &[], false, None, None));

cursor += self.write_account(lamports, data, executable, rent_epoch, account_hash)?;
.unwrap_or((0, &OWNER_NO_OWNER, &[], false, None, None));
let owner_offset = owners_table.insert(owner);
cursor += self.write_account(
lamports,
owner_offset,
data,
executable,
rent_epoch,
account_hash,
)?;
index.push(index_entry);
}
footer.account_entry_count = (len - skip) as u32;
Expand All @@ -588,11 +600,13 @@ impl HotStorageWriter {
cursor += self.storage.write_pod(&0u32)?;
}

// TODO: owner block will be implemented in the follow-up PRs
// expect the offset of each block aligned.
// writing owners block
assert!(cursor % HOT_BLOCK_ALIGNMENT == 0);
footer.owners_block_offset = cursor as u64;
footer.owner_count = 0;
footer.owner_count = owners_table.len() as u32;
footer
.owners_block_format
.write_owners_block(&self.storage, &owners_table)?;

footer.write_footer_block(&self.storage)?;

Expand Down Expand Up @@ -1238,12 +1252,17 @@ pub mod tests {
/// Create a test account based on the specified seed.
/// The created test account might have default rent_epoch
/// and write_version.
///
/// When the seed is zero, then a zero-lamport test account will be
/// created.
fn create_test_account(seed: u64) -> (StoredMeta, AccountSharedData) {
let data_byte = seed as u8;
let owner_byte = u8::MAX - data_byte;
let account = Account {
lamports: seed + 1,
lamports: seed,
data: std::iter::repeat(data_byte).take(seed as usize).collect(),
owner: Pubkey::new_unique(),
// this will allow some test account sharing the same owner.
owner: [owner_byte; 32].into(),
executable: seed % 2 > 0,
rent_epoch: if seed % 3 > 0 {
seed
Expand Down Expand Up @@ -1312,15 +1331,30 @@ pub mod tests {
.unwrap()
.unwrap();

let (account, address, hash, _write_version) = storable_accounts.get(i);
let account = account.unwrap();
let (account, address, account_hash, _write_version) = storable_accounts.get(i);
let (lamports, owner, data, executable, account_hash) = account
.map(|acc| {
(
acc.lamports(),
acc.owner(),
acc.data(),
acc.executable(),
// only persist rent_epoch for those non-rent-exempt accounts
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please prefer "rent paying" instead of "non rent exempt". (1) this is the terminology used elsewhere, and (2) then avoids a double-negative.

(I know this is only a comment in a test, but hopefully this also informs any future code as well.)

Also note, this can be addressed in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! Will do it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have piggybacked in #34969.

Some(*account_hash),
)
})
.unwrap_or((0, &OWNER_NO_OWNER, &[], false, None));

assert_eq!(stored_meta.lamports(), account.lamports());
assert_eq!(stored_meta.data().len(), account.data().len());
assert_eq!(stored_meta.data(), account.data());
assert_eq!(stored_meta.executable(), account.executable());
assert_eq!(stored_meta.lamports(), lamports);
assert_eq!(stored_meta.data().len(), data.len());
assert_eq!(stored_meta.data(), data);
assert_eq!(stored_meta.executable(), executable);
assert_eq!(stored_meta.owner(), owner);
assert_eq!(stored_meta.pubkey(), address);
assert_eq!(stored_meta.hash(), hash);
assert_eq!(
*stored_meta.hash(),
account_hash.unwrap_or(AccountHash(Hash::default()))
);

assert_eq!(i + 1, next);
}
Expand Down
14 changes: 14 additions & 0 deletions accounts-db/src/tiered_storage/owners.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ use {
#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd)]
pub struct OwnerOffset(pub u32);

lazy_static! {
pub static ref OWNER_NO_OWNER: Pubkey = Pubkey::default();
}

/// Owner block holds a set of unique addresses of account owners,
/// and an account meta has a owner_offset field for accessing
/// it's owner address.
Expand Down Expand Up @@ -97,6 +101,16 @@ impl<'a> OwnersTable<'a> {

OwnerOffset(offset as u32)
}

/// Returns the number of unique owner addresses in the table.
pub fn len(&self) -> usize {
self.owners_set.len()
}

/// Returns true if the OwnersTable is empty
pub fn is_empty(&self) -> bool {
self.len() == 0
}
Comment on lines +110 to +113
Copy link
Contributor Author

@yhchiang-sol yhchiang-sol Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FYI: This function isn't used, but is required by the CI check as we have a len() function:


error: struct `OwnersTable` has a public `len` method, but no `is_empty` method
--
  | --> accounts-db/src/tiered_storage/owners.rs:102:5
  | \|
  | 102 \|     pub fn len(&self) -> usize {
  | \|     ^^^^^^^^^^^^^^^^^^^^^^^^^^
  | \|
  | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
  | = note: `-D clippy::len-without-is-empty` implied by `-D warnings`
  | = help: to override `-D warnings` add `#[allow(clippy::len_without_is_empty)]`

}

#[cfg(test)]
Expand Down
Loading