New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: add futures and task system to libcore #2418

Closed
wants to merge 2 commits into
base: master
from

Conversation

@aturon
Member

aturon commented Apr 24, 2018

Note: this is a heavily-revised, more conservative version of #2395

This RFC provides the library component for the first-class async/await syntax proposed in a companion RFC. It is intentionally minimal, including the smallest set of mechanisms needed to support async/await with borrowing and interoperation with the futures crate. Those mechanisms are:

  • The task system of the futures crate, which will be moved into libcore
  • A Future trait, which integrates the PinMut APIs with the task system to provide futures (i.e. asynchronous values).

Rendered

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 24, 2018

Member

Summary of differences from the original RFC:

  • The main trait is called Async rather than Future, to align with async blocks; it now lives in core::ops.
  • No AsyncResult in the proposal; this may be provided in the futures crate.
  • No combinators are included in the proposal; they'll be provided completely out of tree in the futures crate.
  • Futures 0.3 is a more conservative transition that immediately works on stable Rust, but is also compatible with async/await on nightly.
  • There's a minimal path to stabilization that is based almost entirely on tech that has already been vetted for months or years.
Member

aturon commented Apr 24, 2018

Summary of differences from the original RFC:

  • The main trait is called Async rather than Future, to align with async blocks; it now lives in core::ops.
  • No AsyncResult in the proposal; this may be provided in the futures crate.
  • No combinators are included in the proposal; they'll be provided completely out of tree in the futures crate.
  • Futures 0.3 is a more conservative transition that immediately works on stable Rust, but is also compatible with async/await on nightly.
  • There's a minimal path to stabilization that is based almost entirely on tech that has already been vetted for months or years.
@ashfordneil

This comment has been minimized.

Show comment
Hide comment
@ashfordneil

ashfordneil Apr 25, 2018

Firstly, is Executor in this rfc a trait or an object? It is defined as a trait, but then used as an object in the Context::new function, I'm not sure if this was intentional due to the "final Executor trait" not being ready, but right now it's a little confusing.

Secondly, why are Waker, Context and (possibly?) Executor all trait objects / structs instead of just traits? This introduces an extra layer of dynamic dispatch (on top of the probable boxing of Async that any executor would have to do) to poll any future that interacts with its context. As an alternative that would reduce the amount of dynamic dispatch, we could make Async::poll a method that is generic over C: Context.

As far as raw implementations of the Async trait are concerned, writing a generic poll method shouldn't be any harder than writing a poll method that uses trait objects in its context. Writers of async functions or end users of combinators shouldn't be affected at all by the change. The only potential issue I can see is how to make Async usable as a trait object (for an executor) with this change. I had a look at the problem, and I think it can be solved.

Basically, instead of the executor making a trait object out of Async and then working with those objects, the executor could define its own trait that has a monomorphised poll method (to work specifically with its implementation of Context) and work with a trait object of that. I've attached a playground that does just this below, and creation of the trait objects all compiles without issue so I think it would be fine.

https://play.rust-lang.org/?gist=079aa77b8679fcf80fd9d56cf7fd00cb&version=stable

Thinking on this, there could be some situations - Async aware mutexes come to mind - where you may still need to create trait objects of the Waker trait to store them. This change wouldn't prevent anyone from creating that trait object themselves if they needed to, but it wouldn't default to handing out trait objects, meaning that in the situations where a trait object isn't necessary, the trait object could be avoided.

Sorry for the wall of text, but what are your thoughts? At the moment I can't see any real disadvantage to doing this*, but there could be some big issue that I missed entirely, so I look forward to hearing other people's opinions on this.

*my playground doesn't use Pin yet so here's hoping that doesn't break everything 🤞

ashfordneil commented Apr 25, 2018

Firstly, is Executor in this rfc a trait or an object? It is defined as a trait, but then used as an object in the Context::new function, I'm not sure if this was intentional due to the "final Executor trait" not being ready, but right now it's a little confusing.

Secondly, why are Waker, Context and (possibly?) Executor all trait objects / structs instead of just traits? This introduces an extra layer of dynamic dispatch (on top of the probable boxing of Async that any executor would have to do) to poll any future that interacts with its context. As an alternative that would reduce the amount of dynamic dispatch, we could make Async::poll a method that is generic over C: Context.

As far as raw implementations of the Async trait are concerned, writing a generic poll method shouldn't be any harder than writing a poll method that uses trait objects in its context. Writers of async functions or end users of combinators shouldn't be affected at all by the change. The only potential issue I can see is how to make Async usable as a trait object (for an executor) with this change. I had a look at the problem, and I think it can be solved.

Basically, instead of the executor making a trait object out of Async and then working with those objects, the executor could define its own trait that has a monomorphised poll method (to work specifically with its implementation of Context) and work with a trait object of that. I've attached a playground that does just this below, and creation of the trait objects all compiles without issue so I think it would be fine.

https://play.rust-lang.org/?gist=079aa77b8679fcf80fd9d56cf7fd00cb&version=stable

