Bug 102352

Summary: Avoid a second layout of flex items in layoutAndPlaceChildren()
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, mifenton, ojan, tkent, tony, webkit.review.bot
Priority: P2 Keywords: Performance
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 102817, 102851, 102993    
Bug Blocks: 62048, 102585    
Attachments:
Description Flags
Patch
tony: review-, webkit.review.bot: commit-queue-
Updated patch
tony: review-, webkit.review.bot: commit-queue-
Updated patch
none
Patch
none
Updated patch
none
Patch none

Description Carlos Garcia Campos 2012-11-15 01:36:36 PST
In case of nested flexbox containers, it's possible that in layoutAndPlaceChildren(), the children have already been laid out in computeMainAxisPreferredSizes().
Comment 1 Carlos Garcia Campos 2012-11-15 01:47:54 PST
Created attachment 174370 [details]
Patch

I'm still getting a failure in css3/flexbox/auto-margins.html but as I commented in bug #87905 the output is correct, and the inspector shows the right values for margins, just reloading the page turns the FAIL into PASS.
Comment 2 WebKit Review Bot 2012-11-15 02:40:29 PST
Comment on attachment 174370 [details]
Patch

Attachment 174370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14857146

New failing tests:
fast/forms/datalist/update-range-with-datalist.html
fast/multicol/client-rects.html
mathml/presentation/sub.xhtml
mathml/presentation/mo-stretch.html
css3/flexbox/auto-margins.html
fast/css/unknown-pseudo-element-matching.html
fast/forms/range/input-appearance-range.html
fast/forms/range/range-thumb-height-percentage.html
Comment 3 Tony Chang 2012-11-15 09:53:22 PST
(In reply to comment #1)
> Created an attachment (id=174370) [details]
> Patch
> 
> I'm still getting a failure in css3/flexbox/auto-margins.html but as I commented in bug #87905 the output is correct, and the inspector shows the right values for margins, just reloading the page turns the FAIL into PASS.

This is a real failure. It means we're not layout out when we should.  The inspector is probably forcing an extra layout which is causing the margins to correct themselves.  The other failures are probably real too (mathml and the range form control both use flexbox).
Comment 4 Tony Chang 2012-11-15 09:57:17 PST
Comment on attachment 174370 [details]
Patch

I'm not sure why the tests are failing, maybe try using a debugger to see how auto-margins.html is different with and without this patch?
Comment 5 Carlos Garcia Campos 2012-11-16 04:03:23 PST
Created attachment 174641 [details]
Updated patch

Since the major performance issues happen with nested flexbox containers, we can try a more conservative approach and only avoid second layout when the child is a flexbox.
Comment 6 WebKit Review Bot 2012-11-16 04:48:56 PST
Comment on attachment 174641 [details]
Updated patch

Attachment 174641 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14874072

New failing tests:
fast/forms/datalist/update-range-with-datalist.html
mathml/presentation/sub.xhtml
mathml/presentation/mo-stretch.html
Comment 7 Tony Chang 2012-11-16 12:22:48 PST
Comment on attachment 174641 [details]
Updated patch

Tests are still failing.  We need to debug the cause of the failure.  Applying the change less often isn't going to make the bug go away.
Comment 8 Carlos Garcia Campos 2012-11-17 02:06:03 PST
(In reply to comment #7)
> (From update of attachment 174641 [details])
> Tests are still failing.  We need to debug the cause of the failure.  Applying the change less often isn't going to make the bug go away.

Sure, I debugged the auto-margins case, and the problem was that the logical height is updated during the layout and it resets the margins, so when not laying out the margins already had the right value that is used to calculate the margin again, which ended up resetting to 0. So, just calling child->updateLogicalHeight() when avoiding the layout of the child, fixed the test, but I was not sure that was the right approach and that didn't fix the other tests. So, my guess is that when avoiding the layout, we should still do other things that happen during the layout even if they don't affect the size. I think the problem with the other failures has to do with the position, for some reason, when laying out multiple times the child is moved several times, but the final position is the same than when avoiding the layout. Next week I'll continue debugging the test failures.
Comment 9 Carlos Garcia Campos 2012-11-20 09:20:08 PST
I've been debugging fast/forms/range/input-appearance-range.html and the problem is that the slider thumb location is added every time the slider container is laid out. RenderSliderContainer::layout() sets the thumb location assuming the thumb is laid out, which might not be the case after this patch. I've filed it as a separate bug report:

https://bugs.webkit.org/show_bug.cgi?id=102817

I'm still looking at the mathml changes, the different is just a few pixels, for some reason the main axis is different (by one pixel) in some cases when doing the second layout to apply the stretch alignment.
Comment 10 Carlos Garcia Campos 2012-11-20 10:35:03 PST
(In reply to comment #9)
> I've been debugging fast/forms/range/input-appearance-range.html and the problem is that the slider thumb location is added every time the slider container is laid out. RenderSliderContainer::layout() sets the thumb location assuming the thumb is laid out, which might not be the case after this patch. I've filed it as a separate bug report:
> 
> https://bugs.webkit.org/show_bug.cgi?id=102817
> 
> I'm still looking at the mathml changes, the different is just a few pixels, for some reason the main axis is different (by one pixel) in some cases when doing the second layout to apply the stretch alignment.

I meant the cross axis, not the main axis.
Comment 11 Tony Chang 2012-11-20 16:59:23 PST
fixed by bug 102851:
fast/forms/datalist/update-range-with-datalist.html

fixed by bug 102817:
fast/forms/range/input-appearance-range.html
fast/forms/range/range-thumb-height-percentage.html

For these 2, it's because we normally compute margins during layout, but for flex items, we compute auto margins in the flexbox.  When we skip the flex item layout, we end up double computing the margin in a few cases.  There are probably a few different ways to solve this.
fast/multicol/client-rects.html
css3/flexbox/auto-margins.html

This is mysterious, the first :hover selector isn't applying, but the second test passes.  If I reorder the test cases, the first test always fails.
fast/css/unknown-pseudo-element-matching.html

Haven't looked at these yet.:
mathml/presentation/sub.xhtml
mathml/presentation/mo-stretch.html
Comment 12 Carlos Garcia Campos 2012-11-21 01:27:03 PST
(In reply to comment #11)
> For these 2, it's because we normally compute margins during layout, but for flex items, we compute auto margins in the flexbox.  When we skip the flex item layout, we end up double computing the margin in a few cases.  There are probably a few different ways to solve this.
> fast/multicol/client-rects.html
> css3/flexbox/auto-margins.html

I fixed this, by calling updateLogicalHeight() on the child when not laying out it.

> Haven't looked at these yet.:
> mathml/presentation/sub.xhtml
> mathml/presentation/mo-stretch.html

I'll continue debugging these today.
Comment 13 Tony Chang 2012-11-21 17:02:02 PST
(In reply to comment #11)
> This is mysterious, the first :hover selector isn't applying, but the second test passes.  If I reorder the test cases, the first test always fails.
> fast/css/unknown-pseudo-element-matching.html

This will be fixed by 102993.
Comment 14 Carlos Garcia Campos 2012-11-22 03:37:00 PST
(In reply to comment #12)
> > Haven't looked at these yet.:
> > mathml/presentation/sub.xhtml
> > mathml/presentation/mo-stretch.html
> 
> I'll continue debugging these today.

I'm very confused with these, mo-stretch.html fails for me even without the patch, so I'm not sure, and sub.xhtml renders the sub Y one pixel below the expected one, however, what I get when running manually the tests in chromium is different and looks correct even with the patch, so I don't know if it has to do with the fonts used by DRT.
Comment 15 Tony Chang 2012-11-28 17:30:31 PST
I debugged the mathml failures and they uncovered a real problem.  Here's an example:
http://plexode.com/u/#2N=0&-Mle2L%3A2K%0AEJheightEH5olid!EG%3Cdiv29%3E28%3Dj7K!!G!id26%22r54borderL!1px!sb4!styM86b*%3C%2Fdiv92!%20~http://pMxode.com/eval3/#ht=G4displayL-webkit-fMx6978aHblue%3B!JL!30px69a*78bHgreen%3B69b*K*&ohjNjt=document.getEMmentById(6a6).styM.J!8!620px6K&ojhNojjNcex=0


Press eval JS now to see the bug.  What happens is that we previously stretched a child and since we don't layout the second time, we keep using the stretched height, which is incorrect.  We should include this test as a layout test.

A simple fix is to ignore this optimization if the child is stretching (i.e., always relayout stretching children).

For the auto margins case, updateLogicalHeight should work, but we may be able to be smarter about it (maybe just set the margins to 0 if they style is auto?).

I'll try to put together a patch tomorrow.
Comment 16 Carlos Garcia Campos 2012-11-29 06:56:20 PST
Created attachment 176720 [details]
Updated patch

Do not skip the layout when stretching children that has auto size. This fixes the tests, but I think we are still missing some more cases where we could avoid a relayout.
Comment 17 WebKit Review Bot 2012-11-29 12:44:15 PST
Comment on attachment 176720 [details]
Updated patch

Attachment 176720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15054071

New failing tests:
fast/forms/datalist/input-appearance-range-with-datalist-rtl.html
fast/forms/datalist/input-appearance-range-with-padding-with-datalist.html
fast/forms/range/input-appearance-range-rtl.html
Comment 18 Build Bot 2012-11-29 14:07:56 PST
Comment on attachment 176720 [details]
Updated patch

Attachment 176720 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15044429

New failing tests:
fast/forms/range/input-appearance-range-rtl.html
Comment 19 Tony Chang 2012-11-29 16:47:43 PST
Created attachment 176849 [details]
Patch
Comment 20 Carlos Garcia Campos 2012-11-29 23:59:05 PST
Comment on attachment 176849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176849&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
> +    return (!isColumnFlow() && child->style()->logicalHeight().isAuto())
> +        || (isColumnFlow() && child->style()->logicalWidth().isAuto());

I've noticed we might be checking isColumnFlow twice here, it's very minor thing, but we could cache it in a variable like you are doing in resetAutoMarginsAndLogicalTopInCrossAxis() with isHorizontalFlow.
Comment 21 Carlos Garcia Campos 2012-11-30 01:24:55 PST
Created attachment 176919 [details]
Updated patch

Updated patch with the isColumnFlow minor improvement I commented before.
Comment 22 Ojan Vafai 2012-11-30 12:07:11 PST
Comment on attachment 176919 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176919&action=review

> LayoutTests/css3/flexbox/stretch-after-sibling-size-change.html:17
> +  <div id=a class="flex-one" data-expected-width="50" data-expected-height="20" style="background-color: blue; height: 30px;"></div>
> +  <div id=b class="flex-one" data-expected-width="50" data-expected-height="20" style="background-color: green"></div>

Nit: 2 spec indent.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:186
> +        // doesn't move the thumb to the wrong place. FIXME: Make a custom Render class

The FIXME should go at the beginning of a line.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
> +    bool isColumn = isColumnFlow();
> +    return (!isColumn && child->style()->logicalHeight().isAuto()) || (isColumn && child->style()->logicalWidth().isAuto());

I'd find this easier to read as follows:
var crossAxisLength = isColumnFlow() ? child->style->logicalWidth() : child->style->logicalHeight();
return crossAxisLength.isAuto();

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1081
> +    Length start = isHorizontal ? child->style()->marginTop() : child->style()->marginLeft();
> +    Length end = isHorizontal ? child->style()->marginBottom() : child->style()->marginRight();

s/start/before
s/end/after

You could use flowAwareMarginBeforeForChild and flowAwareMarginAfterForChild.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1114
> +            resetAutoMarginsAndLogicalTopInCrossAxis(child);

This could use a comment explaining why we need to do this. I had to go read the comments in the bug to make sense of this.
Comment 23 Tony Chang 2012-11-30 12:26:36 PST
Created attachment 177008 [details]
Patch
Comment 24 Tony Chang 2012-11-30 12:28:27 PST
Comment on attachment 176919 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176919&action=review

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1074
>> +    return (!isColumn && child->style()->logicalHeight().isAuto()) || (isColumn && child->style()->logicalWidth().isAuto());
> 
> I'd find this easier to read as follows:
> var crossAxisLength = isColumnFlow() ? child->style->logicalWidth() : child->style->logicalHeight();
> return crossAxisLength.isAuto();

The old code was wrong in the case of orthogonal flow.  I fixed it in the latest patch.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1081
>> +    Length end = isHorizontal ? child->style()->marginBottom() : child->style()->marginRight();
> 
> s/start/before
> s/end/after
> 
> You could use flowAwareMarginBeforeForChild and flowAwareMarginAfterForChild.

Those return the computed margin, not the margin style. Fortunately, we had a different helper method that does what I want.
Comment 25 Ojan Vafai 2012-11-30 17:01:16 PST
Comment on attachment 177008 [details]
Patch

yay!
Comment 26 WebKit Review Bot 2012-12-02 02:40:43 PST
Comment on attachment 177008 [details]
Patch

Clearing flags on attachment: 177008

Committed r136324: <http://trac.webkit.org/changeset/136324>
Comment 27 WebKit Review Bot 2012-12-02 02:40:48 PST
All reviewed patches have been landed.  Closing bug.