Closed
Bug 1396784
Opened 7 years ago
Closed 7 years ago
Layout, Grid and Fonts views should be lazy loaded
Categories
(DevTools :: Inspector, enhancement, P3)
DevTools
Inspector
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.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
No more occurence of layout nor grid codebase when opening the inspector on the rule view with this patch: https://perfht.ml/2y2vCLM
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ca05f454506a4d462d3740e7df7f735a861cedf
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b21e230689f056c8c0375dd5d61a57ed8dd964de
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review |
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...
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9ffc0949657 https://hg.mozilla.org/mozilla-central/rev/1bf043d31e2e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•