New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 766633 link

Starred by 145 users

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Compat

Blocking:
issue 752078



Sign in to add a comment

Flexbox column layout should be faster

Reported by al.govor...@gmail.com, Sep 19 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Example URL:

Steps to reproduce the problem:
Starting from Chrome 62 some DOM operations take too long. E.g. I checked performance during text typing on custom text editor. Take a look at attached images. Good case is for Chrome 60 and bad case for Chrome 62. The operation that cause long rendering is selection.setBaseAndExtent(), but it is only an example, other things are also affected.

What is the expected behavior?
No rendering issues

What went wrong?
Some rendering operations in v62 take 10-20 times longer than in v60.

Does it occur on multiple sites: N/A

Is it a problem with a plugin? No 

Did this work before? Yes 60

Does this work in other browsers? Yes

Chrome version: 62.0.3202.18  Channel: beta
OS Version: 10.0
Flash Version: 

Currently I can reproduce this issue only using one application. It has a big DOM tree and a lot of CSS styles. If I remove some elements and styles, the rendering issue is not reproduced. I'm investigating what cause this behavior, but I wasn't able to create an example. If it is necessary, I can provide access to the application with this issue.

Reproduced on both Windows and Mac. Chrome Canary 63 also has the bug.
 
rte_ok.PNG
46.5 KB View Download
rte_bad.PNG
46.1 KB View Download
UPDATE: I created an example, please check attached html file.
Steps to reproduce:
1. Open the file in Chrome >= 62
2. Click on the document

Actual result: rendering of the text takes 15-20s
Expected result: rendering takes ~2s (as in Chrome 60)

Selection is not the only one slow operation, it is just an example.
If you remove <section> element from the DOM, the performance is much better. 
test_performance.html
2.0 KB View Download
Labels: ReleaseBlock-Stable Needs-Triage-M62 Performance Needs-Bisect

Comment 3 by woxxom@gmail.com, Sep 19 2017

Per-snapshot bisect info: 494473 (good) - 494498 (bad)
https://chromium.googlesource.com/chromium/src/+log/2d336b8b..b4712b01?pretty=fuller
Suspecting r494488 "When main axis is logical y, lay out flex items before getting intrinsic size"
Landed in 62.0.3187.0
Cc: keerthan...@techmahindra.com
Components: Blink>DOM
Labels: -Needs-Bisect hasbisect-per-revision Triaged-ET OS-Linux OS-Mac
Owner: e...@chromium.org
Status: Assigned (was: Unconfirmed)
Tested the issue on 63.0.3112.90 and on latest canary 63.0.3219.0 using windows 10,Ubuntu 14.04 and Mac 10.12.6 and is reproducible with steps mentioned in comment#1.

Manual Bisect Info:
================
Good Build: 62.0.3186.0 
Bad Build:  62.0.3187.0

You are probably looking for a change made after 494487 (known good), but no later than 494488 (first known bad).

CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/ecd7c6f7c481b506401f889997c3e52a56fd2ac6..c1bb58b26ee6e0d792f6e8aff9a3126cc2fa29af

From the above change log suspecting https://chromium.googlesource.com/chromium/src/+/c1bb58b26ee6e0d792f6e8aff9a3126cc2fa29af

@eae: Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. 

Comment 5 by hayato@chromium.org, Sep 21 2017

Components: -Blink>DOM Blink>Layout

Comment 6 by hayato@chromium.org, Sep 21 2017

Cc: yosin@chromium.org
Components: Blink>Editing
cc-ing: yosin@, because setBaseAndExtent() might be suspicious.

Comment 7 by e...@chromium.org, Sep 21 2017

Owner: msten...@opera.com

Comment 8 by msten...@opera.com, Sep 21 2017

Yes, I did add more layout passes to get things correct, so that will cost more. Is there really any actual website that suffers from this, though?
Yes, our app is very slow in Chrome 62, 63, and it has a lot of users.

Comment 10 by woxxom@gmail.com, Sep 21 2017

Should setBaseAndExtent force layout as per spec? If it's not mandatory then the observed slowdown is a bug. Otherwise the demo script is doing DOM things the wrong way.
This issue is affecting our app which has over 1,000,000 users worldwide, most of them using Chrome since that is the recommended browser for our app. This is a big problem for us since the app became extremely slow with Chrome v62.

We urgently need a fix for this issue before Chrome v62 is moved to the stable channel.
Please note, that other operations are also much slower. E.g. element.setFocus(), retrieving some element properties (like clientHeight, scroll), setting attributes, and so on.
And if we change setBaseAndExtent() to document.createRange() and selection.setRange(), the bug is also reproduced.

Comment 13 by msten...@opera.com, Sep 21 2017

setBaseAndExtent changes the text selection, which probably means that you need to perform any pending layout. Normally we just do "document.body.offsetTop" or something to trigger that in tests. If it turns out that the test in comment #1 is a correct minimization of the original problem (do you really nest ~12 levels of flexboxes, though??), it does indeed seem unreasonable to re-lay out the deeply nested flexboxes every time, since the content that changed is in a sibling of that subtree. But I don't know yet. Attaching simplified test.
tc.html
1.4 KB View Download