Thinking on this, there could be some situations - Async aware mutexes come to mind - where you may still need to create trait objects of the Waker trait to store them. This change wouldn't prevent anyone from creating that trait object themselves if they needed to, but it wouldn't default to handing out trait objects, meaning that in the situations where a trait object isn't necessary, the trait object could be avoided.

Sorry for the wall of text, but what are your thoughts? At the moment I can't see any real disadvantage to doing this*, but there could be some big issue that I missed entirely, so I look forward to hearing other people's opinions on this.

*my playground doesn't use Pin yet so here's hoping that doesn't break everything 🤞

@carllerche

As mentioned in the previous RFC, this is roughly in line with what I had hoped the end result would be 👍. I included a couple of nits / questions inline.

/// Any task executor must provide a way of signaling that a task it owns
/// is ready to be `poll`ed again. Executors do so by providing a wakeup handle
/// type that implements this trait.
pub trait Wake: Send + Sync {

This comment has been minimized.

@carllerche

carllerche Apr 25, 2018

Member

Why does this trait need to be moved into core? If UnsafeWake exists in core, then would it not be possible to leave this trait in a crate?

@carllerche

carllerche Apr 25, 2018

Member

Why does this trait need to be moved into core? If UnsafeWake exists in core, then would it not be possible to leave this trait in a crate?

This comment has been minimized.

@aturon

aturon Apr 25, 2018

Member

Yes, this could live out of tree to start with. To be clear, though, I do expect more of the futures crate to move into core over time.

@aturon

aturon Apr 25, 2018

Member

Yes, this could live out of tree to start with. To be clear, though, I do expect more of the futures crate to move into core over time.

/// Provides the reason that an executor was unable to spawn.
pub struct SpawnErrorKind { .. }
impl SpawnErrorKind {

This comment has been minimized.

@carllerche

carllerche Apr 25, 2018

Member

Tokio currently requires an "at capacity" error variant.

@carllerche

carllerche Apr 25, 2018

Member

Tokio currently requires an "at capacity" error variant.

This comment has been minimized.

@aturon

aturon Apr 25, 2018

Member

👍

@aturon
/// spawn failures.
///
/// NB: this will remain unstable until the final `Executor` trait is ready.
pub fn executor(&mut self) -> &mut BoxExecutor;

This comment has been minimized.

@carllerche

carllerche Apr 25, 2018

Member

BoxExecutor is not defined anywhere. Is it possible to implement FuturesUnordered with this current definition?

Also, given the known issues* with this strategy, would it be wiser to punt on executor being tied to context?

* spawning in drop and spawning from a function.

@carllerche

carllerche Apr 25, 2018

Member

BoxExecutor is not defined anywhere. Is it possible to implement FuturesUnordered with this current definition?

Also, given the known issues* with this strategy, would it be wiser to punt on executor being tied to context?

* spawning in drop and spawning from a function.

This comment has been minimized.

@aturon

aturon Apr 25, 2018

Member

This should be Executor.

We can always delay on stabilizing this portion of the API if we are feeling uneasy. Remember, everything that goes into std goes on the nightly channel indefinitely; there's a separate process for actually committing it to stable.

@aturon

aturon Apr 25, 2018

Member

This should be Executor.

We can always delay on stabilizing this portion of the API if we are feeling uneasy. Remember, everything that goes into std goes on the nightly channel indefinitely; there's a separate process for actually committing it to stable.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

A little downside of Async is that there is also the async keyword, so you can't simply create variables that are called "async".

fn main() {
     let async = print_async(); // No no
     println!("Hello from main");
     futures::block_on(async);
}

Edit: In my PR to the companion RFC, I've just called it my_async to avoid the problem.

Contributor

MajorBreakfast commented Apr 25, 2018

A little downside of Async is that there is also the async keyword, so you can't simply create variables that are called "async".

fn main() {
     let async = print_async(); // No no
     println!("Hello from main");
     futures::block_on(async);
}

Edit: In my PR to the companion RFC, I've just called it my_async to avoid the problem.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

Since the variable cannot be called "async", it could be consistently called "op" in the documentation. A consistent name would be good. "op" would be wonderfully short

let op = print_async(); // Can't call it "async"!
futures::block_on(op);

Edit:
Sry for bikeshedding, but this variable thing somehow really annoys me. (And it looked so promising!) It's inconvenient for everyday code° and it's inconvenient for the documentation.

° Example: In JavaScript, when I create a Promise, I usually call it "promise". This is non-imaginative, I know, but if I have only one it's also super clear and I don't have to come up with a name. Instead, I use the lowercase variant and can continue coding right away.

Alternative names for Async:

