-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Refactor system diagnostics to use a single task #20852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor system diagnostics to use a single task #20852
Conversation
This solves the same problem as #20845: we should pick one and coalesce our efforts :) |
399899a
to
c51744d
Compare
I'll have to compare this with the other PR. It would be good for you too read over the comments on that one. |
I just realized, I will review #20845 as well. |
I measured the CPU usage between main, #20852 and #20845 letting it run for 20 seconds per test. #20852 (this pull request)
#20845It's not reporting correctly, my system CPU usage is 22%.
It works, when we fix https://github.com/bevyengine/bevy/pull/20845/files#r2319871486 , my system CPU usage drops to 16%, and it starts reporting correctly.
main (0ffd9d6)
|
701a0d7
to
1fdfb72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with this solution, I wrote my PR quite quickly honestly. And clearly that did not help with the quality. This looks better, and it's clear @dloukadakis has thoroughly tested this.
crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs
Outdated
Show resolved
Hide resolved
crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs
Outdated
Show resolved
Hide resolved
f022f67
to
f247499
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. This pattern, keeping a task around which we wake every frame, is very interesting. I wonder if there are other places within the engine where this would be useful.
Since this uses some bits of async that people may not be familiar with, I'd really appreciate a small doc comment that explains what's going on. Maybe on the plugin itself?
crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs
Outdated
Show resolved
Hide resolved
crates/bevy_diagnostic/src/system_information_diagnostics_plugin.rs
Outdated
Show resolved
Hide resolved
System information diagnostics now uses a single task that wakes up every time the `First` schedule runs. The task checks if enough time has passed since the last refresh. If enough time has passed, it refreshes the system information and sends it to a channel. The `read_diagonstic_task` system then reads the system information from the channel to add diagnostic data.
c82c366
to
f1157b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work here, thank you.
Objective
Fixes #20802
Solution
System information diagnostics now uses a single task that wakes up every
time the
First
schedule runs. The task checks if enough time haspassed since the last refresh. If enough time has passed, it refreshes
the system information and sends it to a channel. The
read_diagonstic_task
system then reads the system information from thechannel to add diagnostic data.
Testing
I used the log diagnostics example and compare before the changes and after the changes to ensure it works similarly or better than before.
Linux