Nested diagnostics

[LeshaInc]

What problem does this solve or what need does it fill?

bevy_diagnostic should support arbitrarily nested trees like this:

fps: 60
frame_time: 16.6 ms
render
├── elapsed_gpu: 3.2 ms
├── elapsed_cpu: 5.1 ms
├── vertex_shader_invocations: 3712498
├── fragment_shader_invocations: 20431172
├── prepass
│   ├── elapsed_gpu: 1.1 ms
│   ├── elapsed_cpu: 1.2 ms
│   └── vertex_shader_invocations: 1771018
├── directional shadow 0/cascade 0
│   ├── elapsed_gpu: 1.1 ms
│   ├── elapsed_cpu: 1.2 ms
│   ├── vertex_shader_invocations: 1941480
│   └── fragment_shader_invocations: 10223140
└── main pass
    ├── elapsed_gpu: 1.1 ms
    ├── elapsed_cpu: 1.2 ms
    ├── vertex_shader_invocations: 1771018
    └── fragment_shader_invocations: 10208032
system
├── cpu_usage: 31%
└── cpu_name: 31%

This would be very useful for #9135

What solution would you like?

I would imagine an API like this:

// alternatively this could store either `&'static str`, or `Arc<str>`
// or perhaps a `SmallString`
pub struct DiagnosticScope(Cow<'static, str>)

impl DiagnosticScope {
    pub const ROOT: DiagnosticScope = DiagnosticScope(Cow::Borrowed("root"));
}

pub struct DiagnosticName(Cow<'static, str>)

pub struct DiagnosticPath<'a> {
    pub scope_path: &'a [&'a DiagnosticScope],
    pub diagnostic: &'a DiagnosticName
}

• All methods of DiagnosticStore now take DiagnosticPath instead of DiagnosticId.
• DiagnosticId is removed in favor of DiagnosticPath. It already was painful to use due to UUIDs, especially when the names aren't known at runtime (which is the case for shadow passes)
• Add methods for tree traversal to DiagnosticStore

What alternative(s) have you considered?

An alternative is to generate DiagnosticId using the hash of all path components, and set Diagnostic::name to something like render/prepass/elapsed_gpu. This is error-prone and may cause collisions if a bad hash function is used.

Future work

A plugin for displaying this tree would be nice. Until bevy_ui matures, this visualization could just show a textual representation (as shown above), rendered using a monospace font. In the future, collapsible tree entries and filters would be nice to have.

[cart]

I've been wanting "categories" for diagnostics for awhile. I like that this takes that one step further by making it a "folder hierarchy". Seems useful / valuable, especially once we have UI to visualize things.

The biggest downside of replacing DiagnosticId with DiagnosticPath is that we're doing string hashing of potentially long paths to look up and insert diagnostics. Across hundreds of of diagnostics produced once a frame (or more) that might start showing up meaningfully on benchmarks.

Maybe we should have an "exchange" system where we have efficient dense runtime generated ids that you can look up and generate using paths. Core systems could do that lookup / store the dense index in the interest of efficiency. Then we could also have variants that do the look up / generation automatically for users that dont care / want quick results.

[cart]

Generally on board for this idea though!

[LeshaInc]

Maybe we should have an "exchange" system where we have efficient dense runtime generated ids that you can look up and generate using paths. Core systems could do that lookup / store the dense index in the interest of efficiency. Then we could also have variants that do the look up / generation automatically for users that dont care / want quick results.

The downside is that we won't have const DiagnosticPath anymore. A static Lazy<DiagnosticPath> could work, but that requires a global, singleton diagnostic storage (and an id allocator).

An alternative is to have

pub struct DiagnosticPath {
    components: SmallVec<[Cow<'static, str>; 4]>,
    hash: u64,
}

with a const constructor. The hash field would basically be a Uuid replacement. This would make new system just as fast as the old one.

[nicopap]

You don't need to "componentize" the path until displaying it. Since there is no meaningful data associated with non-terminal diagnostics beside the name. So there isn't much point in parsing/hashing individual components. Keeping DiagnosticId as is and adding an additional path field to Diagnostic would be enough I think. The only point where it maters is when displaying the diagnostic, and maybe an additional API that could toggle diagnostics based on path (but then, it would be a singular event, then an activated field of sort could be toggled, not much reason to optimize)

If we want to replace DiagnosticId by DiagnosticPath, I think a const constructor would be ideal, but not sure if possible.

[LeshaInc]

You don't need to "componentize" the path until displaying it

If I understood you correctly, you propose replacing the list of path components by a single string (where path components are separated by "/")? I think it's a good idea.

Keeping DiagnosticId as is and adding an additional path field to Diagnostic would be enough I think

The problem is, its cumbersome to allocate DiagnosticId. For example, diagnostics for shadow passes use a runtime generated name:

let pass_span = diagnostics.pass_span(&mut render_pass, view_light.pass_name.clone());
shadow_phase.render(...);
pass_span.end(&mut render_pass);

Here diagnostics is specific to rendering and keeps track of scopes, so the actual diagnostic would have a name like render/shadows/shadow pass spot light 0. Having to manually generate IDs for each diagnostic isn't very ergonomic. It's much easier to use the hash of the path.

I think a const constructor would be ideal, but not sure if possible.

We could use const-fnv1a-hash crate, a 64 bit hash would be enough I think, but it also supports 128 bit hashes if needed.

[LeshaInc]

This should be closed now as #9266 has been merged