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

@@ -239,6 +241,26 @@ impl TaskPool {
        Task::new(self.executor.spawn(future))
    }

+    /// 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?

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?

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.

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.

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 simple 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 hymm, alice-i-cecile and bjorn3 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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

pubrrr commented Mar 4, 2022

pubrrr commented Mar 4, 2022

cart Mar 19, 2022

DJMcNab Mar 19, 2022

hymm Apr 5, 2022

aliu Apr 13, 2022

hymm May 6, 2022

TheRawMeatball commented Apr 28, 2022

hymm commented May 3, 2022

DJMcNab left a comment

DJMcNab May 3, 2022

pubrrr May 5, 2022

DJMcNab May 3, 2022

pubrrr Jun 12, 2022

DJMcNab May 3, 2022

pubrrr Jun 12, 2022

DJMcNab May 3, 2022

pubrrr May 5, 2022

DJMcNab May 5, 2022

alice-i-cecile commented Jun 2, 2022

hymm commented Jun 8, 2022

pubrrr commented Jun 12, 2022

alice-i-cecile commented Jun 12, 2022

	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"),

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

Are you sure you want to change the base?

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

Conversation

pubrrr commented Mar 4, 2022

Objective

Solution

pubrrr commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRawMeatball commented Apr 28, 2022

hymm commented May 3, 2022

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Jun 2, 2022

hymm commented Jun 8, 2022

pubrrr commented Jun 12, 2022

alice-i-cecile commented Jun 12, 2022