Comment 14 by yosin@chromium.org, Sep 22 2017

Blocking: 752078
Components: -Blink>Layout -Blink>Editing Blink>Layout>Flexbox
Summary: Flexbox column layout should be faster (was: Slow rendering (beta v62))
Remove Blink>Editing, because of the reported slowness is caused by http://crrev.com/c/612179, which makes LayoutFlexibleBox::ConstructFlexItem() to
forces child layout if flex direction is column.
Project Member

Comment 15 by sheriffbot@chromium.org, Sep 22 2017

Cc: ligim...@chromium.org
This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 16 by sheriffbot@chromium.org, Sep 25 2017

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Any news on this issue?
Project Member

Comment 19 by bugdroid1@chromium.org, Sep 26 2017

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

commit 8046a39e7913f848703a69b96586fc9a3fa3937c
Author: Morten Stenshorne <mstensho@opera.com>
Date: Tue Sep 26 19:35:55 2017

Performance test with deeply nested column flex containers.

In this teste there's a (rather) deeply nested subtree of column-flow flex
containers. The root of this subtree is itself a flex item in a row-flow
container. It has a sibling item whose child changes width, without affecting
the flexed size of the deeply nested subtree. This shouldn't require the
subtree to be re-laid out, but it currently does get re-laid out, thanks to the
fix for bug 752078.

Bug:  766633 
Change-Id: I3d76736730f18082a299907f7b2a543d2b087ba8
Reviewed-on: https://chromium-review.googlesource.com/680216
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/heads/master@{#504458}
[add] https://crrev.com/8046a39e7913f848703a69b96586fc9a3fa3937c/third_party/WebKit/PerformanceTests/Layout/flexbox-deeply-nested-column-flow.html

Project Member

Comment 20 by bugdroid1@chromium.org, Sep 28 2017

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

commit daf862b83d13e488a2c69a11d85cad2ee8904708
Author: Morten Stenshorne <mstensho@opera.com>
Date: Thu Sep 28 06:19:52 2017

Less forcing of column flow flex children layout.

If the logical height of a child is intrinsic, we only need to lay it out if
there's a chance that the logical width of the flex container changed.
Otherwise nothing's going to change in there.

We do this by checking for kForceLayout, so that we don't risk descending into
deep subtrees just because some flex item sibling changed, for instance.

TEST=PerformanceTests/Layout/flexbox-deeply-nested-column-flow.html

Bug:  766633 
Change-Id: I29337a176592779f07a37570fef8c85b1b4f6d99
Reviewed-on: https://chromium-review.googlesource.com/680214
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/heads/master@{#504924}
[modify] https://crrev.com/daf862b83d13e488a2c69a11d85cad2ee8904708/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

Comment 21 by msten...@opera.com, Sep 28 2017

Status: Fixed (was: Assigned)
Labels: TE-Verified-M63 TE-Verified-63.0.3227.0
Verified the fix on Mac 10.12.6, Win-10 and Ubuntu 14.04 using Chrome version #63.0.3227.0 as per the comment #1.
Attaching screen cast for reference.
Observed that rendering of the content took ~2s as expected.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
766633.mp4
1015 KB View Download
Thank you very much for the quick solution. Can you please confirm that this will also be solved in Chrome v62 before it moves to the stable channel?
What about v62? Are you planning to cherry pick this fix to v62, too?

Comment 25 by msten...@opera.com, Sep 29 2017

Labels: Merge-Request-62
Project Member

Comment 26 by sheriffbot@chromium.org, Sep 29 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-62 Merge-Approved-62
Approving this for M62. Branch:3202
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 2 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c652757d62ffb43c0453c28393438e6b2b05db20

commit c652757d62ffb43c0453c28393438e6b2b05db20
Author: Morten Stenshorne <mstensho@opera.com>
Date: Mon Oct 02 19:21:57 2017

Less forcing of column flow flex children layout.

If the logical height of a child is intrinsic, we only need to lay it out if
there's a chance that the logical width of the flex container changed.
Otherwise nothing's going to change in there.

We do this by checking for kForceLayout, so that we don't risk descending into
deep subtrees just because some flex item sibling changed, for instance.

TEST=PerformanceTests/Layout/flexbox-deeply-nested-column-flow.html
TBR=mstensho@opera.com

(cherry picked from commit daf862b83d13e488a2c69a11d85cad2ee8904708)

Bug:  766633 
Change-Id: I29337a176592779f07a37570fef8c85b1b4f6d99
Reviewed-on: https://chromium-review.googlesource.com/680214
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@opera.com>
Cr-Original-Commit-Position: refs/heads/master@{#504924}
Reviewed-on: https://chromium-review.googlesource.com/696081
Reviewed-by: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/branch-heads/3202@{#545}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/c652757d62ffb43c0453c28393438e6b2b05db20/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

Sign in to add a comment