Internalize task distinction into TaskPool

[james7132]

Objective

Fixes #1907. IO and Async Compute threads are sitting idle even when the compute threads are at capacity. This results in poor thread utilization.

Solution

Move the distinction between compute, async compute, and IO threads out of userspace, and internalize the distinction into TaskPool. This creates three tiers of threads and tasks: one for each group. Higher priority tasks can run on lower priority threads, if and only if they're currently idle, but the lower priority threads will prioritize their specific workload over higher priority ones. Priority goes compute > io > async compute.

For example, heavy per-frame compute workload, async compute and IO otherwise sitting idle: compute tasks will be scheduled onto async compute and IO threads whenever idle. Any IO task that is awoken will take precedence over the compute tasks.

In another case, a heavy IO workload will schedule onto idle async compute threads, but will never use a compute thread. This prevents lower priority tasks from starving out higher priority tasks.

The priority scheme chosen assumes well-behaved scheduling that adheres to the following principles:

• Compute tasks are latency sensitive and generally do not hold the thread indefinitely or at the very minimum for time longer than the course of a frame.
• IO tasks are not CPU intensive and will do minimal processing. Generally will yield readily at any incoming or outbound communication, and thus will only generally be under consistent heavy load only in specific circumstances.
• Async compute tasks may run for a very long time, across multiple frames, and will generally not yield at any point during a given task's execution. Only under heavy load when a developer explicitly needs it, and can otherwise be used for IO or compute in the general case.

TODO:

• Add two additional executors to TaskPool
• Set up TaskPool threads to run tasks from all 3 with some prioritization
• Add user-facing APIs for choosing which priority to spawn tasks under.
• Remove the usages newtypes. Fix up dependent code.
• Properly set up individual thread groups and their individual prioritization schemes.
• Update docs.

---

Changelog

Changed: Newtypes of TaskPool (ComputeTaskPool, AsyncComputeTaskPool, and IoTaskPool) have been replaced with TaskPool::spawn_as with the appropriate TaskGroup. Thread utilization across these segments has been generally been increased via internal task prioritization. For convenience, TaskPool::spawn will always spawn on the compute task group.

Migration Guide

ComputeTaskPool, AsyncComputeTaskPool, and IoTaskPool have been removed. These individual TaskPools have been merged together, and can be accessed by Res<TaskPool>. To spawn a task on a specific segment of the threads, use TaskPool::spawn_as with the appropriate TaskGroup.

Before:

fn system(task_pool: Res<AsyncComputeTaskPool>) {
   task_pool.spawn(async {
       ...
   });
}

After:

fn system(task_pool: Res<TaskPool>) {
   task_pool.spawn(TaskGroup::AsyncCompute, async {
       ...
   });
}

[hymm]

I had a slightly different idea on how to combine the task pools. I wanted to give tasks spawned on a scope the highest priority; give tasks spawned onto the task pool a lower priority; and then add a new spawn_blocking function that works like it does in tokio where these tasks are spawned onto a different set of threads. It reuses an idle thread if there are any or spawns a new thread when there aren't any.

[james7132]

I had a slightly different idea on how to combine the task pools. I wanted to give tasks spawned on a scope the highest priority; give tasks spawned onto the task pool a lower priority; and then add a new spawn_blocking function that works like it does in tokio where these tasks are spawned onto a different set of threads. It reuses an idle thread if there are any or spawns a new thread when there aren't any.

I think the first two parts are still reconcilable with the current proposed approach; however, the problem with spawn_blocking as a general approach is that if a compute thread is idle, you can easily starve out the compute tasks, which is unacceptable in a game engine. Spinning up and down threads is also an overhead we generally want to avoid too, particularly since it can cause even heavier thrashing on the CPU for the other threads.

[hymm]

however, the problem with spawn_blocking as a general approach is that if a compute thread is idle, you can easily starve out the compute tasks, which is unacceptable in a game engine.

isn't that a problem with this approach too. If a user starts too many blocking tasks the compute task pool won't have any threads to work with either.

edit: thinking on this more I guess. The approach here prevents the user from running more than x blocking tasks at the same time. So the surprise for the user is that when they run too many blocking tasks, they only get x running in parallel. Even if they were expecting them to all run in parallel. In the case I suggest, they run as many as they want, but suddenly their game starts stuttering because they've used all the threads.

[james7132]

isn't that a problem with this approach too. If a user starts too many blocking tasks the compute task pool won't have any threads to work with either.

