rustasync / runtime

Empowering everyone to build asynchronous software
https://docs.rs/runtime
Apache License 2.0
862 stars 28 forks source link

Move from StreamObj to Pin<Box<dyn Future>> #6

Closed yoshuawuyts closed 5 years ago

yoshuawuyts commented 5 years ago

As per https://github.com/rust-lang-nursery/futures-rs/issues/1352 and https://github.com/rust-lang-nursery/futures-rs/pull/1494 FutureObj is going to be deprecated. We should update our usage to use BoxFuture instead.

Nemo157 commented 5 years ago

It looks like runtime's usage of FutureObj is the one remaining usecase, interacting with the Spawn trait. It makes sense to me that the Runtime trait would directly wrap this for performance reasons (I assume this trait is an implementation detail between runtime and its implementors, and users won't be calling Runtime::spawn_obj directly?).

yoshuawuyts commented 5 years ago

I assume this trait is an implementation detail between runtime and its implementors, and users won't be calling Runtime::spawn_obj directly?

@Nemo157 yep, that's accurate. runtime::spawn is the preferred API we expose. For our bindings to Juliex we call spawn_obj on juliex's threadpool to prevent double boxing (introduced in https://github.com/withoutboats/juliex/pull/13), which is indeed just a perf optimization.

I take it this means we can't move away from FutureObj until Spawn is updated. Is there a plan there?

Nemo157 commented 5 years ago

Oh, looking back at https://github.com/rust-lang-nursery/futures-rs/issues/1352 I remember I proposed adding a separate helper for spawning boxed futures to avoid the double indirection:

trait SpawnExt {
    fn spawn_box(&mut self, future: BoxFuture<'static, ()>) -> Result<(), SpawnError> { ... }
}

Since Runtime is std-only it could change its API to take a BoxFuture then pass that to Juliex via spawn_box which will directly convert it to a FutureObj.

EDIT: In fact, even without adding that API I guess Runtime could take in BoxFuture, then the Juliex implementation can wrap that into a FutureObj before passing to Juliex. You won't be able to eliminate FutureObj usage completely without the double indirection if you're using Spawn to interact with Juliex, but you can eliminate it from all the public APIs.


I believe the long-term plan for Spawn is to have

fn spawn(&mut self, future: dyn Future<Output = ()> + 'static) -> Result<(), SpawnError>;

it is actually possible to declare that API today using the unsized_locals feature, but it looks like there's no way to then take that unsized local and box it, and I can't figure out how to convert an impl Future into a dyn Future to be able to call it.

With that API it is then up to the implementor how to allocate space for the future, the simplest solution would be to just Box it like is done currently, but it also allows more interesting allocation strategies (e.g. a limited size memory pool that will return an error if there's not enough space to allocate for the current future).

I don't remember where this was previously discussed, I'll try and find some references for it later. One thing that needs to be checked is what the migration plan was, I expect Runtime would want to switch to a similar unsized argument method once Spawn changes, and you would need to make sure you can migrate to it without unnecessary boxing.

yoshuawuyts commented 5 years ago

@Nemo157 thanks for the detailed response! Def sounds tricky; but yeah I'm also not opposed to having some minor perf regressions if we can later speed it back up again.

DoumanAsh commented 5 years ago

I don't understand exact reasoning for deprecating it if it forces users to use boxed futures always.

Having dyn Future instead of Box<dyn Future> should be alternative to FutureObj, not some long term plan :(

Nemo157 commented 5 years ago

FutureObj is Box<dyn Future>, just with support for other “Box-like types”. For std using usecases there is no reason to use FutureObj over Box<Future>, except when implementing the Spawn trait since that uses it to be no_std compatible.

DoumanAsh commented 5 years ago

Ah my bad I mistook it for LocalFutureObj

Nemo157 commented 5 years ago

Well, a bit more accurately FutureObj<'a, T>Pin<Box<dyn Future<Output = T> + Send + 'a>> and LocalFutureObj<'a, T>Pin<Box<dyn Future<Output = T> + 'a>>, they're both an abstraction over an "owned trait object".