  • Task: Like C#, it's even shorter and it is really descriptive of what it we intend it to represent
  • Promise: Like JavaScript. Not as descriptive because "promise" implies a bit that it is already in progress. However, Rust's implementation is lazy.

I think the similarity between the async keyword and the Async trait was an idea with a really good intention. I thought for a long time that it'd be really nice if there was a symmetry here. I liked the previous "Future" well enough, though, so I never voiced that. But, now I start to see why no other language (that I'm aware of) decided to actually call it "Async". 🙂It's inconvenient because of the async keyword!

Contributor

MajorBreakfast commented Apr 25, 2018

Since the variable cannot be called "async", it could be consistently called "op" in the documentation. A consistent name would be good. "op" would be wonderfully short

let op = print_async(); // Can't call it "async"!
futures::block_on(op);

Edit:
Sry for bikeshedding, but this variable thing somehow really annoys me. (And it looked so promising!) It's inconvenient for everyday code° and it's inconvenient for the documentation.

° Example: In JavaScript, when I create a Promise, I usually call it "promise". This is non-imaginative, I know, but if I have only one it's also super clear and I don't have to come up with a name. Instead, I use the lowercase variant and can continue coding right away.

Alternative names for Async:

  • Task: Like C#, it's even shorter and it is really descriptive of what it we intend it to represent
  • Promise: Like JavaScript. Not as descriptive because "promise" implies a bit that it is already in progress. However, Rust's implementation is lazy.

I think the similarity between the async keyword and the Async trait was an idea with a really good intention. I thought for a long time that it'd be really nice if there was a symmetry here. I liked the previous "Future" well enough, though, so I never voiced that. But, now I start to see why no other language (that I'm aware of) decided to actually call it "Async". 🙂It's inconvenient because of the async keyword!

@MajorBreakfast MajorBreakfast referenced this pull request Apr 25, 2018

Closed

Further changes #6

@yasammez

This comment has been minimized.

Show comment
Hide comment
@yasammez

yasammez Apr 25, 2018

I am not bothered by the naming issue. Normally when I create a future in JS I either await it on the spot (no need for a binding) or gather multiple different things to await them at once later in which case I give them "business" names like customerPromise, basketPromise, etc.. I have never been tempted to name anything "async" :-)

yasammez commented Apr 25, 2018

I am not bothered by the naming issue. Normally when I create a future in JS I either await it on the spot (no need for a binding) or gather multiple different things to await them at once later in which case I give them "business" names like customerPromise, basketPromise, etc.. I have never been tempted to name anything "async" :-)

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

@yasammez In real code you have that option, yes. But, in documentation where the intention is to stay brief and generic it's problematic. It'll bite us, I'm telling you

@alkis suggests "Lazy" as an alternative in the async/await RFC.

Contributor

MajorBreakfast commented Apr 25, 2018

@yasammez In real code you have that option, yes. But, in documentation where the intention is to stay brief and generic it's problematic. It'll bite us, I'm telling you

@alkis suggests "Lazy" as an alternative in the async/await RFC.

@rushmorem

This comment has been minimized.

Show comment
Hide comment
@rushmorem

rushmorem Apr 25, 2018

A little downside of Async is that there is also the async keyword, so you can't simply create variables that are called "async".

@MajorBreakfast In that case, why not just name the variable future?

rushmorem commented Apr 25, 2018

A little downside of Async is that there is also the async keyword, so you can't simply create variables that are called "async".

@MajorBreakfast In that case, why not just name the variable future?

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

@rushmorem Unfortunately that'd be misleading. The new RFC standardizes Future under a different name to highlight the differences to Future from the futures crate (this was first proposed by @aturon). The two traits are very similar, but they're not the same. Good documentation has to be very precise in such matters to not confuse beginners

Contributor

MajorBreakfast commented Apr 25, 2018

@rushmorem Unfortunately that'd be misleading. The new RFC standardizes Future under a different name to highlight the differences to Future from the futures crate (this was first proposed by @aturon). The two traits are very similar, but they're not the same. Good documentation has to be very precise in such matters to not confuse beginners

@tmccombs

This comment has been minimized.

Show comment
Hide comment
@tmccombs

tmccombs Apr 25, 2018

Task is already taken for the overall task.

tmccombs commented Apr 25, 2018

Task is already taken for the overall task.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

@tmccombs Exactly. "overall task". The meaning of "overall task" would be clearer if it consisted of child tasks. Currently we have to explain what an Async is and then what a task is. But a task is really just a top level and scheduled Async with Output=(). We could just call both tasks and be done with it 🙂

Contributor

MajorBreakfast commented Apr 25, 2018

@tmccombs Exactly. "overall task". The meaning of "overall task" would be clearer if it consisted of child tasks. Currently we have to explain what an Async is and then what a task is. But a task is really just a top level and scheduled Async with Output=(). We could just call both tasks and be done with it 🙂

@rpjohnst

This comment has been minimized.

Show comment
Hide comment
@rpjohnst

rpjohnst Apr 25, 2018

A task is more than simply an Async<Output=()>- it is a scheduled Async (with any Output).

rpjohnst commented Apr 25, 2018

A task is more than simply an Async<Output=()>- it is a scheduled Async (with any Output).

@glaebhoerl

This comment has been minimized.

Show comment
Hide comment
@glaebhoerl

glaebhoerl Apr 25, 2018

Contributor

(It doesn't feel right to me to name a variable async in the first place because "async" (unlike "future") isn't even a noun.)

Contributor

glaebhoerl commented Apr 25, 2018

(It doesn't feel right to me to name a variable async in the first place because "async" (unlike "future") isn't even a noun.)

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

@rpjohnst I've edited my post above to reflect this. My point stands, though: Having two terms for this makes it just more complicated than it needs to be. I mean, it's easier to understand if we don't have that. One less thing to learn and such

@glaebhoerl Good point. "reader_async" vs "reader_task"... "async" just doesn't sound so well because it's an adjective

Contributor

MajorBreakfast commented Apr 25, 2018

@rpjohnst I've edited my post above to reflect this. My point stands, though: Having two terms for this makes it just more complicated than it needs to be. I mean, it's easier to understand if we don't have that. One less thing to learn and such

@glaebhoerl Good point. "reader_async" vs "reader_task"... "async" just doesn't sound so well because it's an adjective

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 25, 2018

Member

@MajorBreakfast I think that @rpjohnst's point is that there are two distinct concepts here: async values, and tasks. A task is something that is being actively run by an executor, whereas an async value is by itself inert. Tasks generally work with async values internally, and are responsible for driving them to completion.

Understanding this distinction is critical to understanding Rust's unique approach to async programming, so I personally find having distinct terms for the concepts to be very helpful in teaching.

Regarding the larger question: I think that Async works very well as a name, both because of the very direct relationship to the construction form (async blocks) and because it is generally descriptive: Async<Output = T> is an async computation that produces a T. I understand the concern about examples in generic documentation, but my perspective is that docs where you're tempted to write let future = ... could probably be improved by giving a more meaningful name and computation anyway -- the same way that the best documentation avoids using foo/bar/etc in favor of more real names/examples.

Member

aturon commented Apr 25, 2018

@MajorBreakfast I think that @rpjohnst's point is that there are two distinct concepts here: async values, and tasks. A task is something that is being actively run by an executor, whereas an async value is by itself inert. Tasks generally work with async values internally, and are responsible for driving them to completion.

Understanding this distinction is critical to understanding Rust's unique approach to async programming, so I personally find having distinct terms for the concepts to be very helpful in teaching.

Regarding the larger question: I think that Async works very well as a name, both because of the very direct relationship to the construction form (async blocks) and because it is generally descriptive: Async<Output = T> is an async computation that produces a T. I understand the concern about examples in generic documentation, but my perspective is that docs where you're tempted to write let future = ... could probably be improved by giving a more meaningful name and computation anyway -- the same way that the best documentation avoids using foo/bar/etc in favor of more real names/examples.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

@aturon

A task is something that is being actively run by an executor, whereas an async value is by itself inert.

Does this difference really merit its own terminology? I mean a "moving car" is still called a "car". No special term required to make the difference clear.

my perspective is that docs where you're tempted to write let future = ... could probably be improved by giving a more meaningful name and computation anyway

I'd say it depends. Sometimes it'd just be irrelevant or confusing. For instance in the example code for Future::map().

Contributor

MajorBreakfast commented Apr 25, 2018

@aturon

A task is something that is being actively run by an executor, whereas an async value is by itself inert.

Does this difference really merit its own terminology? I mean a "moving car" is still called a "car". No special term required to make the difference clear.

my perspective is that docs where you're tempted to write let future = ... could probably be improved by giving a more meaningful name and computation anyway

I'd say it depends. Sometimes it'd just be irrelevant or confusing. For instance in the example code for Future::map().

@rpjohnst

This comment has been minimized.

Show comment
Hide comment
@rpjohnst

rpjohnst Apr 25, 2018

It's very similar to the distinction between "process" and "program." A task has extra data associated with it beyond the impl Async that it's running.

rpjohnst commented Apr 25, 2018

It's very similar to the distinction between "process" and "program." A task has extra data associated with it beyond the impl Async that it's running.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

@rpjohnst Yeah sure. But the user doesn't need to care about such implementation details because they're invisible. As far as the user is concerned it's a "running task". (Or a "running program" in your example)

Contributor

MajorBreakfast commented Apr 25, 2018

@rpjohnst Yeah sure. But the user doesn't need to care about such implementation details because they're invisible. As far as the user is concerned it's a "running task". (Or a "running program" in your example)

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 25, 2018

Member

@MajorBreakfast But that's just the thing: an Async is not a running task, and until it's driven by one it will do nothing. It's like the difference between a FnOnce() -> () and a running thread.

Member

aturon commented Apr 25, 2018

@MajorBreakfast But that's just the thing: an Async is not a running task, and until it's driven by one it will do nothing. It's like the difference between a FnOnce() -> () and a running thread.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Apr 25, 2018

Contributor

Async is not a running task

@aturon Yes. it'd be just a "Task" at first. True to the meaning of the english word "task": a piece of work. Then once it's driven by an executor, it becomes a "running task".

Contributor

MajorBreakfast commented Apr 25, 2018

Async is not a running task

@aturon Yes. it'd be just a "Task" at first. True to the meaning of the english word "task": a piece of work. Then once it's driven by an executor, it becomes a "running task".

/// An `Ok` return means the executor is *likely* (but not guaranteed)
/// to accept a subsequent spawn attempt. Likewise, an `Err` return
/// means that `spawn` is likely, but not guaranteed, to yield an error.
fn status(&self) -> Result<(), SpawnErrorKind> {

This comment has been minimized.

@kennytm

kennytm Apr 25, 2018

Member

The documentation means there's basically no guarantee whether the return value corresponds to the reality, as there are both false positive and negative. That means you can't reliably use if x.status().is_xxx() to check anything in either Ok or Err direction. What is the purpose of this function then?

@kennytm

kennytm Apr 25, 2018

Member

The documentation means there's basically no guarantee whether the return value corresponds to the reality, as there are both false positive and negative. That means you can't reliably use if x.status().is_xxx() to check anything in either Ok or Err direction. What is the purpose of this function then?

This comment has been minimized.

@carllerche

carllerche Apr 25, 2018

Member

Hints are useful for fast paths.

@carllerche

carllerche Apr 25, 2018

Member

Hints are useful for fast paths.

This comment has been minimized.

@tmccombs

tmccombs Apr 25, 2018

Is there a reason that if status returns an Err, spawn isn't guaranteed to return an Err?

@tmccombs

tmccombs Apr 25, 2018

Is there a reason that if status returns an Err, spawn isn't guaranteed to return an Err?

This comment has been minimized.

@carllerche

carllerche Apr 25, 2018

Member

Not all error statuses are permanent.

@carllerche

carllerche Apr 25, 2018

Member

Not all error statuses are permanent.

This comment has been minimized.

@tmccombs

tmccombs Apr 25, 2018

Then maybe, SpawnErrorKind should have an API to determine whether or not the executor will/may eventually recover from the error.

@tmccombs

tmccombs Apr 25, 2018

Then maybe, SpawnErrorKind should have an API to determine whether or not the executor will/may eventually recover from the error.

This comment has been minimized.

@bugaevc

bugaevc Apr 27, 2018

If it's a hint, the naming should clearly indicate that; e.g. status_hint()

@bugaevc

bugaevc Apr 27, 2018

If it's a hint, the naming should clearly indicate that; e.g. status_hint()

@Thomasdezeeuw

This comment has been minimized.

Show comment
Hide comment
@Thomasdezeeuw

Thomasdezeeuw Aug 1, 2018

Over the last couple of months I've been implementing a mostly single threaded runtime for running futures. In doing so I see some improvements in the task system for single threaded use, mainly in the Wake and friends traits/types. Currently we already have a split in the wakers; LocalWaker and Waker. However this is currently usually backed by the same type, because UnsafeWake.clone_raw forces you to return a Waker. I would like to see the split in local and thread-safe waker continue to the UnsafeWake trait. So I propose the following:

LocalWaker will be !Send and !Sync (already the case now), but have a upgrade method to upgrade it to a Waker (dropping the From implementation). This will require a new method on UnsafeWake. Also the will_wake_nonlocal method will need to be look at, or even removed.

Wake will be Send and Sync (already the case), but will also require this in new, i.e. pub unsafe fn new(inner: NonNull<dyn UnsafeWake + Send + Sync>). This allows UnsafeWake to drop those requirements.

UnsafeWake will no longer require Send and Sync, only Waker will require those.

/// No longer requires `Send` or `Sync`.
pub unsafe trait UnsafeWake {
    // `LocalWaker` -> `LocalWaker`.
    unsafe fn clone_raw_local(&self) -> LocalWaker;
    // `Waker` -> `Waker`.
    unsafe fn clone_raw(&self) -> Waker;

    // `LocalWaker` -> `Waker`.
    unsafe fn upgrade(&self) -> Waker;

    // Dropping `LocalWaker`.
    unsafe fn drop_raw_local(&self);
    // Dropping `Waker`.
    unsafe fn drop_raw(&self);

    // Waking `LocalWaker`.
    unsafe fn wake_local(&self);
    // Waking `Waker`.
    unsafe fn wake(&self);
}

It could allow for various performance improvements for waking futures on the same thread.

Drawbacks

This nearly double the number of methods on UnsafeWake making it a rather big, maybe too big of a trait, making an already complex trait even more complex.

For mainly multi-threaded system, e.g. Tokio, this doesn't add much.

Thomasdezeeuw commented Aug 1, 2018

Over the last couple of months I've been implementing a mostly single threaded runtime for running futures. In doing so I see some improvements in the task system for single threaded use, mainly in the Wake and friends traits/types. Currently we already have a split in the wakers; LocalWaker and Waker. However this is currently usually backed by the same type, because UnsafeWake.clone_raw forces you to return a Waker. I would like to see the split in local and thread-safe waker continue to the UnsafeWake trait. So I propose the following:

LocalWaker will be !Send and !Sync (already the case now), but have a upgrade method to upgrade it to a Waker (dropping the From implementation). This will require a new method on UnsafeWake. Also the will_wake_nonlocal method will need to be look at, or even removed.

Wake will be Send and Sync (already the case), but will also require this in new, i.e. pub unsafe fn new(inner: NonNull<dyn UnsafeWake + Send + Sync>). This allows UnsafeWake to drop those requirements.

UnsafeWake will no longer require Send and Sync, only Waker will require those.

/// No longer requires `Send` or `Sync`.
pub unsafe trait UnsafeWake {
    // `LocalWaker` -> `LocalWaker`.
    unsafe fn clone_raw_local(&self) -> LocalWaker;
    // `Waker` -> `Waker`.
    unsafe fn clone_raw(&self) -> Waker;

    // `LocalWaker` -> `Waker`.
    unsafe fn upgrade(&self) -> Waker;

    // Dropping `LocalWaker`.
    unsafe fn drop_raw_local(&self);
    // Dropping `Waker`.
    unsafe fn drop_raw(&self);

    // Waking `LocalWaker`.
    unsafe fn wake_local(&self);
    // Waking `Waker`.
    unsafe fn wake(&self);
}

It could allow for various performance improvements for waking futures on the same thread.

Drawbacks

This nearly double the number of methods on UnsafeWake making it a rather big, maybe too big of a trait, making an already complex trait even more complex.

For mainly multi-threaded system, e.g. Tokio, this doesn't add much.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 1, 2018

Member

@Thomasdezeeuw That's a really interesting idea, thanks! I'm assuming you were using this to make an UnsafeWake backed by an Rc that could internally upgrade to an Arc (e.g. an Rc<Arc<(Task, Arc<Executor>>>)? Did you do any performance testing on this? I'd be curious what the comparative overhead is between this strategy and the one used currently (just Arc<(Task, Arc<Executor>)>). It'd mean that you need to double-allocate on task creation in the singlethreaded case (once for the Arc, once for every thread you want to use an Rc on ), but you could potentially save the cost of all of the atomic clones required by Arc.

Member

cramertj commented Aug 1, 2018

@Thomasdezeeuw That's a really interesting idea, thanks! I'm assuming you were using this to make an UnsafeWake backed by an Rc that could internally upgrade to an Arc (e.g. an Rc<Arc<(Task, Arc<Executor>>>)? Did you do any performance testing on this? I'd be curious what the comparative overhead is between this strategy and the one used currently (just Arc<(Task, Arc<Executor>)>). It'd mean that you need to double-allocate on task creation in the singlethreaded case (once for the Arc, once for every thread you want to use an Rc on ), but you could potentially save the cost of all of the atomic clones required by Arc.

@Thomasdezeeuw

This comment has been minimized.

Show comment
Hide comment
@Thomasdezeeuw

Thomasdezeeuw Aug 1, 2018

@cramertj Unfurtently I don't have benchmark numbers. As for the implementation, I'm working with a forked version of the mio crate that only works on a single thread. So yes, internally it uses Rc. However I don't think I would be upgrading to an Arc, but rather to a single Future that would then deligate the wakes (I'm using the actor pattern). I could write/search for a benchmark for Rc vs Arc, but I'm not sure that would show the entire picture.

Thomasdezeeuw commented Aug 1, 2018

@cramertj Unfurtently I don't have benchmark numbers. As for the implementation, I'm working with a forked version of the mio crate that only works on a single thread. So yes, internally it uses Rc. However I don't think I would be upgrading to an Arc, but rather to a single Future that would then deligate the wakes (I'm using the actor pattern). I could write/search for a benchmark for Rc vs Arc, but I'm not sure that would show the entire picture.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 1, 2018

Member

@Thomasdezeeuw Right, I'm not as interested in benchmarking the cost of a single-threaded v.s thread-safe reference counting as the comparative cost of this double-allocation and pointer-chasing vs. reference-counting in a real application

So yes, internally it uses Rc. However I don't think I would be upgrading to an Arc, but rather to a single Future that would then deligate the wakes (I'm using the actor pattern).

Can you say more about this? What is the structure of a Task inside your application? What does "delegate the wakes" mean? I'm curious to hear about alternative ways of structuring tasks, as this is a part of the system that has been particularly in flux since the usize parameter was removed in 0.2.

Member

cramertj commented Aug 1, 2018

@Thomasdezeeuw Right, I'm not as interested in benchmarking the cost of a single-threaded v.s thread-safe reference counting as the comparative cost of this double-allocation and pointer-chasing vs. reference-counting in a real application

So yes, internally it uses Rc. However I don't think I would be upgrading to an Arc, but rather to a single Future that would then deligate the wakes (I'm using the actor pattern).

Can you say more about this? What is the structure of a Task inside your application? What does "delegate the wakes" mean? I'm curious to hear about alternative ways of structuring tasks, as this is a part of the system that has been particularly in flux since the usize parameter was removed in 0.2.

@Thomasdezeeuw

This comment has been minimized.

Show comment
Hide comment
@Thomasdezeeuw

Thomasdezeeuw Aug 1, 2018

Currently for waking on the same thread I use mio-st's (the name of the fork) Notifier struct, which is basically an Id (usize) and an Rc to a struct Poller which holds a Vec<Id>. Waking is as simple as adding the id to the vector, later these will be drained and the futures will be run.

