Skip to content

Make ReplayStage create the parallel fork replay threadpool#137

Merged
steviez merged 5 commits intoanza-xyz:masterfrom
steviez:rs_hold_tpool
Mar 8, 2024
Merged

Make ReplayStage create the parallel fork replay threadpool#137
steviez merged 5 commits intoanza-xyz:masterfrom
steviez:rs_hold_tpool

Conversation

@steviez
Copy link
Copy Markdown

@steviez steviez commented Mar 7, 2024

Problem

As part of work for #105, it will be desirable to make the thread pool size configurable on the CLI. The pool is currently within a lazy_static; configuring the size for that from the CLI is a little "messy". We'd need to add another function that sets a global (before the thread pool is created), and then have thread pool creation read that value. This is of course susceptible to a potential race condition and/or some annoying code to ensure that the pool comes back the size we actually wanted it to be.

Summary of Changes

Instead of using the lazy_static, just have ReplayStage create the thread pool (if parallel replay is enabled).

There will be some whitespace difference - I'd suggest either viewing with "hide whitespace", or view commit-by-commit (I ran cargo fmt in separate commits)

Comment thread core/src/replay_stage.rs
Comment on lines +3196 to +3198
if active_bank_slots.is_empty() {
return false;
}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thought that this was a simple enough sanity check to pull out and explicitly return early for - this enabled us to remove a level of indentation for everything else

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Never nesters unite

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

early returns make code more complex and easier to introduce bugs in the future. CHANGE MY MIND

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Number of bugs in a function is proportional to the maximum indent. CHANGE MY MIND

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🍿

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

early returns make code more complex and easier to introduce bugs in the future

steviez@4bcbbbe?diff=unified&w=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Number of bugs in a function is proportional to the maximum indent. CHANGE MY MIND

you're stuck in the present old man. join us in the future!

@steviez steviez requested a review from bw-solana March 7, 2024 20:07
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (85cfe23) to head (2e3cb8a).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #137   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         838      838           
  Lines      226223   226227    +4     
=======================================
+ Hits       185240   185266   +26     
+ Misses      40983    40961   -22     

Copy link
Copy Markdown

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM!

@bw-solana
Copy link
Copy Markdown

How painful is it on validators to switch up CLI args? It would be nice to replace replay_slots_concurrently with replay_forks_concurrently to better convey what we're actually doing

@steviez
Copy link
Copy Markdown
Author

steviez commented Mar 8, 2024

How painful is it on validators to switch up CLI args? It would be nice to replace replay_slots_concurrently with replay_forks_concurrently to better convey what we're actually doing

I was probably going to partially deprecate this argument anyways in favor an argument that allows for configuration of the number of threads. So, something like:

--concurrent-fork-replay-threads N

We can still accept the old flag (but issue a warning that it is deprecated), but just choose a reasonable default value for the old flag (since the old flag is a bool, not a number)

@steviez steviez merged commit bf0a368 into anza-xyz:master Mar 8, 2024
@steviez steviez deleted the rs_hold_tpool branch March 8, 2024 18:52
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
…#137)

ReplayStage owning the pool allows for subsequent work to configure
the size of the pool; configuring the size of the pool inside of the
lazy_static would have been a little messy
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
We want to have invalidator specific changes in the release branches as well.

Document the synchronization process for release branches.
Extend slow-rebase.sh to accept target branch name as an argument.
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.

5 participants