New issue
Advanced search Search tips

Issue 703512 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Weird flexbox performance cliff: extremely long layouts

Project Member Reported by vdje...@fb.com, Mar 21 2017

Issue description

Chrome Version: Canary 59.0.3046.0
OS: Windows 10, OS X 10.12.3

What steps will reproduce the problem?
(1) Load the attached test case
(2) Type quickly into the textbox. Note there is no JavaScript on the page

What is the expected result? 

Text should appear smoothly


What happens instead?

Typing causes massive layout operations with layouts starting at the document element. However if the simple div with id="magic" is removed, the hangs completely disappear. I believe this is some kind of weird performance cliff for Chrome's implementation of flexbox. This does not reproduce in Firefox nor Safari. It reproduce in Chrome Release + Canary on both Windows & OS X.

Please use labels and text to provide additional information.
 
flexbox_layout_hang.htm
1.4 MB View Download

Comment 1 by vdje...@fb.com, Mar 21 2017

Components: Blink>Layout>Flexbox

Comment 2 by vdje...@fb.com, Mar 21 2017

Labels: DevRel-Facebook

Comment 3 by e...@chromium.org, Mar 21 2017

Owner: cbiesin...@chromium.org
Would you mind triaging this cbiesinger?

Notes when reproducing this hang:

- The hang reproduces on my Win10 ThinkPad, with Chrome window maximized. window.devicePixelRatio=1.5
- It reproduces on my MacBook when connected to external monitor, but only when the window is *not* maximized. I think otherwise the "magic" element doesn't wrap around. window.devicePixelRatio=1
- It does *not* reproduce on the same MacBook when using only the laptop screen (no external display). window.devicePixelRatio=2

So the criteria are:
1) window.devicePixelRation < 2
2) window narrow enough to cause wrapping of the "magic" element

Comment 5 by e...@chromium.org, Mar 27 2017

Status: Assigned (was: Untriaged)

Comment 6 by owe...@chromium.org, Apr 25 2017

Just wondering if this issue got wrapped into any Q2 OKR?
This testcase no longer reproduces with the fix from  issue 596743 - the layout time spent (reported by devtools) from inserting one character goes from 70ms to 0.6ms.

The issue persists with `min-height: 0` applied to flex elements in the test case, as one might expect. I'll look into this some more.
Wait, are you saying that the fix for that bug made things *faster*?
That appears to be the case. Having `min-height: auto` set drastically improves performance over `min-height: 0` on all elements in the testcase.
Cc: acomminos@fb.com
Owner: acomminos@fb.com
Status: Started (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9c25cbbe338e5a2232d3e47d440808a938c40a9

commit d9c25cbbe338e5a2232d3e47d440808a938c40a9
Author: Andrew Comminos <acomminos@fb.com>
Date: Sat Nov 10 01:13:00 2018

Add perf test for CSS flexbox optimization 9.8.1

Test that propagated definite heights of flexboxes avoid a reflow of wrapping
text. With the definite height optimization, the benchmark runs twice as fast
in my local testing.

Bug:  703512 
Change-Id: Ib5fbf14c90735504007883c874e71f7461fe1d11
Reviewed-on: https://chromium-review.googlesource.com/c/1330289
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#607069}
[add] https://crrev.com/d9c25cbbe338e5a2232d3e47d440808a938c40a9/third_party/blink/perf_tests/layout/flexbox-row-stretch-height-definite.html

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e933299c93a4d64e672e26c7ecf53c16df605c5

commit 1e933299c93a4d64e672e26c7ecf53c16df605c5
Author: Andrew Comminos <acomminos@fb.com>
Date: Sat Nov 10 02:56:06 2018

Propagate definite height of single-line row flexboxes to child cross size

When we have a single-line definite height row flexbox with a column
flexbox as its child, we always relayout all children of the column
flexbox during LayoutLineItems with a zero-sized height being set.
This patch leverages the definite height of the row flexbox in order to
avoid a relayout of the column flexbox children.

Bug:  703512 
Change-Id: I1f67efff3ebc67cdcce57dce2d2567e2abe13625
Reviewed-on: https://chromium-review.googlesource.com/c/1306404
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Andrew Comminos <acomminos@fb.com>
Cr-Commit-Position: refs/heads/master@{#607090}
[modify] https://crrev.com/1e933299c93a4d64e672e26c7ecf53c16df605c5/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/1e933299c93a4d64e672e26c7ecf53c16df605c5/third_party/blink/renderer/core/layout/layout_flexible_box.h

Status: Fixed (was: Started)

Sign in to add a comment