1169035 - AbstractCanvasGraph.prototype._getRelativeEventCoordinates causes very costly layouts

Status: RESOLVED FIXED

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

Description

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

Attachment: Screen Shot 2015-05-27 at 5.11.38 PM.png

This is what happens when clicking and dragging in the overview graphs when the waterfall is pretty large.

Comment 1

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

Attachment: profile.json

Profile.

Comment 2

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

The 0fps flatline is when continuously selected different areas in the overview graph 6 times. There are 6 islands of markers, each for one redraw of the waterfall.

Curiously enough, _getRelativeEventCoordinates is the most expensive operation there.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: canvas-bounds.patch

Just one idea - the other idea is to cache the result from getBoxQuads and invalidate either on a short timer or when certain events happen (like a resize on the top level window).

I haven't tested the perf of this approach vs the current one though.  Victor, can you check this out and see how it compares?

Comment 4

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

Comment on attachment 8611486 [details] [diff] [review]
canvas-bounds.patch

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

Nice
Should probably share this logic with the flame graph? Move it in a utils somewhere?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #4)
> Comment on attachment 8611486 [details] [diff] [review]
> canvas-bounds.patch
> 
> Review of attachment 8611486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice
> Should probably share this logic with the flame graph? Move it in a utils
> somewhere?

The flame chart doesn't yet bind to the top level window for mouse movement so it's still using the old _getContainerOffset (that doesn't traverse frames but instead reads offsetLeft/offsetTop).

In order to share this code we'd want to refactor the flamegraph to use the top level mouse movement - which we should do anyway, because there is a bug with the flame graph right now where if you start a drag and then move the mouse up into another window while it's happening, it sticks.

Comment 6

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

Oh I forgot that the FGW doesn't have this new method. Carry on.

Comment 7

[Brian Grinstead [:bgrins]]

I'll take a look at this later next week.  Feel free to steal it in the meantime if you want to

Comment 8

[Brian Grinstead [:bgrins]]

The new test from Bug 1167379 seems to have caught an error with this approach, in that the getBoundsWithoutFlushing value seems wrong after setting frame.docShell.contentViewer.zoomValue.

268 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_graphs-07e.js | The current selection start value is correct (1). - Got 92, expected 100
269 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_graphs-07e.js | The current selection end value is correct (1). - Got 92, expected 100 
...etc at https://treeherder.mozilla.org/#/jobs?repo=try&revision=58342ae1bba4

Running locally with this patch I see the test failure, although I'm not seeing wrong offsets when zooming and moving the selection manually.  I wonder if the layout is flushing when the zoom is changed via the UI (like a keyboard shortcut), but isn't when the zoomValue is set directly.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Running locally with this patch I see the test failure, although I'm not
> seeing wrong offsets when zooming and moving the selection manually.  I
> wonder if the layout is flushing when the zoom is changed via the UI (like a
> keyboard shortcut), but isn't when the zoomValue is set directly.

Never mind - I see that zoom offsets are in fact incorrect with the patch applied

Comment 10

[Brian Grinstead [:bgrins]]

Interesting.  If I open this URL: `data:text/html,<h1>hello</h1>` and then run the following in the browser console I get some different results between getBoundsWithoutFlushing and getBoxQuads.

let utils = gBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                    .getInterface(Ci.nsIDOMWindowUtils);

var width1 = utils.getBoundsWithoutFlushing(gBrowser.contentDocument.querySelector("h1")).width;
var width2 = gBrowser.contentDocument.querySelector("h1").getBoxQuads()[0].bounds.width;

console.log(width1 == width2); // true

gBrowser.docShell.contentViewer.fullZoom = 2;

var width1 = utils.getBoundsWithoutFlushing(gBrowser.contentDocument.querySelector("h1")).width;
var width2 = gBrowser.contentDocument.querySelector("h1").getBoxQuads()[0].bounds.width;

console.log(width1 == width2); // false

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Interesting.  If I open this URL: `data:text/html,<h1>hello</h1>` and then
> run the following in the browser console I get some different results
> between getBoundsWithoutFlushing and getBoxQuads.

And I don't think this has anything to do with flushing.  It just seems like the getBoxQuads API takes the zoom into account while getBoundsWithoutFlushing doesn't.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > Interesting.  If I open this URL: `data:text/html,<h1>hello</h1>` and then
> > run the following in the browser console I get some different results
> > between getBoundsWithoutFlushing and getBoxQuads.
> 
> And I don't think this has anything to do with flushing.  It just seems like
> the getBoxQuads API takes the zoom into account while
> getBoundsWithoutFlushing doesn't.

Actually, I spoke too soon.  If I call getBoxQuads *before* getBoundsWithoutFlushing the second time around the widths do match.

Comment 13

[Brian Grinstead [:bgrins]]

I'm not going to be able to work on this at the moment.. Unassigning in case someone else wants to take it.

Comment 14

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

This is pretty brutal in some cases. Believe it's the cause of bug 1176528. Outside of of getRelativeEventCoordinates, it's usually surrounded by lots of style flushes as well which may or may not be related to this. Check out the browser toolbox recording here: https://bug1176537.bugzilla.mozilla.org/attachment.cgi?id=8625104

Comment 15

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

VP, any chance you can check this out?

Comment 16

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

Sure!

Comment 17

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

Attachment: v1

Turns out we never really have to refresh the bounding box coords, since the canvas graphs positions never change *inside* the performance tool. But the `_maybeDirtyBoundingBox` thing is there in case some other consumer needs it.

Comment 18

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

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

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

nice

Comment 19

[Pulsebot]

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/244da089ea48

Comment 21

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
A/c to what I have understood, the graph of waterfall I am able to see in pieces for Cycle Collection, CC Graph Reduction starting from 0 ms to 70062 ms. But time is not continued as said in STR screenshot.


Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Longer duration of profiler data.

Actual Results: 
Not sure
http://postimg.org/image/k2ok1cidv/