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

Introducing block_on_worker and #[tokio::main_worker] #5446

Closed
huntc opened this issue Feb 10, 2023 · 8 comments · Fixed by #5494
Closed

Introducing block_on_worker and #[tokio::main_worker] #5446

huntc opened this issue Feb 10, 2023 · 8 comments · Fixed by #5494
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-runtime Module: tokio/runtime

Comments

@huntc
Copy link
Contributor

huntc commented Feb 10, 2023

Is your feature request related to a problem? Please describe.

Yes. Using #[tokio::main] or Runtime's block_on method directly does not run my main body or future as a worker task. The consequence is a significant drop in performance where my main body or future awaits some synchronisation involving a worker task.

For example, this:

        rt.block_on(async move {
            for _ in 0..1_000 {
                let (o_tx, o_rx) = oneshot::channel();
                task_tx.send(o_tx).await.unwrap();
                let _ = o_rx.await;
            }
        })

...will perform significantly slower than this:

        rt.block_on(async move {
            let h = tokio::spawn(async move {
                for _ in 0..1_000 {
                    let (o_tx, o_rx) = oneshot::channel();
                    task_tx.send(o_tx).await.unwrap();
                    let _ = o_rx.await;
                }
            });
            h.await
        })

Even on a "current_thread" executor.

The same will apply to any use of #[tokio::main] and all benchmarks that use block_on (most of what I've seen out there).

With the above example, I saw a degradation by a factor of 17 times. My opinion is that the majority of Tokio users would not appreciate the problem and have applications today that perform significantly slower than they can. Where benchmarks are used to provide an indication of performance, they may also be quite inaccurate.

Describe the solution you'd like

In essence, I would like to see a block_on_worker method alongside the Runtime's block_on. This would take a result that is Send. The new method would be implemented by a block_on call with its future performing a spawn.

To complement this new method, I would also like to see a #[tokio::main_worker] macro to that calls block_on_worker. Our documentation should be enhanced to promote the use of #[tokio::main_worker].

Describe alternatives you've considered

An alternative considered was to change the default implementation of #[tokio::main] so that it calls block_on_worker. However, this would not be backward-compatible due to the requirement of the future's return type to be Send.

A macro attribute such as #[tokio::main(spawn)] could also achieve the same goal, however, I feel that that looks like optional behaviour. I believe that #[tokio::main_worker] is what people want most of the time and would present more nicely.

Additional context

My original PR illustrates the problem further by providing a benchmark. There is some discussion on that PR also.

#5440

@huntc huntc added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Feb 10, 2023
@Darksonn Darksonn added the M-runtime Module: tokio/runtime label Feb 14, 2023
@carllerche
Copy link
Member

Thanks for the PR. I agree w/ the premise that #[tokio::main] performance is unfortunate and should be fixed. Adding a new API is not ideal, as it would require users to discover it.

I believe the performance issues can be addressed without an API change by using the thread that block_on is called from as a worker thread. It does add some additional complexity around handling block_on returning before the runtime shuts down, but that is manageable.

I plan on looking into fixing it in Q2 of this year, but if someone else wants to take a stab, I could probably provide some pointers. The change would not be trivial though as it will require some internal refactoring of the multi-threaded scheduler.

@huntc
Copy link
Contributor Author

huntc commented Feb 14, 2023

Thanks for the PR. I agree w/ the premise that #[tokio::main] performance is unfortunate and should be fixed. Adding a new API is not ideal, as it would require users to discover it.

Thanks for the feedback. The present issue is that users are required to discover the current behaviour. Should we at least further document the use of tokio::main and block_on? I'm happy to provide another PR with the team's blessing.

@Darksonn
Copy link
Contributor

You have my blessing for doc PRs.

@huntc
Copy link
Contributor Author

huntc commented Feb 15, 2023

I've just made an interesting discovery while re-factoring some code in my #[tokio:main] function and putting it into a tokio::spawn... I had been using a ThreadRng and on moving it to within the worker task the compiler complained about it not being able to be sent across threads i.e. !Send. While this is easy for me to solve, the issue that it raises is that changing the implementation of block_on to perform as a worker will likely break people's existing code in a similar fashion. I'm therefore wondering if escaping the introduction of an additional API will be possible.

@Darksonn
Copy link
Contributor

I believe that Carl's intended solution is to put a worker thread on the thread where block_on is called, rather than running the block_on future on another thread.

@thomaseizinger
Copy link

I plan on looking into fixing it in Q2 of this year, but if someone else wants to take a stab, I could probably provide some pointers. The change would not be trivial though as it will require some internal refactoring of the multi-threaded scheduler.

Is there a place where we can subscribe to updates around this?

@Qqwy
Copy link

Qqwy commented Oct 14, 2024

Hello from 1.5 years in the future. @carllerche did you ever get around to improving this situation? If not, we should re-open this issue or open a new one, because the problem still requires a solution.

@carllerche
Copy link
Member

I tried to make progress (see the alt-threaded runtime) but have yet to reach the workable point. I ran out of time, so that effort is stalled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants