1580843 - Add a tracingIndex field to the marker table, for tracing markers

Status: RESOLVED WONTFIX

(Core :: Gecko Profiler, task, P2)

Description

[Julien Wajsberg [:julienw]]

Currently, we match tracing markers start and end markers using their names. But it's theorically possible that we get 2 starts, then 2 ends. In that case we assume that they're nested.
The rationale is that AutoProfilerTracing is a RAII and automatically adds start and end markers respectively at the start and end of a block. But this isn't guaranteed if a developer adds these markers without relying on this tool.

So instead we should use an ID in these markers. The ID should be unique in a specific thread, but can be duplicated across threads.
This could easily be added in AutoProfilerTracing but we need to look at other places where we add tracing markers, and also possibly make sure to prevent adding such a marker without an id.

See also the Gecko profiler bug in [1] that is well described.
[1] https://github.com/firefox-devtools/profiler/issues/1771

Comment 1

[Greg Tatum [:gregtatum] (REO Fx132)]

I'm taking this bug for the Marker 2.0 spec.

The work here is to add a tracingIndex field to the marker table in the Gecko profile format. The field will be an array of either null or integer. This field will reference a unique tracing event that can be re-constructed via a start and end marker. Currently, profiler.firefox.com matches up start and end markers where it can find them, but this is can be difficult on deciding on what's correct, and requires markers to be properly nested.

This work will add a tracingIndex which has the following semantic meaning, note that I'm using pseudo-code to represent the final markers. In the Gecko format they will be in a tuple with a schema, and then converted on the front-end.

Non-interval marker, a single point in time.

{
  startTime: 485454492123,
  endTime: null,
  tracingIndex: null
}

These two would collapse into a single interval marker:

{
  startTime: 485454492123,
  endTime: null,
  tracingIndex: 54
} 
{
  startTime: null,
  endTime: 485454494567,
  tracingIndex: 54
}

If only one marker with the index is found, and a start time, then it is assumed to run until the end of the thread time.

{
  startTime: 485454492123,
  endTime: null,
  tracingIndex: 54
}

If only one marker with the index is found, then it is assumed to run from the start of the thread time.

{
  startTime: null,
  endTime: 485454492123,
  tracingIndex: 54
}

This would handle all cases of creating markers, and fix a few bugs we have on file for markers not being complete.

Comment 2

[Greg Tatum [:gregtatum] (REO Fx132)]

I think our new plan is to use a setIndex rather than a tracing index. This would be in the marker tuple itself, not the payload. This allows for correlating multiple markers together, for instance all the different phases of a Network request, and it can still use the phased marker ideas.

Comment 3

[Greg Tatum [:gregtatum] (REO Fx132)]

Closed in favor of Bug 1661114.