Closed Bug 1374587 Opened 7 years ago Closed 7 years ago

The grid inspector panel feels slow on https://stripe.com/connect

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached image checkbox-selection.gif
1. Open https://stripe.com/connect
2. Open the inspector and switch to the new Layout panel in the sidebar
3. Click on any of the checkboxes in the list of grids. The grid should highlight in the page and the checkbox should become selected.
4. Now, click on another checkbox to highlight another grid.

==> The second grid becomes highlighted, no problem, but the checkbox looks like it takes a couple of seconds to become selected.
Actually it switches to the new checkbox, and then goes back to the first checkbox, and then it takes a few seconds to completely switch the new checkbox.

See the attached gif.
I have investigated this problem a little bit and discovered the following:

- the page has at least 2 animations with SVG elements
- these animations are done by mutating attributes in the DOM
- these mutations cause reflows
- DevTools tracks reflows on the server (with the reflow actor) and sends those as events to the client
- the grid inspector listens to reflows on the client, and uses those to refresh the panel

In fact, if you delete the animated SVG nodes from the document, the problem goes away.

So, we need to find a way to prevent those reflows from causing this perceived performance problem in the grid inspector.

I believe we used reflows to update the grid outline so it would always reflect the right grid shape.
The reflow listener was implemented in bug 1348005.
I think the idea was to use reflows to:
- update the grid outline when the shape of the grid on the page changed,
- update the content of the grid list if media queries made the number of grids change when resizing.

I think we should do 3 things:
- First, throttle updates on reflow, if we don't already do it. The stripe page does a lot of them, and we could accept some delay in updating the grid outline.
- Second, if after a reflow, the list of grids is the same, then early return.
- Third, let all reflows update the current grid outline. We can't know if a grid outline will need to be updated after a reflow, so we have no other way than to update the outline all the time. But the rest of the UI doesn't have to update if the list of grids hasn't changed.
Note that the commit I just pushed to mozreview doesn't cover the whole solution described in comment 2.
It does 2 things:

- NodeActor IDs are now stored on GridActor's forms when possible (if the Walker knows of them). This has the benefit of avoiding a round trip on the server to get the NodeActors in some cases (in fact, the first time, there are chances the DOM nodes aren't known yet, because the inspector hasn't yet been expanded to show these nodes, so the Walker doesn't know them yet. But as soon as we refresh the layout panel a second time, the nodes will be known, because they've already been retrieved once, so we won't need to go to the server again. This happens all the time on the stripe page, so this will help with perf a lot).

