runtime: create reactor per worker #660

ghost · 2018-09-25T18:32:38Z

Motivation

Sharing one reactor among all worker threads causes a lot of contention.

Solution

This PR creates a reactor per worker thread. Each worker thread drives its own reactor when it goes to sleep.

ghost · 2018-09-25T18:33:42Z

src/runtime/mod.rs

-    /// ```
-    pub fn reactor(&self) -> &Handle {
-        self.inner().reactor.handle()
-    }


Not sure what to do about reactor() and handle() other than to remove them. Perhaps we could still keep the background reactor, but would it be worth it?

We need to keep this to maintain backwards compatibility. Could this just return a handle to the first reactor? A deprecation warning should provide enough hint to users to stop using it.

ghost · 2018-09-25T18:34:33Z

src/runtime/mod.rs

@@ -152,9 +150,6 @@ pub struct Runtime {

 #[derive(Debug)]
 struct Inner {
-    /// Reactor running on a background thread.
-    reactor: Background,
-
    /// Task execution pool.
    pool: threadpool::ThreadPool,
 }


Should I remove Inner since now it only contains the ThreadPool? Doesn't make much sense to go through .inner.pool rather than just .pool.

Yeah, it is private, so we can change it to whatever is best 👍

ghost · 2018-09-25T18:40:14Z

src/runtime/builder.rs

@@ -241,8 +241,14 @@ impl Builder {
                });
            })
            .custom_park(move |worker_id| {
+                // Create a new reactor
+                let reactor = Reactor::new().unwrap(); // TODO(stjepang): remove unwrap


This is a tricky unwrap(). I think we have two options here:

Change the signature of custom_park so that the closure returns an io::Result<P>.

Construct reactors and timers outside the threadpool builder. For that, we should probably make the inner field of WorkerId public and guarantee that IDs are always numbers in 0..core_threads.

Which one do you prefer?

I don't have much of an opinion here. My initial observations:

tokio-threadpool should be as independent as possible, changing the signature of custom_park to io::Result is requiring knowledge of the runtime needs.

Making WorkerId PartialEq<usize> or something will probably work. It will be 0..self.pool_size() set by the builder. This also is easier to do in a backwards compatible way.

carllerche

Looks good 👍 Thanks for breaking this up, it makes it much easier to review.

I provided thoughts inline to your questions.

carllerche · 2018-09-26T16:13:06Z

src/runtime/builder.rs

@@ -241,8 +241,14 @@ impl Builder {
                });
            })
            .custom_park(move |worker_id| {
+                // Create a new reactor
+                let reactor = Reactor::new().unwrap(); // TODO(stjepang): remove unwrap


I don't have much of an opinion here. My initial observations:

tokio-threadpool should be as independent as possible, changing the signature of custom_park to io::Result is requiring knowledge of the runtime needs.

Making WorkerId PartialEq<usize> or something will probably work. It will be 0..self.pool_size() set by the builder. This also is easier to do in a backwards compatible way.

carllerche · 2018-09-26T16:20:14Z

src/runtime/mod.rs

@@ -152,9 +150,6 @@ pub struct Runtime {

 #[derive(Debug)]
 struct Inner {
-    /// Reactor running on a background thread.
-    reactor: Background,
-
    /// Task execution pool.
    pool: threadpool::ThreadPool,
 }


Yeah, it is private, so we can change it to whatever is best 👍

carllerche · 2018-09-26T16:20:54Z

src/runtime/mod.rs

-    /// ```
-    pub fn reactor(&self) -> &Handle {
-        self.inner().reactor.handle()
-    }


We need to keep this to maintain backwards compatibility. Could this just return a handle to the first reactor? A deprecation warning should provide enough hint to users to stop using it.

carllerche · 2018-09-26T16:22:27Z

src/runtime/shutdown.rs

-            })
-        });
-
+        let inner = Box::new(inner.pool.shutdown_now());


Odds are you can remove the box, but it isn't critical.

carllerche

Looks great. The only change I would ask for is to keep the fields of WorkerId private, instead add a as_usize() method. This should help with forwards compatibility.

I'm a 👍 with that change. Anyone else may feel free to merge this at that point.

carllerche · 2018-10-02T03:54:58Z

tokio-threadpool/src/worker/mod.rs

+/// Worker identifiers in a single thread pool are guaranteed to be integers in
+/// the range `0..pool_size`.
+#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)]
+pub struct WorkerId(pub usize);


