Sleep trait #665

carllerche · 2017-11-30T20:43:18Z

Introduce a Sleep trait that executors can use to customize the strategy used to put the current thread to sleep. This will be used by Tokio to enable running an instance of CurrentThread and an instance of the Tokio reactor on a single thread.

This essentially makes the CurrentThread implementation generic over W: Wakeup. This introduces some trickiness as the thread-local state that allows spawning new futures into the scheduler cannot be generic over the W.

Handling this required some restructuring of scheduler. This work was done incrementally in each commit and each commit includes a commit message that describes the step.

/cc @aturon

This is the first step to supporting customizing executor sleeping strategies. Currently, it is not possible to run a Tokio reactor and a `CurrentThread` executor on the same thread. The executor is hard coded to use condvars for sleeping and the Tokio reactor requires calling `epoll_wait` (or equivalent) for blocking the current thread. The `Sleep` trait is added to abstract over this sleeping strategy. Beyond just supporting the Tokio reactor, adding a `Sleep` trait is useful for integrating any logic that requires hooking into the sleep strategy (e.g. timers).

This will be used in the future to add multiple different node lists.

Currently, `Scheduler` has to move every future it schedules out of the allocated node onto the stack, then move it back when it is done polling. This can be costly with large structs. The comment claims: > The future was extracted above (taken ownership). That way if it > panics we're guaranteed that the future is dropped on this thread > and doesn't accidentally get dropped on a different thread (bad). However, in the event of a panic. It is not possible for another thread to be able to drop the future. This is true even if the future remains in the node. A node is stored in an `Arc`, which means that it is not dropped until the ref count goes to zero. The `Bomb` structure holds a handle to the `Arc`, ensuring that the ref count does not go to zero. The other way a future is dropped is by calling `release_node`. This function is only called in `Bomb::drop`, which happens in the event of a panic, or in `Scheduler::drop`, which is permitted to drop the future as it is the type that logically owns the future value.

`Inner` holds an Arc reference for each node and each node holds an Arc reference to `Inner`. This creates an `Arc` cycle, which before this patch was broken by having `Node` use a weak ref back to `Inner`. However, using weak references requires atomic writes when notifying the future. This patch removes the weak ref and exchanges it for a strong ref. The rational is that the ref cycle is already broken by the `Scheduler::drop` implementation. The `Scheduler` type is outside of the ref cycle and the drop implementation iterates through each node that `Inner` holds and drops the ref. This breaks the cycle. `Node` now has an `Option<Arc<Inner<_>>` field. It is `Option` to support stub nodes not having to reference `Inner` as well as will support future improvements.

This allows queuing up futures to be submitted to a scheduler using the same internal data structures that the scheduler uses. This is added so that futures can be efficiently spawned without knowing the actual type of the scheduler (the `W` generic).

The traits are identical. `scheduler::Wakeup` was private and introduced earlier to allow the scheduler to support both `CurrentThread` and `FuturesUnordered`.

Adds a new function to `executor::CurrentThread` that allows specifying a custom sleep strategy. This will allow integrating `CurrentThread` with reactors, like Tokio, that require being able to block the current thread.

The function was originally named `block_with_init` as it was intended to be used commonly as a strategy to block the current thread waiting on a future to complete. This functionality is now provided with `future::blocking` and `CurrentThread` is intended to be exclusively used as a full executor. Given this change, the emphasis on `block_` is less important and a function name that conveys running the executor is picked instead.

aturon

Ok, I completed the review, and raised a few significant concerns. Thanks for putting this PR together!

aturon · 2017-12-01T23:07:01Z

src/executor/sleep.rs

+}
+
+/// Wake up a sleeping thread.
+pub trait Wakeup: Send + Sync {


Does this also want to be 'static?

aturon · 2017-12-01T23:53:16Z

src/scheduler.rs

@@ -11,7 +11,7 @@ use std::mem;
 use std::ptr;
 use std::sync::atomic::Ordering::{Relaxed, SeqCst, Acquire, Release, AcqRel};
 use std::sync::atomic::{AtomicPtr, AtomicBool};
-use std::sync::{Arc, Weak};


I don't agree with the reasoning laid out in the commit message. In particular, suppose you have a node which is in the ready queue at the point when the scheduler is dropped. While release_node is called on that node, the fact that it's marked as queued means that it's Arc is not decremented at that point, but instead assumed to be picked up by a later call to tick. That later call never comes, and the Inner is never dropped because of the cycle.

More to the point, other threads may have outstanding Node references (via stashed NotifyHandles), which keep the node alive, which keeps the Inner alive, which points back to the Node...

aturon · 2017-12-01T23:58:59Z

src/executor/current_thread.rs

+                let ret = current.enter(|| {
+                    let ret = f(&mut ctx);
+
+                    println!("~~~ DONE ENTER ~~~");


aturon · 2017-12-02T00:14:53Z

src/executor/current_thread.rs

-    /// mut reference is set to null. This is done even if the closure panics.
-    ///
-    /// This reduces the odds of introducing pointer aliasing.
-    fn set_scheduler<F, R>(&self, scheduler: &mut Scheduler, f: F) -> R


There are various comments in the file still referring to this function/it's implications.

aturon · 2017-12-02T00:19:09Z

src/executor/current_thread.rs

-                // b) `cancel_all_executing` is called, forcing the executor to
-                // return.
-                runner.run(thread_notify, current);
+                println!("~~~ DONE ENTER ~~~");


Needs to be cleaned out.

aturon · 2017-12-02T00:24:24Z

src/scheduler.rs

+///
+/// This is safe for the following reasons
+///
+/// 1) The only usage of `W` in `Node` is `Option<Arc<Inner<_, W>>>`. When used


I feel pretty uneasy about this approach; it's "probably fine" but we don't have enough representation guarantees to be sure.

I'd be a lot more comfortable using a SmallVec or something similar to queue up tasks, creating nodes afterward; assuming that most polling will result in at most a couple of new tasks, this is likely to be extremely small overhead and can be done in safe code.

carllerche · 2017-12-02T19:57:34Z

@aturon

I will revert the removal of Weak. Your assessment is correct. I had forgotten that the mpsc channel required a strong ref. Incidentally, this is why I personally don't like punting issues in complex bits of code until later. It is hard to fully reload the context.

I am worried that the weak ref will cause contention on NUMA systems, but we can punt until this is demonstrated. I will add additional comments and hopefully I won't forget the context again in the future.

Re: the W transmute, I think I have another completely different idea that should be safe and not require a vec. I will explore.

This patch addresses the feedback provided in the PR. Specifically, it does a few things: * Revert the removal of the Weak arc reference. * Switch to trait objects to hide the `W` generic. * Add 'static bound to `Wakeup`. The assumptions made when removing the `Weak` arc reference were incorrect. The incorrect assumptions are described in the commit being reverted (85a6351) and the error is explained [here]. [here]: #665 (comment) The implementation details of `current_thread` are reverted back what they were before this PR. The only remaining differences are the naming of the `run` function and the thread-local variable used to spawn futures into the current thread executor now uses a trait object to hide the `W` generic. While this adds a dynamic dispatch, this cost is only incurred on spawn. Finally, a 'static bound is added to the `executor::Wakeup` trait.

carllerche · 2017-12-02T20:59:43Z

@aturon the latest commit should address your feedback.

aturon · 2017-12-04T02:04:50Z

Thanks, the changes look great! Merge when ready.

carllerche · 2017-12-04T17:56:08Z

Ok, I fixed CI + updated impl comments.

carllerche added 9 commits November 29, 2017 14:37

Extract scheduler linked list to struct

fedfabe

This will be used in the future to add multiple different node lists.

Replace scheduler::Wakeup with executor::Wakeup

b63514e

The traits are identical. `scheduler::Wakeup` was private and introduced earlier to allow the scheduler to support both `CurrentThread` and `FuturesUnordered`.

Use executor::Sleep trait with CurrentThread

c19e2d3

Adds a new function to `executor::CurrentThread` that allows specifying a custom sleep strategy. This will allow integrating `CurrentThread` with reactors, like Tokio, that require being able to block the current thread.

Only use Sleep trait when std is present

d9a9309

carllerche added the tokio-reform label Dec 1, 2017

aturon suggested changes Dec 2, 2017

View reviewed changes

Fix benchmarks

c2c3e89

aturon approved these changes Dec 4, 2017

View reviewed changes

Update Scheduler internals comments

a0f7bae

carllerche merged commit a26e594 into rust-lang:tokio-reform Dec 6, 2017

carllerche mentioned this pull request Dec 15, 2017

There is no way to combine the Tokio reactor with CurrentThread #652

Closed

alexcrichton mentioned this pull request Jan 4, 2018

Replace trait Wakeup with NotifyHandle? #688

Closed

stbuehler mentioned this pull request Jun 10, 2019

Park trait, ParkThread implementation and using it in LocalPool #1668

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sleep trait #665

Sleep trait #665

carllerche commented Nov 30, 2017 •

edited

Loading

aturon left a comment

aturon Dec 1, 2017

aturon Dec 1, 2017

aturon Dec 1, 2017

aturon Dec 2, 2017

aturon Dec 2, 2017

aturon Dec 2, 2017

carllerche commented Dec 2, 2017

carllerche commented Dec 2, 2017

aturon commented Dec 4, 2017

carllerche commented Dec 4, 2017

Sleep trait #665

Sleep trait #665

Conversation

carllerche commented Nov 30, 2017 • edited Loading

aturon left a comment

Choose a reason for hiding this comment

aturon Dec 1, 2017

Choose a reason for hiding this comment

aturon Dec 1, 2017

Choose a reason for hiding this comment

aturon Dec 1, 2017

Choose a reason for hiding this comment

aturon Dec 2, 2017

Choose a reason for hiding this comment

aturon Dec 2, 2017

Choose a reason for hiding this comment

aturon Dec 2, 2017

Choose a reason for hiding this comment

carllerche commented Dec 2, 2017

carllerche commented Dec 2, 2017

aturon commented Dec 4, 2017

carllerche commented Dec 4, 2017

carllerche commented Nov 30, 2017 •

edited

Loading