Singlethreaded runtime #308

kpp · 2018-04-08T07:02:39Z

Closes #235

Thanks to: #265, #306

carllerche · 2018-04-09T03:24:13Z

The CI fix was merged to master. Would you be able to rebase? Sorry for the inconvenience.

kpp · 2018-04-09T04:18:32Z

Done

carllerche

Thanks for doing this! This is a great start.

I left some comments inline. On top of that, would it be possible to add some additional fns to match current_thread some.

SingleThreaded::spawn: spawn the future onto the runtime. This does not start running it immediately, but it runs it when run, or block_on is called.
SingleThreaded::run: run the runtime to completion, blocking the current thread and unblocking once all spawned futures have completed executing.

carllerche · 2018-04-10T20:48:58Z

src/runtime/mod.rs

 mod shutdown;
 mod task_executor;

 pub use self::builder::Builder;
+pub use self::singlethreaded_runtime::SinglethreadedRuntime;


Could this be named singlethreaded (and the file singlethreaded.rs). It already is in a runtime module :)

carllerche · 2018-04-10T20:49:20Z

src/runtime/singlethreaded_runtime.rs

+use std::cell::RefCell;
+use std::io;
+
+///


Missing docs?

Yes, I don't know what to write here

carllerche · 2018-04-10T20:52:03Z

src/runtime/builder.rs

