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

ShrinkCandidates only holds slot #33173

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

jeffwashington
Copy link
Contributor

@jeffwashington jeffwashington commented Sep 6, 2023

Problem

Improving startup time.
Now that we only have 1 storage per slot, it is more efficient to not hold an Arc to the append vec in the shrink list.
During index generation, especially with incremental snapshots, we can end up with say 60M duplicate pubkeys across all 432k slots. This results in a lot of record keeping and inefficient HashMap stuffing of almost every (Slot, Arc<AppendVec>). Getting the append vecs is not fast. They can be looked up in the bg thread by shrink itself. A slot is sufficient to request a shrink.

Summary of Changes

Just hold a HashSet<Slot> instead of a HashMap<Slot, Arc<AppendVec>>

Fixes #

@jeffwashington jeffwashington force-pushed the sp5 branch 2 times, most recently from cc8b30c to c4e5848 Compare September 7, 2023 00:48
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #33173 (0cbbf5d) into master (e331275) will decrease coverage by 0.1%.
The diff coverage is 98.1%.

@@            Coverage Diff            @@
##           master   #33173     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         785      785             
  Lines      211205   211194     -11     
=========================================
- Hits       173473   173420     -53     
- Misses      37732    37774     +42     

@jeffwashington jeffwashington marked this pull request as ready for review September 7, 2023 14:34
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.

Great idea!

accounts-db/src/accounts_db.rs Show resolved Hide resolved
@jeffwashington
Copy link
Contributor Author

fyi, had to rebase against the other pr that just went in. The goal of the other one was to reduce the # line changes in this one and get rid of noisy useless changes.

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.

lgtm

Comment on lines +4295 to +4297
let Some(store) = self.storage.get_slot_storage_entry(*slot) else {
continue;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jeffwashington jeffwashington merged commit 0083e42 into solana-labs:master Sep 7, 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