Closed Bug 1169035 Opened 5 years ago Closed 4 years ago

AbstractCanvasGraph.prototype._getRelativeEventCoordinates causes very costly layouts

Categories

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

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: vporof, Assigned: vporof)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached image Screen Shot 2015-05-27 at 5.11.38 PM.png (obsolete) —
This is what happens when clicking and dragging in the overview graphs when the waterfall is pretty large.
Attached file profile.json (obsolete) —
Profile.
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.
Attached patch canvas-bounds.patch (obsolete) — Splinter Review
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?
Attachment #8611486 - Flags: feedback?(vporof)
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?
Attachment #8611486 - Flags: feedback?(vporof) → feedback+
(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.
Oh I forgot that the FGW doesn't have this new method. Carry on.
I'll take a look at this later next week.  Feel free to steal it in the meantime if you want to
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Depends on: 1167379
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.
(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
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
(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.
(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.
I'm not going to be able to work on this at the moment.. Unassigning in case someone else wants to take it.
Assignee: bgrinstead → nobody
Status: ASSIGNED → NEW
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
VP, any chance you can check this out?
Flags: needinfo?(vporof)
Sure!
Flags: needinfo?(vporof)
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
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.
Attachment #8611455 - Attachment is obsolete: true
Attachment #8611456 - Attachment is obsolete: true
Attachment #8611486 - Attachment is obsolete: true
Attachment #8709476 - Flags: review?(jsantell)
Comment on attachment 8709476 [details] [diff] [review]
v1

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

nice
Attachment #8709476 - Flags: review?(jsantell) → review+
https://hg.mozilla.org/mozilla-central/rev/244da089ea48
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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/
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.