Bug 222202 - REGRESSION (r266695): twitch.tv: when in fullscreen, WebKit continually does 350ms layouts. Firefox and Chrome do not
Summary: REGRESSION (r266695): twitch.tv: when in fullscreen, WebKit continually does ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-19 14:32 PST by Simon Fraser (smfr)
Modified: 2021-07-06 14:47 PDT (History)
17 users (show)

See Also:


Attachments
Some logging and render tree dump (4.84 MB, text/plain)
2021-02-19 22:31 PST, Simon Fraser (smfr)
no flags Details
Inspector timeline from before the regression (1.49 MB, image/png)
2021-02-20 11:16 PST, Simon Fraser (smfr)
no flags Details
Inspector timeline from after the regression (1.42 MB, image/png)
2021-02-20 11:17 PST, Simon Fraser (smfr)
no flags Details
Patch (9.91 KB, patch)
2021-02-22 04:43 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (10.35 KB, patch)
2021-02-22 08:11 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-02-19 14:32:58 PST
View a twitch livestream in fullscreen, and open web inspector on another screen. Take a timeline, and look at the layout time.

WebKit continually does very long layouts (longer than 350ms). Firefox and Chrome show no long layouts.
Comment 1 Radar WebKit Bug Importer 2021-02-19 14:33:12 PST
<rdar://problem/74537782>
Comment 2 Simon Fraser (smfr) 2021-02-19 22:30:46 PST
Looks like flexbox layout is O(n^2) or more on depth. One call to RenderFlexibleBox::layoutFlexItems() can layout a given child more than once, so as soon as you nest flexboxes, this multiplies down the tree.

On twitch.tv, a layout can takes > 500ms can call RenderFlexibleBox::layoutFlexItems() more than 32,000 times on a single RenderFlexibleBox.
Comment 3 Simon Fraser (smfr) 2021-02-19 22:31:50 PST
Created attachment 421081 [details]
Some logging and render tree dump
Comment 4 Simon Fraser (smfr) 2021-02-19 22:32:31 PST
I bet 99.9% of those layouts result in no size change. We need to find out how to optimize them away.
Comment 5 Simon Fraser (smfr) 2021-02-20 11:16:57 PST
Created attachment 421094 [details]
Inspector timeline from before the regression
Comment 6 Simon Fraser (smfr) 2021-02-20 11:17:13 PST
Created attachment 421095 [details]
Inspector timeline from after the regression
Comment 7 Sergio Villar Senin 2021-02-22 01:19:59 PST
I haven't inspected the page in detail but if it's using nested column flexboxes with percentages then it could indeed hit hard because in the calls to updateBlockChildDirtyBitsBeforeLayout() we have:

    if (relayoutChildren || (child.hasRelativeLogicalHeight() && !isRenderView()))

That very same method has the following comment:
    // FIXME: Technically percentage height objects only need a relayout if their percentage isn't going to be turned into
    // an auto value. Add a method to determine this, so that we can avoid the relayout.

However changing it seems risky as it might potentially affect everything not only flexbox.
Comment 8 Sergio Villar Senin 2021-02-22 04:43:52 PST
Created attachment 421181 [details]
Patch
Comment 9 Sergio Villar Senin 2021-02-22 04:44:23 PST
(In reply to Sergio Villar Senin from comment #7)
> I haven't inspected the page in detail but if it's using nested column
> flexboxes with percentages then it could indeed hit hard because in the
> calls to updateBlockChildDirtyBitsBeforeLayout() we have:
> 
>     if (relayoutChildren || (child.hasRelativeLogicalHeight() &&
> !isRenderView()))
> 
> That very same method has the following comment:
>     // FIXME: Technically percentage height objects only need a relayout if
> their percentage isn't going to be turned into
>     // an auto value. Add a method to determine this, so that we can avoid
> the relayout.
> 
> However changing it seems risky as it might potentially affect everything
> not only flexbox.

I managed to fix the issue without having to change that.
Comment 10 Sergio Villar Senin 2021-02-22 08:11:08 PST
Created attachment 421192 [details]
Patch

Fixing some tests flakiness
Comment 11 EWS 2021-02-22 11:12:10 PST
Committed r273264: <https://commits.webkit.org/r273264>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421192 [details].