If and only if they start it as a normal compute task, something that would require spawn_blocking should be scheduled to the async compute segment, where under this approach, it'll be delayed until the thread(s) are available.

[james7132]

bors try

[bors]

try

Build failed:

• miri

[james7132]

@cart @hymm I've mostly updated the PR and this is once again up for review. #4466 added an unusual interaction since we need a temporary Executor, which I originally made with the assumption that it would be static and never be dropped. There were a few optimizations that definitely decrease overhead when (re)scheduling tasks. I can make another "SimpleExecutor" (as a Send-only MPSC executor) for this case that resolves this, but the problem remains that Executor is not meant to handle being dropped.

Should we:

1. Take the perf hit and revert those changes, supporting dropping Executor and by proxy TaskPool?
2. Or go all in on a global TaskPool and keep the optimizations?

2 would move us closer to a tokio-esque implementation where there's a single global runtime, while 1 would keep what we have and retain the approach that bevy_tasks has had thus far. Which one should we pursue? I'm personally inclined to push for 2 since:

• it lets us make those assumptions and cut more overhead out
• allows for more ergonomic initialization (i.e. #[tokio::main] style #[bevy::main]),
• this PR already reduces/removes the need to pass around &TaskPool or some newtype of it.
• provides a bit of familiarity for people using tokio's APIs if there's a clear mapping between the two.

[hymm]

We don't necessarily need a temporary executor. We just need a way of running tasks that only run on the thread the scope is running on. This could potentially be a thread local storage queue that can't be stolen from.

A consideration here is that in #6503, I removed the temporary executor for a global main thread executor that has run time checks to only allow it to be ticked on the main thread. But not sure if that is going to get merged or not.

[hymm]

I'm leaning towards option 2. Seems like there isn't much need for users to construct their own taskpools now that we're using global task pools. The one issue might be if users will want to reconfigure the number of threads or thread distribution during execution.

On another note, it might be a good idea to port the micro benchmarks over from async_executor to get a better idea of how the current executor compares to the original.

[hymm]

Will this completely block us off from using the tokio ecosystem forever on our task pools? Or would we still be able to switch the tokio executor into our task pools somehow even if it doesn't have all the niceties (priorities)?

[hymm]

My biggest problem with reviewing this PR is that I have no idea how to understand the changes that were made to the async executor. I can look at the diff https://github.com/james7132/bevy/compare/b5fc0d2..task-pool-internalization and figure out what it's doing, but I have no context for why some things were changed. If you could port over the tests and benches from async executor I would have a lot more confidence that things are working as they're supposed to. Bonus points if you can get loom working somehow.

[inodentry]

Just something to note: this sort of PR, where we have very tricky performance implications due to multithreading, different CPU cores being involved, etc, can be very sensitive to CPU hardware boost behaviors and such!

I am worried that there is a high risk of benchmarks being wildly inaccurate (when it comes to judging how much more/less efficient bevy's scheduling has become), due to different thread utilization, causing different boost clocks of different CPUs, etc. The benchmark results may therefore be skewed because of the particular CPU model of the computer they are run on.

I recommend, if possible, to benchmark this PR (and other multithreading/scheduling things) on a machine where all CPU cores can be forced to run at the same clock speeds (via BIOS settings, etc, to disable boost and clock frequency scaling).

Otherwise we can't really know how much we actually win/lose due to better multithreading / thread utilization, and how much of it was due to the particular CPU hardware boosting differently due to its cores being loaded differently.

This kind of thing is important in general, but for these sorts of PRs especially so. I would not trust benchmarks that were not done that way.

[goshhhy]

I recommend, if possible, to benchmark this PR (and other multithreading/scheduling things) on a machine where all CPU cores can be forced to run at the same clock speeds (via BIOS settings, etc, to disable boost and clock frequency scaling).

i have a machine with 272 threads, where the cores are all running the same speed, and as long as at least 3 cores are active then there is only 100mhz difference between base and boost clocks (1400MHz vs 1500MHz; 1600 is possible with 2 or fewer cores active), and i may be able to disable boost entirely.

i also have a simulation workload for that machine, which is affected by #1907, with rather severe underutilization of threads compared to what i would hope for.

i will test this PR with that machine/workload and share the results.

[james7132]

Going to return to the drawing board on this one. Taking on an entire fork of async_executor might not be in our interests. In the meantime, I'm potentially looking to experiment with switchyard, which provides better per-thread control over task allocation.