Bug 222202

Summary: REGRESSION (r266695): twitch.tv: when in fullscreen, WebKit continually does 350ms layouts. Firefox and Chrome do not
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, jbedard, jfernandez, kondapallykalyan, pdr, rbuis, rego, rniwa, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=210089
https://bugs.webkit.org/show_bug.cgi?id=226764
Attachments:
Description Flags
Some logging and render tree dump
none
Inspector timeline from before the regression
none
Inspector timeline from after the regression
none
Patch
none
Patch none

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].