implement the executor RFC #763

aturon · 2018-02-14T06:04:20Z

This patch series implements the executor RFC, and along the way:

Removes the id parameters for Notify, as we now have a way to reap their benefits without the convoluted API.
Merges NotifyHandle and Waker.
Removes the handle factory business from task::Context, as per IRC discussion.
Removes much of the enter functionality, which I believe will be obviated if Tokio ends up baking in default reactors tied to executors, as @carllerche wants to do.

The LocalPool API is slightly different than in the RFC:

It includes a separate LocalExecutor type, necessary to ensure the lack of Rc cycles.
Instead of the all_done future (which has issues with re-entrancy), it provides a run function which runs all spawned tasks to completion.

aturon · 2018-02-14T06:05:17Z

r? @alexcrichton @cramertj

The PR needs a couple more things to be complete:

Work through remaining crate test suites, which still use current_thread
Move cpupool into the executor crate and update its APIs

carllerche · 2018-02-14T06:08:51Z

@aturon

Removes the id parameters for Notify, as we now have a way to reap their benefits without the convoluted API.

What is the way that you are referencing?

carllerche · 2018-02-14T06:16:00Z

Removes much of the enter functionality, which I believe will be obviated if Tokio ends up baking in default reactors tied to executors, as @carllerche wants to do.

I'm also not following why enter (proposed in the Tokio RFC I believe) is obviated.

aturon · 2018-02-14T06:29:22Z

@carllerche

What is the way that you are referencing?

https://gist.github.com/alexcrichton/4408c4f317fca5122d37b718c2625169

I'm also not following why enter (proposed in the Tokio RFC I believe) is obviated.

It's not that enter is obviated (it's still there), but rather that this PR removes on_exit (which leads to some other simplifications) which was added originally to help simulate executor-local data. We can always add it back if need be.

cramertj · 2018-02-14T06:43:24Z

futures-executor/src/local_pool.rs

