updates on diagnostics (log + new diagnostics) #1085

mockersf · 2020-12-18T01:32:56Z

move diagnostics to log now that we have a log framework setup
add entity count diagnostic
add asset count diagnostic

I found those two diagnostics useful while debugging game that slowed down over time, to track lost entities or extra assets (less true with new auto cleanup assets)

DJMcNab · 2020-12-22T09:15:43Z

crates/bevy_diagnostic/src/log_diagnostics_plugin.rs

-/// State used by the [PrintDiagnosticsPlugin]
-pub struct PrintDiagnosticsState {
+/// State used by the [LogDiagnosticsPlugin]
+pub struct LogDiagnosticsState {


This is pub but is not exported from the crate

Maybe use pub(crate)?

It has to be pub because it's in a public interface:

crate-visible type `LogDiagnosticsState` in public interface can't leak crate-visible type

It's public, just not user addressable...

If we stop exporting the systems, then that error goes away. I'm not sure why we would be exporting the systems anyway?

It was created as public in 704a742, which was in May. I suspect it's just an oversight.

pubs are removed

DJMcNab · 2020-12-22T09:18:17Z

crates/bevy_diagnostic/src/log_diagnostics_plugin.rs

            if let Some(ref filter) = state.filter {
                for diagnostic in filter.iter().map(|id| diagnostics.get(*id).unwrap()) {
-                    Self::print_diagnostic(diagnostic);
+                    Self::log_diagnostic(diagnostic);
                }
            } else {
                for diagnostic in diagnostics.iter() {


Can these be sorted so that the diagnostics are in a consistent order?

It's not helpful every time you restart the app getting a different order.

Currently you can have them ordered if you use a filter, as the filter is stored in a Vec and then iterated over.

Without filter, the diagnostics are stored in a HashMap and that doesn't keep order... We could switch to a BTreeMap, or add a Vec to keep order of insertion and use it to iterate... What do you think?

So BTreeMap would probably be a good start, although a Vec sorted by name or by insertion order could both also be a good choice.

Basically, anything deterministic would be better than the current situation.

Edit: Given that there are generally only a handful of diagnostic types, it might make sense to just use a Vec to store them anyway and binary search to get a specific diagnostic.

Moved to a BTreeMap. After looking a little into a Vec solution, I didn't like it as it would have mean every diagnostic recording a value would have to go through the Vec...

O(logn) is better than O(n), but for a large number of diagnostics idk if its a good idea to replace a hash map. While we currently only have a small number, I don't want to assume that will always be the case. Maybe LogDiagnosticsPlugin could store its own order? Sorting is a behavior inherent to the LogDiagnosticsPlugin so that makes sense to me.

The issue is keeping the order synced up / accounting for new diagnostics. Lol we could have a Diagnostic event. Ex: DiagnosticEvent::Added(DiagnosticId), DiagnosticEvent::Removed(DiagnosticId), etc

I looked into adding an event, seems not easy as adding a diagnostics can happen at app build time by mutating a resource, so the new diagnostic plugin would be responsible from calling the event (I think)
Keeping the order in LogDiagnosticsPlugin without events seemed not pretty
I added a BTreeSet of the DiagnosticId to Diagnostics that is updated when adding a diagnostics, and that is used for iterating orderedly

DJMcNab · 2020-12-22T21:34:05Z

I have confirmed that the change does make the order consistent, which is definitely an improvement. I'm less sure about the default output format, just because there's an awful lot of noise.

mockersf · 2020-12-22T21:39:19Z

output of example log_diagnostics:

Dec 22 14:34:03.374  INFO bevy_diagnostic::log_diagnostics_plugin: frame_count                                                      : 58.000000   (avg 58.000000)
Dec 22 14:34:03.374  INFO bevy_diagnostic::log_diagnostics_plugin: frame_time                                                       : 0.015162    (avg 0.016622)
Dec 22 14:34:03.375  INFO bevy_diagnostic::log_diagnostics_plugin: fps                                                              : 60.642875   (avg 60.021922)

before, it was:

Diagnostics:
---------------------------------------------------------------------------------------------
frame_time                                                       : 0.017387    (avg 0.016682)

fps                                                              : 60.059073   (avg 59.994266)

frame_count                                                      : 58.000000   (avg 58.000000)

cart

Good calls all around!

cart · 2020-12-23T23:03:30Z

crates/bevy_diagnostic/src/entity_count_diagnostics_plugin.rs

+
+    pub fn diagnostic_system(mut diagnostics: ResMut<Diagnostics>, query: Query<Entity>) {
+        let mut count = 0.;
+        for _entity in &mut query.iter() {


we could replace this with let count = query.iter().count(). Thats simpler, but also if/when we re-implement ExactSizeIterator for query it could make this O(1)

Or alternatively we could make it O(1) now by adding a world.entity_count() function and making this system thread-local.

Your call!

Tried adding it to World, in my test with a system adding and removing entities every frame, the two counts are the same

cart · 2020-12-23T23:15:02Z

crates/bevy_diagnostic/src/log_diagnostics_plugin.rs

            if let Some(ref filter) = state.filter {
                for diagnostic in filter.iter().map(|id| diagnostics.get(*id).unwrap()) {
-                    Self::print_diagnostic(diagnostic);
+                    Self::log_diagnostic(diagnostic);
                }
            } else {
                for diagnostic in diagnostics.iter() {


O(logn) is better than O(n), but for a large number of diagnostics idk if its a good idea to replace a hash map. While we currently only have a small number, I don't want to assume that will always be the case. Maybe LogDiagnosticsPlugin could store its own order? Sorting is a behavior inherent to the LogDiagnosticsPlugin so that makes sense to me.

The issue is keeping the order synced up / accounting for new diagnostics. Lol we could have a Diagnostic event. Ex: DiagnosticEvent::Added(DiagnosticId), DiagnosticEvent::Removed(DiagnosticId), etc

cart · 2020-12-24T19:28:02Z

I do think we might want to try cutting down on the line-noise of the logs a bit (maybe by printing the whole table as a single log).

I also think we could probably maintain an up-to-date "entity count" number within Entities::flush to avoid the need to do atomic ops every time entity_count is needed (I had it in my head that this number already exists).

But for now this all looks good.

Moxinilian added core C-Feature A new feature, making something new possible labels Dec 18, 2020

mockersf added 3 commits December 19, 2020 21:51

move print diagnostics to log

9b00f26

entity count diagnostic

20b8319

asset count diagnostic

57332cc

mockersf force-pushed the print-to-log branch from 9256886 to 57332cc Compare December 19, 2020 20:52

DJMcNab reviewed Dec 22, 2020

View reviewed changes

mockersf added 2 commits December 22, 2020 14:16

remove useless pubs

0111837

use BTreeMap instead of HashMap

3c4c3cf

cart reviewed Dec 23, 2020

View reviewed changes

mockersf added 2 commits December 24, 2020 01:21

get entity count from world

d3fefc2

keep ordered list of diagnostics

c69f3a9

cart merged commit b28365f into bevyengine:master Dec 24, 2020

mockersf deleted the print-to-log branch April 27, 2021 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates on diagnostics (log + new diagnostics) #1085

updates on diagnostics (log + new diagnostics) #1085

mockersf commented Dec 18, 2020 •

edited

Loading

DJMcNab Dec 22, 2020

mockersf Dec 22, 2020

DJMcNab Dec 22, 2020

mockersf Dec 22, 2020

DJMcNab Dec 22, 2020

mockersf Dec 22, 2020

DJMcNab Dec 22, 2020 •

edited

Loading

mockersf Dec 22, 2020

cart Dec 23, 2020

mockersf Dec 24, 2020

DJMcNab commented Dec 22, 2020

mockersf commented Dec 22, 2020

cart left a comment

cart Dec 23, 2020

cart Dec 23, 2020

mockersf Dec 24, 2020

cart Dec 23, 2020

cart commented Dec 24, 2020 •

edited

Loading

updates on diagnostics (log + new diagnostics) #1085

updates on diagnostics (log + new diagnostics) #1085

Conversation

mockersf commented Dec 18, 2020 • edited Loading

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

DJMcNab Dec 22, 2020 • edited Loading

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

DJMcNab commented Dec 22, 2020

mockersf commented Dec 22, 2020

cart 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

cart commented Dec 24, 2020 • edited Loading

mockersf commented Dec 18, 2020 •

edited

Loading

DJMcNab Dec 22, 2020 •

edited

Loading

cart commented Dec 24, 2020 •

edited

Loading