feat(runtime): implement thread affinity for runtime and dispatcher #445
Conversation
There was a problem hiding this comment.
I suggest using core_affinity (or other equivalents) rather than nix, to support more platforms.
@George-Miao Do you think we need a feature gate for this PR?
|
I think given that Will take a look into |
|
One more question tho, what behavior should we expose given an input that exceeds number of available CPUs ? |
numinnex
force-pushed
the
runtime_dispatcher_thread_affinity
branch
from
c90cb74 to
ae48b26
Compare
|
I have pushed changes that replace |
I think an error won't hurt. |
|
|
||
| thread_builder.spawn(move || { | ||
| Runtime::builder() | ||
| .with_proactor(proactor_builder) | ||
| .thread_affinity(cpus) |
There was a problem hiding this comment.
Well, you're binding all runtimes to same CPUs?
There was a problem hiding this comment.
I have changed it to provide an callback (similar to thread name), so the user can decide on the affinity strategy.
There was a problem hiding this comment.
Maybe we could provide a more sophisticated WorkerThread builder as the number of options starts to grow
There was a problem hiding this comment.
Is it OK to set default value to empty vec?
There was a problem hiding this comment.
Yes, I've added early exit inside of bind_to_cpu_set method in case of an empty collection.
numinnex
force-pushed
the
runtime_dispatcher_thread_affinity
branch
from
15a0b5e to
7530be6
Compare
numinnex
force-pushed
the
runtime_dispatcher_thread_affinity
branch
from
06f1389 to
593fae3
Compare
compio-runtime/src/runtime/mod.rs
Outdated
| @@ -433,6 +441,7 @@ impl criterion::async_executor::AsyncExecutor for &Runtime { | |||
| #[derive(Debug, Clone)] | |||
| pub struct RuntimeBuilder { | |||
| proactor_builder: ProactorBuilder, | |||
| thread_affinity: Option<Vec<usize>>, | |||
There was a problem hiding this comment.
Is there difference between None and empty vec?
There was a problem hiding this comment.
There is none, so we technically could just treat empty collection as None variant.
|
I have changed the implementation from using |
numinnex
force-pushed
the
runtime_dispatcher_thread_affinity
branch
from
b201309 to
85f2aaf
Compare
| match (ids.iter().max(), cpus.iter().max()) { | ||
| (Some(max_id), Some(max_cpu)) if *max_cpu > *max_id => { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::InvalidInput, | ||
| format!("CPU ID: {max_cpu} exceeds maximum available CPU ID: {max_id}"), | ||
| )); | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
I mean, I am not sure whether such error is needed anymore tho... Since the set intersection will result in dropping the outliers, but at the same time, there will be no feedback to the end user what they are doing wrong...
numinnex
force-pushed
the
runtime_dispatcher_thread_affinity
branch
2 times, most recently
from
d91ac7f to
c3ea248
Compare
numinnex
force-pushed
the
runtime_dispatcher_thread_affinity
branch
from
c3ea248 to
a390c77
Compare
|
I don't understand why this test fails on MacOS and sadly I don't own one to test it locally :( but running those on my Linux setup works fine. |
This PR adds an extra field to both
RuntimeandDispatcherbuilder that allows user to set thread affinity.