Flexbox column layout should be faster
Reported by
al.govor...@gmail.com,
Sep 19 2017
|
||||||||||||||
Issue descriptionUserAgent: 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.
,
Sep 19 2017
,
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
,
Sep 20 2017
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.
,
Sep 21 2017
,
Sep 21 2017
cc-ing: yosin@, because setBaseAndExtent() might be suspicious.
,
Sep 21 2017
,
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?
,
Sep 21 2017
Yes, our app is very slow in Chrome 62, 63, and it has a lot of users.
,
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.
,
Sep 21 2017
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.
,
Sep 21 2017
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.
,
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.
,
Sep 22 2017
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.
,
Sep 22 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
,
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
,
Sep 26 2017
Any news on this issue?
,
Sep 26 2017
Fix soon in review: https://chromium-review.googlesource.com/c/chromium/src/+/680214
,
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
,
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
,
Sep 28 2017
,
Sep 29 2017
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...!!
,
Sep 29 2017
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?
,
Sep 29 2017
What about v62? Are you planning to cherry pick this fix to v62, too?
,
Sep 29 2017
,
Sep 29 2017
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
,
Oct 2 2017
Approving this for M62. Branch:3202
,
Oct 2 2017
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 |
||||||||||||||
Comment 1 by al.govor...@gmail.com
, Sep 19 20172.0 KB
2.0 KB View Download