Bug 192648

Summary: Web Inspector: Debugger: virtualize the list of variables in the Scope sidebar
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews201 for win-future
none
Patch none

Description Devin Rousso 2018-12-12 17:19:46 PST
On pages with lots of variables per-scope (e.g. most minified scripts), WebInspector can slow down significantly as we try to create/render a `WI.ObjectTreeView` per-variable.
Comment 1 Radar WebKit Bug Importer 2018-12-17 21:33:04 PST
<rdar://problem/46800949>
Comment 2 Devin Rousso 2019-01-01 21:20:07 PST
Created attachment 358179 [details]
Patch
Comment 3 EWS Watchlist 2019-01-02 01:49:47 PST Comment hidden (obsolete)
Comment 4 Joseph Pecoraro 2019-01-08 11:09:36 PST
Comment on attachment 358179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358179&action=review

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:713
> +        const observerHandler = (entries) => {
> +            console.assert(entries.length === 1);
> +            if (entries[0].isIntersecting)
> +                this.updateVirtualizedElements({shouldRefresh: true});
> +        };
> +        const observerOptions = {
> +            root: this._virtualizedScrollContainer,
> +            threshold: [0],
> +        };
> +        let observer = new IntersectionObserver(observerHandler, observerOptions);
> +        observer.observe(this.element);

What is this doing that the scroll handler is not doing? Is it supposed to be handling the first display of the element?

I did notice this assert firing quite a bit in local testing.

            console.assert(entries.length === 1);

I also noticed this handler firing a lot during scrolling, which seems like it would maybe eliminate the need for the throttler?

I also noticed an IntersectionObserver crash, which I've filed appropriately.
Comment 5 Devin Rousso 2019-01-10 21:17:06 PST
Comment on attachment 358179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358179&action=review

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:713
>> +        observer.observe(this.element);
> 
> What is this doing that the scroll handler is not doing? Is it supposed to be handling the first display of the element?
> 
> I did notice this assert firing quite a bit in local testing.
> 
>             console.assert(entries.length === 1);
> 
> I also noticed this handler firing a lot during scrolling, which seems like it would maybe eliminate the need for the throttler?
> 
> I also noticed an IntersectionObserver crash, which I've filed appropriately.

Yeah, this is supposed to handle the case where you start out with it being visible, but I think the `updateVirtualizedElements` call below handles that just as well.  I'll remove this unless there is some other case I'm forgetting (or didn't know about) that needs this.

As far as the assertions, my guess is that in the process of adding/resizing the top/bottom spacers, we cause the element to shift suddenly.  I'll investigate a bit more.
Comment 6 Joseph Pecoraro 2019-01-22 11:22:22 PST
Comment on attachment 358179 [details]
Patch

r- for now given the assertions I was seeing imply something amiss. If you can't reproduce let me know.
Comment 7 Devin Rousso 2019-03-12 13:36:08 PDT
Created attachment 364437 [details]
Patch
Comment 8 EWS Watchlist 2019-03-12 17:46:08 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-03-12 17:46:20 PDT Comment hidden (obsolete)
Comment 10 Joseph Pecoraro 2019-03-20 12:07:53 PDT
Comment on attachment 364437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364437&action=review

r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:588
> +                return true;

Nice. Should update the existing Array.prototype.remove tests in:
LayoutTests/inspector/unit-tests/array-utilities.html

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:452
> +        if (this.virtualized)
> +            this._virtualizedDebouncer.delayForFrame();

Previously we would only delayForFrame if the element was not known. Do we want to do this always now (probably fine).
Comment 11 Devin Rousso 2019-03-20 13:55:10 PDT
Comment on attachment 364437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364437&action=review

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:452
>> +            this._virtualizedDebouncer.delayForFrame();
> 
> Previously we would only delayForFrame if the element was not known. Do we want to do this always now (probably fine).

I don't think there's a difference.  It should be an error for us to re-add the same element twice without removing it in between.  Always doing it also gives us an "assurance" that each time we try to add an element, we should try to show it.
Comment 12 Devin Rousso 2019-03-20 13:57:56 PDT
Created attachment 365395 [details]
Patch
Comment 13 WebKit Commit Bot 2019-03-20 14:49:21 PDT
Comment on attachment 365395 [details]
Patch

Clearing flags on attachment: 365395

Committed r243242: <https://trac.webkit.org/changeset/243242>
Comment 14 WebKit Commit Bot 2019-03-20 14:49:24 PDT
All reviewed patches have been landed.  Closing bug.