Bug 201735

Summary: Web Inspector: Improve auto completion typing performance by avoiding global forced layouts
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: hi, inspector-bugzilla-changes, joepeck, nvasilyev, simon.fraser, thorton, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix hi: review+, hi: commit-queue-

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
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (4.99 KB, patch)
2019-09-13 12:42 PDT, Joseph Pecoraro 		hi: review+
hi: commit-queue-
DetailsFormatted DiffDiff
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.