1077444 - Create Flamechart Widget

Status: RESOLVED FIXED

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

Description

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

For flame charts in new performance tool

Comment 1

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

Attachment: v1

Not bug 1077459

Comment 2

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

Comment on attachment 8544016 [details] [diff] [review]
v1

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

some nits and other enhancements, r+

::: browser/devtools/shared/test/browser_flame-graph-01.js
@@ +7,5 @@
> +let {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});
> +let {Promise} = devtools.require("resource://gre/modules/Promise.jsm");
> +let {Hosts} = devtools.require("devtools/framework/toolbox-hosts");
> +
> +let test = Task.async(function*() {

We should probably use the new `add_task(function*(){})` function for doing tests to be in sync with all the other DT tests recently converted

::: browser/devtools/shared/test/browser_flame-graph-03.js
@@ +93,5 @@
> +  y /= window.devicePixelRatio;
> +  graph._onMouseMove({ clientX: x, clientY: y });
> +}
> +
> +function dragStart(graph, x, y = 1) {

dragStart and dragStop are defined in a handful of these graph tests -- maybe good time to pull these out into head.js, or atleast a bug for it

::: browser/devtools/shared/widgets/FlameGraph.jsm
@@ +28,5 @@
> +const TIMELINE_TICKS_SPACING_MIN = 75; // px
> +
> +const OVERVIEW_HEADER_HEIGHT = 18; // px
> +const OVERVIEW_HEADER_SAFE_BOUNDS = 50; // px
> +const OVERVIEW_HEADER_TEXT_COLOR = "#18191a";

Colors should be defined on the prototype by default, not directly called via `ctx.fillStyle()` or something, so we can easily add the ability to change these colors, like the other themeable graphs. Like, in the prototype:

`headerTextColor: OVERVIEW_HEADER_TEXT_COLOR`

Then just called via `this.headerTextColor` in the actual drawing.

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +12,3 @@
>  
>  this.EXPORTED_SYMBOLS = [
> +  "GraphCursor",

is GraphCursor or GraphSelectionResizer used anywhere?

Comment 3

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Comment on attachment 8544016 [details] [diff] [review]
> v1
> 
> ::: browser/devtools/shared/test/browser_flame-graph-03.js
> @@ +93,5 @@
> > +  y /= window.devicePixelRatio;
> > +  graph._onMouseMove({ clientX: x, clientY: y });
> > +}
> > +
> > +function dragStart(graph, x, y = 1) {
> 
> dragStart and dragStop are defined in a handful of these graph tests --
> maybe good time to pull these out into head.js, or atleast a bug for it
> 

I'm a bit wary of moving them into head.js, since this is a shared devtools test folder, and those functions are only useful for a handful of tests.

Comment 4

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

Comment on attachment 8544016 [details] [diff] [review]
v1

BenWa said he might want to take a look at this too.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 8544016 [details] [diff] [review]
v1

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

Looks excellent. Great work!

::: browser/devtools/shared/widgets/FlameGraph.jsm
@@ +333,5 @@
> +    let ctx = this._ctx;
> +
> +    let fontSize = FLAME_GRAPH_BLOCK_TEXT_FONT_SIZE * this._pixelRatio;
> +    let fontFamily = FLAME_GRAPH_BLOCK_TEXT_FONT_FAMILY;
> +    let visibleBlocks = this._drawPyramidFill(dataSource, dataOffset, dataScale);

In theory it should be better performance if you fill+draw-text together since you'll get better data/cache locality. Not sure if it's worth making the change in practice.

Actually looking further in your patch if you make this change you'll need to do more canvas state change (i.e. change styles). I'm not sure which you want to optimize, perhaps the state change you minimize buys you more than cache locality does.

Comment 6

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

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

Comment 7

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

Backed out for dt2 orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=1645879&repo=fx-team
https://hg.mozilla.org/integration/fx-team/rev/2c169c448081

Comment 8

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

Trying again. https://hg.mozilla.org/integration/fx-team/rev/c92d931cbd4a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Brendan Gregg]

Nice flame chart!

The code refers to "flame graph". While the differentiation between "flame chart" and "flame graph" may be accidental, it is useful to have two different terms for two different visualizations. FWIW, a flame graph is similar, but maximizes grouping frames by sorting stacks alphabetically on the x-axis. Ie, the x-axis is no longer time. (see http://www.slideshare.net/brendangregg/blazing-performance-with-flame-graphs/35). Since the data and infrastructure is here, it would be great (if it doesn't already), to have a toggle button to switch between flame chart (time-ordered) and flame graph (alphabet-sorted). I could file a separate ticket...

Comment 11

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

(In reply to Brendan Gregg from comment #10)
> Since the data and infrastructure is here, it would be great (if it
> doesn't already), to have a toggle button to switch between flame chart
> (time-ordered) and flame graph (alphabet-sorted). I could file a separate
> ticket...

That's a good idea. A few bugs were fixed in the meantime to reduce quite a bit of noise from the visualization (e.g. bug 1119932, bug 1119933), but having an "alphabetize" toggle could prove to be a secondary interesting visualization of the call tree.

It's probably a good first bug as well (i.e.: easy to implement).

Feel free to file the bug, and even hack around if you want to. I can mentor if needed.

Comment 12

[Brendan Gregg]

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> (In reply to Brendan Gregg from comment #10)
> > Since the data and infrastructure is here, it would be great (if it
> > doesn't already), to have a toggle button to switch between flame chart
> > (time-ordered) and flame graph (alphabet-sorted). I could file a separate
> > ticket...
> 
> That's a good idea. A few bugs were fixed in the meantime to reduce quite a
> bit of noise from the visualization (e.g. bug 1119932, bug 1119933), but
> having an "alphabetize" toggle could prove to be a secondary interesting
> visualization of the call tree.
> 
> It's probably a good first bug as well (i.e.: easy to implement).
> 
> Feel free to file the bug, and even hack around if you want to. I can mentor
> if needed.

Ok, filed 1123495. Thanks for the offer, and I'd like to hack Mozilla one day, but probably won't have time for a while. :)