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

Attempt to skip redundant startup account verification #26999

Merged

Conversation

bw-solana
Copy link
Contributor

@bw-solana bw-solana commented Aug 8, 2022

Problem

We potentially run accounts hash/capitalization verification from two places at almost the same time, which seems wasteful and appears to slow down startup by ~30 seconds (Takes ~2 minutes on current master and ~1.5 minutes after this change). Here is the call stack for each:
image

Summary of Changes

Wait for any existing verifications to complete. Only check capitalization from process_blockstore_from_root in the event we loaded frozen bank forks

@bw-solana bw-solana force-pushed the redundant_account_hash_verify branch from c5705fe to 74f4ec3 Compare August 12, 2022 22:16
@steviez
Copy link
Contributor

steviez commented Aug 12, 2022

Only check capitalization from process_blockstore_from_root in the event we loaded frozen bank forks

I think this is right / a valid adjustment to make:

  • Unpacking the snapshot will check cap. on the snapshot slot
  • So, we only need to check cap again on BankForks::root_bank() in the scenario that the root bank has been updated in load_frozen_forks()

Hypothetically, we could have a SlotMeta for snapshot slot, but not be able to process any further (ie incomplete slots). Something you could do to cover this case would be to

  • Grab the new root_bank after calling load_frozen_forks()
  • Run the cap. check only if new_root_bank.slot() != start_slot, recalling that start_slot is the slot from root_bank at the start of things

instead of moving the cap. check inside the if let Some(start_meta_slot) ... block

@bw-solana bw-solana marked this pull request as ready for review August 12, 2022 23:31
@bw-solana bw-solana requested a review from steviez August 12, 2022 23:31
steviez
steviez previously approved these changes Aug 15, 2022
@jeffwashington
Copy link
Contributor

I'd like to separate out the wait for the bg hash calc:
#27145
This keeps the 2 issues separate and makes both prs simple.

@mergify mergify bot dismissed steviez’s stale review August 15, 2022 16:59

Pull request has been modified.

@jeffwashington jeffwashington requested a review from carllin August 15, 2022 18:15
@jeffwashington
Copy link
Contributor

@carllin I roped you in.

@bw-solana bw-solana force-pushed the redundant_account_hash_verify branch from 64112f6 to 0fc2ee0 Compare August 15, 2022 18:21
@carllin
Copy link
Contributor

carllin commented Aug 24, 2022

Yeah this seems fine, good catch! The redundant check in blockstore_processor was added here: #11927

at which time the existing snapshot verification also already existed https://github.com/ryoqun/solana/blob/95cb371ecd6ba8dfb0527ee4e828081c1d0e937f/runtime/src/snapshot_utils.rs#L591

@bw-solana bw-solana merged commit 5150877 into solana-labs:master Aug 25, 2022
@bw-solana bw-solana deleted the redundant_account_hash_verify branch August 25, 2022 16:30
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