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

Jump to bottom

[TieredStorage] Allow HotStorage to handle more account data #34155

Merged
merged 1 commit into from
Dec 1, 2023
Merged

[TieredStorage] Allow HotStorage to handle more account data #34155

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
13 changes: 11 additions & 2 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 @@ -36,6 +36,10 @@ const MAX_HOT_PADDING: u8 = 7;
/// The maximum allowed value for the owner index of a hot account.
const MAX_HOT_OWNER_OFFSET: OwnerOffset = OwnerOffset((1 << 29) - 1);

/// The multiplier for converting AccountOffset to the internal hot account
/// offset. This increases the maximum size of a hot accounts file.
const HOT_ACCOUNT_OFFSET_MULTIPLIER: usize = 8;

#[bitfield(bits = 32)]
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq)]
Expand Down Expand Up @@ -228,7 +232,9 @@ impl HotStorageReader {
&self,
account_offset: AccountOffset,
) -> TieredStorageResult<&HotAccountMeta> {
let (meta, _) = get_type::<HotAccountMeta>(&self.mmap, account_offset.block as usize)?;
let internal_account_offset = account_offset.block as usize * HOT_ACCOUNT_OFFSET_MULTIPLIER;
Copy link
Contributor

Choose a reason for hiding this comment

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

To me this further confirms why I think AccountOffset should be Hot and Cold specific.

With this PR, we'd be putting implementation details to two places: (1) in the index file that defines AccountOffset, and then another here that needs to say "oh wait, but if we're a Hot storage, we treat the value differently".

I'd rather this implementation detail be in a single place. Wdyt?

Copy link
Contributor Author

@yhchiang-sol yhchiang-sol Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Offline discussed with @brooksprumo. We will use different offset types for hot and cold. And I will create a separate PR for making the current AccountOffset to HotAccountOffset.


let (meta, _) = get_type::<HotAccountMeta>(&self.mmap, internal_account_offset)?;
Ok(meta)
}

Expand Down Expand Up @@ -468,7 +474,10 @@ pub mod tests {
.map(|meta| {
let prev_offset = current_offset;
current_offset += file.write_type(meta).unwrap() as u32;
AccountOffset { block: prev_offset }
assert_eq!(prev_offset % HOT_ACCOUNT_OFFSET_MULTIPLIER as u32, 0);
AccountOffset {
block: prev_offset / HOT_ACCOUNT_OFFSET_MULTIPLIER as u32,
}
})
.collect();
// while the test only focuses on account metas, writing a footer
Expand Down