Skip to content

Make "frame" spans go on a separate trace line than others #4451

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Stypox
Copy link
Contributor

@Stypox Stypox commented Jul 7, 2025

tracing_chrome supports two styles of data that are shown differently in https://ui.perfetto.dev:

  • "Threaded", i.e. each thread has its own trace line in the UI, each line with a tree structure of nested spans. Since Miri is single-threaded, this is equivalent to having a single trace line with all of the spans.
  • "Async", i.e. each span has its own trace line in the UI (this is also the representation the Firefox profiler uses).

I believe the "Threaded" style generally allows for more insights as argued here, however it's a bit cumbersome to use because the "frame" spans (i.e. those that indicate stack frames in the interpreted program) are very big with respect to other spans and are therefore in the way. So this PR puts them (and only them) on a separate trace line:

Ideally this kind of filtering should be doable from the trace viewer UI, but I don't think https://ui.perfetto.dev supports anything like that.

Let me know if I should make the changes more general, e.g. by adding a new TraceStyle like ThreadedWithExceptions(Vec<&'static str>) instead of a hardcoded if. I don't think we're going to need this for other spans though. <- this was done

.id()
.into_u64()
),
_ => if span.metadata().name() == "frame" {
Copy link
Member

Choose a reason for hiding this comment

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

I'm definitely not a fan of hard-coding a particular name here, that way lies total spaghetti code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I've made it more general

"frame" spans indicate stack frames in the interpreted program
@Stypox Stypox force-pushed the separate-frame-in-traces branch from 7b27a2f to d7ec893 Compare July 7, 2025 08:01
@Stypox
Copy link
Contributor Author

Stypox commented Jul 7, 2025

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Jul 7, 2025
@RalfJung
Copy link
Member

RalfJung commented Jul 7, 2025

You just moved the "frame" name checking to a different file, I don't see how this is better.

The "frame" span itself should have appropriate metadata that controls this behavior, rather than special-casing the name after it has already entered the logging system. IOW, the code here is in charge of saying what happens with frame.

@RalfJung
Copy link
Member

RalfJung commented Jul 7, 2025

I don't fully understand neither tracing nor the visualizations you show, but it seems like you might want the perf-related spans in one "line" and the debugging-related ones in another? frame is just an example here for the one debugging span that is actually enabled in regular rustc build, but there is nothing very special about it.

@RalfJung RalfJung added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants