Skip to content
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

Evaluate fine-grained-ness of feature flags (to enable components individually) #1709

Closed
najamelan opened this issue Oct 30, 2019 · 5 comments

Comments

@najamelan
Copy link
Contributor

This is a follow up from the discussion in #1702. The different sub-crates are being merged into one tokio crate. To avoid code bloat, the idea is to let users only enable features they need with cargo feature flags.

Currently the merging of crates has reduced a few options, and some of the features can be reviewed. From what I can tell for tokio-executor (which is what I was using most) we can enable gradual inclusion as:

  • traits (Executor/TypedExecutor)
  • current_thread Executor
  • thread_pool Executor

Maybe it can be extended to the runtime:

  • one of the two above executors
  • enable reactor
  • enable timer

In that case the distinction between executor and runtime is maybe not so relevant anymore?

On a sidenote, I might mention that tokio seems geared more towards a global spawn function approach (enter execution context and then use tokio::spawn). From a poll I did this summer, it would seem most people are more in favor of an approach where libraries take a generic executor with a E: Spawn trait bound.

I have been trying to work out for a while now a way to create a convenient way for libraries to spawn futures, while leaving it up to application code to choose which executor to use. I am settling on enabling 3 different models:

  • global spawn function (naja_async_runtime) -> I will release a new, improved version in the upcoming weeks
  • generic executor with trait bound (async_executors) -> in the works, I'm wrapping different executors to provide the Spawn and LocalSpawn traits from futures-rs and probably will add Executor and TypedExecutor from tokio.
  • a python trio like nursery -> not yet started, it would also implement spawn traits and be able to use any executor as backend.

Hence the interest for being able to build from components instead of an entire framework. It is harder to make something like this work with the need to combine reactor with executor, but if reactor and timer are conditional, it's still possible to provide wrappers around the runtime and let the end user enable the reactor feature if they need it.

@carllerche
Copy link
Member

The goal is to be able to opt-in / out of components with the same level of granularity as was possible w/ separate crates. Work on this will probably start after the tokio crate is cleaned up post collapsing all the sub crates.

You are correct that executor doesn't really make sense anymore now that everything exists in the same crate. My guess is there will be a single Runtime type that can be configured with feature flags and builder settings.

I don't think that the Executor trait is holding its own. In fact, it forces an additional allocation by virtue of using Box<dyn Future> as the argument. My guess is this trait will go away.

Could you expand on how you are using TypedExecutor? I haven't seen compelling usage of it yet. As such, I am trying to figure out if it should stay in tokio proper or be moved into tokio-util.

@najamelan
Copy link
Contributor Author

najamelan commented Oct 31, 2019

I'm writing async_executors to have traits like Spawn and LocalSpawn from futures-rs available on all current executors that people might want to chose from. Given that I have wrapper types, I thought might as well implement Executor and TypedExecutor for all of them, because why not. So I haven't yet used them as such, but was planning to do so soon.

That being said, if the traits from the futures lib are good for everyone, then the tokio traits could be removed. I would be in favor of having only one set of traits.

It would also be possible to just implement the futures traits in each executor library rather than making a wrapper. I just felt progress would be faster by wrapping since async-std doesn't provide the traits either, nor juliex and who knows for future executor implementations. I feel getting to an ecosystem wide consensus on such things is always long term work.

Since executing requires a &mut, a cheap Clone is important too. It can come from a handle method in which case the returned type should implement the traits too. I ran into one issue with the current_thread executor in that it's handle is Send and thus requires futures spawned on it to be Send too, so it can't be used for !Send futures.

Another question is the error handling. Currently the Spawn traits from futures return a futures specific error which has only a shutdown variant, whereas tokio has at_capacity. Maybe things would be easier if the Error type on those traits would be an associated type. If there would be some consensus that it's good that executors implement these traits, it would be worth proposing changing to an associated type in the futures lib I suppose.

@najamelan
Copy link
Contributor Author

ps: there is currently no trait that specifies the possiblity to spawn getting a JoinHandle. It's probably worth having an ecosystem wide way of expressing that functionality.

@carllerche
Copy link
Member

Thanks for the feedback.

As you pointed out there are. I traits that support join handles and there is no “zero cost” trait... except TypedExecutor which got minimal use.

Given this, my current plan is to just delete Tokio’s executor traits for the time being.

@carllerche
Copy link
Member

This has been done (feature flags re-evaluated).

Separately, executor types are removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants