201735 – Web Inspector: Improve auto completion typing performance by avoiding global forced layouts

RESOLVED FIXED 201735

Web Inspector: Improve auto completion typing performance by avoiding global forced layouts

https://bugs.webkit.org/show_bug.cgi?id=201735

Summary Web Inspector: Improve auto completion typing performance by avoiding global ...

Joseph Pecoraro

Reported 2019-09-12 13:07:07 PDT

Improve auto completion typing performance by avoiding global forced layouts Steps to Reproduce: 1. Inspect <http://bogojoker.com/shell/> 2. Open inspector² 3. Set a breakpoint in closeAllContentViews loop 4. Reload inspector¹ => Should pause in inspector² 5. In console drawer quickly type "entry.contentView" => Typing is very slowly, with a forced layout each keypress Notes: • Content jumps between the body and back and has multiple layouts. • Main element keeps getting re-attached to the document unnecessarily causing another layout

Attachments
[PATCH] Proposed Fix (5.80 KB, patch) 2019-09-12 13:37 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
[PATCH] Proposed Fix (4.99 KB, patch) 2019-09-13 12:42 PDT, Joseph Pecoraro	hi: review+ hi: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2019-09-12 13:37:52 PDT

Created attachment 378675 [details] [PATCH] Proposed Fix This was a dramatic improvement for me.

Devin Rousso

Comment 2 2019-09-12 15:16:04 PDT

Comment on attachment 378675 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378675&action=review > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:44 > + position: absolute; Should we also move the container offscreen, or would that break the `offsetWidth`/`offsetHeight`? > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:51 > + this._completionLayoutElement = document.createElement("div"); You could just do a deep clone of the container element. ``` this._completionLayoutElement = document.body.appendChild(this._containerElement.cloneNode(true)); this._completionLayoutElement.classList.add("completion-layout-container"); ``` We may also want some sort of safeguard for possible future class extension (which could modify `this._containerElement` which wouldn't be applied to `this._completionLayoutElement`), so maybe we should add an assert. ``` console.assert(this.constructor === WI.CompletionSuggestionsView && this instanceof WI.CompletionSuggestionsView, "This class should not be subclassed, as that may interfere with the `this._completionLayoutElement` performance optimization."); ``` > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:124 > + var containerWidth = this._completionLayoutElement.offsetWidth; > + var containerHeight = this._completionLayoutElement.offsetHeight; `let` > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:163 > hide() We likely also need to remove `this._completionLayoutElement` from the <body> when we hide (and should therefore re-add it when we `show`). > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:201 > + this._completionLayoutElement.appendChild(itemElementMirror); Ditto (51) for using `cloneNode`. ``` this._completionLayoutElement.appendChild(itemElement.cloneNode(true)); ```

Nikita Vasilyev

Comment 3 2019-09-12 15:26:08 PDT

Comment on attachment 378675 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378675&action=review > Source/WebInspectorUI/ChangeLog:14 > + (WI.CompletionSuggestionsView.prototype.show): > + Scroll position doesn't seem to ever need to be maintained. I just tried to find the case when it would be needed and I couldn't. It should be safe to remove.

Joseph Pecoraro

Comment 4 2019-09-12 15:35:19 PDT

Comment on attachment 378675 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378675&action=review >> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:44 >> + position: absolute; > > Should we also move the container offscreen, or would that break the `offsetWidth`/`offsetHeight`? It is hidden. Is there an advantage to moving it offscreen? >> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:51 >> + this._completionLayoutElement = document.createElement("div"); > > You could just do a deep clone of the container element. > ``` > this._completionLayoutElement = document.body.appendChild(this._containerElement.cloneNode(true)); > this._completionLayoutElement.classList.add("completion-layout-container"); > ``` > > We may also want some sort of safeguard for possible future class extension (which could modify `this._containerElement` which wouldn't be applied to `this._completionLayoutElement`), so maybe we should add an assert. > ``` > console.assert(this.constructor === WI.CompletionSuggestionsView && this instanceof WI.CompletionSuggestionsView, "This class should not be subclassed, as that may interfere with the `this._completionLayoutElement` performance optimization."); > ``` Hmm, that seems unlikely. >> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:163 >> hide() > > We likely also need to remove `this._completionLayoutElement` from the <body> when we hide (and should therefore re-add it when we `show`). I'm avoiding the add/remove to body specifically to avoid layout performance issues. That would likely bring back a forced layout we don't want.

Devin Rousso

Comment 5 2019-09-12 15:52:38 PDT

Comment on attachment 378675 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378675&action=review >>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:44 >>> + position: absolute; >> >> Should we also move the container offscreen, or would that break the `offsetWidth`/`offsetHeight`? > > It is hidden. Is there an advantage to moving it offscreen? It's _extra_ hidden :P In all seriousness though, having it be offscreen completely removes any chance of it ever being visible (or having to be rendered). I don't think that'll affect any sizing calculations, but that may be worth checking with one of the layout folks. >>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:163 >>> hide() >> >> We likely also need to remove `this._completionLayoutElement` from the <body> when we hide (and should therefore re-add it when we `show`). > > I'm avoiding the add/remove to body specifically to avoid layout performance issues. That would likely bring back a forced layout we don't want. How about we make the layout container a static instance that's shared among all instances, and is initialized when the first one is created? We already `removeChildren` each time we update (and all instances of `WI.CompletionSuggestionsView` share the same container structure) so we could just have one.

Joseph Pecoraro

Comment 6 2019-09-13 12:42:37 PDT

Created attachment 378746 [details] [PATCH] Proposed Fix Much better.

Joseph Pecoraro

Comment 7 2019-09-13 12:43:52 PDT

Before: ~100ms per update (just the measure portion) > [101.762, 98.976, 102.695, 101.644, 100.673, 101.136, 103.207, 100.807, 103.894, 103.913, 104.202, 100.951, 100.806, 103.633, 104.596, 95.166, 95.29] (17) > MIN: 95.166 > MAX: 104.596 > SUM: 1723.351 > AVG: 101.3736 > SD: 2.7871 After: ~2.1ms per update (just the measure portion) > [2.229, 2.197, 1.744, 2.115, 1.963, 1.986, 2.147, 1.963, 2.32, 1.976, 1.953, 1.851, 2.202, 2.066, 1.977, 2.809, 1.999, 1.817, 2.094, 1.883, 1.827, 2.179, 1.958, 2.11, 1.91, 2.05, 1.946, 1.88, 2.184, 1.876, 2.533, 2.068, 1.97] (33) > MIN: 1.744 > MAX: 2.809 > SUM: 67.782 > AVG: 2.054 > SD: 0.211

Devin Rousso

Comment 8 2019-09-13 17:43:24 PDT

Comment on attachment 378746 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378746&action=review r=me, with some quick changes > Source/WebInspectorUI/ChangeLog:12 > + Provide a helper for measuring an element in a hidden containment You could just say “container” instead of “containment div”. > Source/WebInspectorUI/UserInterface/Base/Main.js:300 > + WI.layoutMeasurementContainer = document.body.appendChild(document.createElement("div")); Can we lazily create this element, or at the very least wait to add it to the DOM till the first invocation? Or is the reason you’re doing this to also avoid an “expensive” layout on the first call too? > Source/WebInspectorUI/UserInterface/Base/Main.js:2789 > +WI.measureElement = function(element) This is awesome!!! > Source/WebInspectorUI/UserInterface/Base/Main.js:2791 > + WI.layoutMeasurementContainer.className = element.className; I don’t think we want to do this, especially since `cloneNode` will copy any CSS classes on the child for us, so we’d potentially have a duplicate selector on BOTH the parent and child. That could also cause issues if any class selectors have any `!important` and overrode our hiding styles. > Source/WebInspectorUI/UserInterface/Base/Main.js:2797 > + WI.layoutMeasurementContainer.className = ""; Ditto (2791)

Joseph Pecoraro

Comment 9 2019-09-13 17:48:54 PDT

> Can we lazily create this element, or at the very least wait to add it to > the DOM till the first invocation? Or is the reason you’re doing this to > also avoid an “expensive” layout on the first call too? Yes, I've seen large layouts when appending an element to the body for the first time and I want to avoid that. > > Source/WebInspectorUI/UserInterface/Base/Main.js:2791 > > + WI.layoutMeasurementContainer.className = element.className; > > I don’t think we want to do this, especially since `cloneNode` will copy any > CSS classes on the child for us, so we’d potentially have a duplicate > selector on BOTH the parent and child. That could also cause issues if any > class selectors have any `!important` and overrode our hiding styles. Good point, I don't know why I did this. I was thinking the client might want to add class names to the layout container element, but then I went and added the child element's class names. That was dumb!

Joseph Pecoraro

Comment 10 2019-09-13 17:50:58 PDT

https://trac.webkit.org/changeset/249863/webkit

Radar WebKit Bug Importer

Comment 11 2019-09-13 17:51:16 PDT

<rdar://problem/55359073>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Joseph Pecoraro

Reported

2019-09-12 13:07 PDT

Modified

2019-09-13 17:51 PDT History

CC List

8 users Show

URL

Keywords InRadar

Depends on

Blocks