Skip to content
This repository has been archived by the owner on Oct 30, 2019. It is now read-only.

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

Closed
yoshuawuyts opened this issue Apr 18, 2019 · 8 comments · Fixed by #18
Closed

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

yoshuawuyts opened this issue Apr 18, 2019 · 8 comments · Fixed by #18
Labels
enhancement New feature or request

Comments

@yoshuawuyts
Copy link
Collaborator

As per rust-lang/futures-rs#1352 and rust-lang/futures-rs#1494 FutureObj is going to be deprecated. We should update our usage to use BoxFuture instead.

@yoshuawuyts yoshuawuyts changed the title Upgrade from FutureObj Move from StreamObj to Pin<Box<dyn Future>> Apr 20, 2019
@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Apr 22, 2019
@Nemo157
Copy link
Member

Nemo157 commented Apr 22, 2019

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
Copy link
Collaborator Author

yoshuawuyts commented Apr 23, 2019

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 withoutboats/juliex#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
Copy link
Member

Nemo157 commented Apr 23, 2019

Oh, looking back at rust-lang/futures-rs#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
Copy link
Collaborator Author

@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
Copy link

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
Copy link
Member

Nemo157 commented Apr 27, 2019

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
Copy link

Ah my bad I mistook it for LocalFutureObj

@Nemo157
Copy link
Member

Nemo157 commented Apr 27, 2019

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".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants