[TieredStorage] Optimize the storage size of HotAccountMeta from 16 bytes to 8 bytes by yhchiang-sol · Pull Request #146 · anza-xyz/agave

yhchiang-sol · 2024-03-08T05:03:00Z

Problem

Lamports-balance requires a u64 to fully represent its value range.
However, most lamports-balance can fit in fewer bits (> 95% can fit in 25 bits).
This finding allows us to utilize the reserved bits inside AccountMetaFlags
to store lamports-balance, or as an optional field when it cannot be represented
using the remaining bits inside AccountMetaFlags.

Summary of Changes

This PR moves the lamports field from HotAccountMeta to AccountsMetaFlags.
For more than 95% of the accounts, this PR can save 8 bytes per account.

Test Plan

Added new tests to cover zero lamports, lamports inside meta, and lamports inside optional fields.
Update existing tests to cover all cases.

codecov-commenter · 2024-03-08T07:38:50Z

Codecov Report

Attention: Patch coverage is 99.40120% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (69b6d5a) to head (5a37959).

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #146     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         836      836             
  Lines      226629   226728     +99     
=========================================
+ Hits       185650   185726     +76     
- Misses      40979    41002     +23

yhchiang-sol

Here're some tips to make the PR review easier:

yhchiang-sol · 2024-03-08T09:32:22Z

+    /// this fewer-than-u64 lamports info stores lamports that can fit
+    /// within its limitation, or a bit indicating the lamport is stored
+    /// separately as an optional field.
+    ///
+    /// Note that the number of bits using in this field must match
+    /// the const LAMPORTS_INFO_BITS.
+    pub lamports_info: B30,
 }

+/// The number of bits used in lamports_info field.
+/// Note that this value must match the bits in AccountMetaFlags::lamports_info.
+pub const LAMPORTS_INFO_BITS: u64 = 30;
+/// The max lamports balance that the lamports_info field can handle.
+/// Any lamports beyond this value will be stored separately in optional fields.
+pub const LAMPORTS_INFO_MAX_BALANCE: u64 =
+    ((1u64 << LAMPORTS_INFO_BITS) - 1) - LAMPORTS_INFO_RESERVED_VALUES;
+
+/// The number of special values inside lamports_info.
+/// This const MUST be updated when adding new reserved values.
+pub const LAMPORTS_INFO_RESERVED_VALUES: u64 = 2;
+
+/// A reserved lamports_info value indicating zero-lamports balance.
+pub const LAMPORTS_INFO_IS_ZERO_BALANCE: u32 = 0;
+/// A reserved lamports_info value indicating the lamports balance is stored
+/// in optional fields.
+pub const LAMPORTS_INFO_HAS_OPTIONAL_FIELD: u32 = 1;


Here's a good place to start the PR review as it includes most of the high-level concepts.

I think the LSB == 0 should be the case for when lamports are stored here inside the flags. This allows zero lamports to be a real value of 0. And if LSB == 1, then go to the full u64 storage.

So lamports can be 29 bits, and an explicit bool can be added to the struct. This should obviate the need for LAMPORTS_INFO_RESERVED_VALUES.

nit: Can you also move these constants above the struct definition? For me I find it easier to read code where the struct impl is close to the definition. These constants make that distance larger currently.

Talked with Jeff and he explained the idea for the 30 vs 29 bits. I had it wrong in my head before, and now I understand the idea. So please disregard my previous comment about stealing a bit.

Sounds good. And I will move those constants above the struct definition.

yhchiang-sol · 2024-03-08T09:34:21Z

+    /// Whether the account has zero lamports.
+    fn has_zero_lamports(&self) -> bool;
+
+    /// Returns the balance of the lamports associated with the account
+    /// from the TieredAccountMeta, or None if the lamports is stored
+    /// inside the optional field.
+    fn lamports_from_meta(&self) -> Option<u64>;
+
+    /// Returns the balance of the lamports associated with the account
+    /// from the optional fields, or None if the lamports is stored
+    /// inside the TieredAccountMeta.
+    fn lamports_from_optional_fields(&self, _account_block: &[u8]) -> Option<u64>;


Here's the API change for the TieredStorageMeta, and TieredReadableAccount will use these APIs to implement its ReadableAccount::lamport() function.

Hmm, feels like another case there the implementation details are leaking into the interface.

I think the TieredAccountMeta trait should stick with just lamports(), and let the HotPacked Meta implementation handle where to read from.

I think the TieredAccountMeta trait should stick with just lamports(), and let the HotPacked Meta implementation handle where to read from.

It is because HotAccountMeta alone cannot provide the full picture because it does not possess the data.