Instead of making the field pub, could you instead keep the field private and add a new fn: WorkerId::as_usize()? This should help with forwards compatibility.

I decided to name it WorkerId::into_usize because:

fn as_usize(&self) -> &usize

fn to_usize(&self) -> usize

fn into_usize(self) -> usize

I made WorkerId a Copy type (just like e.g. ThreadId is) so now into_usize seems most fitting.

carllerche · 2018-10-02T03:55:31Z

src/runtime/builder.rs

-                let timer_handle = t1.lock().unwrap()
-                    .get(w.id()).unwrap()
-                    .clone();
+                let index = w.id().0;


Related to the comment on WorkerId, this would become w.id().as_usize().

carllerche · 2018-10-02T03:56:24Z

src/runtime/builder.rs

-                    .insert(worker_id.clone(), timer.handle());
-
-                timer
+                timers[worker_id.0]


This would become worker_id.as_usize().

jonhoo · 2018-10-02T04:02:20Z

I'm working on tidying up the benchmarks for https://github.com/mit-pdos/noria. In theory that should serve as a good benchmark of the impact of this change.

carllerche · 2018-10-02T05:46:10Z

@jonhoo Thanks for letting us know.

That said, I don't think this change will make much of a difference. This is the first step. The next step will be assigning tasks to "home" workers.

jonhoo · 2018-10-02T06:11:23Z

@carllerche Hmm, if I understand the change right, this should at least be able to replace tokio-io-pool?

carllerche · 2018-10-02T16:01:50Z

tokio-threadpool/src/worker/mod.rs

@@ -79,7 +79,7 @@ struct CurrentTask {
 ///
 /// This identifier is unique scoped by the thread pool. It is possible that
 /// different thread pool instances share worker identifier values.
-#[derive(Debug, Clone, Hash, Eq, PartialEq)]
+#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)]


Given that the type is already Clone, I would avoid Copy for now (forwards compat hazard).

carllerche · 2018-10-02T16:02:49Z

tokio-threadpool/src/worker/mod.rs

+    ///
+    /// Worker identifiers in a single thread pool are guaranteed to correspond to integers in the
+    /// range `0..pool_size`.
+    pub fn into_usize(&self) -> usize {


This is still taking &self. That said, I would opt for to_usize for now (but it isn't critical either way).

carllerche · 2018-10-02T18:16:02Z

@jonhoo According to @stjepang, I am wrong and this PR should take us there.

Also, @seanmonstar described how to run the hyper benchmarks here. We should check the impact of this PR.

carllerche

❤️

jonhoo · 2018-11-13T19:31:54Z

@stjepang just to check my own understanding of this, this means that, with work stealing, we could end up with lots of futures being driven by reactors on different threads, right? Because there's no migration between reactors?

ghost · 2018-11-13T19:39:59Z

@jonhoo Yes... in theory.

However, if a worker is running a stolen task, the situation is no worse than what we previously had - where all futures are driven by the single reactor on a dedicated background thread.

Furthermore, in order to minimize the amount of stealing going on, I submitted #683 that attempts to distribute futures onto workers & reactors as evenly as possible.

jonhoo · 2018-11-13T19:50:04Z

Okay, yeah, that's what I figured; just wanted to double-check. I wonder if we could keep track of the "original" worker for a future, and try to bias the stealing so that things tend to end up back where they were spawned when possible.

ghost · 2018-11-13T20:06:50Z

I wonder if we could keep track of the "original" worker for a future, and try to bias the stealing so that things tend to end up back where they were spawned when possible.

That is already happening. When a reactor decides to notify a task, it inserts it into the current worker's queue.

Say task T is polled by worker W1, its IO resource gets registered in reactor R1, and then it goes back to W1's task queue. Then another worker W2 steals T and polls it, but it has to block on T's IO resource. T gets inserted into R1 (because the IO resource was registered there) and waits there until it becomes ready. Eventually, R1 will decide to notify T and insert it into W1's queue. This way T ends up back in W1.

jonhoo · 2018-11-13T20:08:54Z

Neat! I didn't realize reactors also had their own queues? I'm doing some reconnaissance ahead of https://twitter.com/Jonhoo/status/1062392012878606339 :)

ghost · 2018-11-13T20:37:32Z

Oh nice! Looking forward to it :)

So the way a worker finds and runs a task is:

Grab a task from my own queue.
If not found, then steal it from someone else.
If I've got a task, then run it.
a. If the task has completed, drop it.
b. If the task has to block on an IO resource, give a task reference to the reactor the IO resource is registered in.
- If the IO resource hasn't been registered anywhere yet, register it in my reactor.
If I don't have a task, block on my own reactor.
a. If the reactor sees IO activity, it will notify appropriate tasks and wake me up.
b. It's also possible for another thread to spawn a new future and put it into my queue. That action will trigger a dummy IO resource in my reactor in order to wake me up.

Besides waiting on the reactor when no tasks are found, the worker also calls function named "sleep_light" every X task runs, which asks the reactor for new events without blocking. We do this so that IO notifications keep chugging along even when the task queues are not empty.

The reactor has a list of IO resources together with tasks they belong to. When the reactor is blocked on, it will wait until at least one IO resource becomes ready. Then it notifies tasks associated with all readied IO resources, and the act of notification simply puts task references into the current worker's queue.

runtime: create reactor per worker

1e950e5

ghost requested a review from carllerche September 25, 2018 18:32

ghost commented Sep 25, 2018

View reviewed changes

ghost mentioned this pull request Sep 25, 2018

Explore improvements to the thread pool scheduling logic #145

Closed

carllerche reviewed Sep 26, 2018

View reviewed changes

Stjepan Glavina added 2 commits September 28, 2018 14:59

Address comments

6757967

Merge branch 'master' into reactor-per-worker

be57a80

carllerche requested changes Oct 2, 2018

View reviewed changes

Add WorkerId::into_usize

86698f8

jonhoo mentioned this pull request Oct 2, 2018

Avoid using branches for different benchmark setups mit-pdos/noria-benchmarks#5

Open

carllerche requested changes Oct 2, 2018

View reviewed changes

Resolve the remaining nits

98081be

carllerche approved these changes Oct 3, 2018

View reviewed changes

carllerche merged commit d35d051 into tokio-rs:master Oct 3, 2018

ghost deleted the reactor-per-worker branch October 3, 2018 09:35

ghost mentioned this pull request Oct 3, 2018

threadpool: spawn new tasks onto a random worker #683

Merged

guydunigo mentioned this pull request Nov 26, 2018

Error : "socket already registered" when upgrading from 0.1.11 to 0.1.13 #774

Closed

tobz mentioned this pull request Nov 28, 2018

Default Runtime configuration / Executor & Reactor on different threads is suboptimal #265

Closed

ghost mentioned this pull request Jan 15, 2019

Remove LocalWaker and simplify the RawWakerVTable aturon/rfcs#16

Merged

ghost mentioned this pull request Jan 30, 2019

multiple reactor model #878

Closed

curtiseng mentioned this pull request Jul 30, 2021

reactor相关请教 tony612/tokio-internals#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime: create reactor per worker #660

runtime: create reactor per worker #660

ghost commented Sep 25, 2018

ghost Sep 25, 2018

carllerche Sep 26, 2018

ghost Sep 25, 2018

carllerche Sep 26, 2018

ghost Sep 25, 2018

carllerche Sep 26, 2018

carllerche left a comment

carllerche Sep 26, 2018

carllerche Sep 26, 2018

carllerche Sep 26, 2018

carllerche Sep 26, 2018

carllerche left a comment

carllerche Oct 2, 2018

ghost Oct 2, 2018

carllerche Oct 2, 2018

carllerche Oct 2, 2018

jonhoo commented Oct 2, 2018

carllerche commented Oct 2, 2018

jonhoo commented Oct 2, 2018

carllerche Oct 2, 2018

carllerche Oct 2, 2018

carllerche commented Oct 2, 2018

carllerche left a comment

jonhoo commented Nov 13, 2018

ghost commented Nov 13, 2018

jonhoo commented Nov 13, 2018

ghost commented Nov 13, 2018

jonhoo commented Nov 13, 2018

ghost commented Nov 13, 2018

runtime: create reactor per worker #660

runtime: create reactor per worker #660

Conversation

ghost commented Sep 25, 2018

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonhoo commented Oct 2, 2018

carllerche commented Oct 2, 2018

jonhoo commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche commented Oct 2, 2018

carllerche left a comment

Choose a reason for hiding this comment

jonhoo commented Nov 13, 2018

ghost commented Nov 13, 2018

jonhoo commented Nov 13, 2018

ghost commented Nov 13, 2018

jonhoo commented Nov 13, 2018

ghost commented Nov 13, 2018