+    /// # let _ = runtime;
+    /// # }
+    /// ```
+    pub fn singlethreaded(&mut self) -> io::Result<SinglethreadedRuntime> {


This should probably be named single_threaded and the type SingleThreaded (since it is two words).

I would probably also recommend dropping Runtime from SingleThreaded since it is scoped in the runtime module already.

carllerche · 2018-04-10T20:53:55Z

src/runtime/singlethreaded_runtime.rs

+pub struct SinglethreadedRuntime {
+    reactor_handle: tokio_reactor::Handle,
+    timer_handle: timer::Handle,
+    executor: RefCell< CurrentThread<Timer<Reactor>> >,


Why is the RefCell needed here?

carllerche · 2018-04-10T20:56:08Z

src/runtime/singlethreaded_runtime.rs

+    reactor_handle: tokio_reactor::Handle,
+    timer_handle: timer::Handle,
+    executor: RefCell< CurrentThread<Timer<Reactor>> >,
+    enter: RefCell< Enter >,


Enter should not be stored on the struct. Instead, it should be created "on demand" when a function needs to "enter" the executor.

Here is how tokio-core does it: https://github.com/tokio-rs/tokio-core/blob/master/src/reactor/mod.rs#L224-L241.

carllerche · 2018-04-10T21:09:13Z

src/runtime/singlethreaded_runtime.rs

+                    // Run the provided future
+                    executor.block_on(f).unwrap();
+                    // Run all the other futures that are still left in the executor
+                    executor.run().unwrap();


I would follow the current_thread behavior here and return immediately once the executor.block_on call returns.

So, that would mean removing executor.run() after calling executor.block_on.

carllerche · 2018-04-10T21:09:19Z

src/runtime/singlethreaded_runtime.rs

+                tokio_executor::with_default(&mut default_executor, enter, |enter| {
+                    let mut executor = executor.enter(enter);
+                    // Run the provided future
+                    executor.block_on(f).unwrap();


Could you remove the unwrap and return the result of executor.block_on to the caller?

carllerche · 2018-04-10T21:13:54Z

src/runtime/singlethreaded_runtime.rs

+    /// Blocks and runs the given future.
+    ///
+    /// This is similar to running a runtime, but uses only the current thread.
+    pub fn block_on<F: Future<Item = (), Error = ()>>(&self, f: F) -> () {


This can probably be &mut self.

carllerche · 2018-04-10T21:14:10Z

src/runtime/singlethreaded_runtime.rs

+        let reactor_handle = self.reactor_handle.clone();
+        let timer_handle = self.timer_handle.clone();
+        let mut executor = self.executor.borrow_mut();
+        let mut enter = self.enter.borrow_mut();


The enter instance would be created here.

carllerche · 2018-04-10T21:16:53Z

src/runtime/builder.rs

@@ -68,6 +68,22 @@ impl Builder {
        self
    }

+    /// Create a new singlethreaded runtime
+    /// # Examples


Extra line needed before # Examples to make rustdoc happy (i think).

sdroege · 2018-04-11T08:56:31Z

This looks like a good start, thanks :) It's also more or less what I'm doing in my code right now. However it would currently be impossible to spawn futures from another thread on this runtime as it can't be shared with other threads.

A solution to that would be to provide some kind of mpsc channel (obviously not directly but wrapped in some kind of "Spawner"/Handle struct for this runtime) from the runtime that can be used for other threads to spawn futures, by passing those futures over the channel and then spawning them locally from the runtime thread directly.

kpp · 2018-04-11T11:48:04Z

@carllerche all your review notes are fixed.

@sdroege I am not sure that that's kind of functionality should be implemented for now.

sdroege · 2018-04-11T12:12:28Z

For my use case I would need that and it seems quite easy to implement. I mean, in my implementation of the same kind of runtime I did exactly that :)

kpp · 2018-04-11T13:05:09Z

@carllerche Carl?

sdroege · 2018-04-11T18:50:35Z

@kpp If you don't implement it, I'll send a PR for that once yours here is merged :)

kpp · 2018-04-12T05:09:04Z

@kpp If you don't implement it, I'll send a PR for that once yours here is merged :)

Good news! I don't actually get what must be implement it so I would be grateful for you if you implemented it.

carllerche

Thanks for the updates. This looks much better.

I added some inline comments as well as a bikeshedding level question that I am unsure about.

Also, would it be possible to add a test? It could basically just be a copy of this updated for the single threaded runtime.

src/runtime/builder.rs

src/runtime/singlethreaded.rs

carllerche · 2018-04-14T04:50:22Z

src/runtime/singlethreaded.rs

+                    // Run the provided future
+                    executor.block_on(f).unwrap();
+                    // Run all the other futures that are still left in the executor
+                    executor.run().unwrap();


This would be removed.

carllerche · 2018-04-14T04:50:48Z

src/runtime/singlethreaded.rs

+                tokio_executor::with_default(&mut default_executor, enter, |enter| {
+                    let mut executor = self.executor.enter(enter);
+                    // Run the provided future
+                    executor.block_on(f).unwrap();


The unwrap isn't needed. Just return the ret value out of this function.

kpp · 2018-04-14T07:22:37Z

Also, would it be possible to add a test? It could basically just be a copy of this updated for the single threaded runtime.

Done

src/runtime/singlethreaded.rs

kpp · 2018-04-15T12:25:45Z

src/runtime/singlethreaded.rs

+                tokio_executor::with_default(&mut default_executor, enter, |enter| {
+                    let mut executor = executor.enter(enter);
+                    // Run all the other futures that are still left in the executor
+                    executor.run().unwrap();


@sdroege should it be done like this?

The unwrap should not be needed once the return type of the function is changed.

stbuehler · 2018-04-15T14:42:30Z

If CurrentThread would forward the inner park errors, some of those unwrap calls could return the (io) errors instead. This might change the return types of some functions, so perhaps think about this before merging?

Also if CurrentThread (and maybe Entered too) would provide get_park/get_park_mut (as Timer does) the handles wouldn't need to be stored.

Also Timer doesn't park if it reached a deadline (afaict), which means it stalls any IO handling (it should park with a duration of 0 seconds). CurrentThread doesn't have this problem, apart from turn it always parks afaics.

src/runtime/singlethreaded.rs

sdroege · 2018-04-16T06:58:39Z

src/runtime/singlethreaded.rs

+
+impl Drop for SingleThreaded {
+    fn drop(&mut self) {
+        self.shutdown_on_idle();


This seems suboptimal as it would always block when dropping the runtime if there are still futures pending, which can be quite unexpected

This is how it is done for multi-threaded runtime

The multi-threaded runtime uses shutdown_now, which immediately cancels any pending futures. That's equivalent, in the single threaded case, of just dropping the executor/reactor/timer

carllerche

Sorry for the response delay. I have been without internet connectivity for the past week.

After discussing this with a bunch of people, I have another module layout proposal.

Instead of putting all the types in tokio::runtime, I suggest that a new module is added: tokio::runtime::current_thread. Then, this would contain the following types:

Runtime - The current thread runtime.
Builder - Builds the current_thread::Runtime.
TaskExecutor - An executor handle for current_thread::Runtime (though this can be added in a later PR).

The reasoning behind this change is that:

The thread pool version of Runtime is intended to be the primary and default.
SingleThread does not really reflect the main reason for this separate type, which is that all the logic happens on the current thread. A threadpool based runtime can be started with 1 thread.
The builder options will be significantly different between the current thread runtime and the threadpool runtime.
Since there are multiple types that are "duplicated", it makes sense to split them up into another module.

Does anyone else have thoughts on this API?

src/runtime/builder.rs

src/runtime/mod.rs

src/runtime/singlethreaded.rs

carllerche · 2018-04-23T21:16:40Z

src/runtime/singlethreaded.rs

+                tokio_executor::with_default(&mut default_executor, enter, |enter| {
+                    let mut executor = executor.enter(enter);
+                    // Run all the other futures that are still left in the executor
+                    executor.run().unwrap();


The unwrap should not be needed once the return type of the function is changed.

src/runtime/singlethreaded.rs

stbuehler · 2018-04-24T05:49:58Z

Splitting the builder sounds good to me. Although not directly related to this pull request, but as a response to:

The thread pool version of Runtime is intended to be the primary and default.

I think this is the wrong choice in the long term; imho the single threaded executor should become the default. A thread-pool executor is going to hide lots of bad code (running long-running cpu-heavy calculations, blocking dist/database IO, ... in the polled futures); as long as a developer has lots of cores he will never see those issues. And almost no one will actually need the event handling to run on multiple cores to handle the "event handling" load; and if you do, you probably are better off manually distributing the load across multiple single-threaded event loops to avoid all those context switches. (I.e. my main issue with the pool is the default number of threads being set to the number of cores.)

Not sure if this PR already adds support for this, but it would be nice to run a single threaded runtime as default executor (and reactor+timer) for all threads.

sdroege · 2018-04-24T05:52:23Z

Instead of putting all the types in tokio::runtime, I suggest that a new module is added: tokio::runtime::current_thread

@carllerche seems fine with me, but you would still be fine to have some kind of handle for that runtime which can be sent to other threads for scheduling futures on this runtime?

sdroege · 2018-04-24T08:24:12Z

I think this is the wrong choice in the long term; imho the single threaded executor should become the default.

I would agree with this. Also the multi-threaded version has non-trivial performance penalty (due to context switching, 2-3x slower in one of my cases) and I'm not aware of a workload where it is actually faster (does someone?), apart from the ones that @stbuehler mentions: doing CPU intense work on the runtime directly instead of offloading to a thread-pool.

Is the recommended usage of the runtime going to be to directly do CPU intense work there instead of using a separate thread pool?

kpp · 2018-04-24T09:02:56Z

@carllerche I did all requested changes except for TaskExecutor

kpp · 2018-04-24T09:03:01Z

oops

* don't copy timer_hanler with reactor_handle

carllerche · 2018-05-01T23:37:18Z

Ok, I pushed some tweaks. Let me know if this looks good to y'all.

It currently has no options.

src/runtime/current_thread/runtime.rs

src/runtime/current_thread/mod.rs

sdroege · 2018-05-02T11:21:29Z

src/runtime/current_thread/mod.rs

+//!
+//! # Examples
+//!
+//! Creating a new `Runtime` with default configuration values.


... and running a future f until its completion and returning its result

Also there currently is no configuration at all, so saying default configuration is a bit useless :)

These docs are also duplicated further down

src/runtime/current_thread/runtime.rs

sdroege

Looks all good to me except for the docs

main-- · 2018-05-08T20:12:43Z

Is it intentional that this Runtime does not expose a reactor Handle? After some digging I finally figured out that I'm supposed to use Handle::current() inside a future::lazy but coming from tokio-core 0.1.9 (when I last used tokio) I miss the simplicity and straightforwardness of being guided by types and documentation instead of having to worry about being in the right thread local context.

carllerche · 2018-05-08T20:43:12Z

@main-- what do you need the handle for?

It is kind of intended that it is omitted. The default runtime exposes it, but I think it was a mistake. So, I'm taking more care here. Could you open a new issue explaining why you need to call Handle::current()?

kpp force-pushed the singlethreaded_runtime branch from 254f5f6 to a9430df Compare April 8, 2018 07:20

kpp force-pushed the singlethreaded_runtime branch from a9430df to 2769af9 Compare April 9, 2018 04:18

kpp mentioned this pull request Apr 9, 2018

Include a ~~single-threaded~~ manually built runtime example #306

Merged

kpp force-pushed the singlethreaded_runtime branch from 2769af9 to f640cc5 Compare April 9, 2018 08:08

carllerche requested changes Apr 10, 2018

View reviewed changes

carllerche requested changes Apr 14, 2018

View reviewed changes

sdroege reviewed Apr 15, 2018

View reviewed changes

src/runtime/singlethreaded.rs Outdated Show resolved Hide resolved

sdroege reviewed Apr 15, 2018

View reviewed changes

src/runtime/singlethreaded.rs Outdated Show resolved Hide resolved

sdroege reviewed Apr 15, 2018

View reviewed changes

src/runtime/singlethreaded.rs Outdated Show resolved Hide resolved

kpp commented Apr 15, 2018

View reviewed changes

sdroege reviewed Apr 16, 2018

View reviewed changes

src/runtime/singlethreaded.rs Outdated Show resolved Hide resolved

sdroege reviewed Apr 16, 2018

View reviewed changes

carllerche requested changes Apr 23, 2018

View reviewed changes

kpp closed this Apr 24, 2018

kpp reopened this Apr 24, 2018

kpp added 12 commits April 30, 2018 09:24

Fix Builder::new().build() doc

da4f7af

Add SinglethreadedRuntime

82c6a3a

Review fixes

3f4dba8

Add more docs

24cae47

Add tests for single-threaded runtime

815faea

* rename Builder::single_threaded -> build_single_threaded

c291601

* don't copy timer_hanler with reactor_handle

add more docs

b82010b

add SingleThreaded::shutdown_on_idle

3d9418e

Add function SingleThreaded::run

cea77f1

Move runtime::SingleThreaded into runtime::current_thread::Runtime

14ad1d9

Rename methods and propagate errors from block_on and run

fe0da8c

Make block_on the same signature as block_on from tokio-rs#328

b01607a

kpp force-pushed the singlethreaded_runtime branch from ab33eaf to c0e5dfd Compare April 30, 2018 06:40

^C^V docs to current_thread/mod.rs

c5c0383

kpp force-pushed the singlethreaded_runtime branch from c0e5dfd to c5c0383 Compare April 30, 2018 06:41

Tweak docs and public API

cce85a0

Remove runtime::current_thread::Builder

cb252ed

It currently has no options.

bluetech reviewed May 2, 2018

View reviewed changes

src/runtime/current_thread/runtime.rs Outdated Show resolved Hide resolved

sdroege reviewed May 2, 2018

View reviewed changes

src/runtime/current_thread/mod.rs Show resolved Hide resolved

sdroege reviewed May 2, 2018

View reviewed changes

src/runtime/current_thread/runtime.rs Outdated Show resolved Hide resolved

sdroege reviewed May 2, 2018

View reviewed changes

Fix docs

0572f2d

carllerche approved these changes May 2, 2018

View reviewed changes

carllerche merged commit 2465483 into tokio-rs:master May 2, 2018

kpp deleted the singlethreaded_runtime branch May 2, 2018 19:00

main-- mentioned this pull request May 8, 2018

Need a Handle for UdpSocket::from_std #349

Closed

Singlethreaded runtime #308

Singlethreaded runtime #308

Conversation

kpp commented Apr 8, 2018

carllerche commented Apr 9, 2018

kpp commented Apr 9, 2018

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

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

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

sdroege commented Apr 11, 2018

kpp commented Apr 11, 2018

sdroege commented Apr 11, 2018

kpp commented Apr 11, 2018 • edited Loading

sdroege commented Apr 11, 2018 • edited Loading

kpp commented Apr 12, 2018 • edited Loading

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

kpp commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stbuehler commented Apr 15, 2018

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

stbuehler commented Apr 24, 2018

sdroege commented Apr 24, 2018

sdroege commented Apr 24, 2018

kpp commented Apr 24, 2018

kpp commented Apr 24, 2018

carllerche commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdroege left a comment

Choose a reason for hiding this comment

main-- commented May 8, 2018

carllerche commented May 8, 2018

kpp commented Apr 11, 2018 •

edited

Loading

sdroege commented Apr 11, 2018 •

edited

Loading

kpp commented Apr 12, 2018 •

edited

Loading