One solution is to convert the current HotAccountMeta to HotAccountStoredMeta, and create a new HotAccountMeta that contains everything.

(Sounds like this would be a separate PR and this PR on top of it?)

Now they belong to HotAccount.

yhchiang-sol · 2024-03-08T09:37:28Z

+        if let Some(lamports) = self.meta.lamports_from_meta() {
+            return lamports;
+        }
+
+        // If the meta does not have the lamports, then it must inside the optional
+        // fields.
+        self.meta
+            .lamports_from_optional_fields(self.account_block)
+            .unwrap()


And, here's the updated lamports() function. Note that as we currently have 30 bits, this PR currently doesn't use the concept of storing the minimum non-zero lamports in the footer. It will be in a separate PR.

jeffwashington · 2024-03-08T19:13:59Z

I don't see a concept of 'min lamports within this storage'.

jeffwashington · 2024-03-08T19:14:06Z

This also seems like a breaking change. Do we have a version # we can increment to cause validators running previous versions to panic when they try to load the old format. We don't currently want to support mutation or backwards compatibility.

yhchiang-sol · 2024-03-08T22:33:02Z

I don't see a concept of 'min lamports within this storage'.

It's not in this PR yet. Will do it in a separate PR.

This also seems like a breaking change. Do we have a version # we can increment to cause validators running previous versions to panic when they try to load the old format. We don't currently want to support mutation or backwards compatibility.

Yes, it is a breaking change. For AccountMeta change, it will be a new AccountMetaFormat, and the code for old-format will still there.

 #[repr(u16)]
 #[derive(
     Clone,
     Copy,
     Debug,
     Default,
     Eq,
     Hash,
     PartialEq,
     num_enum::IntoPrimitive,
     num_enum::TryFromPrimitive,
 )]
 pub enum AccountMetaFormat {
     #[default]
     Hot = 0,
     HotPacked = 1,  // if we want to introduce a new one, then we will introduce a new format here.
 }

So do we want to introduce a new version here as the previous one isn't fully released to master?

jeffwashington · 2024-03-08T23:02:00Z

So do we want to introduce a new version here as the previous one isn't fully released to master?

I think we should start this practice now. There is value in making it so we don't waste time debugging old formats. It will also make it clear when we make a breaking change.

jeffwashington · 2024-03-08T23:02:32Z

and the code for old-format will still there.

do you mean we'll still have a struct in the code with the old format?

yhchiang-sol · 2024-03-08T23:33:51Z

I think we should start this practice now. There is value in making it so we don't waste time debugging old formats. It will also make it clear when we make a breaking change.

I think that's a good idea. Have updated the PR to bump the new version.

do you mean we'll still have a struct in the code with the old format?

If a format has been fully released, then we need to have a struct in the code with the old format until the format has been fully deprecated.

For this PR, I can add the old format back so that we can also practice it? Then another PR to make it deprecated?

yhchiang-sol · 2024-03-09T00:02:53Z

Offline discussed with @jeffwashington. The PR now uses the new version and keeps only the lastest version.

brooksprumo

First pass done. I think we have some opportunities here to simplify the impl.

brooksprumo · 2024-03-12T14:27:08Z

+    #[error("UnsupportedAccountMetaFormat: format is deprecated or unsupported.")]
+    UnsupportedAccountMetaFormat(),


nit: I think we can remove "deprecated", and also the () on the variant.

Suggested change

#[error("UnsupportedAccountMetaFormat: format is deprecated or unsupported.")]

UnsupportedAccountMetaFormat(),

#[error("UnsupportedAccountMetaFormat: format is unsupported.")]

UnsupportedAccountMetaFormat,

