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.

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

disk index: move capacity to contents #31040

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

jeffwashington
Copy link
Contributor

Problem

See #30711

Summary of Changes

Move capacity to contents within each disk bucket. This allows data and index buckets to have different algorithms for growth.

Fixes #

@jeffwashington jeffwashington marked this pull request as ready for review April 4, 2023 15:11
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #31040 (3980ff4) into master (aa3e0b9) will increase coverage by 0.0%.
The diff coverage is 95.8%.

@@           Coverage Diff           @@
##           master   #31040   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         728      728           
  Lines      205737   205751   +14     
=======================================
+ Hits       167695   167719   +24     
+ Misses      38042    38032   -10     

Copy link
Contributor

@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.

Looks good. Since there's a merge conflict that needs to be resolved, I added a nit/request too.

@@ -14,7 +14,17 @@ use {

/// allocated in `contents` in a BucketStorage
pub struct BucketWithBitVec {
pub occupied: BitVec,
occupied: BitVec,
capacity_pow2: Capacity,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can this field be just capacity?

Suggested change
capacity_pow2: Capacity,
capacity: Capacity,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but soon I will distinguish them as this one has to be pow2 because it will only be data bucket. So, to make it clear what it needed to represent, I decided to leave it named the same.

brooksprumo
brooksprumo previously approved these changes Apr 4, 2023
@jeffwashington
Copy link
Contributor Author

merge conflicts caused a force push. Sorry.

@jeffwashington jeffwashington merged commit fd28fd1 into solana-labs:master Apr 4, 2023
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.

2 participants