Prioritize blocked task messaging over idle tasks #627

ryoqun · 2024-04-06T14:28:17Z

Problem

Unified scheduler is slow. ;)

Summary of Changes

Add very primitive (under 100 loc pr) yet effective adaptive behavioral heuristic (i.e. no constant knob) at the scheduler layer to complement SchedulingStateMachine's buffer bloat ignorance.

EDIT: phew just wrote the commentary. the irony is that the accompanied unit test took longer time to write than the actual code changes. Moreover the commented monologue prose took even longer time to write than the unit test.. lol

Perf numbers

at least ~5% consistent perf improvement is observed

(see solana-labs#35286 (comment) if you want to reproduce the results)

before (just after #129 is merged):

ledger processed in 21 seconds, 954 ms
ledger processed in 21 seconds, 643 ms
ledger processed in 21 seconds, 895 ms

after:

ledger processed in 20 seconds, 264 ms
ledger processed in 19 seconds, 983 ms
ledger processed in 20 seconds, 16 ms

(for the record) the merged commit

before(ad316fd):

ledger processed in 21 seconds, 75 ms
ledger processed in 21 seconds, 92 ms
ledger processed in 21 seconds, 64 ms

after(fb465b3):

ledger processed in 19 seconds, 490 ms
ledger processed in 19 seconds, 277 ms
ledger processed in 19 seconds, 281 ms

context: extracted from #593

ryoqun · 2024-04-06T15:15:41Z

unified-scheduler-pool/src/lib.rs

+                    recv(idle_task_receiver) -> task => {
+                        if let Ok(task) = task {
+                            (task, &finished_idle_task_sender)
                        } else {
+                            idle_task_receiver = never();
                            continue;
                        }
                    },


here (back ref: solana-labs#34676 (comment))

unified-scheduler-pool/src/lib.rs

codecov-commenter · 2024-04-06T16:11:36Z

Codecov Report

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

Project coverage is 81.8%. Comparing base (c207274) to head (1261092).
Report is 124 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #627   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         851      851           
  Lines      230236   230326   +90     
=======================================
+ Hits       188535   188609   +74     
- Misses      41701    41717   +16

ryoqun · 2024-04-08T07:05:39Z

unified-scheduler-pool/src/lib.rs

+                    recv(idle_task_receiver) -> task => {
+                        if let Ok(task) = task {
+                            (task, &finished_idle_task_sender)
+                        } else {
+                            idle_task_receiver = never();
+                            continue;
+                        }
+                    },


here (back ref: solana-labs#34676 (comment))

unified-scheduler-pool/src/lib.rs

apfitzge · 2024-04-15T16:35:42Z

unified-scheduler-pool/src/lib.rs

+                    recv(blocked_task_receiver.for_select()) -> message => {
+                        if let Some(task) = blocked_task_receiver.after_select(message.unwrap()) {
+                            (task, &finished_blocked_task_sender)
                        } else {
                            continue;
                        }
                    },
+                    recv(idle_task_receiver) -> task => {
+                        if let Ok(task) = task {
+                            (task, &finished_idle_task_sender)
+                        } else {
+                            idle_task_receiver = never();
+                            continue;
+                        }
+                    },


I'm not convinced that the multi-channel set up works correctly without select_biased!.

Let's say we have something along the lines of:

blocked_task_sender => new context

idle_task_sender => idle tasks

The idle tasks should be for the new context, but as far as I can tell, there's nothing preventing them from being randomly picked up before the new context in the handler threads.

I do believe this will work with a select_biased! call and the proper ordering, but with the random selecting it seems like there's random chance of the chained channel stuff getting messed up wrt to the idle tasks.
Maybe I am missing something?

If I am wrong about this, it'd be great to add a test to convince me 😄

You're totally correct. You saved me. This is a race condition...: f02ebd8, 4e045d7

I do believe this will work with a select_biased! call and the proper ordering

Well, this won't work even with select_biased!... It'll firstly try to receive blocked, then idle. After that, it'll sched_yield. Before the sched_yeild, the handler thread still could see the idle task for the next context, if it became visible to the thread just after trying to receive blocked and missed to see new context. this means scheduler thread managed to send the new context then the context's idle task successively between the two try_recvs.

I think this is the root cause of a mysterious panic, which i only observed once while running against mb...

apfitzge

see above comment about potential issue with chained channels and the idle task senders

ryoqun · 2024-04-16T17:39:48Z

see above comment about potential issue with chained channels and the idle task senders

I'm really grateful for you to catch the issue.. Still ci is pending, but i think i've fixed it...

apfitzge · 2024-04-17T15:11:29Z

Hey was reviewing this again, and had some concerns about the ChainedChannel design in general. After digging, I do think the impl is working now, but I think the way it's written are a bit confusing. The big select! makes things seem potentially racy since there's, at least to me, an implicit assumption that any of the branches could happen whenever. Then the danger comes when we have an OpenSubchannel. Logically, this should be the "event" that kicks off the rest of the process until we are finished.

I think making the order of things more explicit can help clarify the code, wdyt?
Something like:

loop {
    let NewTaskPayload::OpenSubchannel(new_context) = new_task_receiver.recv() else {
		// handle logical error.
	};
	
	loop {
		// your select! loop here, but with the OpenSubChannel stuff being removed.
	}
}

This code makes it more clear we expect to open a subchannel, loop until we are finished, then eventually open another new one.
It makes it easier to reason about, since we do not expect a new subchannel to be opened when we have any outstanding tasks.

ryoqun · 2024-04-17T16:11:09Z

Hey was reviewing this again

thanks as always for taking a deep look.

After digging, I do think the impl is working now, but I think the way it's written are a bit confusing. The big select! makes things seem potentially racy since there's, at least to me, an implicit assumption that any of the branches could happen whenever. Then the danger comes when we have an OpenSubchannel. Logically, this should be the "event" that kicks off the rest of the process until we are finished.

Good point. This understanding is correct.

Something like:

...

This code makes it more clear we expect to open a subchannel, loop until we are finished, then eventually open another new one. It makes it easier to reason about, since we do not expect a new subchannel to be opened when we have any outstanding tasks.

done: 6904938

I think this incurs additional recv per session. but i think this is a good compromise to improve readability.

ryoqun · 2024-04-17T16:17:22Z

unified-scheduler-pool/src/lib.rs

+                            None
+                        );
+                    } else {
+                        unreachable!();


like bunch of .unwrap()s here and there, this unreachable!() will be replaced with proper thread shutdown code in later prs.

apfitzge · 2024-04-17T17:55:27Z

I think this incurs additional recv per session. but i think this is a good compromise to improve readability.

Not sure I understand where the additional recv per session is happening, could you give an example? I thought this would just be moving the initial recv, which is effectively assumed to be a opensubchannel, but not causing an additional one.

ryoqun · 2024-04-18T04:13:58Z

I think this incurs additional recv per session. but i think this is a good compromise to improve readability.

Not sure I understand where the additional recv per session is happening, could you give an example? I thought this would just be moving the initial recv, which is effectively assumed to be a opensubchannel, but not causing an additional one.

yeah, that's correct. i was wrong... Actually, this commit results in fewer try_recv operation inside select!, actually. That's because the conceptual initial loop iteration is now only operating with the new_task_receiver, ignoring others.

* Prioritize blocked task messaging over idle tasks * Add test_scheduler_schedule_execution_blocked * Add commentary * Reword commentary a bit * Document not-chosen approach in detail * Use {crossbeam,chained}_channel::unbounded() * Add test for race condition around SchedulingContext * Fix race condition around SchedulingContext * Comment about finished channels * Clean up the scheduling context race test * Codify hidden assumption with extra 1 recv/session (EDIT there's actually no extra recv)

Prioritize blocked task messaging over idle tasks

3d1be8d

ryoqun commented Apr 6, 2024

View reviewed changes

ryoqun mentioned this pull request Apr 6, 2024

Introduce primitive threading in unified scheduler solana-labs/solana#34676

Merged

ryoqun commented Apr 6, 2024

View reviewed changes

unified-scheduler-pool/src/lib.rs Show resolved Hide resolved

ryoqun force-pushed the blocked-task-prioritization branch 4 times, most recently from 4866afa to 763a443 Compare April 8, 2024 04:03

Add test_scheduler_schedule_execution_blocked

43a1bf2

ryoqun force-pushed the blocked-task-prioritization branch 3 times, most recently from 4b91987 to 670682c Compare April 8, 2024 06:53

Add commentary

654a8c8

ryoqun force-pushed the blocked-task-prioritization branch from 670682c to 654a8c8 Compare April 8, 2024 06:54

ryoqun marked this pull request as ready for review April 8, 2024 07:01

ryoqun requested a review from apfitzge April 8, 2024 07:01

ryoqun commented Apr 8, 2024

View reviewed changes

Reword commentary a bit

9d1e074

apfitzge reviewed Apr 12, 2024

View reviewed changes

unified-scheduler-pool/src/lib.rs Show resolved Hide resolved

unified-scheduler-pool/src/lib.rs Show resolved Hide resolved

ryoqun force-pushed the blocked-task-prioritization branch 3 times, most recently from 70cd44c to b4724d1 Compare April 14, 2024 02:38

Document not-chosen approach in detail

fa918be

ryoqun force-pushed the blocked-task-prioritization branch from b4724d1 to fa918be Compare April 14, 2024 02:39

Use {crossbeam,chained}_channel::unbounded()

0dca5de

ryoqun requested a review from apfitzge April 15, 2024 15:07

apfitzge reviewed Apr 15, 2024

View reviewed changes

unified-scheduler-pool/src/lib.rs Show resolved Hide resolved

apfitzge reviewed Apr 15, 2024

View reviewed changes

apfitzge requested changes Apr 15, 2024

View reviewed changes

ryoqun added 3 commits April 17, 2024 01:49

Add test for race condition around SchedulingContext

f02ebd8

Fix race condition around SchedulingContext

4e045d7

Comment about finished channels

43ec2af

ryoqun mentioned this pull request Apr 16, 2024

New exec code path rebased solana-labs/solana#33070

Closed

8 tasks

ryoqun requested a review from apfitzge April 16, 2024 17:38

Clean up the scheduling context race test

1261092

ryoqun force-pushed the blocked-task-prioritization branch from 3543a21 to 1261092 Compare April 17, 2024 07:05

Codify hidden assumption with extra 1 recv/session

6904938

ryoqun commented Apr 17, 2024

View reviewed changes

apfitzge approved these changes Apr 17, 2024

View reviewed changes

ryoqun merged commit fb465b3 into anza-xyz:master Apr 18, 2024
38 checks passed

ryoqun mentioned this pull request May 21, 2024

Fine-tune unified scheduler loops by select_biased #1437

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize blocked task messaging over idle tasks #627

Prioritize blocked task messaging over idle tasks #627

ryoqun commented Apr 6, 2024 •

edited

Loading

ryoqun Apr 6, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024 •

edited

Loading

ryoqun Apr 8, 2024

apfitzge Apr 15, 2024

apfitzge Apr 15, 2024

ryoqun Apr 16, 2024 •

edited

Loading

apfitzge left a comment

ryoqun commented Apr 16, 2024 •

edited

Loading

apfitzge commented Apr 17, 2024

ryoqun commented Apr 17, 2024

ryoqun Apr 17, 2024 •

edited

Loading

apfitzge commented Apr 17, 2024

ryoqun commented Apr 18, 2024 •

edited

Loading

Prioritize blocked task messaging over idle tasks #627

Prioritize blocked task messaging over idle tasks #627

Conversation

ryoqun commented Apr 6, 2024 • edited Loading

Problem

Summary of Changes

Perf numbers

(for the record) the merged commit

ryoqun Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 6, 2024 • edited Loading

Codecov Report

ryoqun Apr 8, 2024

Choose a reason for hiding this comment

apfitzge Apr 15, 2024

Choose a reason for hiding this comment

apfitzge Apr 15, 2024

Choose a reason for hiding this comment

ryoqun Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

ryoqun commented Apr 16, 2024 • edited Loading

apfitzge commented Apr 17, 2024

ryoqun commented Apr 17, 2024

ryoqun Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

apfitzge commented Apr 17, 2024

ryoqun commented Apr 18, 2024 • edited Loading

ryoqun commented Apr 6, 2024 •

edited

Loading

ryoqun Apr 6, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024 •

edited

Loading

ryoqun Apr 16, 2024 •

edited

Loading

ryoqun commented Apr 16, 2024 •

edited

Loading

ryoqun Apr 17, 2024 •

edited

Loading

ryoqun commented Apr 18, 2024 •

edited

Loading