Closed Bug 1396784 Opened 2 years ago Closed 2 years ago

Layout, Grid and Fonts views should be lazy loaded

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Followup of bug 1335419. That bug introduced lazy loading of computed and rules view, but we still load at inspector startup all dependencies from Layout, Grid and Fonts views.

After bug 1396783 is applied, this comes as the most significant piece of JS computation in the parent process.
See https://perfht.ml/2eBpem4 which highlight layout.js cost (15ms/5% of overall JS computation in parent process).

This is about tweaking this code:
http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#629-651
To do like computed and rules views and only load when we switch to each view.
Priority: -- → P3
Assignee: nobody → poirot.alex
No more occurence of layout nor grid codebase when opening the inspector on the rule view with this patch:
  https://perfht.ml/2y2vCLM
Comment on attachment 8907113 [details]
Bug 1396784 - Move gridInspector property to layout view.

https://reviewboard.mozilla.org/r/178822/#review183998
Attachment #8907113 - Flags: review?(gl) → review+
Comment on attachment 8907112 [details]
Bug 1396784 - Lazy load Layout view when opening the inspector.

https://reviewboard.mozilla.org/r/178820/#review184014

::: devtools/client/inspector/toolsidebar.js:82
(Diff revision 1)
>  
>      let sidebar = Tabbar({
>        menuDocument: this._toolPanel._toolbox.doc,
>        showAllTabsMenu: true,
>        onSelect: this.handleSelectionChange.bind(this),
> +      renderOnlySelected: true,

Move this before onSelect
Attachment #8907112 - Flags: review?(gl) → review+
Well, DAMP reports a 2% win:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=d32c79e1b4510750734492c7d97f9c5bd7e62f3c&framework=1&showOnlyImportant=0&selectedTimeRange=172800

But try also reported issues with the renderOnlySelected: true. Sidebar panels are not meant to be completely unrendered when unselected, it breaks their state and are not updated correctly when selected again.
We may want to fix that but that sounds outside of the scope of this bug.

So I tweaked Tabs.render again to only lazy create panel if `selected` is true.
Could you confirm you are still ok with this interdiff:
  https://reviewboard.mozilla.org/r/178820/diff/1-2/

(I also added a inspector.sidebar.once("select") in shared-head which may help chase intermittents...)
Flags: needinfo?(gl)
Yes, it's fine.
Flags: needinfo?(gl)
Comment on attachment 8907112 [details]
Bug 1396784 - Lazy load Layout view when opening the inspector.

https://reviewboard.mozilla.org/r/178820/#review184674

::: devtools/client/shared/components/tabs/tabbar.js
(Diff revision 2)
> -        url: tab.url,
> -      });
> -    }
> -
> -    return tab.panel;
> -  },

I had to revert that back in order to unbreak this test:
devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html
Hopefully, we don't keep this code just for a test...
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1061923744e8
Lazy load Layout view when opening the inspector. r=gl
https://hg.mozilla.org/integration/autoland/rev/99f6b31d5f44
Move gridInspector property to layout view. r=gl
Backed out for leaks in devtools' browser_boxmodel_computed-accordion-state.js:

https://hg.mozilla.org/integration/autoland/rev/3b18e792cc3eb0bf381a422511c5bdde66f10af8
https://hg.mozilla.org/integration/autoland/rev/7d4a2f21a7b3b04981f8b4c6cb8ddd36cb6deaa8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=99f6b31d5f44f2a3106fac760a7546fbd598e1a8&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=130810455&repo=autoland

 63 ERROR TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_computed-accordion-state.js | leaked 4 window(s) until shutdown [url = about:blank]
64 ERROR TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_computed-accordion-state.js | leaked 2 window(s) until shutdown [url = chrome://devtools/content/inspector/markup/markup.xhtml]
65 ERROR TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_computed-accordion-state.js | leaked 1 window(s) until shutdown [url = about:devtools-toolbox]
66 ERROR TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_computed-accordion-state.js | leaked 1 window(s) until shutdown [url = chrome://devtools/content/inspector/inspector.xhtml]
Flags: needinfo?(poirot.alex)
Oh sorry, it looks like I forgot to push to mozreview my local fixes before merging to autoland...

I got a gree try with my local fixes:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b21e230689f056c8c0375dd5d61a57ed8dd964de
And it is not what I pushed via mozreview!
Flags: needinfo?(poirot.alex)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9ffc0949657
Lazy load Layout view when opening the inspector. r=gl
https://hg.mozilla.org/integration/autoland/rev/1bf043d31e2e
Move gridInspector property to layout view. r=gl
https://hg.mozilla.org/mozilla-central/rev/c9ffc0949657
https://hg.mozilla.org/mozilla-central/rev/1bf043d31e2e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.