solana icon indicating copy to clipboard operation
solana copied to clipboard

Introduce primitive threading in unified scheduler

Open ryoqun opened this issue 1 year ago • 6 comments

(no need to start to review this at weekend).

Problem

There's no threading support in the unified scheduler, which will exhibit an extremely minimized overhead for unbatched processing.

Summary of Changes

Add minimum threading model and wire it, which still lacks suspending (and resuming) threads and aborting them on transaction error in dead blocks. Also, current impl is still full of .unwrap()s.

also, note that the actual scheduling code will come in later prs. after that, the code in this pr is ready for multi threaded with proper scheduling code. yet, it's still single threaded at the time of this pr. all this breakdowns are done for bite-sized pr reviewing.

Also, patching crossbeam is planned but yet not included in this pr as well.

Context

extracted from: https://github.com/solana-labs/solana/pull/33070

ryoqun avatar Jan 06 '24 14:01 ryoqun

Codecov Report

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

Comparison is base (e2c2029) 81.7% compared to head (afd580e) 81.7%. Report is 22 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #34676    +/-   ##
========================================
  Coverage    81.7%    81.7%            
========================================
  Files         825      825            
  Lines      223261   223447   +186     
========================================
+ Hits       182605   182778   +173     
- Misses      40656    40669    +13     

codecov[bot] avatar Jan 06 '24 15:01 codecov[bot]

@apfitzge thanks for timely review! i think I've addressed all your review comments.

ryoqun avatar Jan 09 '24 16:01 ryoqun

@apfitzge thanks for in-depth code review! it took time at my side, but i think it's ready for another review round from you.

also, i clarified comments a bit from the prev review round: https://github.com/solana-labs/solana/pull/34676/commits/d6093e1447fed735a5ba193ea454dfa2cd29c8b7

thanks as always.

if possible, i want to land this pr soon before the feature freeze: https://discord.com/channels/428295358100013066/910937142182682656/1194783158676230268

ryoqun avatar Jan 12 '24 15:01 ryoqun

@apfitzge thanks for review! I've replied to you: https://github.com/solana-labs/solana/pull/34676#discussion_r1457609123

also, i pushed new commit: https://github.com/solana-labs/solana/pull/34676/commits/cc5a1f071fd95bdf624308936b509856ce8aee4b and rebase was needed to pick 0e8f2de1da2e38451dfd4cc45aa51d10dfe428f1

ryoqun avatar Jan 18 '24 15:01 ryoqun

Just a couple of minor comments, feel free to just do them as follow-up PRs. Appreciate all the back-and-forth on the PR!

thanks for timely review as always. according to the discord talk, this pr is blocked (https://discord.com/channels/428295358100013066/910937142182682656/1197716176764153966) and i just addressed nits meanwhile and re-requested another code review.

ryoqun avatar Jan 19 '24 15:01 ryoqun

Just a couple of minor comments, feel free to just do them as follow-up PRs. Appreciate all the back-and-forth on the PR!

thanks for timely review as always. according to the discord talk, this pr is blocked (discord.com/channels/428295358100013066/910937142182682656/1197716176764153966) and i just addressed nits meanwhile and re-requested another code review.

Sorry my slowness made us miss the cutoff!

apfitzge avatar Jan 19 '24 17:01 apfitzge

Sorry my slowness made us miss the cutoff!

well, i don't think so. as i said above, your reviews were very timely. missing the cutoff was mainly due to my hacking and benching, resulting in extended back-and-forth..

seems v1.18 branching is imminent (https://discord.com/channels/428295358100013066/910937142182682656/1199023180598218803), so i just humbly request another approval other then a normal comment. :)

ryoqun avatar Jan 23 '24 14:01 ryoqun