For the multi-threaded waker I have to following design idea, but not yet implemented. Have a single struct, lets call this MultiThreadedWaker, hold a reference to Poller much like Notifier. The other side of the channel will the be Waker implementation that will just send an Id to the MultiThreadedWaker. MultiThreadedWaker will poll from the channel in a loop, implemented as a Future, and just adds the Id to Poller, much like the thread-local version.

I think this wouldn't allocate (much), only the initial setup and maybe adding onto the channel, this would depend on the implementation. So I wouldn't be double-allocating in this design.

(I know of the design flaw that It's unknown when to poll the MultiThreadedWaker future.)

Thomasdezeeuw commented Aug 1, 2018

Currently for waking on the same thread I use mio-st's (the name of the fork) Notifier struct, which is basically an Id (usize) and an Rc to a struct Poller which holds a Vec<Id>. Waking is as simple as adding the id to the vector, later these will be drained and the futures will be run.

For the multi-threaded waker I have to following design idea, but not yet implemented. Have a single struct, lets call this MultiThreadedWaker, hold a reference to Poller much like Notifier. The other side of the channel will the be Waker implementation that will just send an Id to the MultiThreadedWaker. MultiThreadedWaker will poll from the channel in a loop, implemented as a Future, and just adds the Id to Poller, much like the thread-local version.

I think this wouldn't allocate (much), only the initial setup and maybe adding onto the channel, this would depend on the implementation. So I wouldn't be double-allocating in this design.

(I know of the design flaw that It's unknown when to poll the MultiThreadedWaker future.)

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 24, 2018

Member

A similar API to the one proposed by this RFC is currently implemented and available on nightly (std::task, std::future) and is being experimented with in the 0.3 release of the futures-preview crate, which supports the nightly implementation of the async/await RFC. The implemented API has been evolving over time as it is used in Fuchsia and in combination with Tokio via the futures 0.1 compatibility shim.

I'd like to propose that we postpone this RFC as-is until we've had a chance to test out and iterate the current implementation. Proposals for changes can be discussed via issues on the futures-rs Github and on the wg-net-async Discord channel. Once the system has stopped undergoing significant churn or new discussion around the APIs that would be stabilized in std::task and std::future, we can reopen a new RFC that reflects the results of the experimentation. I think this approach gives us maximal flexibility while iterating and testing, while at the same time reducing the confusion that could result from in-place changes to this RFC.

Member

cramertj commented Aug 24, 2018

A similar API to the one proposed by this RFC is currently implemented and available on nightly (std::task, std::future) and is being experimented with in the 0.3 release of the futures-preview crate, which supports the nightly implementation of the async/await RFC. The implemented API has been evolving over time as it is used in Fuchsia and in combination with Tokio via the futures 0.1 compatibility shim.

I'd like to propose that we postpone this RFC as-is until we've had a chance to test out and iterate the current implementation. Proposals for changes can be discussed via issues on the futures-rs Github and on the wg-net-async Discord channel. Once the system has stopped undergoing significant churn or new discussion around the APIs that would be stabilized in std::task and std::future, we can reopen a new RFC that reflects the results of the experimentation. I think this approach gives us maximal flexibility while iterating and testing, while at the same time reducing the confusion that could result from in-place changes to this RFC.

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Aug 24, 2018

Contributor

@cramertj I've been wondering what the difference is between an "accepted RFC that has an unstable impl in std" and a "postponed/not-accepted RFC that has an unstable impl in std". They seem functionally the same...

Contributor

seanmonstar commented Aug 24, 2018

@cramertj I've been wondering what the difference is between an "accepted RFC that has an unstable impl in std" and a "postponed/not-accepted RFC that has an unstable impl in std". They seem functionally the same...

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 24, 2018

Member

@seanmonstar

I've been wondering what the difference is between an "accepted RFC that has an unstable impl in std" and a "postponed/not-accepted RFC that has an unstable impl in std". They seem functionally the same...

There are definitely similarities, but the key difference is that we won't stabilize a feature without first accepting an RFC followed by FCP, then having a separate stabilization proposal for the feature followed by FCP. Normally we don't allow implementation of features that haven't been through the RFC/eRFC process, but in this case the work in std was necessary in order to implement and experiment with RFC 2394 (async/await).

The key takeaway is that nothing here can be proposed for stabilization until an RFC has been merged.

Member

cramertj commented Aug 24, 2018

@seanmonstar

I've been wondering what the difference is between an "accepted RFC that has an unstable impl in std" and a "postponed/not-accepted RFC that has an unstable impl in std". They seem functionally the same...

There are definitely similarities, but the key difference is that we won't stabilize a feature without first accepting an RFC followed by FCP, then having a separate stabilization proposal for the feature followed by FCP. Normally we don't allow implementation of features that haven't been through the RFC/eRFC process, but in this case the work in std was necessary in order to implement and experiment with RFC 2394 (async/await).

The key takeaway is that nothing here can be proposed for stabilization until an RFC has been merged.

@jimmycuadra

This comment has been minimized.

Show comment
Hide comment
@jimmycuadra

jimmycuadra Aug 24, 2018

This would've been the perfect application of an eRFC in my opinion. Ideally the general outline of what was desired would've been proposed here and implementation experiments begun once it was merged. That way there's a clear process of:

  1. eRFC, stating what the goal is and forming agreement that experimentation in nightly is a desired.
  2. Experimentation in implementation to determine the best API.
  3. RFC proposing the API that was settled on.

jimmycuadra commented Aug 24, 2018

This would've been the perfect application of an eRFC in my opinion. Ideally the general outline of what was desired would've been proposed here and implementation experiments begun once it was merged. That way there's a clear process of:

  1. eRFC, stating what the goal is and forming agreement that experimentation in nightly is a desired.
  2. Experimentation in implementation to determine the best API.
  3. RFC proposing the API that was settled on.
@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 24, 2018

Member

@jimmycuadra I agree! I think that would've been a good way to do it. As the situation stands, I think the async/await RFC is roughly acting as our eRFC here in that it explains in detail what syntax will be supported, but not what the actual libraries or implementation will look like, so that aspect is still undergoing experimentation, and a new RFC will be opened once the API has consensus and is changing infrequently.

Member

cramertj commented Aug 24, 2018

@jimmycuadra I agree! I think that would've been a good way to do it. As the situation stands, I think the async/await RFC is roughly acting as our eRFC here in that it explains in detail what syntax will be supported, but not what the actual libraries or implementation will look like, so that aspect is still undergoing experimentation, and a new RFC will be opened once the API has consensus and is changing infrequently.

@MajorBreakfast

This comment has been minimized.

Show comment
Hide comment
@MajorBreakfast

MajorBreakfast Aug 24, 2018

Contributor

I agree with you both:

  • I think this should have been an eRFC
  • and I think we should write a proper RFC once everything has settled down. This will make it possible to get extensive feedback on the proposed final design
Contributor

MajorBreakfast commented Aug 24, 2018

I agree with you both:

  • I think this should have been an eRFC
  • and I think we should write a proper RFC once everything has settled down. This will make it possible to get extensive feedback on the proposed final design
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 30, 2018

Member

@rfcbot fcp postpone

Formally registering @cramertj's postponement above!

Member

alexcrichton commented Aug 30, 2018

@rfcbot fcp postpone

Formally registering @cramertj's postponement above!

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 30, 2018

Team member @alexcrichton has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Aug 30, 2018

Team member @alexcrichton has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 3, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Sep 3, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Sep 3, 2018

Member

If the RFC has been postponed, why is there a final comment period?

Member

carllerche commented Sep 3, 2018

If the RFC has been postponed, why is there a final comment period?

@mark-i-m

This comment has been minimized.

Show comment
Hide comment
@mark-i-m

mark-i-m Sep 3, 2018

Contributor

FCP is part of the normal process. The RFC is not actually postponed until FCP finishes.

Contributor

mark-i-m commented Sep 3, 2018

FCP is part of the normal process. The RFC is not actually postponed until FCP finishes.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 3, 2018

Member
Member

steveklabnik commented Sep 3, 2018

@golddranks

This comment has been minimized.

Show comment
Hide comment
@golddranks

golddranks Sep 4, 2018

Is there going to be quick-n-dirty eRFC for justifying the experimentation in compiler/stdlib, or is that too much bureaucracy? (Only the proper RFC once we are ready for that?)

golddranks commented Sep 4, 2018

Is there going to be quick-n-dirty eRFC for justifying the experimentation in compiler/stdlib, or is that too much bureaucracy? (Only the proper RFC once we are ready for that?)

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Sep 5, 2018

Member

@golddranks The RFC justifying the current experimentation is the async/await RFC.

Member

cramertj commented Sep 5, 2018

@golddranks The RFC justifying the current experimentation is the async/await RFC.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 13, 2018

The final comment period, with a disposition to postpone, as per the review above, is now complete.

By the power vested in me by Rust, I hereby postpone this RFC.

rfcbot commented Sep 13, 2018

The final comment period, with a disposition to postpone, as per the review above, is now complete.

By the power vested in me by Rust, I hereby postpone this RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment