1007202 - Create a framerate widget

Status: RESOLVED FIXED

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

Comment 4

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

Attachment: v1 wip

WIP. Just needs a few tests.

Comment 5

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

Attachment: screenshot

Looks like this (top half).

Comment 6

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

Attachment: screencap.gif

Interacts like this (select,drag,zoom,scroll).

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looks nice :)

Comment 8

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

Attachment: v2 wip

Wrote some tests. Maybe we need a few more; I'll think about it, then ask for review.

Comment 9

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

Attachment: v3

Comment 10

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

Note that bug 1007460 will be adding a bar graph.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Created attachment 8425809 [details]
> screencap.gif
> 
> Interacts like this (select,drag,zoom,scroll).
Holy cow!!! That's awesome.

Comment 12

[Angelina Fabbro (:angelina)]

Is this something I could show next week and explain it is coming? It's applicable to both the mobile debugging and canvas performance talks I'm giving next week for CSSConf and JSConf.

Let me know what you're comfortable with - 'cause honestly this is pretty awesome as pbrosset correctly assessed.

Comment 13

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

Thanks Angelina :) This is just a piece of a larger puzzle [0] [1], and won't be something users can play with until bug 879008 is actually fixed. I... could create a simple tool that only displays this widget, specifically for a demo, but I don't actually think a framerate graph just by itself is all that useful.

[0] https://bugzilla.mozilla.org/showdependencytree.cgi?id=879008&hide_resolved=0
[1] http://cl.ly/image/0O1T251O1U16

Comment 14

[Angelina Fabbro (:angelina)]

I've had (what seems to me a disproportionate number, had no idea developers wanted this so much) a lot of requests for this which is why I suspect it would make a good mention about current-and-future work. Demos don't have to be exciting so much as informative. I try and balance between 'here are things you can use today' and 'look what we're doing'.

Does that sound alright?

Comment 15

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

Yup, sounds alright to me.

However, even if this bug lands there won't be any user-visible changes, nor something to actually use for a demo, since there's no tool to host this widget yet. A simple panel or add-on that uses this widget could be created though, and shouldn't be too hard. I'll try to cook something up this weekend.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8427244 [details] [diff] [review]
v3

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

Nice.
I have very little to say about the code. It looks pretty solid, and is rather nicely tested so far (of course we can't actually test the content of the graph itself, in the canvas, but the class API is well tested).
Things that can be cached are cached, and the graph is only redrawn when needed, so it even looks good performance-wise.

::: browser/devtools/shared/test/browser_graphs-01.js
@@ +15,5 @@
> +}
> +
> +function whenReady(_, graph) {
> +  ok(graph._container.classList.contains("line-graph-widget-container"),
> +    "The correct graph contaienr was created.");

s/contaienr/container

@@ +17,5 @@
> +function whenReady(_, graph) {
> +  ok(graph._container.classList.contains("line-graph-widget-container"),
> +    "The correct graph contaienr was created.");
> +  ok(graph._canvas.classList.contains("line-graph-widget-canvas"),
> +    "The correct graph contaienr was created.");

ditto

::: browser/devtools/shared/test/browser_graphs-02.js
@@ +33,5 @@
> +    graph.setRegions(TEST_REGIONS);
> +  } catch (e) {
> +    thrown2 = true;
> +  }
> +  ok(thrown2, "Setting regions twice shouldn't work.");

Why? Regions seem like a useful thing to be able to add, remove, clear, ...

::: browser/devtools/shared/test/browser_graphs-03.js
@@ +22,5 @@
> +  gBrowser.removeCurrentTab();
> +  finish();
> +});
> +
> +let testSelection = Task.async(function*(graph) {

testSelection is already called in an async function with yield, can't you just define it like so:
function* testSelection(graph) {
 ...
}

@@ +68,5 @@
> +  ok(graph.getSelection().end === null,
> +    "The graph's selection now has a null end value.");
> +});
> +
> +let testCursor = Task.async(function*(graph) {

Same as for testSelection

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +215,5 @@
> +    if (!this._cachedGraphImage) {
> +      throw "Can't highlighted regions on a graph with no data displayed.";
> +    }
> +    if (this._regions) {
> +      throw "Regions were already highlighted on the graph.";

See my earlier comment in one of the tests. I don't see why we really need to only support one set of regions.
Won't we need to be able to remove regions at some stage? Maybe regions will be used to highlight parts of a graph, depending on dynamic inputs given be the user.

@@ +444,5 @@
> +    if (this.hasSelection() || this.hasSelectionInProgress()) {
> +      this._drawSelection();
> +    }
> +    if (this.hasData()) {
> +      ctx.drawImage(this._cachedGraphImage, 0, 0, this._width, this._height);

I'm slowly going through the code so I might have missed something, but seeing that you draw the cached graph here from 0,0, to width/height tells me that the class doesn't let consumers zoom in on parts of the data to see more details or zoom out to see the bigger picture. Instead the selection only grows or shrinks.
This means that if this is a requirement for the tools we'll build with this graph class, then consumers will have to reset the data to handle zoom in/out themselves.
Isn't zooming something we would have built-in this class?

::: browser/themes/shared/devtools/widgets.inc.css
@@ +907,5 @@
>  .arrow[invisible] {
>    visibility: hidden;
>  }
>  
> +/* Canvas graphs */

widgets.inc.css is starting to look like a giant all-devtools.css CSS file.
I don't know if we need to start worrying about performance at all, but it isn't very good for its maintainability.

Comment 17

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> Comment on attachment 8427244 [details] [diff] [review]
> v3
> 
> ::: browser/devtools/shared/test/browser_graphs-02.js
> @@ +33,5 @@
> > +    graph.setRegions(TEST_REGIONS);
> > +  } catch (e) {
> > +    thrown2 = true;
> > +  }
> > +  ok(thrown2, "Setting regions twice shouldn't work.");
> 
> Why? Regions seem like a useful thing to be able to add, remove, clear, ...
> 

This is a very simple case of "I'm not needing this right now, so not implementing it yet". A potentially tricky problem is that the regions are baked (rendered) into the cached canvas graph, so removing regions would involve rebuilding the graph, etc. I'm just thinking of going to lazy/easy route here and invoke the YAGNI rule :)

> 
> I'm slowly going through the code so I might have missed something, but
> seeing that you draw the cached graph here from 0,0, to width/height tells
> me that the class doesn't let consumers zoom in on parts of the data to see
> more details or zoom out to see the bigger picture. Instead the selection
> only grows or shrinks.
> This means that if this is a requirement for the tools we'll build with this
> graph class, then consumers will have to reset the data to handle zoom
> in/out themselves.
> Isn't zooming something we would have built-in this class?
> 

You're right, there's no scaling support on zoom. This may be required for bug 1007460, but not for the framerate widget, so I'm not adding it here. It should be easy to implement though.

> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +907,5 @@
> >  .arrow[invisible] {
> >    visibility: hidden;
> >  }
> >  
> > +/* Canvas graphs */
> 
> widgets.inc.css is starting to look like a giant all-devtools.css CSS file.
> I don't know if we need to start worrying about performance at all, but it
> isn't very good for its maintainability.

Yeah :(

Comment 18

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

Attachment: v4

Addressed comments and fixed a few things I noticed while I was working on an addon for comment 14.

Comment 19

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

Angelina, here's an add-on good enough for a demo, the xpi is in the build folder: https://github.com/victorporof/FirefoxFramerateAddon Make sure you have attachment 8428703 [details] [diff] [review] (v4) applied.

Comment 20

[Patrick Brosset <:pbro>]

2 things I noticed while playing with the extension:

- start with a selection, then click outside the selection, on the graph, the selection disappears but a new selection starts with the click event. I don't know if this is intentional but it feels a bit weird. I would simply drop the selection.
- click to start a selection, then move your mouse to the far right or left until it leaves the browser window: the selection disappears. It might be good to keep the selection going until a 2nd click is done, especially if you're trying to create a selection that ends very close to the browser edge, then it's hard to do it.

Comment 21

[Patrick Brosset <:pbro>]

One more thing: it seems I can't make a selection reach the right edge of the graph by zooming it out.
When I use the mouse to click-drag it, then I can drag it as far as I want, but using the mouse wheel to zoom it out so that it expands, then it stops at about the gutter width from the right side.

Comment 22

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

Thanks! Good suggestions, will fix.

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8427244 [details] [diff] [review]
v3

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

not much to add to this. Code looks nice.

Some of the interactions we played with using a mousewheel are a little weak. Zooming with scroll wheel feels very slow compared to a trackpad with its ballistics. Might need to improve that.

Maybe using a key modifier with the DOMScrollWheel events to multiply by 10 or something would help here and not break people who have a trackpad.

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +444,5 @@
> +    if (this.hasSelection() || this.hasSelectionInProgress()) {
> +      this._drawSelection();
> +    }
> +    if (this.hasData()) {
> +      ctx.drawImage(this._cachedGraphImage, 0, 0, this._width, this._height);

good point. Also, at some point we'll probably want to use this widget in different tools, maybe even live streaming the framerate graph (as in a widget). Having either a rolling graph or the ability to only show a subset of the whole thing would be really valuable.

Comment 24

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> 
> - click to start a selection, then move your mouse to the far right or left
> until it leaves the browser window: the selection disappears. It might be
> good to keep the selection going until a 2nd click is done, especially if
> you're trying to create a selection that ends very close to the browser
> edge, then it's hard to do it.

Addressed all comments except this one. Since I don't know what the UI will end up looking like, I'll file a followup.

(In reply to Rob Campbell [:rc] (:robcee) from comment #23)
> Comment on attachment 8427244 [details] [diff] [review]
> v3
 
> good point. Also, at some point we'll probably want to use this widget in
> different tools, maybe even live streaming the framerate graph (as in a
> widget). Having either a rolling graph or the ability to only show a subset
> of the whole thing would be really valuable.

True; however, since this isn't blocking work on bug 879008, and landing code that won't be used for quite some time is usually problematic, I prefer fixing this in a followup, when necessary.

Comment 25

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

To fix the scrolling problem, turns out I had to use the MozMousePixelScroll event instead of "wheel", which normalizes sensitivity across different inputs.

Comment 26

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

Attachment: v5

Addressed comments, going through try.

Comment 27

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

Attachment: v6

I fixed a few failures on try and made a few changes. Can you please have another look at this Patrick?

* Tests now use toolbox hosts to contain the graph. Previously, the container was gBrowser.selectedBrowser.parentNode, which seemed to be a source of leaks, even though the graph was properly removed.
* Removed the quadratic curve in favor of a simple line. I've seen some graphs looking pretty hideous, because of the very crude way control points were calculated. Since calculating beautiful curved lines is somewhat CPU intensive, let's avoid it for the time being.
* Addressed comments about selection, mouse sensitivity etc. Let me know if it feels right now.

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8429292 [details] [diff] [review]
v6

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

Nothing to say about the code. Given the assumptions and requirements, I think it's great.

Now, playing with the addon in the toolbox:
- The quadratic curve looked better, but the lines look more precise, if you know what I mean. So I'm fine with using the lines.
- The mouse interactions are better. Selecting/deselecting feels more natural.
- One thing I noted which is related to the cached graph canvas: resizing the toolbox and/or browser window feels like we are resizing an image, which is exactly what we're doing. So making the window smaller or bigger makes the resolution of the graph look kind of weird. This would be easily fixed by re-rendering the graph, and in the window resize use case, performance isn't going to be a problem because we can always throttle the number of times we re-render the graph and it's not like users resize their windows very often. This also happens if you switch host orientation (from footer to side) though.
I think this one is worth fixing in this bug, but you may convince me otherwise :)
- I noticed a bug when resizing the browser window: this completely breaks the mouse coordinates detection. Now selections appear at an offset with regards to the actual mouse position.

R+ because the code is good and the resize thing can probably be fixed without tooooo much extra code.

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +172,5 @@
> +    return this._height;
> +  },
> +
> +  /**
> +   * Destroys this grpah.

s/grpah/graph

@@ +174,5 @@
> +
> +  /**
> +   * Destroys this grpah.
> +   */
> +  remove: function() {

Good addition, I didn't even think of this during the first review :(
Classes in the devtools tend to use the verb "destroy" for this kind of method though.
Up to you to change it or not. I think destroy it would feel more natural here, especially that "remove" usually goes in pair with an "add" method that lets consumer re-create the graph.

@@ +910,5 @@
> +    let totalTicks = 0;
> +    let firstTick;
> +    let lastTick;
> +
> +    for (let [tick, value] of Iterator(this._data)) {

I had never seen this way of looping over objects until now. Very cool!

@@ +1012,5 @@
> +    this._maxTooltip.querySelector("[text=value]").textContent = maxValue|0;
> +    this._avgTooltip.querySelector("[text=value]").textContent = avgValue|0;
> +    this._minTooltip.querySelector("[text=value]").textContent = minValue|0;
> +
> +    function map(value, istart, istop, ostart, ostop) {

Either a comment line or a more self-explanatory function name would help here.

Comment 29

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #28)
> Comment on attachment 8429292 [details] [diff] [review]
> v6
> 
> - One thing I noted which is related to the cached graph canvas: resizing
> the toolbox and/or browser window feels like we are resizing an image, which
> is exactly what we're doing. So making the window smaller or bigger makes
> the resolution of the graph look kind of weird. This would be easily fixed
> by re-rendering the graph, and in the window resize use case, performance
> isn't going to be a problem because we can always throttle the number of
> times we re-render the graph and it's not like users resize their windows
> very often. This also happens if you switch host orientation (from footer to
> side) though.
> I think this one is worth fixing in this bug, but you may convince me
> otherwise :)
> - I noticed a bug when resizing the browser window: this completely breaks
> the mouse coordinates detection. Now selections appear at an offset with
> regards to the actual mouse position.
> 

You're right, resizing isn't handled at all in this patch, and taking care of it would solve the above problems. I was thinking of postponing this until starting work on the profiler frontend, but it does seem like this bug is a better place for that. I'll take a look at it!

> R+ because the code is good and the resize thing can probably be fixed
> without tooooo much extra code.
> 

Yup, just a listener, draining the events, redrawing the graph and updating the canvas size. I'll fix it here.

> @@ +174,5 @@
> > +
> > +  /**
> > +   * Destroys this grpah.
> > +   */
> > +  remove: function() {
> 
> Good addition, I didn't even think of this during the first review :(
> Classes in the devtools tend to use the verb "destroy" for this kind of
> method though.
> Up to you to change it or not. I think destroy it would feel more natural
> here, especially that "remove" usually goes in pair with an "add" method
> that lets consumer re-create the graph.
> 

Destroy sounds good.

> @@ +1012,5 @@
> > +    this._maxTooltip.querySelector("[text=value]").textContent = maxValue|0;
> > +    this._avgTooltip.querySelector("[text=value]").textContent = avgValue|0;
> > +    this._minTooltip.querySelector("[text=value]").textContent = minValue|0;
> > +
> > +    function map(value, istart, istop, ostart, ostop) {
> 
> Either a comment line or a more self-explanatory function name would help
> here.

Heh, I first used this in Processing [0], like a million years ago. It basically maps a value from one range to another. Agreed that a comment would help.

http://www.processing.org/reference/map_.html

Comment 30

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

Attachment: v7

Now handling resizing. Added test. Going through try.

Comment 31

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

https://tbpl.mozilla.org/?tree=Try&rev=cad7cdb13740

Comment 32

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

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

Comment 33

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

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