brooksprumo · 2024-03-12T14:28:49Z

 pub enum AccountMetaFormat {
    #[default]
    Hot = 0,
+    HotPacked,


Anything written to disk must have an explicit value. Don't want to let an errant refactor break everything.

Suggested change

HotPacked,

HotPacked = 1,

Aghh, my bad. Should definitely assign a value.

brooksprumo · 2024-03-12T14:33:20Z

-    /// Returns the balance of the lamports associated with the account.
-    fn lamports(&self) -> u64;
+    /// Whether the account has zero lamports.
+    fn has_zero_lamports(&self) -> bool;


nit: There's already an existing trait that does something similar:

agave/accounts-db/src/accounts_index.rs

Lines 600 to 602 in 0f1ca20

pub trait ZeroLamport {

fn is_zero_lamport(&self) -> bool;

}

We don't need to use that trait here, but can we at least use the same naming?

Suggested change

fn has_zero_lamports(&self) -> bool;

fn is_zero_lamport(&self) -> bool;

Neat! Will use ZeroLamport trait!

brooksprumo · 2024-03-12T14:38:55Z

+    /// Whether the account has zero lamports.
+    fn has_zero_lamports(&self) -> bool;
+
+    /// Returns the balance of the lamports associated with the account
+    /// from the TieredAccountMeta, or None if the lamports is stored
+    /// inside the optional field.
+    fn lamports_from_meta(&self) -> Option<u64>;
+
+    /// Returns the balance of the lamports associated with the account
+    /// from the optional fields, or None if the lamports is stored
+    /// inside the TieredAccountMeta.
+    fn lamports_from_optional_fields(&self, _account_block: &[u8]) -> Option<u64>;


Hmm, feels like another case there the implementation details are leaking into the interface.

I think the TieredAccountMeta trait should stick with just lamports(), and let the HotPacked Meta implementation handle where to read from.

brooksprumo · 2024-03-12T14:48:57Z

+    /// this fewer-than-u64 lamports info stores lamports that can fit
+    /// within its limitation, or a bit indicating the lamport is stored
+    /// separately as an optional field.
+    ///
+    /// Note that the number of bits using in this field must match
+    /// the const LAMPORTS_INFO_BITS.
+    pub lamports_info: B30,
 }

+/// The number of bits used in lamports_info field.
+/// Note that this value must match the bits in AccountMetaFlags::lamports_info.
+pub const LAMPORTS_INFO_BITS: u64 = 30;
+/// The max lamports balance that the lamports_info field can handle.
+/// Any lamports beyond this value will be stored separately in optional fields.
+pub const LAMPORTS_INFO_MAX_BALANCE: u64 =
+    ((1u64 << LAMPORTS_INFO_BITS) - 1) - LAMPORTS_INFO_RESERVED_VALUES;
+
+/// The number of special values inside lamports_info.
+/// This const MUST be updated when adding new reserved values.
+pub const LAMPORTS_INFO_RESERVED_VALUES: u64 = 2;
+
+/// A reserved lamports_info value indicating zero-lamports balance.
+pub const LAMPORTS_INFO_IS_ZERO_BALANCE: u32 = 0;
+/// A reserved lamports_info value indicating the lamports balance is stored
+/// in optional fields.
+pub const LAMPORTS_INFO_HAS_OPTIONAL_FIELD: u32 = 1;


I think the LSB == 0 should be the case for when lamports are stored here inside the flags. This allows zero lamports to be a real value of 0. And if LSB == 1, then go to the full u64 storage.

So lamports can be 29 bits, and an explicit bool can be added to the struct. This should obviate the need for LAMPORTS_INFO_RESERVED_VALUES.

brooksprumo · 2024-03-12T15:00:52Z

+    /// this fewer-than-u64 lamports info stores lamports that can fit
+    /// within its limitation, or a bit indicating the lamport is stored
+    /// separately as an optional field.
+    ///
+    /// Note that the number of bits using in this field must match
+    /// the const LAMPORTS_INFO_BITS.
+    pub lamports_info: B30,
 }

+/// The number of bits used in lamports_info field.
+/// Note that this value must match the bits in AccountMetaFlags::lamports_info.
+pub const LAMPORTS_INFO_BITS: u64 = 30;
+/// The max lamports balance that the lamports_info field can handle.
+/// Any lamports beyond this value will be stored separately in optional fields.
+pub const LAMPORTS_INFO_MAX_BALANCE: u64 =
+    ((1u64 << LAMPORTS_INFO_BITS) - 1) - LAMPORTS_INFO_RESERVED_VALUES;
+
+/// The number of special values inside lamports_info.
+/// This const MUST be updated when adding new reserved values.
+pub const LAMPORTS_INFO_RESERVED_VALUES: u64 = 2;
+
+/// A reserved lamports_info value indicating zero-lamports balance.
+pub const LAMPORTS_INFO_IS_ZERO_BALANCE: u32 = 0;
+/// A reserved lamports_info value indicating the lamports balance is stored
+/// in optional fields.
+pub const LAMPORTS_INFO_HAS_OPTIONAL_FIELD: u32 = 1;


nit: Can you also move these constants above the struct definition? For me I find it easier to read code where the struct impl is close to the definition. These constants make that distance larger currently.

