Ensure only one bevy diagnostics task exists #20845
Conversation
Olle-Lukowski
added
A-Tasks
|
Man, I love that I can write an issue and have someone implement it literally three days later. Thanks! I'll give this a look today. |
| { | ||
| let mut sys = self.sysinfo.lock().unwrap(); | ||
| let data = get_diagnostic_data(&mut sys); | ||
| _ = self.data_rx.send(data); |
There was a problem hiding this comment.
I think this may have been a mistake in the sketch from my example. Since send returns a future, it needs to be awaited rather than dropped.
This should be self.data_rx.send(data).await.
There was a problem hiding this comment.
send doesn't return a Future (using crossbeam-channel here). and also, how would you await aFuture inside the impl Future...
There was a problem hiding this comment.
Ah, using crossbeam-channel this will block the executor when full. You need to use try_send or an async channel.
We could also write this task as an async block to make using a channel easier.
async move {
loop {
// all the polling and stuff
data_rx.send(data).await;
futures::yeild_now().await;
}
}Should be approximately the same.
| @@ -209,8 +226,7 @@ pub mod internal { | |||
| &SystemInformationDiagnosticsPlugin::PROCESS_MEM_USAGE, | |||
| || data.process_mem_usage, | |||
| ); | |||
| false | |||
| }); | |||
| } | |||
There was a problem hiding this comment.
Looking at this a bit closer, the api for Diagnostics vs Diagnostic strikes me as very odd.
There's no reason to be using a closure for these, and we should be using the Instant::now() from the task itself and passing it back over the channel. Currently add_measurement calls Instant::now() again.
Probably out of scope for this PR.
|
We got a new PR for this, woops! I'll have to compare this with the other PR later today. You may want to give a look over #20852 as well. |
There was a problem hiding this comment.
Um..I happen to work on the same issue, I opened a duplicate pull request #20852
I have a few comments for this one.
| type Output = (); | ||
|
|
||
| fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| cx.waker().wake_by_ref(); |
There was a problem hiding this comment.
I was working on the same issue, I tried this too, but it ended up waking the task 240k times per sysinfo::MINIMUM_CPU_UPDATE_INTERVAL, it caused on average ~4% more CPU load on my system.
On #20852, to prevent this and reduce the CPU load caused by it, I decided to use a different approach, and wake up the task in the First schedule that runs at the start of every frame.
| @@ -93,9 +96,9 @@ pub mod internal { | |||
|
|
|||
| pub(super) fn setup_plugin(app: &mut App) { | |||
| app.add_systems(Startup, setup_system) | |||
| .add_systems(First, launch_diagnostic_tasks) | |||
| .add_systems(First, launch_diagnostic_job) | |||
There was a problem hiding this comment.
The First schedule runs first on every frame, it needs to be moved to the Startup schedule to have it run one time.
| let (tx, rx) = crossbeam_channel::bounded(1); | ||
| data.data_rx = Some(rx); | ||
| AsyncComputeTaskPool::get() | ||
| .spawn(DiagnosticJob { |
There was a problem hiding this comment.
This will leak because launch_diagnostic_job runs on the First schedule on every frame, adding one DiagnosticJob per frame. Please see #20845 (comment)
|
Closed in favor of #20852 |
|
Thanks for working on this. |
Objective
Fixes #20802.
Solution
Implemented the suggested approach (using a future that reschedules itself).
Testing
I tried to test it with the
log_diagnosticsexample, but ran into #20844.