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
Created attachment 378675 [details] [PATCH] Proposed Fix This was a dramatic improvement for me.
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)); ```
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.
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.
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.
Created attachment 378746 [details] [PATCH] Proposed Fix Much better.
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
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)
> 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!
https://trac.webkit.org/changeset/249863/webkit
<rdar://problem/55359073>