brooksprumo · 2024-03-12T15:03:13Z


 impl AccountMetaFlags {
-    pub fn new_from(optional_fields: &AccountMetaOptionalFields) -> Self {
+    pub fn new_from(optional_fields: &AccountMetaOptionalFields, lamports: u64) -> Self {


I haven't fully groked all the code yet, but this does seem strange to me that we take in two lamports here. One in the optional fields, and then another one here. Feels like an opportunity to refactor and make this simpler. (And also remove/simplify the other newly added methods.)

Agree, I don't like the current approach either since I the meta, flags, and optional fields were created before we had some special packing for lamports. Let me think more about it.

#### Problem As we further optimize the HotStorageMeta in #146, there is a need for a HotAccount struct that contains all the hot account information. Meanwhile, we currently don't have plans to develop a cold account format at this moment. As a result, this makes it desirable to repurpose TieredReadableAccount to HotAccount. #### Summary of Changes Repurpose TieredReadableAccount to HotAccount. #### Test Plan Existing tiered-storage tests.

…::lamports

…rait. Have HotPacked enum value 1. Improve Error type.

yhchiang-sol · 2024-03-13T17:58:11Z

Rebased on top of master.

brooksprumo · 2024-03-22T17:38:04Z

I've removed myself as a reviewer for now. Please re-add me once it's ready again. Thanks!

…yz#218) #### Problem As we further optimize the HotStorageMeta in anza-xyz#146, there is a need for a HotAccount struct that contains all the hot account information. Meanwhile, we currently don't have plans to develop a cold account format at this moment. As a result, this makes it desirable to repurpose TieredReadableAccount to HotAccount. #### Summary of Changes Repurpose TieredReadableAccount to HotAccount. #### Test Plan Existing tiered-storage tests.

steviez · 2025-04-09T14:57:02Z

Closing this PR since it is more than 1 year old

yhchiang-sol force-pushed the ts-pack-hot-meta branch 3 times, most recently from 1183eb5 to 5bebea7 Compare March 8, 2024 06:13

yhchiang-sol commented Mar 8, 2024

View reviewed changes

yhchiang-sol requested a review from brooksprumo March 8, 2024 09:38

yhchiang-sol changed the title ~~[TieredStorage] Move lamports to AccountsMetaFlags~~ [TieredStorage] Optimize the storage size of HotAccountMeta from 16 bytes to 8 bytes Mar 11, 2024

brooksprumo reviewed Mar 12, 2024

View reviewed changes

yhchiang-sol mentioned this pull request Mar 13, 2024

[TieredStorage] Repurpose TieredReadableAccount to HotAccount #218

Merged

yhchiang-sol force-pushed the ts-pack-hot-meta branch 2 times, most recently from 3e0cd3b to 5f54148 Compare March 13, 2024 04:14

yhchiang-sol requested a review from brooksprumo March 13, 2024 17:32

yhchiang-sol added 4 commits March 13, 2024 10:56

[TieredStorage] Add AccountMetaFlags::lamports_info and OptionalField…

9bd5751

…::lamports

Bump the version of the AccountMetaFormat.

59c3e68

Forgot to remove the u64 lamports field in the HotAccountMeta

a1b4484

* Move LAMPORTS_INFO consts above AccountMetaFlags. Use ZeroLamport t…

5a37959

…rait. Have HotPacked enum value 1. Improve Error type.

yhchiang-sol force-pushed the ts-pack-hot-meta branch from 5f54148 to 5a37959 Compare March 13, 2024 17:57

yhchiang-sol marked this pull request as draft March 20, 2024 08:34

brooksprumo removed their request for review March 22, 2024 17:37

mergify Bot added the community label Mar 31, 2024

mergify Bot added the need:merge-assist label Mar 31, 2024

steviez closed this Apr 9, 2025

OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025

pre-rebase script to use --show-toplevel (anza-xyz#146)

ba49e19

		#[error("UnsupportedAccountMetaFormat: format is deprecated or unsupported.")]
		UnsupportedAccountMetaFormat(),

	fn has_zero_lamports(&self) -> bool;
	fn is_zero_lamport(&self) -> bool;

Conversation

yhchiang-sol commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Test Plan

Uh oh!

codecov-commenter commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yhchiang-sol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffwashington commented Mar 8, 2024

Uh oh!

jeffwashington commented Mar 8, 2024

Uh oh!

yhchiang-sol commented Mar 8, 2024

Uh oh!

jeffwashington commented Mar 8, 2024

Uh oh!

jeffwashington commented Mar 8, 2024

Uh oh!

yhchiang-sol commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhchiang-sol commented Mar 9, 2024

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhchiang-sol commented Mar 8, 2024 •

edited

Loading

codecov-commenter commented Mar 8, 2024 •

edited

Loading

yhchiang-sol commented Mar 8, 2024 •

edited

Loading