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

Move BankHashStats from account's db to bank #3821

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

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Nov 27, 2024

Problem

We keep track of bank_hash_stats for each bank. Currently, this stats live inside a locked hash map in Accounts DB. This cause a lock contention for every store of an accounts and hurts performance.

We want to get rid of the lock in the store code path.

Summary of Changes

Move bank_hash_stats from accounts db to bank.
Introduce AtomicBankHashStats to accumulate the stats in the bank.

Fixes #

@HaoranYi HaoranYi changed the title remove lock on BankHashStat in accounts-db store Remove lock on BankHashStat in accounts-db store Nov 27, 2024
@HaoranYi HaoranYi changed the title Remove lock on BankHashStat in accounts-db store Move BankHashStats from account's db to bank Nov 27, 2024
runtime/src/bank.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db/tests.rs Show resolved Hide resolved
runtime/src/snapshot_package.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/serde_snapshot.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
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.

Looks like we're missing updating the slot's bank_hash_stats in two other places:

  1. store_stake_accounts()
  2. commit_transactions()

Both of these functions call into accounts/accountsdb directly, so they bypass the Bank::store_accounts() logic.

@HaoranYi
Copy link
Author

HaoranYi commented Dec 3, 2024

Looks like we're missing updating the slot's bank_hash_stats in two other places:

  1. store_stake_accounts()
  2. commit_transactions()

Both of these functions call into accounts/accountsdb directly, so they bypass the Bank::store_accounts() logic.

done in 284e738

@HaoranYi
Copy link
Author

HaoranYi commented Dec 3, 2024

The log for BankHashStats matched with master when running on mainnet.

  • this pr
[2024-12-03T14:34:51.113908867Z INFO  solana_runtime::bank] bank frozen: 305167208 hash: 4qzkBXbkow8KTSQ3gpgwL7oGEQVmHkMFyxRJ2qSjzF63 accounts_delta: W2cZQQKRPkkx5gGaHjMKpTCvz6SNAjnLMKw2tXtmqoG signature_count: 2131 last_blockhash: CRSrNfrT2CMC32SxTZpXs65yx4ShaxyRHo9NASL3Ew93 capitalization: 589423807506551765, stats: BankHashStats { num_updated_accounts: 6359, num_removed_accounts: 233, num_lamports_stored: 17440095079623122, total_data_len: 39964086, num_executable_accounts: 3 }
[2024-12-03T14:34:51.533962317Z INFO  solana_runtime::bank] bank frozen: 305167209 hash: 3JFoM5ceBXWhrL41Hk2igBRe4s8Qq3JALFRGdGA77VJh accounts_delta: GyymD8hD8PYWiLtzUXZNbamY77pMzqYiNVNZYk3JPrg3 signature_count: 1136 last_blockhash: J9U3bY8CwwotGynEazzEN7MUC5vnsa15AgepuGY74sbe capitalization: 589423807419181024, stats: BankHashStats { num_updated_accounts: 3790, num_removed_accounts: 113, num_lamports_stored: 6829130698133182, total_data_len: 27757127, num_executable_accounts: 5 }

  • master
[2024-12-03T14:34:50.954047096Z INFO  solana_runtime::bank] bank frozen: 305167208 hash: 4qzkBXbkow8KTSQ3gpgwL7oGEQVmHkMFyxRJ2qSjzF63 accounts_delta: W2cZQQKRPkkx5gGaHjMKpTCvz6SNAjnLMKw2tXtmqoG signature_count: 2131 last_blockhash: CRSrNfrT2CMC32SxTZpXs65yx4ShaxyRHo9NASL3Ew93 capitalization: 589423807506551765, stats: BankHashStats { num_updated_accounts: 6359, num_removed_accounts: 233, num_lamports_stored: 17440095079623122, total_data_len: 39964086, num_executable_accounts: 3 }
[2024-12-03T14:34:51.365612394Z INFO  solana_runtime::bank] bank frozen: 305167209 hash: 3JFoM5ceBXWhrL41Hk2igBRe4s8Qq3JALFRGdGA77VJh accounts_delta: GyymD8hD8PYWiLtzUXZNbamY77pMzqYiNVNZYk3JPrg3 signature_count: 1136 last_blockhash: J9U3bY8CwwotGynEazzEN7MUC5vnsa15AgepuGY74sbe capitalization: 589423807419181024, stats: BankHashStats { num_updated_accounts: 3790, num_removed_accounts: 113, num_lamports_stored: 6829130698133182, total_data_len: 27757127, num_executable_accounts: 5 }

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.

I think the logic looks correct now. We should test this against another node on mnb to ensure both produce the same bank hash stats, as these numbers are used to debug consensus mismatches.

Can you run master vs this PR and ensure the bank hash details that are logged in the "bank frozen" line are always the same? Ideally over an epoch boundary, since that'll exercise also storing the stake accounts.

Edit: Nice! I see you just did this test: #3821 (comment)

runtime/src/bank.rs Outdated Show resolved Hide resolved
@HaoranYi
Copy link
Author

HaoranYi commented Dec 3, 2024

Yes. I will keep it running and check for the slot at the next epoch boundary, which is about 1d7h from now.

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.

:shipit:

Looks good to me! Probably best to wait and confirm the stats match master across an epoch boundary.

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