Add --unified-scheduler-handler-threads #35195

ryoqun · 2024-02-14T06:53:09Z

Problem

There's no way to adjust the number of threads via cli for unified scheduler because it's still single-threaded. However, this won't hold eternally. :)

Summary of Changes

Provide such a way in advance for upcoming pr, which makes the unified scheduler multi-threaded with actual scheduler logic.

`--help`

(note: both output is identical)

$ SOLANA_NO_HIDDEN_CLI_ARGS="" solana-validator --help
...
        --unified-scheduler-handler-threads <COUNT>
            Change the number of the unified scheduler's transaction execution threads dedicated to each block,
            otherwise calculated as cpu_cores/4 [default: 12]
...

$ SOLANA_NO_HIDDEN_CLI_ARGS="" solana-ledger-tool --help
...
        --unified-scheduler-handler-threads <COUNT>
            Change the number of the unified scheduler's transaction execution threads dedicated to each block,
            otherwise calculated as cpu_cores/4 [default: 12]
...

context

extracted from #33070

ryoqun · 2024-02-14T07:25:14Z

unified-scheduler-pool/src/lib.rs

+        let handler_count = handler_count.unwrap_or(1);
+        // we're hard-coding the number of handler thread to 1, meaning this impl is currently
+        // single-threaded still.
+        assert_eq!(handler_count, 1); // replace this with assert!(handler_count >= 1) later
+


so, --unified-scheduler-handler-threads is meaningless for now.

(diff would be a lot clearer if i had more foresight at #34676..)

ryoqun · 2024-02-14T12:56:54Z

ledger-tool/src/main.rs

+            Arg::with_name("unified_scheduler_handler_threads")
+                .long("unified-scheduler-handler-threads")


outside unified-scheduler-pool code, i think handler thread count is too verbose and impl-oriented naming. so, I used handler threads intentionally. Also note that there's bunch of --*-threads cli flags in solana-validator as precedents. so, ending at -treads should be no-brainer.

on the other hand, handler is left for technical rigidness, because there's additional thread scScheduler, which are created for each scheduler. and the thread doesn't count towards to unified-scheduler-handler-threads.

ryoqun · 2024-02-14T12:58:27Z

ledger-tool/src/main.rs

+            Arg::with_name("unified_scheduler_handler_threads")
+                .long("unified-scheduler-handler-threads")
+                .value_name("COUNT")
+                .takes_value(true)
+                .validator(|s| is_within_range(s, 1..))
+                .global(true)
+                .hidden(hidden_unless_forced())
+                .help(DefaultSchedulerPool::cli_message()),


the actual builder method invocation order and lack of .global(true) are the subtle differences from the solana-validator counterpart. however, this is my best attempt to be consistent within respective surrounding convention (btw, hard to see consistencies in each places; they're already inconsistent, thus this is my arbitrary best effort)...

ryoqun · 2024-02-14T13:13:36Z

unified-scheduler-pool/src/lib.rs

+
+        MESSAGE.get_or_init(|| {
+            format!(
+                "Change the number of the unified scheduler's transaction execution threads \


while i'm leaving impl jargon (handler) inside the cli flag name, I intentionally avoided to use the word handler in this cli message, instead using the phrase transaction execution threads. That's my habit for achieving both grep-ablity and rephrase-based quick introduction of unfamiliar words (= the handler in the cli flag) for human.

ryoqun · 2024-02-14T13:19:46Z

unified-scheduler-pool/src/lib.rs

+        MESSAGE.get_or_init(|| {
+            format!(
+                "Change the number of the unified scheduler's transaction execution threads \
+                 dedicated to each block, otherwise calculated as cpu_cores/4 [default: {}]",


dedicated to each block has rather important significance here to avoid misunderstanding of this number being the global thread count inside the whole solana-validator process.

also, this whole message is specifically worded to put other calculated as cpu_cores/4 at the end of this message, so that it's lexically close enough to the actual default value.

ryoqun · 2024-02-14T13:51:37Z

ledger-tool/src/main.rs

+                .long("unified-scheduler-handler-threads")
+                .value_name("COUNT")
+                .takes_value(true)
+                .validator(|s| is_within_range(s, 1..))


i thought about introducing some sensible upper bound like 1024, but opted not to do so because this flag doesn't need to be friendly that much and that restriction would hamper some pathological stress testing (again, this isn't strong point, as this is veerry niche & hypothetical use case).

ryoqun · 2024-02-14T14:07:28Z

unified-scheduler-pool/src/lib.rs

+    }
+
+    pub fn cli_message() -> &'static str {
+        static MESSAGE: OnceLock<String> = OnceLock::new();


back into the very beginnings of this looong unified scheduler journey, OnceLock hasn't been stabilized yet.. ;) ref: #30746 (review)

codecov · 2024-02-14T15:34:02Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (897adb2) 81.6% compared to head (857e952) 81.6%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #35195     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         833      833             
  Lines      224827   224863     +36     
=========================================
+ Hits       183523   183547     +24     
- Misses      41304    41316     +12

apfitzge

cli arg(s) lgtm - feels a bit odd to introduce a new CLI argument that will cause the code to break if you actually use it, even if it is hidden. I know the impl is coming shortly, so I don't want to block your work.

* Add --unified-scheduler-handler-threads * Adjust value name * Warn if the flag was ignored * Tweak message a bit

ryoqun requested a review from apfitzge February 14, 2024 06:53

Add --unified-scheduler-handler-threads

5036047

ryoqun force-pushed the unified-scheduler-cli branch from 083971a to 5036047 Compare February 14, 2024 07:24

ryoqun commented Feb 14, 2024

View reviewed changes

Adjust value name

b7ea2e0

ryoqun commented Feb 14, 2024

View reviewed changes

Warn if the flag was ignored

61dce27

ryoqun force-pushed the unified-scheduler-cli branch from a55af13 to 61dce27 Compare February 14, 2024 13:40

ryoqun commented Feb 14, 2024

View reviewed changes

Tweak message a bit

857e952

ryoqun commented Feb 14, 2024

View reviewed changes

ryoqun mentioned this pull request Feb 14, 2024

Add --block-{verification,production}-method flags (noop atm) #30746

Merged

apfitzge approved these changes Feb 21, 2024

View reviewed changes

ryoqun merged commit 024d6ec into solana-labs:master Feb 22, 2024
46 checks passed

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 27, 2024

Add --unified-scheduler-handler-threads (solana-labs#35195)

a2849df

* Add --unified-scheduler-handler-threads * Adjust value name * Warn if the flag was ignored * Tweak message a bit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --unified-scheduler-handler-threads #35195

Add --unified-scheduler-handler-threads #35195

ryoqun commented Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024

apfitzge left a comment

		Arg::with_name("unified_scheduler_handler_threads")
		.long("unified-scheduler-handler-threads")

Add --unified-scheduler-handler-threads #35195

Add --unified-scheduler-handler-threads #35195

Conversation

ryoqun commented Feb 14, 2024 • edited Loading

Problem

Summary of Changes

--help

context

ryoqun Feb 14, 2024

Choose a reason for hiding this comment

ryoqun Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

ryoqun Feb 14, 2024

Choose a reason for hiding this comment

ryoqun Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

ryoqun Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

ryoqun Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

ryoqun Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2024

Codecov Report

apfitzge left a comment

Choose a reason for hiding this comment

ryoqun commented Feb 14, 2024 •

edited

Loading

`--help`

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading

ryoqun Feb 14, 2024 •

edited

Loading