Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551403002:
[DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup (Closed)
|
Created:
4 years ago by phulce Modified:
4 years ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup
Reduces the number of layouts performed.
Moves update to be run in a requestAnimationFrame callback.
Cleans up the code a bit to move pieces of the massive update functions
into their own methods for a clearer breakdown of where time is spent.
BUG=657141
R=lushnikov,paulirish
Committed: https://crrev.com/bd86a87396a5ddb188b44744b5a46b111b7bf6aa
Cr-Commit-Position: refs/heads/master@{#437742}
Patch Set 1 #Patch Set 2 : remove comments #Patch Set 3 : Include performance improvements #
Total comments: 8
Patch Set 4 : address feedback #
Total comments: 14
Patch Set 5 : address more feedback #
Messages
Total messages: 20 (8 generated)
Description was changed from ========== [DevTools] Cleanup Elements Breadcrumbs As step one of 657141 to make breadcrumbs more performant and prevent a single massive review, this patch cleans up the code a bit to move pieces of the massive update functions into their own methods. BUG=657141 R=lushnikov ========== to ========== [DevTools] Cleanup ElementsBreadcrumbs As step one of 657141 to make breadcrumbs more performant and prevent a single massive review, this patch cleans up the code a bit to move pieces of the massive update functions into their own methods. BUG=657141 R=lushnikov ==========
Description was changed from ========== [DevTools] Cleanup ElementsBreadcrumbs As step one of 657141 to make breadcrumbs more performant and prevent a single massive review, this patch cleans up the code a bit to move pieces of the massive update functions into their own methods. BUG=657141 R=lushnikov ========== to ========== [DevTools] Cleanup ElementsBreadcrumbs Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. Reduces the number of layouts performed and moves update logic into requestAnimationFrame. BUG=657141 R=lushnikov,paulirish ==========
phulce@chromium.org changed reviewers: + paulirish@chromium.org
Description was changed from ========== [DevTools] Cleanup ElementsBreadcrumbs Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. Reduces the number of layouts performed and moves update logic into requestAnimationFrame. BUG=657141 R=lushnikov,paulirish ========== to ========== [DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup Reduces the number of layouts performed. Moves update to be run in a requestAnimationFrame callback. Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. BUG=657141 R=lushnikov,paulirish ==========
Updated to include the perf improvements since they were smaller than expected Screenshots of the difference on my Macbook w/10x CPU throttling Before https://drive.google.com/open?id=0B6vpe0x8Kd7VRGg5c2ppQUhtdHM After https://drive.google.com/open?id=0B6vpe0x8Kd7VT0Z1WFBMVGVPQzg
oops missed your patch a bunch of intermediate comments for now https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js (right): https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:103: } else { style: no "else" after the "then" branch was terminated with the "return". if (domNode.pseudoType()) return; Componens.DOMPresentation... https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:162: var onClickCrumb = this._onClickCrumb.bind(this); can we inline this? https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:227: var available = crumbs.offsetWidth; can we use .measurePrefferedSize() method? https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:255: return {normal, compact, collapsed, available}; so far we don't use shorthand notation yet. No one checked if it compiles successfully with closure, and we haven't discussed if it's a good thing to have
https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js (right): https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:103: } else { On 2016/12/08 05:37:03, lushnikov wrote: > style: no "else" after the "then" branch was terminated with the "return". > > if (domNode.pseudoType()) > return; > Componens.DOMPresentation... done, should I apply that to all the case statements as well? (i.e. remove break?) https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:162: var onClickCrumb = this._onClickCrumb.bind(this); On 2016/12/08 05:37:03, lushnikov wrote: > can we inline this? sure, we'll bind inside a loop but I guess the breadcrumbs aren't supposed to be *that* deep? https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:227: var available = crumbs.offsetWidth; On 2016/12/08 05:37:03, lushnikov wrote: > can we use .measurePrefferedSize() method? oh, jw why would that be better? looks like that will force an extra layout :/ https://codereview.chromium.org/2551403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:255: return {normal, compact, collapsed, available}; On 2016/12/08 05:37:03, lushnikov wrote: > so far we don't use shorthand notation yet. No one checked if it compiles > successfully with closure, and > we haven't discussed if it's a good thing to have apparently it does :) haha will remove for now
https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js (right): https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:98: _determineElementTitle(domNode, crumbElement) { I would expect this method to accept only a domNode and return title, without causing any side-effects. Having it mutating crumbElement doesn't feel clean. Can we either keep it inline or avoid passing crumbElement in? https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:177: * @return {!Object} let's annotate it with details: {{selectedIndex: number, focusedIndex: number, selectedCrumb: Element}} https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:201: return {selectedIndex, focusedIndex, selectedCrumb}; nit: let's avoid object shorthands here as well https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:205: * @return {!Object} let's annotate this with details https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:211: // dummy element for the collapsed size style: "." in the end of the comment https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:212: var collapsedElem = createElementWithClass('span', 'crumb collapsed'); var collapsedElement = ... style: we try to avoid as much abbreviations as possible https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:216: var collapsed = crumbs.firstChild.offsetWidth; var collapsed = collapsedElem.offsetWidth;
https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js (right): https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:98: _determineElementTitle(domNode, crumbElement) { On 2016/12/08 21:20:43, lushnikov wrote: > I would expect this method to accept only a domNode and return title, without > causing any > side-effects. Having it mutating crumbElement doesn't feel clean. > > Can we either keep it inline or avoid passing crumbElement in? > > Done. https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:177: * @return {!Object} On 2016/12/08 21:20:43, lushnikov wrote: > let's annotate it with details: {{selectedIndex: number, focusedIndex: number, > selectedCrumb: Element}} Done. https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:201: return {selectedIndex, focusedIndex, selectedCrumb}; On 2016/12/08 21:20:43, lushnikov wrote: > nit: let's avoid object shorthands here as well Done. https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:205: * @return {!Object} On 2016/12/08 21:20:43, lushnikov wrote: > let's annotate this with details Done. https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:211: // dummy element for the collapsed size On 2016/12/08 21:20:43, lushnikov wrote: > style: "." in the end of the comment Done. https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:212: var collapsedElem = createElementWithClass('span', 'crumb collapsed'); On 2016/12/08 21:20:43, lushnikov wrote: > var collapsedElement = ... > > style: we try to avoid as much abbreviations as possible > Done. https://codereview.chromium.org/2551403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsBreadcrumbs.js:216: var collapsed = crumbs.firstChild.offsetWidth; On 2016/12/08 21:20:43, lushnikov wrote: > var collapsed = collapsedElem.offsetWidth; Done.
lgtm
lgtm the move to rAF helps a lot. nice work. :)
The CQ bit was checked by paulirish@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481332155227470,
"parent_rev": "98b805b4c569dd8c506e18af675bea29ad2ce43f", "commit_rev":
"5b52b93aa7f9f0cb59ff0ae06e24e2415ee8de04"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup Reduces the number of layouts performed. Moves update to be run in a requestAnimationFrame callback. Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. BUG=657141 R=lushnikov,paulirish ========== to ========== [DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup Reduces the number of layouts performed. Moves update to be run in a requestAnimationFrame callback. Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. BUG=657141 R=lushnikov,paulirish Review-Url: https://codereview.chromium.org/2551403002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup Reduces the number of layouts performed. Moves update to be run in a requestAnimationFrame callback. Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. BUG=657141 R=lushnikov,paulirish Review-Url: https://codereview.chromium.org/2551403002 ========== to ========== [DevTools] ElementsBreadcrumbs Perf Improvements & Cleanup Reduces the number of layouts performed. Moves update to be run in a requestAnimationFrame callback. Cleans up the code a bit to move pieces of the massive update functions into their own methods for a clearer breakdown of where time is spent. BUG=657141 R=lushnikov,paulirish Committed: https://crrev.com/bd86a87396a5ddb188b44744b5a46b111b7bf6aa Cr-Commit-Position: refs/heads/master@{#437742} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bd86a87396a5ddb188b44744b5a46b111b7bf6aa Cr-Commit-Position: refs/heads/master@{#437742} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
