1152992 - [marker] Timeline should handle markers without being explicitly defined

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

To ease integration of timeline markers, a platform developer should not have to modify the front end code -- We should have a default marker group, color.

Subsequently after the platform markers have been added, we should decide on which group this marker should be in, localize it, pick a color, and whether or not this should be behind a pref.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Update https://wiki.mozilla.org/DevTools/OperationInstrument on completion

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1152992-othermarkers.patch

This was harder than you would imagine

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8614865 [details] [diff] [review]
1152992-othermarkers.patch

Review of attachment 8614865 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/markers.js
@@ +57,5 @@
>  const TIMELINE_BLUEPRINT = {
> +  /* Default definition used for markers that occur but
> +   * are not defined here. Should ultimately be defined, but this gives
> +   * us room to work on the front end separately from the platform. */
> +  "OTHER": {

Nit: how about UNKNOWN instead of OTHER?

@@ +58,5 @@
> +  /* Default definition used for markers that occur but
> +   * are not defined here. Should ultimately be defined, but this gives
> +   * us room to work on the front end separately from the platform. */
> +  "OTHER": {
> +    group: 1,

Why group 1? I'd put it either in the first or the last group.

@@ +59,5 @@
> +   * are not defined here. Should ultimately be defined, but this gives
> +   * us room to work on the front end separately from the platform. */
> +  "OTHER": {
> +    group: 1,
> +    colorName: "graphs-blue",

A bleak color is probably nicer and signifies that we have no idea what this is.

::: browser/devtools/performance/modules/widgets/marker-view.js
@@ +172,5 @@
>  
> +    // If this marker type has been marked hidden via marker filtering,
> +    // then don't display it. This is so we can also handle "Other" markers
> +    // that do not have a blueprint, but we want to show those anyway unless "Other"
> +    // markers are filtered.

If we rename "Other" to UNKNOWN, then update this comment.

::: browser/devtools/performance/modules/widgets/markers-overview.js
@@ +74,5 @@
>    setBlueprint: function(blueprint) {
>      this._paintBatches = new Map();
>      this._lastGroup = 0;
>  
> +    for (let type of Object.keys(blueprint).filter(e => !blueprint[e].hidden)) {

If this graph is passed the filtered blueprint without the hidden markers, then why would this be necessary?

If this graph isn't passed the filtered blueprint, then extra rows won't be hidden will they?

@@ +109,1 @@
>        if (markerType) {

This conditional is useless now, right?

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

+1 on color, naming and row. Can use grey for it, and change console time colors. 

We can't remove filtered markers from the blueprint; then they become UNKNOWN. So we just tag a "hidden" prop on there to truly filter.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1152992-othermarkers.patch

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/49acc2446457

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/61485af70eca for dt2 orange on at least osx 10.10:
https://treeherder.mozilla.org/logviewer.html#?job_id=3378495&repo=fx-team

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

This one is fascinating

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1152992-othermarkers.patch

Crash has something to do with the Cu.cloneInto stuff for copying the blueprint. Changed to always use the same blueprint, but use a filter array of marker types that should not be displayed. r? again for those changes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=134ba2b35cad

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Passes on 10.10 debug https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3876c1f076

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8617725 [details] [diff] [review]
1152992-othermarkers.patch

Review of attachment 8617725 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ +26,5 @@
> + * Takes a marker, blueprint, and filter list and
> + * determines if this marker should be filtered or not.
> + */
> +function isMarkerValid (marker, blueprint, filter) {
> +  let isUnknown = !blueprint[marker.name];

Nit: !(marker.name in blueprint) is probably more accurate. Maybe someday we want to handle null or undefineds differently?

::: browser/devtools/performance/modules/markers.js
@@ +135,5 @@
>      }],
>    },
>    "TimeStamp": {
>      group: 2,
> +    colorName: "graphs-yellow",

Can this stay blue? It might be confusing to have it yellow, just like the reflow/js markers.

::: browser/devtools/performance/modules/widgets/marker-view.js
@@ +10,5 @@
>  
>  const { Cc, Ci, Cu, Cr } = require("chrome");
>  const { Heritage } = require("resource:///modules/devtools/ViewHelpers.jsm");
>  const { AbstractTreeItem } = require("resource:///modules/devtools/AbstractTreeItem.jsm");
> +const { getBlueprintFor } = require("devtools/performance/markers");

This should be lazy required?

::: browser/devtools/performance/modules/widgets/markers-overview.js
@@ +80,4 @@
>  
> +    let observedGroups = new Set();
> +
> +    for (let type of Object.keys(this._blueprint)) {

No need for Object.keys here. You could either use a for..in loop, or more nicely:

for (let [type, details] of Iterator(this._blueprint)) {
}

@@ +98,1 @@
>      }

Nice

@@ +124,2 @@
>      for (let marker of markers) {
> +

Nit: extra newline.

::: browser/devtools/performance/performance-controller.js
@@ +403,5 @@
>    /**
>     * Gets the current timeline blueprint without the hidden markers.
>     * @return object
>     */
>    getTimelineBlueprint: function() {

Well this function is useless now. Remove it.

@@ +410,5 @@
> +
> +  /**
> +   * Returns the currently hidden marker types as an array.
> +   */
> +  getHiddenMarkerTypes: function () {

Is this really necessary?

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8617725 [details] [diff] [review]
1152992-othermarkers.patch

Review of attachment 8617725 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/markers.js
@@ +135,5 @@
>      }],
>    },
>    "TimeStamp": {
>      group: 2,
> +    colorName: "graphs-yellow",

Sure -- the yellow seemed to make sense due to always being triggered by JS, and able to be seen ontop the blue-grey of the ConsoleTime

::: browser/devtools/performance/modules/widgets/markers-overview.js
@@ +98,1 @@
>      }

Yeah i was pretty proud of this one.

::: browser/devtools/performance/performance-controller.js
@@ +411,5 @@
> +  /**
> +   * Returns the currently hidden marker types as an array.
> +   */
> +  getHiddenMarkerTypes: function () {
> +    return this.getPref("hidden-markers");

both getHiddenMarkerTypes and getTimelineBlueprint could be done wherever they're needed -- probably can get rid of the blueprint one, since we no longer have the notion of multiple blueprints.

For the hidden markers, wanted the controller to be the arbiter of info that gets distributed to the views... but we have other prefs accessible there. I think for the extensibility story, this'll need to be changed up anyway.

Removing these two functions

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e4738a23d0c4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4738a23d0c4