firefox-devtools / firefox-devtools/profiler
[Unify markers] Spec
- Dominant language
- TypeScript
- Stars
- 1.5k
- Forks
- 491
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 30
Description
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)
```diff
export type GeckoMarkerStruct = {
name: IndexIntoStringTable[],
- time: Milliseconds[],
+ startTime: Array,
+ endTime: Array,
+ intervalIndex: Array,
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.
[time property]: https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/tools/profiler/core/ProfilerMarker.h#58
[timing information in the payload]: https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/tools/profiler/core/ProfilerMarkerPayload.cpp#51-55
## 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](https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/tools/profiler/core/ProfilerMarkerPayload.cpp#37)
* [StreamSamplesAndMarkers](https://searchfox.org/mozilla-central/source/tools/profiler/core/ProfiledThreadData.cpp#109-177) 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](https://perfht.ml/2CqXlXc):
```json
{
"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:
```js
// 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](https://github.com/devtools-html/perf.html/blob/master/docs/upgrading-profiles.md), plus care should be taken with the DevTools performance tool.
┆Issue is synchronized with this [Jira Task](https://mozilla-hub.atlassian.net/browse/FP-701)
Contributor guide
Assessment
This issue has not been assessed yet.