Skip to content
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

Is it sensible to sum overlapping thread scopes? #60

Open
MarijnS95 opened this issue Jan 24, 2022 · 1 comment
Open

Is it sensible to sum overlapping thread scopes? #60

MarijnS95 opened this issue Jan 24, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@MarijnS95
Copy link
Contributor

Is your feature request related to a problem? Please describe.

While visualizing delays from an identical starting point - effectively overlapping each other - through scopes manually added to puffin::Stream, the resulting scopes get summed and show a prolonged track. This is an example registering the same 500ms child workload - starting at 0 - three times:

image

Likewise we have GPU workloads where the next command buffer starts running ahead of the previous one completing. Here too - albeit with different names - their entire track gets prolonged to fit every item on the line, even when it exceeds the parent Context `frame 0` Command buffer `2` parent scope despite setting explicit start and end timings for a scope.

image

(I take no responsibility for three different pipelines in the same frame having both a space, hyphen, and underscore 🤣)

That's done by:

puffin/puffin/src/merge.rs

Lines 128 to 133 in 17d0429

// Make sure sibling scopes do not overlap:
let mut relative_ns = 0;
for scope in &mut scopes {
scope.relative_start_ns = scope.relative_start_ns.max(relative_ns);
relative_ns = scope.relative_start_ns + scope.duration_per_frame_ns;
}

This is somewhat related to GPU profiling in #59.

Describe the solution you'd like
I expected either a panic/Err() because of submitting invalid data through the puffin::global_reporter, and not initially knowing that - presumably - profiling submitted for "threads" (a CPU thread in the literal sense) is assumed to run serially. Ie. if the start of the next sibling scope lies before the end of the current, that should be an error?

Describe alternatives you've considered

It'd be great if puffin could somehow visualize these overlapping scopes, maybe a color or pattern to display overdraw? Displaying on multiple tracks is bound to be tricky, hard to see, and pretty much breaks the "flamegraph" concept. Perhaps a different waterfall view like Radeon Graphics Profiler could be considered? This may need a different kind of "profiling mode" to allow such kind of overlaps though.

@MarijnS95 MarijnS95 added the enhancement New feature or request label Jan 24, 2022
@VZout
Copy link
Member

VZout commented Jan 25, 2022

I agree that returning an error here makes sense. I don't think puffin should return early though but instead let the user know the data is invalid after the fact.

A radeon gpu profiler like view makes sense as well. Definitely open for contributions for that even if its just for egui and not puffin_viewer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants