Skip to content

Adding an API to calculate storage size from the stored data lengths #6132

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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

roryharr
Copy link

@roryharr roryharr commented May 7, 2025

Problem

There is no API to calculate storage size of an account from the data length stored in account. This will be required for out dated account tracking.

Summary of Changes

Modified storage_size API to return data length instead.
Added API to convert from data length to storage_size.

See conversation here: https://github.com/anza-xyz/agave/pull/5950/files#r2069132522

Fixes #

@roryharr roryharr force-pushed the add_api_to_calculate_storage_size_from_data_len branch from 9d8306e to 2a02c67 Compare May 7, 2025 00:21
@roryharr roryharr force-pushed the add_api_to_calculate_storage_size_from_data_len branch from 2a02c67 to 7f68a76 Compare May 7, 2025 00:42
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.9%. Comparing base (64e19be) to head (9523e72).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #6132     +/-   ##
=========================================
- Coverage    82.9%    82.9%   -0.1%     
=========================================
  Files         842      842             
  Lines      377491   377525     +34     
=========================================
+ Hits       312968   312989     +21     
- Misses      64523    64536     +13     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@roryharr roryharr marked this pull request as ready for review May 7, 2025 04:57
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Idea looks good to me.

Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

Can we split the renaming of get_account_size function and the addition of the new function into separate PR?

@roryharr
Copy link
Author

roryharr commented May 7, 2025

Can we split the renaming of get_account_size function and the addition of the new function into separate PR?

I don't think so in this case, because the definition of the function is changing.

@brooksprumo
Copy link

FYI, you can ignore the whole "this branch is out-of-date with the base branch" thing:

Screenshot 2025-05-07 at 3 24 48 PM

No need to update the base branch unless there are merge conflicts to resolve.

@roryharr
Copy link
Author

roryharr commented May 7, 2025

FYI, you can ignore the whole "this branch is out-of-date with the base branch" thing:

Screenshot 2025-05-07 at 3 24 48 PM No need to update the base branch unless there are merge conflicts to resolve.

Had to update it I think because of this:
https://buildkite.com/anza/agave/builds/23451#0196abc8-eb19-4ecc-b569-779fc95c49c5

Unless there is a better way of resolving issues like that.

It looks like someone removed a file that had an extra blank line, so it sees my change as having added an improperly formatted file .

@brooksprumo
Copy link

Had to update it I think because of this: https://buildkite.com/anza/agave/builds/23451#0196abc8-eb19-4ecc-b569-779fc95c49c5

Unless there is a better way of resolving issues like that.

It looks like someone removed a file that had an extra blank line, so it sees my change as having added an improperly formatted file .

Gotcha, yeah if the build on master is broken, gotta rebase (or merge) to fix. I personally always manually do the rebase on the cli, but I imagine GH "update branch" works too.

Self::TieredStorage(ts) => ts
.reader()
.and_then(|reader| reader.get_account_sizes(sorted_offsets).ok())
.map_or(0, |reader| reader.calculate_stored_size(data_len)),

Choose a reason for hiding this comment

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

If we don't have a valid reader here for Tiered Storage, I think that's a programmer bug. I'd recommend we do expect. Something like:

Suggested change
.map_or(0, |reader| reader.calculate_stored_size(data_len)),
.expect("tiered storage must be ready-only")
.calculate_stored_size(data_len)

Copy link
Author

Choose a reason for hiding this comment

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

Can see what i put. It took me a while to figure out why the reader was needed at all, so I put that in the expect message.

Adding expects statement
Updating assert to assert_eq
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants