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

New layout for the marker chart: Display stack-based markers in a stack-chart-like layout #3141

Open
mstange opened this issue Jan 22, 2021 · 4 comments
Open

New layout for the marker chart: Display stack-based markers in a stack-chart-like layout #3141

mstange opened this issue Jan 22, 2021 · 4 comments

Comments

@mstange
Copy link
Contributor

mstange commented Jan 22, 2021 •
edited by data-sync-user
Loading

I'd like to propose two changes:

  1. For each marker type, the profiler should know whether markers of that type are "stack-based" or not. Probably with a new flag in the schema.
  2. In the marker chart, all stack-based markers should be collected in one section of the chart, and not split into separate rows by type. This will display them in a layout that looks similar to the stack chart: markers that "run" further to the root of the stack will be at the top, and nested markers that run during those calls will displayed underneath. Markers of different types will share the same row, based on their nesting depth. The other, non-stack-based, markers can keep their own rows.

This will make it a lot more obvious "what's happening" at any given time on the selected thread. It eliminates the need to scroll down to find a marker that runs during the time that the user is looking at.

┆Issue is synchronized with this Jira Task
@gregtatum
Copy link
Member

I believe this feature request could be handled by the marker set feature. Creating a marker set would logically group markers together into a stack-like chart. I'm not sure how it would work with markers of different types though, and if that's something different.

Also, it would be good to be explicit on how much we care if markers we receive really form a stack.

True stack-based markers layout:

[A--------------------]
  [B----------------]
    [C---][D---]

This is not a true stack-based marker, and creates difficulties in resolving a stack-like view. Is this worth caring about and checking? There's no guarantee in the markers of how to resolve something like this. In the JS tracer view, I ended up creating some heuristics to guess what this meant. My issues there were from float precision issues, even though the underlying data was a real stack. However, markers provide no guarantees of how well ordered they are.

[A--------------------]
  [B----------------]
    [C---][D----------]

@mstange
Copy link
Contributor Author

mstange commented Jan 25, 2021

I would enforce a nesting by clamping the endTime of the "inner" marker, and trusting the startTime for deciding which marker is the inner marker. E.g. in your example, since B started before D, D is the inner marker, and D must end before B ends. We could try to avoid these issues entirely by switching from float milliseconds to integer nanoseconds for our timestamps.

@gregtatum
Copy link
Member

The logic how I handled it with JS tracer has ASCII art examples of fixing various issues. I think it's decently robust.

profiler/src/profile-logic/js-tracer.js

Lines 314 to 328 in 63f4c69

if (
prefixStart <= currentStart &&
prefixEnd > currentStart &&
prefixEnd < currentEnd
) {
// This check handles precision errors that creates timing like so:
// [prefix=======]
// [current========]
//
// It reformats the current as:
//
// [prefix=======]
// [current===]
currentEnd = prefixEnd;
}

Open tracing has a bit more complicated and unique solution for this type of nesting issue. I wouldn't advocate for it, but it's probably worth listing some other prior art.

@squelart
Copy link
Contributor

👀

We could try to avoid these issues entirely by switching from float milliseconds to integer nanoseconds for our timestamps.

I'd be happy to help on the Firefox side, please file bugs as needed.

Thought: Maybe an even more flexible (but obviously more complex) solution would be to use whatever integer value we get from the OS, and mention the corresponding unit somewhere in the profile metadata. This could potentially remove all the float/double computations happening until the very end when it is displayed in human-friendly format, and would also carry the inherent precision of each platform if that was useful. But we can discuss all this later...

mstange added a commit to mstange/perf.html that referenced this issue Nov 1, 2024
@mstange
Profile format changes for flows and stack-based markers
dbc0f75 
This changeset changes the type definition for marker schema
field types and for the marker schema itself, to include the new
field types 'flow-id' and 'terminating-flow-id'. It also has some
minimal changes to treat the new field types as if they were unique-string
fields.
Somewhat unrelated to flows, it also adds an optional `isStackBased`
field to the marker schema type, which is currently unused. This is
for firefox-devtools#3141 .

Additionally, it increments the version of the two profile formats,
so that profiles with the new field types won't be loaded by
older front-ends.
This was referenced Nov 1, 2024
Open
Open
Merged
mstange added a commit to mstange/perf.html that referenced this issue Nov 7, 2024
@mstange
Profile format changes for flows and stack-based markers
8141498 
This changeset changes the type definition for marker schema
field types and for the marker schema itself, to include the new
field types 'flow-id' and 'terminating-flow-id'. It also has some
minimal changes to treat the new field types as if they were unique-string
fields.
Somewhat unrelated to flows, it also adds an optional `isStackBased`
field to the marker schema type, which is currently unused. This is
for firefox-devtools#3141 .

Additionally, it increments the version of the two profile formats,
so that profiles with the new field types won't be loaded by
older front-ends.
mstange added a commit that referenced this issue Nov 7, 2024
@mstange
Profile format changes for flows and stack-based markers (#5186)
e51f644 
This PR changes the type definition for marker schema field types and
for the marker schema itself, to include the new field types 'flow-id'
and 'terminating-flow-id'. It also has some minimal changes to treat the
new field types as if they were unique-string fields.
Somewhat unrelated to flows, it also adds an optional `isStackBased`
field to the marker schema type, which is currently unused. This is for
#3141 .

Additionally, it increments the version of the two profile formats, so
that profiles with the new field types won't be loaded by older
front-ends.
mstange added a commit to mstange/perf.html that referenced this issue Nov 7, 2024
@mstange
Profile format changes for flows and stack-based markers
a5b7c6c 
This changeset changes the type definition for marker schema
field types and for the marker schema itself, to include the new
field types 'flow-id' and 'terminating-flow-id'. It also has some
minimal changes to treat the new field types as if they were unique-string
fields.
Somewhat unrelated to flows, it also adds an optional `isStackBased`
field to the marker schema type, which is currently unused. This is
for firefox-devtools#3141 .

Additionally, it increments the version of the two profile formats,
so that profiles with the new field types won't be loaded by
older front-ends.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mstange @gregtatum @squelart