TaskPool.spawn_pollable #2691
TaskPool.spawn_pollable #2691
Conversation
… poll_once and block_on in user code
hanabi1224
force-pushed
the
pollable_task
branch
from
104882f to
1164ee6
Compare
alice-i-cecile
added
A-Tasks
|
I've used something similar recently: create a one-shot async channel, return the wrapped receiver as the thing that can be "polled", wrap the compute future in a future that sends the result into the sender, spawn it and detach the task. |
| /// on every frame update without blocking on a future | ||
| #[derive(Debug)] | ||
| pub struct PollableTask<T> { | ||
| result: Arc<RwLock<Option<T>>>, |
There was a problem hiding this comment.
I think it's unclear why this is an Arc<RwLock>, instead of e.g. a std::sync::mpsc::Reciever<T> constructed using std::sync::mpsc::sync_channel(1).
| @@ -76,22 +75,22 @@ fn spawn_tasks(mut commands: Commands, thread_pool: Res<AsyncComputeTaskPool>) { | |||
| /// removes the task component from the entity. | |||
| fn handle_tasks( | |||
| mut commands: Commands, | |||
| mut transform_tasks: Query<(Entity, &mut Task<Transform>)>, | |||
| transform_tasks: Query<(Entity, &PollableTask<Transform>)>, | |||
There was a problem hiding this comment.
Using the PollableTask as a component is probably an anti-pattern (due to the potential for multiple libraries to create such tasks with the same result type); we should make a custom wrapper struct for this task.
(PollableTask should not be Component once [derive(Component)] lands)
| } | ||
| } | ||
|
|
||
| pub fn poll(&self) -> LockResult<RwLockReadGuard<Option<T>>> { |
There was a problem hiding this comment.
We do not need interior mutability here - having interior mutable things in the ecs is an anti-pattern, because it makes ambiguity detection less useful, for example. Additionally, we should simplify the return type to an Option, which just takes the result if it exists.
That is, imo the only method should be:
pub fn poll(&self)->Option<T> { ... }| pub struct PollableTask<T> { | ||
| result: Arc<RwLock<Option<T>>>, | ||
| // this is to keep the task alive | ||
| _task: Task<()>, |
There was a problem hiding this comment.
We should provide a way to destructure this if the user has an owned valud, to allow e.g. Task::detach to be called.
| .into_iter() | ||
| .map(|i| { | ||
| pool.spawn_pollable(async move { | ||
| futures_timer::Delay::new(Duration::from_secs_f32(0.5)).await; |
There was a problem hiding this comment.
We shouldn't have time based tests here. Just removing both sets of delays would make this a good enough test.
| let transform_fn = |i: usize| i * 3; | ||
|
|
||
| let pool = TaskPool::new(); | ||
| let nums: Vec<usize> = (1..10).into_iter().collect(); |
There was a problem hiding this comment.
Afaik, into_iter is unneeded here. I.e.
| let nums: Vec<usize> = (1..10).into_iter().collect(); | |
| let nums: Vec<usize> = (1..10).collect(); |
would be just as valid.
| @@ -309,6 +336,42 @@ mod tests { | |||
| assert_eq!(count.load(Ordering::Relaxed), 100); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_spawn_pollable() { | |||
| let transform_fn = |i: usize| i * 3; | |||
There was a problem hiding this comment.
It's unclear why you use usize throughout this function - in this case, u32 or even u8 might be clearer, since these values are only numbers; they're not used for anything other than simple arithmetic and comparison (i.e. no pointer maths/indexing)
| for i in 0..pollables.len() { | ||
| let locked = pollables[i].poll().unwrap(); | ||
| if let Some(r) = *locked { | ||
| assert_eq!(r, transform_fn(nums[i])); | ||
| } else { | ||
| done = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
| for i in 0..pollables.len() { | |
| let locked = pollables[i].poll().unwrap(); | |
| if let Some(r) = *locked { | |
| assert_eq!(r, transform_fn(nums[i])); | |
| } else { | |
| done = false; | |
| break; | |
| } | |
| } | |
| for (pollable, source) in pollables.iter().zip(nums.clone()) { | |
| let locked = pollable.poll().unwrap(); | |
| if let Some(r) = *locked { | |
| assert_eq!(r, transform_fn(source)); | |
| } else { | |
| done = false; | |
| break; | |
| } | |
| } |
Doing it this way also lets you avoid allocating nums as a vec entirely - it can stay as a Range
| if done { | ||
| return; | ||
| } | ||
| std::thread::sleep(Duration::from_secs_f32(1.0 / 60.0)); |
There was a problem hiding this comment.
If you remove the timing based parts of this task, it will make sense to change this to std::thread::yield_now
| fn test_spawn_pollable() { | ||
| let transform_fn = |i: usize| i * 3; | ||
|
|
||
| let pool = TaskPool::new(); |
There was a problem hiding this comment.
Ideally we'd have some way to limit the number of threads this spawns, since each task is extremely lightweight. That's not a problem for this PR to solve
|
Is there any progress on merging this PR? I guess this would mostly solve my questions in #4045 |
|
It just needs more reviews. If we get 3 community reviews we can then add a S-Ready-For-Final-Review label and show it to cart. It would also help if the author responded to the current reviews |
alice-i-cecile
added
C-Performance
|
Closing in favor of #4062. |
Add fn spawn_pollable->PollableTask api to TaskPool to avoid using poll_once and block_on in user code
Objective
poll_onceandblock_onin user system code as they actually do nothing but add non-trival performance overheadSolution