+
+fn run_executor<T, F: FnMut(&Waker) -> Async<T>>(mut f: F) -> T {
+    let _enter = enter()
+        .expect("cannot execute `LocalPool` executor from within \


Is this still necessary?

Yes -- I think we still want enter to ensure that executors aren't accidentally nested.

cramertj · 2018-02-14T06:50:19Z

futures-core/src/executor.rs

+    /// TODO: dox
+    pub trait Executor {
+        /// TODO: dox
+        fn spawn(&self, f: Box<Future<Item = (), Error = ()> + Send>) -> Result<(), SpawnError>;


We had discussed making this &mut self on the RFC-- I think this would allow you to get rid of the RefCell in LocalPool.

Then you can't implement Executor on an Arc<E: Executor>.

Also, we can't get rid of the RefCell, because the handles are behind an Rc.

I just did a big complicated dance to satisfy myself that I couldn't make it work-- the piece I was missing is that you want to be able to spawn_local from inside Future::poll, while also holding onto a task::Context.

@aturon fwiw, the doc did switch to &mut self.

Is being able to impl Executor on Arc<E: Executor> a requirement? What is the use case? The executor can always provide a cloneable handle.

Either way, &mut self vs. &self isn't a critical distinction.

I don't think the Executor for Arc<E: Executor> is the critical piece: the important part is that this needs to work somehow:

let local_pool = LocalPool::new(); let local_exec = local_pool.executor(); local_pool.run_until(|f| future .and_then(|_| local_exec.spawn_local(some_other_task)) .and_then(...));

local_exec needs to be accessible from inside the run_until method of local_pool.

@cramertj Why does that depend on &mut self vs. &self on the executor trait?

It looks like executor() returns a handle by value backed by an Rc, so it should be able to be moved into the closure.

You can also impl Executor on &LocalExecutor to be able to spawn with an immut ref to the local executor.

cramertj · 2018-02-14T06:51:15Z

futures-core/src/task/mod.rs

    map: &'a mut LocalMap,
+    executor: &'a Executor,


As above, I think &'a mut self here would remove the need for a RefCell.

carllerche · 2018-02-14T06:51:43Z

@aturon I will look at the gist in more detail tomorrow when I am more rested. At a glance, it looks like this strategy only works if the executor is bounded and slots is preallocated to the max. i.e. it requires slots on Inner to be fully preallocated as it cannot grow / move.

I agree that it is ideal for the main Wakeup trait to be simple (a single fn w/ no args). However, would it be possible to keep the notify fn w/ the IDs on the UnsafeNotify equivalent instead?

Besides just preventing being able to impl a growable executor backed by a slab, it also makes it much more complicated to impl executors like this.

Specifically, it is quite nice to be able to be called with a pointer to the executor AND a pointer to the task. If the id based fns are removed, it will require more complicated ptr management to impl this executor.

cramertj · 2018-02-14T06:54:30Z

futures-executor/src/local_pool.rs

+    incoming: Weak<Incoming>,
+}
+
+type Incoming = RefCell<Vec<Task>>;


Why are new tasks not placed directly on the FuturesUnordered?

Because of re-entrancy: you may be in the middle of executing poll on the FuturesUnordered when new tasks are spawned.

aturon · 2018-02-14T06:54:38Z

@carllerche If we do need to retain this API, I would indeed much prefer if we could move it to the unsafe trait, though I'm not sure whether that's possible.

carllerche · 2018-02-14T17:14:42Z

@aturon The way I see it, the extra id field allows:

Implementing an executor backed by a Slab (growable).
Pass in extra data.

For example, in the slab case, if the capacity is capped, you can use the rest of the id's space for extra data. One concrete usage would be a slot counter. Given that, in the slab case, slots are reused. Wakeup handles can point to slots that have since been reclaimed by new futures. The additional id space can be used for a counter that allows avoiding spurious wakeups when a wakeup handle points to a slot for a different future.

Besides that, it also makes some executors easier to implement (as linked above). Specifically, if each task object needs to have a pointer back to the executor, that introduces cycles which are definitely trickier to reason about.

It should be possible to move the id based notification to the UnsafeNotify trait. In this case, it would no longer extend Notify. You'd then impl UnsafeNotify for Arc<T: Notify>. When constructing the context, assuming a builder, you'd probably have one path that just took &Arc<T: Notify> and another path that took T: IntoNotifyHandle and the id.

cramertj · 2018-02-14T18:13:32Z

@carllerche

Implementing an executor backed by a Slab (growable).

I was thinking about this, and using an atomic linked list as a backing store (similar to FuturesUnordered) would allow you to keep things growable, while also allowing the collection to shrink once the tasks end (which isn't easily accomplished with the current Slab-based design). You could even store a fixed-size buffer of tasks in each node so that you don't have to allocate each individually. WDYT?

carllerche · 2018-02-14T18:57:41Z

@cramertj it's all about tradeoffs. IMO it should be possible to implement an executor backed by a growable (but not shrinkable) slab. It's a balance between pre-allocating the full capacity and getting deterministic runtime characteristics.

There are also the other benefits I mentioned above.

I don't think that removing these capabilities is a critical issue, but if this can be entirely supported at the UnsafeNotify level, is there a strong argument to not support these cases?

cramertj · 2018-02-14T19:02:06Z

@carllerche

is there a strong argument to not support these cases?

IMO the major argument here would be to reduce the size of Wakeup by eliminating the need for it to store an id.

carllerche · 2018-02-14T19:04:53Z

Reducing the size is valid.

It isn't a critical blow either way. I'm just trying to get the full set of trade offs figured out.

aturon · 2018-02-14T19:08:35Z

I'm also happy to go either way on this.

carllerche · 2018-02-14T19:16:00Z

TBH, there probably is still a way to growable "slab" like thing, it just would require pointers for everything instead of indices as well as some funky unsafe code. You'd lose the ability to use the rest of the id space for user data.

Smaller wakeup handles is appealing.

aturon · 2018-02-14T19:25:32Z

OK, I've now moved in and revamped CpuPool, and gotten the full test suite passing.

There's one more thing I want to do. In making ThreadPool match the executor API, we lose the ability to keep a "join handle" on the task you spawned. But this ability is not tied to ThreadPool anyway; it's a generic thing you can layer on any executor.

So, I want to add free functions, spawn and spawn_with_join, that wrap the context's spawning ability as a future, and in the latter case gives you a join handle.

That said, given that I'm going to be tied up in meetings for the rest of the day, it probably makes sense to finish the review and merge this PR, and add the join handle stuff in a follow-up PR.

aturon · 2018-02-14T19:29:47Z

I also think we should not block on the &self vs &mut self question; either way a follow-up commit would be needed. @carllerche feel free to open an issue if you want to discuss further.

cramertj · 2018-02-14T19:39:04Z

futures-executor/src/thread_pool.rs

+    }
+}
+
+impl ThreadPoolBuilder {


It might be good to offer a block_on function here that works the same as the top-level block_on function, but uses the ThreadPool as the Executor, e.g. let res = ThreadPool::new_num_cpus().block_on(main_future);.

(feel free to ignore this for now-- I'll open a PR later. I just wanted to make a note about it so I didn't forget)

I don't understand what block_on would mean in this context.

@aturon Spawn the future on the thread pool, then block the current thread until the result of the future is available.

I'm unclear on why that would be desirable, though.

@aturon Isn't that what you'd want by default when running an application with a single async_main on a thread pool?

I figured you'd instead do:

LocalPool::new() .run_until(async_main, &ThreadPool::new_num_cpus())

cramertj · 2018-02-14T19:40:53Z

LGTM!

carllerche · 2018-02-14T19:49:37Z

I opened #767 to follow up.

alexcrichton · 2018-02-14T20:42:38Z

CI is still red but I think that's just benchmarks/no_std builds, mind touching those up though?

alexcrichton · 2018-02-14T20:46:15Z

Other than that though looks good!

carllerche · 2018-02-14T21:48:06Z

@aturon

Oh, re this:

It's not that enter is obviated (it's still there), but rather that this PR removes on_exit (which leads to some other simplifications) which was added originally to help simulate executor-local data. We can always add it back if need be.

I actually still plan on having executor local reactors and am will probably re-raise the question of a default global once the dust settles on futures & before Tokio 0.2

Specifically, if Handle::default() is lazily bound to when there is a &mut Context, I'm wondering if it is still necessary to have a "global" reactor (I do not have an answer for this).

aturon · 2018-02-15T00:19:12Z

@carllerche Yeah that's what I meant. So yes, let's revisit this once it's more clear what Tokio's needs will be.

aturon · 2018-02-15T17:55:28Z

I think this is good to go once CI is happy.

I'll do a follow-up PR changing to &mut self for executors.

Closes rust-lang#767

aturon added 4 commits February 13, 2018 16:45

Remove _id from Notify, simplify Waker setup

e5f6f80

Add Executor to futures-core

646a2b6

rewrite futures-executor

db414aa

rewrite executor tests

205782c

aturon mentioned this pull request Feb 14, 2018

RFC: executor design for futures 0.2 rust-lang-nursery/futures-rfcs#3

Merged

cramertj reviewed Feb 14, 2018

View reviewed changes

aturon added 2 commits February 14, 2018 09:52

Add block_on top-level convenience

f111906

rewrite downstream tests

cb2c68a

Incorporate CpuPool into the executor crate as ThreadPool, revamp it

8198563

Fix up tests

1cfdf2a

aturon changed the title ~~[WIP] implement the executor RFC~~ implement the executor RFC Feb 14, 2018

cramertj reviewed Feb 14, 2018

View reviewed changes

carllerche mentioned this pull request Feb 14, 2018

&self vs. &mut self for executors. #767

Closed

Fix up benches

7e1d3e7

Use &mut self for executors.

2a2f4cc

Closes rust-lang#767

aturon force-pushed the exec-rfc branch from 76c268c to 2a2f4cc Compare February 15, 2018 22:00

aturon added 4 commits February 15, 2018 16:07

Fix no_std

f35ec2e

Add Poll, Async export

4a7e1e6

more bench fixes

07aa2a0

remove cpupool from travis

4b562a1

aturon merged commit eb9f603 into rust-lang:0.2 Feb 16, 2018

jeehoonkang mentioned this pull request Mar 13, 2018

Comparision of tokio-threadpool and futures-rs's thread pool? tokio-rs/tokio#211

Closed

ChristophWurst mentioned this pull request Mar 31, 2018

Migrate to tokio-threadpool ChristophWurst/gotham-middleware-threadpool#1

Open

implement the executor RFC #763

implement the executor RFC #763

Conversation

aturon commented Feb 14, 2018

aturon commented Feb 14, 2018 • edited Loading

carllerche commented Feb 14, 2018

carllerche commented Feb 14, 2018

aturon commented Feb 14, 2018

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

carllerche commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented Feb 14, 2018

carllerche commented Feb 14, 2018

cramertj commented Feb 14, 2018

carllerche commented Feb 14, 2018

cramertj commented Feb 14, 2018 • edited Loading

carllerche commented Feb 14, 2018

aturon commented Feb 14, 2018

carllerche commented Feb 14, 2018

aturon commented Feb 14, 2018

aturon commented Feb 14, 2018

cramertj Feb 14, 2018 • edited Loading

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

cramertj commented Feb 14, 2018

carllerche commented Feb 14, 2018

alexcrichton commented Feb 14, 2018

alexcrichton commented Feb 14, 2018

carllerche commented Feb 14, 2018

aturon commented Feb 15, 2018

aturon commented Feb 15, 2018

aturon commented Feb 14, 2018 •

edited

Loading

cramertj commented Feb 14, 2018 •

edited

Loading

cramertj Feb 14, 2018 •

edited

Loading