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

Add CLI args for AccountsDb thread pools to validator #3281

Merged
merged 6 commits into from
Oct 25, 2024

Conversation

steviez
Copy link

@steviez steviez commented Oct 23, 2024

Done: This PR is on top of #3270 and should ship after it.
Done: Rebase on top of #3233 after it ships

Problem

See #105 - we want a way to control the size of all thread pools

Summary of Changes

  • Add args for the following thread pools:
    • AccountsDb clean
    • AccountsDb "foreground"
    • Accounts Index

Copy link
Author

@steviez steviez left a comment

Choose a reason for hiding this comment

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

I have other commits to add the other two threadpools (hash and "tx processing"), but left them off to simplify the diff while we figure a route forward. I do want to get the account index num threads hooked up as well

accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
@steviez steviez force-pushed the val_acc_db_thread_args branch 2 times, most recently from 4f2e788 to 4596af9 Compare October 24, 2024 19:58
@steviez steviez changed the title Add hidden CLI arg to control number of AccountsDb clean threads Add (hidden) CLI args for AccountsDb thread pools Oct 24, 2024
@steviez steviez changed the title Add (hidden) CLI args for AccountsDb thread pools Add CLI args for AccountsDb thread pools Oct 24, 2024
@steviez steviez changed the title Add CLI args for AccountsDb thread pools Add CLI args for AccountsDb thread pools to validator Oct 24, 2024
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.

Code looks good! Just a rename request:

accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
Copy link
Author

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Just did a series of rebases to clean up the commit history; happy with how minimal these commits are after the handful of cleanup PR's that came before them

I think the logic is straight forward, but I would appreciate thoughts on the "process" pool naming. This is the one that I think I've heard y'all call "foreground" before. I wasn't sure if we wanted to bubble foreground all the way up tho.

I intentionally used "foreground" on the more hidden function, and "process" for AccountsDbConfig and the CLI; not sure which way we'll go but I anticipate having to incorporate y'alls feedback and adjusting so I left the mix in

HaoranYi
HaoranYi previously approved these changes Oct 24, 2024
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.

lgtm.

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:

Thanks for this!

@steviez steviez merged commit 51f3ba8 into anza-xyz:master Oct 25, 2024
40 checks passed
@steviez steviez deleted the val_acc_db_thread_args branch October 25, 2024 05:37
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.

3 participants