[Unify markers] Spec

[gregtatum]

julienw mstange I'm going to make all kinds of recommendations here. Please leave feedback below, and I'll work on updating this post with the feedback and filing all needed Gecko and perf.html bugs.

(Simplified on 12/17/2018)

---

As of now markers have been added in an ad hoc manner, and are inconsistent in their usage. In perf.html it's hard to know exactly how to display a marker based on its data structure alone.

Marker timing

Markers in the Gecko profiler come in four different varieties:

• Single point in time
• Start and end time (duration)
• Start time marker (incomplete marker)
• End time marker (incomplete marker)

In perf.html we only use 2 types, by merging the incomplete markers together

• Single point in time
• Start and end time (duration)

Right now markers are currently inconsistent with a time property in the marker itself, and timing information in the payload. In perf.html in the header timeline, and in the marker chart we turn all markers we can into IntervalMarkers, and treat them as if they are all IntervalMakers. In the marker table we ignore the IntervalMakers and naively show every single marker emitted.

I think the proper change here would be to add the startTime endTime to the marker itself in Gecko, but then have them both be optionally null. The data emitted would be changed to look like this (in Flow types)

 export type GeckoMarkerStruct = {
   name: IndexIntoStringTable[],
-  time: Milliseconds[],
+  startTime: Array<Milliseconds | null>,
+  endTime: Array<Milliseconds | null>,
+  intervalIndex: Array<number | null>,
   data: MarkerPayload_Gecko[],
   length: number,
 };

Then in perf.html we could construct the markers based on different criteria.

Non-interval marker:

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

These two would collapse into a single interval marker:

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

{
  startTime: null,
  endTime: 485454494567,
  intervalIndex: 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,
  intervalIndex: 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,
  intervalIndex: 54
}

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

Simplify stacks

Right now the stacks are exported as if they are a new thread for every marker. This is very heavy handed. Markers should be emitted as an index into the stack table, and a timestamp of when the stack was taken. The reason for a timestamp is that a marker created at a point of time could be referring to some cause at a point back in time.

See:

• ProfilerMarkerPayload::StreamCommonProps
• StreamSamplesAndMarkers is called for the stack information

De-duplicate and structure all data

Right now the data payloads have been created in an ad hoc fashion, or worse, the information is packed into the name of the marker. Timing information is usually duplicated, and we don't surface all of the information.

Here is an example of an unstructured marker:

{
  "name": "Non-blank paint after 2359ms for URL https://en.wikipedia.org/wiki/Barack_Obama, foreground tab",
  "time": 9051516.478496548,
  "data": null
}

Unify categories with stack labels, and ensure they are correct

I will probably file a follow-up issue to this.

Marker formatting schema

Gecko should tell perf.html how to format markers. Right now a Gecko hacker has to land a patch in perf.html to trivially surface some UI for a marker, then they must modify the marker in Gecko. This could be simplified if the markers could define a schema inside of Gecko that would explain how to format the data. This schema could then be used to format tooltips and sidebar information.

Schema in Flow types:

// Provide different formatting options for strings.
type FormatType =
 | "string"       // "Label: Some String"
 | "bytes"        // "Label: 5mb"
 | "seconds"      // "Label: 5s"
 | "milliseconds" // "Label: 5ms"
 | "microseconds" // "Label: 5μs"
 | "integer"      // "Label: 5323"
 | "number"       // "Label: 52.23"
 | "percentage"   // "Label: 50%"
 | "stack"        // Prints a full stack trace using the value.

type MarkerSchema = {
  // The unique identifier for this marker.
  name: string, // e.g. "GCMajor-complete"

  // The label of how this marker should be displayed in the UI.
  label: string, // e.g. "GCMajor"

  // Display in the timeline header.
  inHeader: boolean,
  
  // Display in the marker chart.
  inChart: boolean,
  
  // The category information is shared with the frame label information.
  // See https://github.com/devtools-html/perf.html/issues/836
  category: IndexIntoCategoryTable,
  data: Array<
    {
      key: string,
      label: string,
      format: FormatType,
    }
  >
}

// Example:
const markerSchema = [
  {
    name: "GCSlice",
    label: "GCSlice",
    inHeader: false,
    inChart: true,
    data: [
      { key: 'reason', label: 'Reason', format: 'string' },
      { key: 'budget', label: 'Budget', format: 'string' },
      { key: 'intial_state', label: 'Initial state', format: 'string' },
      { key: 'final_state', label: 'Final state', format: 'string' },
      { key: 'gcbudget', label: 'Budget', format: 'string' },
      { key: 'page_faults', label: 'Page faults', format: 'string' },
    ]
  },
  {
    name: "Bailout",
    label: "Bailout",
    inHeader: false,
    inChart: true,
    data: [
      { key: 'bailoutType', label: 'Type', format: 'string' },
      { key: 'where', label: 'Where', format: 'string' },
      { key: 'script', label: 'Script', format: 'string' },
      { key: 'functionLine', label: 'Function Line', format: 'integer' },
      { key: 'bailoutLine', label: 'Bailout Line', format: 'integer' },
    ]
  }
]

