add TaskPool::spawn_pollable so that the consumer does not need a blo… #4102

pubrrr · 2022-03-04T15:02:22Z

Objective

add TaskPool::spawn_pollable so that the consumer does not need a block_on method

Solution

add TaskPool::spawn_pollable that uses a receiver to get the result of the async task.

This is basically a rework of PR #2691 with the review comments incorporated. In PR #4062 this was considered the better option.
Also see #2677 and #4045.

crates/bevy_tasks/src/pollable_task.rs

examples/async_tasks/async_compute.rs

pubrrr · 2022-03-04T15:55:02Z

@DJMcNab you're welcome to provide feedback :)

crates/bevy_tasks/src/task_pool.rs

examples/async_tasks/async_compute.rs

cart · 2022-03-19T04:37:16Z

crates/bevy_tasks/src/pollable_task.rs

+/// on every frame update without blocking on a future
+#[derive(Debug)]
+pub struct PollableTask<T> {
+    receiver: Receiver<T>,


Is using channels actually preferable to calling future::block_on? Has anyone run benchmarks? Will this work on wasm? (pretty sure future::block_on doesn't, but its still worth checking)
Are there alternatives that don't involve async channels (which will allocate and use atomics)?

That looks like #2650.

I haven't run the numbers on it though. We need to get data into the ecs from outside the ecs, which means that it needs to live somewhere with a stable address so that both sides can talk to each other. Currently, that's where the Task lives, in this PR it's in the channel internals.

Adding new items can also happen at any time relative to the system code happening, which means we definitely need atomics in some form.

There were these benches from the original author, hanabi1224.

https://github.com/hanabi1224/bevy/blob/accee1bb4e1b3ea8c628199c0fd4ec02badc594f/examples/async_tasks/async_bench.rs

Looking at it, they probably should be ported to criterion and a bench needs to be added for the approach in this PR.

I think another option is to use or implement something similar to FuturesExt::now_or_never. It would have almost no overhead compared to using future::block_on.

I can confirm the example doesn't currently compile on wasm, since the single threaded task pool doesn't have the spawn_pollable api. You might be able to get it to work on wasm by detaching the task instead of storing it and then copying the code for spawn pollable over to the single threaded task pool.

TheRawMeatball · 2022-04-28T20:10:09Z

#4627 aims to be a simpler and faster alternative to this.

hymm · 2022-05-03T18:36:01Z

I updated #2650 to also include the approach in this branch: https://github.com/hymm/bevy/blob/async-bench/examples/async_tasks/async_bench.rs

[no_poll_once]        n_frames executed: 292, avg fps: 59.1(target:120), duration: 4.939s
[noop_waker]          n_frames executed: 221, avg fps: 44.6(target:120), duration: 4.959s
[handle_tasks]        n_frames executed: 121, avg fps: 24.2(target:120), duration: 5.008s
[handle_tasks_par]    n_frames executed: 146, avg fps: 29.1(target:120), duration: 5.013s
[handle_tasks_par_2]  n_frames executed: 134, avg fps: 26.9(target:120), duration: 4.983s
[async_channel]       n_frames executed: 292, avg fps: 59.3(target:120), duration: 4.925s

This seems to be significantly faster than the noop waker approach.

DJMcNab

Broadly I'm in agreement with this change, I think it's a lot nicer.

No concerns inherent to the solution, just some concerns around naming and similar things.

DJMcNab · 2022-05-03T18:55:14Z

crates/bevy_tasks/src/task_pool.rs


+    /// Spawns a static future onto the thread pool. The returned `PollableTask` is not a future,
+    /// but can be polled in system functions on every frame update without being blocked on
+    pub fn spawn_pollable<T>(


I'm not a huge fan of this name (and its derivates, i.e. PollableTask and PollableTask::poll); normal tasks can also be polled, using Future::poll.

Unfortunately, I don't have any concrete suggestions for a better name.

I'm not very of the name either, but I couldn't come up with an alternative, so I just took the name from the original PR.
CheckableTask? ChannelledTask? Simply calling it Task, too, identifying it via the package: ...::channelled::Task?

Based on C++, I think spawn_async would be a good candidate.

https://en.cppreference.com/w/cpp/thread/async

I was also looking for similar names when I first started to investigate.

crates/bevy_tasks/src/task_pool.rs

DJMcNab · 2022-05-03T20:06:06Z

crates/bevy_tasks/src/task_pool.rs

+            let result = future.await;
+            match sender.send(result).await {
+                Ok(_) => {}
+                Err(_) => warn!("Could not send result of task to receiver"),


This error message could/should be expanded. E.g.

Suggested change

Err(_) => warn!("Could not send result of task to receiver"),

Err(_) => error!("Sending result for future {future_name} (`Future<Output={return_name}>`) failed, because the recieving `PollableTask` was dropped"),

where future_name is std::any::type_name::<[F]>(), i.e. the type name of the future and return_name was std::any::type_name::<T>()

Additionally, it might be worth tracking the caller location of this function for this diagnostic. That would need to be outside of the async move block.

How to do the tracking of the caller location?

The caller location, or caller id could be given optionally to the spawn interface to let the user decide what identification system to use.

However I think this suggestion might be a better for for a later PR, it would be best introduced if
a. there's a real need for it from the users
b. later on when the initial feature is already introduced

DJMcNab · 2022-05-03T20:09:29Z

crates/bevy_tasks/src/task_pool.rs

+                return;
+            }
+
+            std::thread::sleep(Duration::from_secs_f32(1. / 100.));


Sleeping in tests is very much not ideal. I think we would check the output from the relevant channel from each task

I.e. write this test relatviely async, using e.g. TaskPoll::scope

I don't quite get how I should use TaskPool::scope here. I want to test that a PollableTask will return something. Should I wrap those pollable tasks in a scope to do the waiting there?

examples/async_tasks/async_compute.rs

DJMcNab · 2022-05-03T20:15:45Z

crates/bevy_tasks/src/pollable_task.rs

+            Err(try_error) => match try_error {
+                TryRecvError::Empty => None,
+                TryRecvError::Closed => {
+                    panic!("Polling on the task failed because the connection was already closed.")


Is this panic-worthy? I'm not sure exactly when the actual spawned future is dropped, but I think it would be reasonable for the executor to drop it once it reaches Ready once.

I'd say it might be better to add e.g. an AtomicBool to this object, and print a warning in this case, at most once for each task. I'm not sure of the exact details of the executor though

I added the panic due to this discussion: #4102 (comment)

Hmm, it seems quite harsh to stop the world for running the check after the task has ended.

I don't want to block on this; I don't feel very strongly either way.

One solution would be to return a result, but also log a warning

alice-i-cecile · 2022-06-02T20:17:19Z

We need to decide between this approach and the one in #4627, correct?

hymm · 2022-06-08T21:22:12Z

We need to decide between this approach and the one in #4627, correct?

Given the perf results I think this PR would be preferred. I think we're just waiting on DJMcNab's comments to be addressed.

…ck_on method

pubrrr · 2022-06-12T11:27:22Z

After the rebase, the example I initially changed became irrelevant (https://github.com/bevyengine/bevy/blob/main/examples/async_tasks/async_compute.rs). Should I add another one or is the unit test enough?

alice-i-cecile · 2022-06-12T15:30:37Z

@pubrrr I'd like to see another example; working with async tasks like this is a common point of confusion.

bas-ie · 2024-10-02T06:06:38Z

PR backlog cleanup: I'm less certain if this PR needs a tracking issue. @alice-i-cecile is the problem still relevant in current Bevy? Will close, but happy to make an issue as required. @pubrrr feel free to chime in, of course.

davids91

I added my comments already to the review before me, I couldn't find anything else to note.

@alice-i-cecile Do you still require adding example code for this?

@pubrrr do you think it would be feasible to finish this? I too need this functionality, but would rather avoid yet another PR for the same issue.
Would much rather support you in this one.

pubrrr · 2025-05-17T12:47:43Z

@pubrrr do you think it would be feasible to finish this? I too need this functionality, but would rather avoid yet another PR for the same issue.
Would much rather support you in this one.

Unfortunately I don't have time currently. Feel free to pick up my changes, you seem to have read it anyway already, I don't really know what I did here 3 years ago. Also I don't know how much of a hassle it would be to rebase it to the current main.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 4, 2022

pubrrr force-pushed the pollableTask branch from 2d868eb to 7c655f1 Compare March 4, 2022 15:03

pubrrr commented Mar 4, 2022

View reviewed changes

crates/bevy_tasks/src/pollable_task.rs Outdated Show resolved Hide resolved

pubrrr commented Mar 4, 2022

View reviewed changes

examples/async_tasks/async_compute.rs Outdated Show resolved Hide resolved

pubrrr force-pushed the pollableTask branch 2 times, most recently from 4bfe52c to 9a2a7e8 Compare March 4, 2022 15:29

alice-i-cecile added A-Tasks Tools for parallel and async work C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 4, 2022

hymm reviewed Mar 5, 2022

View reviewed changes

crates/bevy_tasks/src/task_pool.rs Outdated Show resolved Hide resolved

examples/async_tasks/async_compute.rs Outdated Show resolved Hide resolved

pubrrr mentioned this pull request Mar 6, 2022

[Merged by Bors] - do not impl Component for Task #4113

Closed

pubrrr force-pushed the pollableTask branch 3 times, most recently from 6bdf4ee to 681afa1 Compare March 8, 2022 16:00

pubrrr requested review from alice-i-cecile, bjorn3 and hymm March 12, 2022 21:30

hymm approved these changes Mar 13, 2022

View reviewed changes

pubrrr mentioned this pull request Mar 13, 2022

Task - delete from public API or add an example? #4198

Open

cart reviewed Mar 19, 2022

View reviewed changes

hymm mentioned this pull request Apr 28, 2022

Add convenience method for polling Task<> #4627

Closed

hymm pushed a commit to hymm/bevy that referenced this pull request May 3, 2022

add approach from bevyengine#4102

44097b7

DJMcNab reviewed May 3, 2022

View reviewed changes

alice-i-cecile added the C-Performance A change motivated by improving speed, memory usage or compile times label May 18, 2022

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 2, 2022

pubrrr added 3 commits June 12, 2022 12:20

add TaskPool::spawn_pollable so that the consumer does not need a blo…

f51e639

…ck_on method

make test more robust

550a0f1

improve error message when sending the result fails

8163fde

pubrrr force-pushed the pollableTask branch from 681afa1 to 8163fde Compare June 12, 2022 11:25

bjorn3 removed their request for review December 11, 2022 15:58

hymm mentioned this pull request Aug 27, 2023

Update Async Task documentation #9594

Open

bas-ie closed this Oct 2, 2024

davids91 mentioned this pull request May 14, 2025

TaskPool.spawn_pollable #2691

Closed

davids91 reviewed May 14, 2025

View reviewed changes

	Err(_) => warn!("Could not send result of task to receiver"),
	Err(_) => error!("Sending result for future {future_name} (`Future<Output={return_name}>`) failed, because the recieving `PollableTask` was dropped"),

Uh oh!

add TaskPool::spawn_pollable so that the consumer does not need a blo… #4102

add TaskPool::spawn_pollable so that the consumer does not need a blo… #4102

Uh oh!

Conversation

pubrrr commented Mar 4, 2022

Objective

Solution

Uh oh!

Uh oh!

Uh oh!

pubrrr commented Mar 4, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRawMeatball commented Apr 28, 2022

Uh oh!

hymm commented May 3, 2022

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jun 2, 2022

Uh oh!

hymm commented Jun 8, 2022

Uh oh!

pubrrr commented Jun 12, 2022

Uh oh!

alice-i-cecile commented Jun 12, 2022

Uh oh!

bas-ie commented Oct 2, 2024

Uh oh!

davids91 left a comment

Choose a reason for hiding this comment

Uh oh!

pubrrr commented May 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!