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

Re-add the "frame" span for tracy comparisons #8362

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

MJohnson459
Copy link
Contributor

Objective

In #6503 there were two changes to the top level tracing span "frame. Firstly it was renamed to "main app" and secondly the scope was reduced to only the main app. This means that there is no longer a top level span that can be used to compare frame times.

In addition to making it easier to compare across versions again, it also helps when running without the render feature. Previously the "frame" span was present in both cases but now to compare "headless" and render the comparison is between "main app" and "winit event_handler" and Tracy doesn't handle comparing non-matching frames well.

Before (0.9)
image

After (0.10)
image

Solution

This PR reintroduces the "frame" span so comparisons across versions and features is again easy.

image

image

Alternative

As an alternative route, the tracy-client crate that is used by tracing-tracy supports the ability to set multiple "Frame" markers using the secondary_mark_frame function. It may be possible to create a PR for tracing-tracy that exposes that functionality and then publish an "update" frame marker instead. This might have additional benefits in the future as it could be used for other systems we might want to track as frames without endless nesting.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@hymm
Copy link
Contributor

hymm commented Apr 15, 2023

I don't like the term "frame" for this span. The frame time is the time between when one frame is presented to the next. Without pipelined rendering the span misses capturing the i/o processing time. When pipelined rendering is enabled, this span is only vaguely related to the frame time. With the current behavior the new span is more like "main app + sub apps".

@james7132 james7132 added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Apr 16, 2023
@MJohnson459
Copy link
Contributor Author

MJohnson459 commented Apr 17, 2023

I don't like the term "frame" for this span.

That's fair. What about something like "update"? It's explicit about what the span is and "updates per second" is a fairly common metric. I just chose "frame" to maintain compatibility with the older releases which had this name.

Without pipelined rendering the span misses capturing the i/o processing time.

Sorry, I don't understand, which i/o processing is missed? Don't all the services run within an update cycle? Should I add something else to the span to help cover this case?

@hymm
Copy link
Contributor

hymm commented Apr 17, 2023

"update" sounds good to me.

which i/o processing is missed?

The winit runner processes events and then calls app.update(). You'll see these as the small winit event_handler spans between the bigger ones that includes the main app spans. It's hard to create a span that covers these since we'd need to create the span on one call to the callback and drop it on a different call to the callback function. https://github.com/bevyengine/bevy/blob/main/crates/bevy_winit/src/lib.rs#L652

@JMS55
Copy link
Contributor

JMS55 commented Apr 19, 2023

What about calling it specifically "app_update"? Just "update" sounds a bit vague to me, and what if you have more subapps?.

@MJohnson459
Copy link
Contributor Author

In context I think "update" should seem less vague. The two places it will appear is in the Tracy view and in printed log messages. In both cases there will be context either side that helps. Related, because it's printed in line with log messages, there is an incentive to keep it short if possible.

e.g.

2023-04-19T09:07:21.407456Z  INFO bevy_app:update:main app:schedule{name=Outer}...

Anyway, I can change it to "app_update" if you still prefer it.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think app_update is ever so slightly more clear, but this is fine.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 24, 2023
@alice-i-cecile
Copy link
Member

@MJohnson459 once merge conflicts are resolved I'm ready to merge this.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 25, 2023
Merged via the queue into bevyengine:main with commit dea91e9 Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants