Bug 150993

Summary: Web Inspector: Batch all scheduled layouts and update the view tree in a single animation frame
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, simon.fraser, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 151058    
Attachments:
Description Flags
[Patch] WIP
none
[Patch] Proposed Fix
none
[Patch] Proposed Fix none

Description Matt Baker 2015-11-06 19:01:06 PST
* SUMMARY
Batch all scheduled updates and layout view tree in a single animation frame.
Comment 1 Matt Baker 2015-11-06 19:13:05 PST
Created attachment 264990 [details]
[Patch] WIP
Comment 2 Matt Baker 2015-11-06 19:32:23 PST
Noticed a bug in View._unscheduleLayoutForView. Will fix for the next WIP.
Comment 3 Matt Baker 2015-11-06 19:34:05 PST
While trying out this change I noticed that View._visitViewTreeForLayout is called *very* rarely. Almost every scheduled animation frame is canceled by a forced layout (a call to View.updateLayout). I suspect we're performing synchronous layouts when scheduled layouts would suffice.

AFAIK, there are only two cases where updateLayout must be used instead of needsLayout:

1) An element's position/size must be read after updating styles, etc, in order to lay out a related part of the UI. NavigationBar is an example of this.
2) Responsiveness of layouts in quick succession is needed, such as when resizing a sidebar or other "split" view.
Comment 4 Joseph Pecoraro 2015-11-06 20:17:10 PST
Comment on attachment 264990 [details]
[Patch] WIP

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

Nifty.

> Source/WebInspectorUI/UserInterface/Views/View.js:174
> +        this._dirty = false;

This should probably also set this._dirtyDescendants = 0, if you are keeping that value up to date.

> Source/WebInspectorUI/UserInterface/Views/View.js:220
> +        let viewStack = [[WebInspector.View._rootView]];
> +        while (viewStack.length) {

An array of arrays of views sounds unnecessarily complicated. How about just an array of views?:

    let views = [View._rootView];
    while (views.length) {
        let view = views.shift();
        if (view.dirty)
            view._layoutSubtree();
        else {
            views = views.concat(view.subviews);
            view._dirtyDescendants = 0;
        }
    }

Note:
  - this takes into account _layoutSubtree re-setting dirtyDescendants.
  - no need to create a copy of the subviews lists if you don't modify those lists directly, here we just add the values to the main list
  - JoePeck Fun Fact: "WebInspector.View" can just be "View" in this case, because we are inside "class View {...}" scope!

Or just use the recursive solution, since the depth of our view tree should not be large at all.
Comment 5 Timothy Hatcher 2015-11-07 13:38:02 PST
Comment on attachment 264990 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:212
> -        if (!this._endTimePinned && this.layoutPending)
> +        if (!this._endTimePinned && this.dirty)

I like the layoutPending name better, at least as a public API.
Comment 6 Radar WebKit Bug Importer 2015-11-07 13:38:23 PST
<rdar://problem/23454092>
Comment 7 Matt Baker 2015-11-08 17:20:10 PST
Comment on attachment 264990 [details]
[Patch] WIP

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:212
>> +        if (!this._endTimePinned && this.dirty)
> 
> I like the layoutPending name better, at least as a public API.

It does look better. We might want to keep the private variable named this._dirty, since it's consistent with this._dirtyDescendants.

>> Source/WebInspectorUI/UserInterface/Views/View.js:220
>> +        while (viewStack.length) {
> 
> An array of arrays of views sounds unnecessarily complicated. How about just an array of views?:
> 
>     let views = [View._rootView];
>     while (views.length) {
>         let view = views.shift();
>         if (view.dirty)
>             view._layoutSubtree();
>         else {
>             views = views.concat(view.subviews);
>             view._dirtyDescendants = 0;
>         }
>     }
> 
> Note:
>   - this takes into account _layoutSubtree re-setting dirtyDescendants.
>   - no need to create a copy of the subviews lists if you don't modify those lists directly, here we just add the values to the main list
>   - JoePeck Fun Fact: "WebInspector.View" can just be "View" in this case, because we are inside "class View {...}" scope!
> 
> Or just use the recursive solution, since the depth of our view tree should not be large at all.

I was on autopilot and wrote it as a depth-first traversal. Definitely not the behavior we want. Nice catch.
Comment 8 Matt Baker 2015-11-09 15:41:55 PST
Created attachment 265107 [details]
[Patch] Proposed Fix
Comment 9 BJ Burg 2015-11-10 12:45:03 PST
Comment on attachment 265107 [details]
[Patch] Proposed Fix

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

r=me, nice work

> Source/WebInspectorUI/UserInterface/Views/View.js:37
> +        this._dirtyDescendants = 0;

I would prefer `this._dirtyDescendantsCount` to emphasize it's a counter, not a list of descendants.

> Source/WebInspectorUI/UserInterface/Views/View.js:212
> +        let unscheduledLayouts = view._dirtyDescendants;

This variable name took me a while to grok. Maybe `forcedLayoutsCount` or something?

> Source/WebInspectorUI/UserInterface/Views/View.js:226
> +        cancelAnimationFrame(WebInspector.View._scheduledLayoutUpdateIdentifier);

Comment: 'No views need layout, so cancel the pending requestAnimationFrame.'
Comment 10 Matt Baker 2015-11-10 13:40:32 PST
Created attachment 265227 [details]
[Patch] Proposed Fix
Comment 11 WebKit Commit Bot 2015-11-10 14:31:21 PST
Comment on attachment 265227 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 265227

Committed r192272: <http://trac.webkit.org/changeset/192272>
Comment 12 WebKit Commit Bot 2015-11-10 14:31:25 PST
All reviewed patches have been landed.  Closing bug.