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.

Sign up for GitHub

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

Jump to bottom

Don't merge scopes that have non-matching dynamic data #73

Closed
hrydgard opened this issue Apr 25, 2022 · 3 comments · Fixed by #102
Closed

Don't merge scopes that have non-matching dynamic data #73

hrydgard opened this issue Apr 25, 2022 · 3 comments · Fixed by #102
Labels
enhancement New feature or request

Comments

@hrydgard
Copy link
Contributor

Scopes can have dynamic data, like this:

puffin::profile_scope!("my_scope_id", my_dynamic_data);

However, puffin's UI still happily merges blocks with different dynamic data together if merging is enabled. It would be nice with a mode that would only merge identical blocks if their dynamic data is also identical, not if it's different.
@hrydgard hrydgard added the enhancement New feature or request label Apr 25, 2022
@MarijnS95
Copy link
Contributor

Unrelated note: I wasn't aware of "dynamic data", but can perhaps utilize this to resolve #60 in some way.

@TimonPost
Copy link
Member

TimonPost commented Oct 18, 2022 •
edited

@repi, your PR #102 fixes this right?

@repi
Copy link
Contributor

repi commented Oct 18, 2022

ah yes! didn't know we had an issue on it, was just a 10 min fix. will mark it resolved by my PR

@repi repi mentioned this issue Oct 18, 2022
Merged
@repi repi closed this as completed in #102 Oct 19, 2022
repi added a commit that referenced this issue Oct 19, 2022
@repi
Don't merge scopes that have different data (#102)
1efc17b 
Previously we would merge all scopes that have the same `id`, but if you
have multiple scopes after each that have same `id` but different
dynamic `data` field they would be merged together and we wouldn't
display the `data` field.

This is something we ran into in our codebase that made it harder to see
why a certain heavy operation was happening as we were missing the
context for it. Which we did specify but was merged together and not
shown here as merging scopes is on by default (and is a general good
idea).

Adds a simple visual test for in the imgui and egui clients.

Resolves: #73

## Example

This is how it looks like now:


![image](https://user-images.githubusercontent.com/1262692/196470164-64076531-647c-48dc-84d3-7bb1a3d502a6.png)

from the following code:
```rust
// test to verify these spikes timers are not merged together as they have different data
for (name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
    puffin::profile_scope!("Spike", name);
    std::thread::sleep(std::time::Duration::from_millis(ms))
}
// these are however fine to merge together as data is the same
for (_name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
    puffin::profile_scope!("Spike");
    std::thread::sleep(std::time::Duration::from_millis(ms))
}
```

Before this PR this would have been a single "4x Spike"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't merge scopes that have different data EmbarkStudios/puffin
4 participants
@hrydgard @repi @MarijnS95 @TimonPost