Confirmed that there's an issue with M56 and the CSS file in the attachment.

Adding CSS component.

Bumping to P1 while we investigate

Hi Elliott,

Can you take a look at this today?

Sounds like an n^2, someone from the style team should profile this with perf or instruments. It'll probably tell you where to look.

Added RBS label for the next few hours while it is investigated.

+meade@

Bisect results from Prudhvi:

You are probably looking for a change made after 429108 (known good), but no lat
er than 429109 (first known bad).

CHANGELOG URL:
 https://chromium.googlesource.com/chromium/src/+log/bb98274e9983636f2b903b4877
9b75e54c24429a..b5243ab8c1a95305aff233a6175aa3a1f94d092f

This is a regression in M56, was broken before M56 branching.

Good Build: 56.0.2906.0
Bad Build : 56.0.2907.0 

CL from the above range: https://chromium.googlesource.com/chromium/src/+log/56.0.2906.0..56.0.2907.0?pretty=fuller&n=10000

Per revision bisect result:

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

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/bb98274e9983636f2b903b48779b75e54c24429a..b5243ab8c1a95305aff233a6175aa3a1f94d092f

Assigning to the owner & also looping the reviewer.

FYI: We are planning a stable RC cut today,please get a fix/ revert ASAP.

Thanks to Prudhvi for the bisect.

Will update the impact on other OS soon.

The change in question is a correctness issue that was addressed in part to solve another SAP reported issue ( issue 637811 ) and it looks like you have code that depends on the old, incorrect, fast-path. Without a reduction this is hard to verify.

It is too late in the release process to do anything about this for 56, reverting the change would require further testing and would break content, including SAP.

Please also note that another CL by the same author landed above this code. 

The one referred to in the regression here is: https://codereview.chromium.org/2441373002

They then landed another CL above it here: 
https://codereview.chromium.org/2535173006

So I don't think the bisect is correct here. Emil, does that seem correct?

Reproducible in Mac and Win but not on Linux and CrOS.

I did bisect again and got the same result which eliminated the first part i.e., "Needs-Bisect", We need to reach out to SAP to see if someone can get address the "Needs-Reduction" part. 

Please correct me if the interpretation was wrong and misleading.


Note :I see similar behavior on Dev-M57(57.0.2987.19) and Canary-M58(58.0.2998.0).

Tried the bisect again on windows.This is regression in M56. Please find the bisect information as below

Narrow bisect::
==================
Good Build: 56.0.2906.0  (build revision 428890)
Bad Build : 56.0.2907.0  (build revision 429169)

per revision bisect info::
==========================
You are probably looking for a change made after 429108 (known good), but no lat
er than 429109 (first known bad).

CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/bb98274e9983636f2b903b48779b75e54c24429a..b5243ab8c1a95305aff233a6175aa3a1f94d092f

Possible suspect::
https://chromium.googlesource.com/chromium/src/+/b5243ab8c1a95305aff233a6175aa3a1f94d092f

robhogan @ Could you please look into this issue.

Thanks,

Someone on the layout or style teams should make a reduction. That shouldn't be too hard given the self contained test case attached here. I don't think we need anything from SAP.

Someone also needs to look at this in pprof. For instructions see:
http://x20web/~esprehn/chrome_profiling.md.html

That'll tell you where the time is being spent.

Our team tried multiple bisects and got the same results.Julian tried applying the revert both to master and the 56 branch in both cases it  took seconds to render the page as it was not a clean revert. 

Looping to few folks in who made recent fixes in Blink-> Layout.

FYI: We are blocked with M56 Stable Release which was scheduled to be deployed tomorrow, 02/01. 

rob to cc.

Naina kindly offered to take a crack at profiling this.

FWIW, making a reduction could be difficult since the test case is basically the whole page, and the CSS is 440kB and obfuscated.

I'd start by pretty printing the CSS (ex. http://www.cleancss.com/css-beautify/), then try binary searching it by deleting large chunks of it until you find the minimal set of CSS needed to make things slow. Then similarly binary search the markup, deleting chunks and removing attributes (regexes work great) until you have the minimal set.

Fwiw the test case also only has 956 elements on it, and 298 table cells. There's no reason it should take 27 seconds.

Lowering priority to P1 and removing RB status. It is much too late in the process for us to even consider a functional change to a release branch as detailed in comment 11.

I had a go at minimising, and I got this, which is taking about 2s for me to load (the original was about 8s on my macbook pro).

index.html
- If you start removing the remaining inner table elements from the very nested table, the time taken reduces quickly.
- If you remove the visibility:hidden form at the bottom, the time taken is very small

is_standards.css
- Removing either rule makes it fast.

trying to post at the same time as eae ate my attachments.

Deleted: index.html 34.5 KB

index.html 34.5 KB View Download

Deleted: ls_standards.css 318 bytes

ls_standards.css 318 bytes View Download

Thanks meade!

Hi everyone, 

Using the instructions mentioned by esprehn@ in #17 and the test case provided by meade@ in #24/25, I get the attached profile. (Also attaching the profile data in case someone wants to take a stab at looking at this some more.)

From looking at the CL and the output seems like LayoutTableSection::relayoutCellIfFlexed() is relayouting more often post change. I also synced back to M54 to compare this result against old performance and verified this. 

Hope this is helpful! Let me know if you have any questions

Deleted: symbolized-profile_canary.svg 232 KB

symbolized-profile_canary.svg 232 KB Download

Deleted: profile_canary.data 4.6 MB

profile_canary.data 4.6 MB Download

The page in question has seventeen nested tables and percentage heights. Before we could incorrectly handle the percentage height value which was a very fast operation. Since r 429109 percentage heights are handled correctly which depends on the containing  cell. In a deeply nested table structure like this that is much more expensive.

The easiest way to fix this would be to remove the height: 100% rule as detailed in comment 24.

nainar@ - this is with me, unless you're already progressing a fix. If you have a reduced test case please add here. I'm reducing it myself atm.

It sounds like everything is understood now, which is great. I did some ETW trace analysis of the slow case yesterday and I thought I'd paste it in, even though it sounds like it is too late now.


Essentially all of the CPU time is in chrome.exe (20368), one of the renderer processes. Within that virtually all of the CPU time is consumed by thread 17,928. Drilling down I found through the CPU Usaged (Sampled) data on that thread I found two different call stacks that led through chrome_child.dll!blink::FrameView::layout so I did a 'butterfly view' to show all stacks that go through that function - basically all samples go through there.

To do the butterfly view on FrameView::layout you need to find it (drill down or use Ctrl+F in the stack column) and then right-click and select "View Callees", "By Function".

Fully 80% of the samples go through this call stack - is it something wrong with tables?

chrome_child.dll!blink::FrameView::layout
 chrome_child.dll!blink::FrameView::performLayout
 |- chrome_child.dll!blink::layoutFromRootObject
 |    chrome_child.dll!blink::LayoutView::layout
 |    chrome_child.dll!blink::LayoutBlock::layout
 |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlock
 |    |- chrome_child.dll!blink::LayoutBlockFlow::layoutBlockFlow
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockChildren
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockChild
 |    |    chrome_child.dll!blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded
 |    |    chrome_child.dll!blink::LayoutBlock::layout
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlock
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockFlow
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockChildren
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockChild
 |    |    chrome_child.dll!blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded
 |    |    chrome_child.dll!blink::LayoutBlock::layout
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlock
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockFlow
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockChildren
 |    |    chrome_child.dll!blink::LayoutBlockFlow::layoutBlockChild
 |    |    chrome_child.dll!blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded
 |    |    |- chrome_child.dll!blink::LayoutTable::layout

I noticed a lot of samples going through chrome_child.dll!blink::LayoutBlockFlow::layoutBlockFlow so I did a butterfly view on that - that shows that basically all samples go through that function.

Does it mean anything that almost half of the samples are going through chrome_child.dll!blink::FrameView::scrollbarExistenceDidChange? My guess is probably not - it's probably a normal scrollbar existence change that has been made far more expensive by something else.

brucedawson@ scrollbarExistenceDidChange just does a layout:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/FrameView.cpp?q=scrollbarExistenceDidChange+package:%5Echromium$&l=2082

that's all you're seeing. You're seeing the layout, which is that bested callstack, since we call layoutBlock for each block recursively. I think you'd want to look at it bottom up inside layout(). 

Given how small the page is this is likely an n^2 inside some layout method that's walking the table repeatedly computing the same thing over and over again.

Still reducing it, but down to 200 lines.

Updated reduction.

Hi robhogan, 

Sorry for the late response, I just walked into work. I used the minimized testcase provided by meade@ in comment # 25. It did the job as she describes in comment #24. Hope it helps. Though it looks like you have a reduced test case too. Sorry for the delay on this.  

Hi Naina, can you share any updates on where things stand?

blumberg: We're trying to track down the exact root cause to see if there is anything we can do about it. That will be for chrome 57 at the earliest though.

Comment 24 and 28 have explanations and suggestions for ways to fix this by modifying the content.

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

commit d3ceb0147ad9f4d4a9b41cea0362d6221cb72302
Author: robhogan <robhogan@gmail.com>
Date: Wed Feb 08 22:07:08 2017

Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
during the layout of the section - and this cascades down the tree with the lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.

BUG= 687061 

Review-Url: https://codereview.chromium.org/2670603002
Cr-Commit-Position: refs/heads/master@{#449101}

[modify] https://crrev.com/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[modify] https://crrev.com/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[add] https://crrev.com/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302/third_party/WebKit/Source/core/layout/LayoutTableSection.h

Amit/Mano, can anyone of you please verify this on Canary?

Verified the fix on the latest canary(58.0.3010.0) of Windows-10 and Mac OS 10.12.2 and this is working as intended.

index.html from attached file in C#0 loads in few ms.

Deleted: index.html 122 KB

index.html 122 KB View Download

Can we merge back to 56 & 57?

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

If possible, please merge your change to M57 branch 2987 today (Monday) by 5:00 PM PT so we can pick it up for this week beta release. Thank you.

+ bustamante@ for M56 merge review.

LGTM, approved for merge into M56.

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

commit 6956d4ec3f63e0fec4100bcbe6f502d111b189e7
Author: Robert Hogan <robhogan@gmail.com>
Date: Mon Feb 13 20:25:58 2017

Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
during the layout of the section - and this cascades down the tree with the lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.

BUG= 687061 

Review-Url: https://codereview.chromium.org/2670603002
Cr-Commit-Position: refs/heads/master@{#449101}
(cherry picked from commit d3ceb0147ad9f4d4a9b41cea0362d6221cb72302)

Review-Url: https://codereview.chromium.org/2692083002 .
Cr-Commit-Position: refs/branch-heads/2987@{#485}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/6956d4ec3f63e0fec4100bcbe6f502d111b189e7/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[modify] https://crrev.com/6956d4ec3f63e0fec4100bcbe6f502d111b189e7/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/6956d4ec3f63e0fec4100bcbe6f502d111b189e7/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[add] https://crrev.com/6956d4ec3f63e0fec4100bcbe6f502d111b189e7/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/6956d4ec3f63e0fec4100bcbe6f502d111b189e7/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/6956d4ec3f63e0fec4100bcbe6f502d111b189e7/third_party/WebKit/Source/core/layout/LayoutTableSection.h

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

commit f799408c02bfd85d001b2318cf0af0f98d2146ba
Author: Robert Hogan <robhogan@gmail.com>
Date: Mon Feb 13 20:31:58 2017

Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
during the layout of the section - and this cascades down the tree with the lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.

BUG= 687061 

Review-Url: https://codereview.chromium.org/2670603002
Cr-Commit-Position: refs/heads/master@{#449101}
(cherry picked from commit d3ceb0147ad9f4d4a9b41cea0362d6221cb72302)

Review-Url: https://codereview.chromium.org/2696653003 .
Cr-Commit-Position: refs/branch-heads/2924@{#912}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/f799408c02bfd85d001b2318cf0af0f98d2146ba/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[modify] https://crrev.com/f799408c02bfd85d001b2318cf0af0f98d2146ba/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/f799408c02bfd85d001b2318cf0af0f98d2146ba/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[add] https://crrev.com/f799408c02bfd85d001b2318cf0af0f98d2146ba/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/f799408c02bfd85d001b2318cf0af0f98d2146ba/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/f799408c02bfd85d001b2318cf0af0f98d2146ba/third_party/WebKit/Source/core/layout/LayoutTableSection.h

Verified this issue on Windows-7 and Mac OS 10.12 using chrome latest M57-57.0.2987.54 by following steps mentioned in the original comment. By opening the index.html file from comment #1 observed the page loads immediately without any delay. Hence adding TE-Verified label.

 Issue 687255  has been merged into this issue.

This should not have been merged into 56. We can't make a habit of merging risky changes into the stable channel.

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

commit 91d8afa35df85de452b08ca5c7032fb1456a541a
Author: eae <eae@chromium.org>
Date: Sat Feb 18 10:04:36 2017

Revert of Avoid pathological layout on nested percent height tables (patchset #5 id:80001 of https://codereview.chromium.org/2670603002/ )

Reason for revert:
Broke Google Hangouts.

BUG=693722

Original issue's description:
> Avoid unnecessary cell layout on nested percent height tables
>
> In the test, the nested percent height tables mean that each cell will get at
> least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
> during the layout of the section - and this cascades down the tree with the lowest
> children getting hundreds of repeated layouts.
>
> This inefficiency has always been there, we've just introduced it for the
> percent height situation with crrev.com/2441373002.
>
> To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
> unless the section itself needed a layout - the previously calculated value will do.
>
> The only complication this introduces is: any extra logical height from the
> table that we previously discarded when distributing to the rows we will now
> try to distribute again. This can change the size of the section and cause extra
> paints.
>
>
> BUG= 687061 
>
> Review-Url: https://codereview.chromium.org/2670603002
> Cr-Commit-Position: refs/heads/master@{#449101}
> Committed: https://chromium.googlesource.com/chromium/src/+/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302

TBR=mstensho@opera.com,robhogan@gmail.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 687061 
NOTRY=true

Review-Url: https://codereview.chromium.org/2700183003
Cr-Commit-Position: refs/heads/master@{#451451}

[modify] https://crrev.com/91d8afa35df85de452b08ca5c7032fb1456a541a/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[modify] https://crrev.com/91d8afa35df85de452b08ca5c7032fb1456a541a/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/91d8afa35df85de452b08ca5c7032fb1456a541a/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[delete] https://crrev.com/2b4c744a184c96c5ea0778e91cb11a0acc7215df/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/91d8afa35df85de452b08ca5c7032fb1456a541a/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/91d8afa35df85de452b08ca5c7032fb1456a541a/third_party/WebKit/Source/core/layout/LayoutTableSection.h

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

commit a4b3b0ba32356230afd307830442db9a807008ad
Author: robhogan <robhogan@gmail.com>
Date: Mon Feb 20 22:10:04 2017

Avoid pathological layout on nested percent height tables

A second go at https://codereview.chromium.org/2670603002.

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
during the layout of the section - and this cascades down the tree with the lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.

BUG= 687061 

Review-Url: https://codereview.chromium.org/2701163002
Cr-Commit-Position: refs/heads/master@{#451662}

[modify] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[add] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes-2.html
[add] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes-3.html
[add] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html
[modify] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[add] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/a4b3b0ba32356230afd307830442db9a807008ad/third_party/WebKit/Source/core/layout/LayoutTableSection.h

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

commit f3b0f9b1e57c31ffd21e4bd6d131a0ef2643423b
Author: Emil A Eklund <eae@chromium.org>
Date: Tue Feb 21 19:33:31 2017

Revert of Avoid pathological layout on nested percent height tables (patchset #5 id:80001 of https://codereview.chromium.org/2670603002/ )

Reason for revert:
Broke Google Hangouts.

BUG=693722

Original issue's description:
> Avoid unnecessary cell layout on nested percent height tables
>
> In the test, the nested percent height tables mean that each cell will get at
> least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
> during the layout of the section - and this cascades down the tree with the lowest
> children getting hundreds of repeated layouts.
>
> This inefficiency has always been there, we've just introduced it for the
> percent height situation with crrev.com/2441373002.
>
> To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
> unless the section itself needed a layout - the previously calculated value will do.
>
> The only complication this introduces is: any extra logical height from the
> table that we previously discarded when distributing to the rows we will now
> try to distribute again. This can change the size of the section and cause extra
> paints.
>
>
> BUG= 687061 
>
> Review-Url: https://codereview.chromium.org/2670603002
> Cr-Commit-Position: refs/heads/master@{#449101}
> Committed: https://chromium.googlesource.com/chromium/src/+/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302

TBR=mstensho@opera.com,robhogan@gmail.com
BUG= 687061 
NOTRY=true

Review-Url: https://codereview.chromium.org/2700183003
Cr-Commit-Position: refs/heads/master@{#451451}
(cherry picked from commit 91d8afa35df85de452b08ca5c7032fb1456a541a)

Review-Url: https://codereview.chromium.org/2709863002 .
Cr-Commit-Position: refs/branch-heads/2987@{#618}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/f3b0f9b1e57c31ffd21e4bd6d131a0ef2643423b/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[modify] https://crrev.com/f3b0f9b1e57c31ffd21e4bd6d131a0ef2643423b/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/f3b0f9b1e57c31ffd21e4bd6d131a0ef2643423b/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[delete] https://crrev.com/50dce2837d94d319260d6ff269c09514d0691fb0/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/f3b0f9b1e57c31ffd21e4bd6d131a0ef2643423b/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/f3b0f9b1e57c31ffd21e4bd6d131a0ef2643423b/third_party/WebKit/Source/core/layout/LayoutTableSection.h

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

commit d22377e14ce7c0fd6324b7771cee0fd3fa200ed1
Author: Grace Kihumba <gkihumba@google.com>
Date: Tue Feb 21 20:42:53 2017

Revert of Avoid pathological layout on nested percent height tables (patchset #5 id:80001 of https://codereview.chromium.org/2670603002/ )

Reason for revert:
Broke Google Hangouts.

BUG=693722

Original issue's description:
> Avoid unnecessary cell layout on nested percent height tables
>
> In the test, the nested percent height tables mean that each cell will get at
> least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows())
> during the layout of the section - and this cascades down the tree with the lowest
> children getting hundreds of repeated layouts.
>
> This inefficiency has always been there, we've just introduced it for the
> percent height situation with crrev.com/2441373002.
>
> To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
> unless the section itself needed a layout - the previously calculated value will do.
>
> The only complication this introduces is: any extra logical height from the
> table that we previously discarded when distributing to the rows we will now
> try to distribute again. This can change the size of the section and cause extra
> paints.
>
>
> BUG= 687061 
>
> Review-Url: https://codereview.chromium.org/2670603002
> Cr-Commit-Position: refs/heads/master@{#449101}
> Committed: https://chromium.googlesource.com/chromium/src/+/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302

TBR=mstensho@opera.com,robhogan@gmail.com
BUG= 687061 
NOTRY=true

Review-Url: https://codereview.chromium.org/2700183003
Cr-Commit-Position: refs/heads/master@{#451451}
(cherry picked from commit 91d8afa35df85de452b08ca5c7032fb1456a541a)

Review-Url: https://codereview.chromium.org/2705263003 .
Cr-Commit-Position: refs/branch-heads/2924@{#927}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/d22377e14ce7c0fd6324b7771cee0fd3fa200ed1/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
[modify] https://crrev.com/d22377e14ce7c0fd6324b7771cee0fd3fa200ed1/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
[modify] https://crrev.com/d22377e14ce7c0fd6324b7771cee0fd3fa200ed1/third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt
[delete] https://crrev.com/7515d488e42277a70ee3aa3ad6bc40a3dd50f5e9/third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
[modify] https://crrev.com/d22377e14ce7c0fd6324b7771cee0fd3fa200ed1/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/d22377e14ce7c0fd6324b7771cee0fd3fa200ed1/third_party/WebKit/Source/core/layout/LayoutTableSection.h

Tested this issue on Windows-7 and Mac OS 10.12.3 using chrome latest Beta M57-57.0.2987.74. By opening the file index.html from comment #1 observed the page loads so long and the fix is not working as intended. But tested on previous beta M57-57.0.2987.54 and latest canary M58-58.0.3019.0 and observed the fix is working fine as expected.

robhogan@ Attaching screencast for reference, could you please take a look in to it?

Thanks!

Deleted: 687061.mp4 431 KB

	687061.mp4 431 KB View Download

The fix was rolled out as it broke table rendering.

New fix landed in #57. The CL that broke everything has been fully reverted.

Correct, thanks for the clarification Robhogan. That fix is only in canary at the moment though.

That's correct. 

Do we have an ETA on what release and when it will be released to Chrome?

eae - adding a merge request for M57. How long do you think we should let it bake before merging? How much time do we have before M57 no longer accepts merges?

The ship has sailed for M56 I think.

This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

We're cutting M57 Stable final RC soon and it is too late to take this merge in now. Justification below:

* This is not an M57 regression
* Previous fix for this bug cause major regression (broke hangout)
* per eae@ comment #11 and #23, it is much too late in the process for us to even consider a functional change to a release branch as detailed in comment 11.

Please let me know if there is any concern here. Thank you.

Agree with govind here, it is too risky t merge this into 57. It'll have to wait until 58.

Thank you eae@. Rejecting merge to M57 based on comment #68 and #69.

SAP has had a large number of support cases opened from their customers as well as publicly mentioned this is broken in Chrome (warning their Enterprise users):

https://blogs.sap.com/2017/01/30/portal-is-not-loaded-on-chrome-56/

While I agree, it was frustrating that this was reported to us initially very late, I think it is worth reviewing and further testing in a future 57 release if possible. It will be very difficult for a large number of users if they need to wait another 6 weeks.  Can we consider a  57-2nd cut?

Re #71, M57 1st release just went out today to very small percentage users. We may have M57 refresh depends on 1st stable release data. But it is too risky to take this change in for M57 as eae@ said at #69. Thank you.

Can we get confirmation this will be delivered in M58?

It will, yes.