- On reflow, we now do some filtering. When we detect a reflow, we refresh the panel, which means we go and get the new list of grids. So we have the previous list of grids, and the next list of grids. Now, if we know the DOM nodes for both of these lists (which we do, as soon as we've refreshed at least once), we can compare if they're the same.
Here, I assume that if we get the same list of DOM nodes, then we have the same list of grids, and therefore we don't need to refresh.
This should be ok, because the layout panel only lists DOM nodes where grids are attached. It doesn't make any assumptions to how many grid fragments are attached or that sort of stuff. So this should be a nice perf improvement too, since we won't be refreshing the panel anymore, only if the list of dom nodes with grids changes.

Now, as I said, this does not cover everything: the grid outline now does not refresh on all reflows, since we do filtering. So, if you resize the window for instance, and the current grid changes, but nothing else does (still as many grids, and still attached to the same DOM nodes), then the outline won't be updated.

To fix this, we need something at the start of onReflow that would, no matter what, just update the outline. I don't know how to do this yet. I need your help Gabriel to make this work.
Flags: needinfo?(gl)
Comment on attachment 8879537 [details]
Bug 1374587 - Avoid getting NodeActors for grids when we already know them and filter reflows;

https://reviewboard.mozilla.org/r/150826/#review155730
Attachment #8879537 - Flags: review?(gl) → review+
(In reply to Patrick Brosset <:pbro> from comment #4)
> Note that the commit I just pushed to mozreview doesn't cover the whole
> solution described in comment 2.
> It does 2 things:
> 
> - NodeActor IDs are now stored on GridActor's forms when possible (if the
> Walker knows of them). This has the benefit of avoiding a round trip on the
> server to get the NodeActors in some cases (in fact, the first time, there
> are chances the DOM nodes aren't known yet, because the inspector hasn't yet
> been expanded to show these nodes, so the Walker doesn't know them yet. But
> as soon as we refresh the layout panel a second time, the nodes will be
> known, because they've already been retrieved once, so we won't need to go
> to the server again. This happens all the time on the stripe page, so this
> will help with perf a lot).
> 
> - On reflow, we now do some filtering. When we detect a reflow, we refresh
> the panel, which means we go and get the new list of grids. So we have the
> previous list of grids, and the next list of grids. Now, if we know the DOM
> nodes for both of these lists (which we do, as soon as we've refreshed at
> least once), we can compare if they're the same.
> Here, I assume that if we get the same list of DOM nodes, then we have the
> same list of grids, and therefore we don't need to refresh.
> This should be ok, because the layout panel only lists DOM nodes where grids
> are attached. It doesn't make any assumptions to how many grid fragments are
> attached or that sort of stuff. So this should be a nice perf improvement
> too, since we won't be refreshing the panel anymore, only if the list of dom
> nodes with grids changes.
> 
> Now, as I said, this does not cover everything: the grid outline now does
> not refresh on all reflows, since we do filtering. So, if you resize the
> window for instance, and the current grid changes, but nothing else does
> (still as many grids, and still attached to the same DOM nodes), then the
> outline won't be updated.
> 
> To fix this, we need something at the start of onReflow that would, no
> matter what, just update the outline. I don't know how to do this yet. I
> need your help Gabriel to make this work.

What if we added a check to see if the grid fragment data is different before updating as well?
Flags: needinfo?(gl)
Priority: -- → P3
I'm working on a new version of the patch based on comment 6.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8879537 [details]
Bug 1374587 - Avoid getting NodeActors for grids when we already know them and filter reflows;

I made substantial changes to this patch, so I'd like a second review from you Gabriel.
Attachment #8879537 - Flags: review+ → review?(gl)
Blocks: dt-grid
Comment on attachment 8879537 [details]
Bug 1374587 - Avoid getting NodeActors for grids when we already know them and filter reflows;

https://reviewboard.mozilla.org/r/150826/#review156966

I w

::: devtools/client/inspector/grids/grid-inspector.js:672
(Diff revision 2)
> + * @param  {Array} fragments2
> + *         Another list of gridFragment objects to compare to the first list.
> + * @return {Boolean}
> + *         True if the fragments are the same, false otherwise.
> + */
> +function compareFragmentsGeometry(fragments1, fragments2) {

Let's move this into an utils.js file in grids/utils/ if we are gonna export it.

::: devtools/client/inspector/inspector.js:26
(Diff revision 2)
>  
>  const Menu = require("devtools/client/framework/menu");
>  const MenuItem = require("devtools/client/framework/menu-item");
>  
>  const {HTMLBreadcrumbs} = require("devtools/client/inspector/breadcrumbs");
> -const GridInspector = require("devtools/client/inspector/grids/grid-inspector");
> +const {GridInspector} = require("devtools/client/inspector/grids/grid-inspector");

Can be undone if we move compareFragmentsGeometry to utils.js

::: devtools/client/inspector/shared/utils.js:133
(Diff revision 2)
>  }
>  
> +exports.debounce = debounce;
> +
> +/**
> + * From underscore's `_.throttle`

I think it would still be good to copy over the throttle description, @params, and JSDocs from underscore.js

::: devtools/shared/fronts/layout.js:30
(Diff revision 2)
> +
> +  /**
> +   * In some cases, the GridActor already knows the NodeActor ID of the node where the
> +   * grid is located. In such cases, this getter returns the NodeFront for it.
> +   */
> +  get containerNodeFront() {

Move this before get gridFragments()
Attachment #8879537 - Flags: review?(gl) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/909ccc166a06
Avoid getting NodeActors for grids when we already know them and filter reflows; r=gl
Backed out on suspicion of causing timeouts in browser_grids_number-of-css-grids-telemetry.js:

https://hg.mozilla.org/integration/autoland/rev/9cc878c45ffb35a7f426c5cb3574802763fdb97c

First push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1e96a289d28acba3ebf1382cf128e2ec49e9d002&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=109587293&repo=autoland

[task 2017-06-23T15:55:31.900905Z] 15:55:31     INFO - Entering test bound 
[task 2017-06-23T15:55:31.901799Z] 15:55:31     INFO - Adding a new tab with URL: data:text/html;charset=utf-8,%0A%20%20%3Cdiv%3E%3C%2Fdiv%3E%0A
[task 2017-06-23T15:55:31.902620Z] 15:55:31     INFO - Tab added and finished loading
[task 2017-06-23T15:55:31.903321Z] 15:55:31     INFO - Opening the inspector
[task 2017-06-23T15:55:31.904030Z] 15:55:31     INFO - Opening the toolbox
[task 2017-06-23T15:55:31.905197Z] 15:55:31     INFO - Buffered messages logged at 15:54:08
[task 2017-06-23T15:55:31.907017Z] 15:55:31     INFO - Toolbox opened and focused
[task 2017-06-23T15:55:31.908787Z] 15:55:31     INFO - Waiting for actor features to be detected
[task 2017-06-23T15:55:31.910537Z] 15:55:31     INFO - Buffered messages logged at 15:54:09
[task 2017-06-23T15:55:31.911938Z] 15:55:31     INFO - Selecting the layoutview sidebar
[task 2017-06-23T15:55:31.913171Z] 15:55:31     INFO - Navigate to TEST_URI2
[task 2017-06-23T15:55:31.914922Z] 15:55:31     INFO - Waiting for state predicate "state => state.grids.length == 1"
[task 2017-06-23T15:55:31.916639Z] 15:55:31     INFO - Navigating to: data:text/html;charset=utf-8,%0A%20%20%3Cstyle%20type%3D'text%2Fcss'%3E%0A%20%20%20%20%23grid%20%7B%0A%20%20%20%20%20%20display%3A%20grid%3B%0A%20%20%20%20%7D%0A%20%20%3C%2Fstyle%3E%0A%20%20%3Cdiv%20id%3D%22grid%22%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cell1%22%3Ecell1%3C%2Fdiv%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cell2%22%3Ecell2%3C%2Fdiv%3E%0A%20%20%3C%2Fdiv%3E%0A
[task 2017-06-23T15:55:31.917639Z] 15:55:31     INFO - Buffered messages logged at 15:54:10
[task 2017-06-23T15:55:31.919231Z] 15:55:31     INFO - Waiting for markup view to load after navigation.
[task 2017-06-23T15:55:31.920562Z] 15:55:31     INFO - Buffered messages logged at 15:54:11
[task 2017-06-23T15:55:31.921798Z] 15:55:31     INFO - Waiting for new root.
[task 2017-06-23T15:55:31.922994Z] 15:55:31     INFO - Waiting for inspector to update after new-root event.
[task 2017-06-23T15:55:31.923794Z] 15:55:31     INFO - Buffered messages finished
[task 2017-06-23T15:55:31.924503Z] 15:55:31     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/grids/test/browser_grids_number-of-css-grids-telemetry.js | Test timed out - 
[task 2017-06-23T15:55:31.925163Z] 15:55:31     INFO - GECKO(1302) | --DOCSHELL 0x7fec372a1800 == 10 [pid = 1302] [id = {21b54df6-c0cc-4049-88dc-96b4ca1166af}]
[task 2017-06-23T15:55:31.925846Z] 15:55:31     INFO - GECKO(1302) | ++DOMWINDOW == 25 (0x7fec29751000) [pid = 1302] [serial = 351] [outer = 0x7fec33c46000]
[task 2017-06-23T15:55:31.926496Z] 15:55:31     INFO - GECKO(1302) | --DOCSHELL 0x7fec3e686000 == 9 [pid = 1302] [id = {8d037a95-baab-4ee9-96c8-10de721114c9}]
[task 2017-06-23T15:55:31.927363Z] 15:55:31     INFO - GECKO(1302) | --DOCSHELL 0x7fec3d3b0800 == 8 [pid = 1302] [id = {bb04723b-294d-4c0e-a571-fdb05039e1be}]
[task 2017-06-23T15:55:31.927948Z] 15:55:31     INFO - Removing tab.
[task 2017-06-23T15:55:31.928642Z] 15:55:31     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2017-06-23T15:55:31.929618Z] 15:55:31     INFO - Got event: 'TabClose' on [object XULElement].
[task 2017-06-23T15:55:31.986410Z] 15:55:31     INFO - Tab removed and finished closing
Flags: needinfo?(pbrosset)
Attachment #8881471 - Attachment is obsolete: true
Sorry about the many new review request updates here. I was having a problem to push to try and ending up having to push to mozreview instead and then triggering try pushes from there instead.
Anyway, it looks like I've figured out the problem that required the patch to be backed out:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbcedc11b2bf0609d26eccd212ddfc299cd1b3f2&group_state=expanded
Flags: needinfo?(pbrosset)
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbe757a7ab32
Avoid getting NodeActors for grids when we already know them and filter reflows; r=gl
https://hg.mozilla.org/mozilla-central/rev/bbe757a7ab32
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I have reproduced this bug with Nightly 56.0a1 (2017-06-20) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 56.0a1 !

Build   ID    20170630030203
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170628]
Depends on: 1378306
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.