Considerations

Modifying the format of markers needs to follow the upgrade profiles guide, plus care should be taken with the DevTools performance tool.

┆Issue is synchronized with this Jira Task

[julienw]

All this sounds good, but I have 2 remarks or questions.

1. about intervalIndex

You don't enter into much detail about what this is. I understand that you'd like to increment this index so that each emitted "tracing" marker (ie: a marker where we get the start and the end separately) get its unique index. I think this brings challenges, depending on where they're used, because this would need more states inside Firefox: state for the unique index, state for the "open" marker. Right now I believe tracing markers are very cheap and need no state.

2. about Simplify stacks

I think what you propose would be nice, but again I don't know how easy it is. Markers are designed to be very cheap, but maybe tapping into the current stack table could be too expensive. (This is a lot of maybe, I don't know for real, I'm merely pointing a risk for this part).

[gregtatum]

1. intervalIndex:

Looking at one particular case we could use thread local storage to store a static incrementing index that is incremented on the constructor for AutoProfilerTracing. Then we could add an mIntervalIndex private variable to store the index until the destructor is called.

2. Simplify stacks:

I believe this data is already at hand, and the only tweak is needed during the serialization process. Right now we mimic the structure of outputting a thread with a single sample in it. For instance this is the structure of a single Paint marker's payload:

{
  "type": "tracing",
  "stack": {
    "processType": "default",
    "name": "SyncProfile",
    "tid": 9866215,
    "pid": 10027,
    "registerTime": null,
    "unregisterTime": null,
    "samples": {
      "schema": {
        "stack": 0,
        "time": 1,
        "responsiveness": 2,
        "rss": 3,
        "uss": 4
      },
      "data": [
        [
          10874,
          53655026.543845996
        ]
      ]
    },
    "markers": {
      "schema": {
        "name": 0,
        "time": 1,
        "data": 2
      },
      "data": []
    }
  },
  "category": "Paint",
  "interval": "start"
}

[PaulBone]

About marker formatting

I like the idea on principal. however I think I'll always be able to find little things that I'd prefer to format by hand. Look at a GCMInor marker, they show the amount of data in the nursery and the capacity and how "full' it is in a single label, this is three things of information for the user, taken from two marker fields:

usage / capacity (full%)

I'm not saying don't provide this formatting ability, just don't make it compulsory.

[julienw]

@PaulBone I believe this is what Greg tried to implement with the key percentageDivisorKey (look it up in the initial comment).

I read the proposal again, I have some more comments.

1. There's no example of a marker with both startTime and endTime.
2. about FormatType, I think we should use a string table for this, so that the format is given using integers and take less space in the resulting JSON. Also there's a typo for millseconds :)
3. For the data array, we miss the information about where we surface these properties, especially tooltip / sidebar. Some properties could be surfaced nowhere explicitely but used only as percentageDivisorKey for other properties.
4. All markers should always be surfaced in the marker table. (could be filtered out using expertise or categories though)
5. About categories: I believe the word category is too overloaded. So we need to take care about what we put here. I think that what we really need is a way to group markers logically: eg marker GCMajor belongs to the GC, marker Paint belongs to Graphics. We could encode this information in the marker name: Graphics::Layout instead of name: "Layout", category: "Graphics" and GC::Major instead of name: "GCMajor", category: "GC", but I believe having a dedicated property has more advantages: possible to use a string table, and easier to keep the number of categories organized.

[julienw]

Something I miss from the proposal: indicate what payload's properties we need to search into. For example, for DOMEvent we should search eventType, for UserTiming we should search for name.

Alternatively we can have a dedicated property that we'd look when searching.

[gregtatum]

per mstange in Slack:

Kannan wants to add more JS markers and is intimidated by the fact that he needs to update the frontend

[gregtatum]

I spent a bit of time simplifying the marker schema. I think some of the more complicated bits should be handled on the front-end only, in order to have a simpler schema for adding markers.

[digitarald]

@gregtatum found while pruning the product backlog – is there more work needed here that we should track here or in a separate bug?

[gregtatum]

Seems like this defines the problem and a solution, but it's not broken out with more detailed work.

[gregtatum]

We could also use this infrastructure to create meta information about what markers are appropriate to show for what types of profiling presets:

https://bugzilla.mozilla.org/show_bug.cgi?id=1589681

This way, for example, if we are in a webdev mode, we can remove all of the platform-specific markers which are not useful.