Changeset 206253 in webkit


Ignore:
Timestamp:
Sep 22, 2016 12:57:32 AM (8 years ago)
Author:
svillar@igalia.com
Message:

[css-grid] Remove the x2 computation of row sizes with indefinite heights
​https://bugs.webkit.org/show_bug.cgi?id=162150

Reviewed by Darin Adler.

PerformanceTests:

Added a new test case which checks the layout performance of grids inside other grids, i.e,
grids acting both as grid container and grid item.

  • Layout/nested-grid.html: Added.

Source/WebCore:

On r192154, among other things, we added a second pass of the track sizing algorithm for
rows in order to properly compute row sizes when the height was indefinite. We did that in
order to have a symmetrical implementation for columns and rows, but unfortunatelly that was
not correct.

Apart from issuing incorrect results in some cases it created a huge performance issue in
the case of having nested grids because we were exponentially increasing the amount of
executions of the track sizing algorithm. The attached performance test shows a 200%
improvement with the patch (26 vs 80 runs/s).

Test: fast/css-grid-layout/nested-grid.html

  • rendering/RenderGrid.cpp:

(WebCore::RenderGrid::layoutBlock):
(WebCore::RenderGrid::computeIntrinsicLogicalHeight):
(WebCore::RenderGrid::layoutGridItems):

LayoutTests:

Added a new reftest to check the behavior of grids acting also as grid items and how the
track sizing of rows depend on that. It includes tests for grids which stretch their
children and grids which do not.

Apart from that some expected results were updated so that they no longer fail.

  • fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html: Updated expectations.
  • fast/css-grid-layout/nested-grid-expected.html: Added.
  • fast/css-grid-layout/nested-grid.html: Added.
  • fast/css-grid-layout/percent-track-breadths-regarding-container-size.html: Removed FIXME.
  • fast/css-grid-layout/percent-track-breadths-regarding-container-size-expected.txt: Fixed 2

failing tests.

Location:
trunk
Files:
3 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r206248 r206253  
     12016-09-19  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-grid] Remove the x2 computation of row sizes with indefinite heights
     4        https://bugs.webkit.org/show_bug.cgi?id=162150
     5
     6        Reviewed by Darin Adler.
     7
     8        Added a new reftest to check the behavior of grids acting also as grid items and how the
     9        track sizing of rows depend on that. It includes tests for grids which stretch their
     10        children and grids which do not.
     11
     12        Apart from that some expected results were updated so that they no longer fail.
     13
     14        * fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html: Updated expectations.
     15        * fast/css-grid-layout/nested-grid-expected.html: Added.
     16        * fast/css-grid-layout/nested-grid.html: Added.
     17        * fast/css-grid-layout/percent-track-breadths-regarding-container-size.html: Removed FIXME.
     18        * fast/css-grid-layout/percent-track-breadths-regarding-container-size-expected.txt: Fixed 2
     19        failing tests.
     20
    1212016-09-21  Jiewen Tan  <jiewen_tan@apple.com>
    222

  • trunk/LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html

    r200510 r206253  
    5555
    5656<div class="grid max-content max-height-35" data-expected-width="40" data-expected-height="35">
    57     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="35">XX XXX XX XXX</div>
     57    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XXX XX XXX</div>
    5858</div>
    5959
    6060<div class="grid max-content max-height-min-content" data-expected-width="40" data-expected-height="0">
    61     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">XX XXX X</div>
     61    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XXX X</div>
    6262</div>
    6363
    6464<div class="grid max-height-min-content" data-expected-width="40" data-expected-height="0">
    65     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">XX XXX</div>
     65    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XXX</div>
    6666</div>
    6767
    … …  
    133133
    134134<div class="grid min-content" data-expected-width="40" data-expected-height="0">
    135     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">XX XX XX</div>
     135    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XX XX</div>
    136136</div>
    137137
    138138<div class="grid min-content min-height-50" data-expected-width="40" data-expected-height="50">
    139     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="50">XX X</div>
     139    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX X</div>
    140140</div>
    141141
    … …  
    151151
    152152<div class="grid min-content min-height-min-content" data-expected-width="40" data-expected-height="0">
    153     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">XX XXX</div>
     153    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XXX</div>
    154154</div>
    155155
    156156<div class="grid min-content min-height-35" data-expected-width="40" data-expected-height="35">
    157     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="35">XX XX</div>
     157    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XX</div>
    158158</div>
    159159
    … …  
    163163
    164164<div class="grid min-content min-height-50" data-expected-width="40" data-expected-height="50">
    165     <div class="sizedToGridArea" data-expected-width="40" data-expected-height="50">XXXX XXXX XXXX XXXX</div>
     165    <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XXXX XXXX XXXX XXXX</div>
    166166</div>
    167167
    168168<div class="grid min-content max-height-50" data-expected-width="40" data-expected-height="0">
    169     <div class="sizedToGridArea min-height-fill-available" data-expected-width="40" data-expected-height="50">XXXX X X XXXX</div>
     169    <div class="sizedToGridArea min-height-fill-available" data-expected-width="40" data-expected-height="100">XXXX X X XXXX</div>
    170170</div>
    171171
    … …  
    210210    </div>
    211211    <div class="grid max-height-min-content" data-expected-width="40" data-expected-height="0">
    212         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">XX XX XX</div>
     212        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX XX XX</div>
    213213    </div>
    214214    <div class="grid fit-content" data-expected-width="40" data-expected-height="100">
    … …  
    231231    </div>
    232232    <div class="grid max-height-min-content" data-expected-width="40" data-expected-height="0">
    233         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">X XXX XX</div>
     233        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">X XXX XX</div>
    234234    </div>
    235235    <div class="grid fit-content" data-expected-width="40" data-expected-height="100">
    … …  
    240240<div class="fit-content min-height-50" style="height: 75px;">
    241241    <div class="grid fill-available" data-expected-width="40" data-expected-height="75">
    242         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="75">XX X</div>
     242        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX X</div>
    243243    </div>
    244244</div>
    … …  
    246246<div style="height: 25px;">
    247247    <div class="grid fit-content" data-expected-width="40" data-expected-height="25">
    248         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="25">XX X</div>
     248        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX X</div>
    249249    </div>
    250250    <div class="grid fill-available" data-expected-width="40" data-expected-height="25">
    251         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="25">XX X</div>
     251        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX X</div>
    252252    </div>
    253253    <div class="grid fit-content min-height-35" data-expected-width="40" data-expected-height="35">
    254         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="35">XX X</div>
     254        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX X</div>
    255255    </div>
    256256    <div class="grid fit-content max-height-min-content" data-expected-width="40" data-expected-height="0">
    257         <div class="sizedToGridArea" data-expected-width="40" data-expected-height="0">XX X</div>
     257        <div class="sizedToGridArea" data-expected-width="40" data-expected-height="100">XX X</div>
    258258    </div>
    259259</div>

  • trunk/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size-expected.txt

    r202974 r206253  
    3636XXXXX
    3737XXX
    38 FAIL:
    39 Expected 10 for height, but got 4.
    40 Expected 10 for height, but got 4.
    41 Expected 10 for height, but got 4.
    42 
    43 <div class="grid absolutelyPositioned">
    44             <div class="firstRowFirstColumn sizedToGridArea" data-expected-width="20" data-expected-height="10">XX</div>
    45             <div class="firstRowSecondColumn sizedToGridArea" data-expected-width="50" data-expected-height="10">XXXXX</div>
    46             <div class="firstRowThirdColumn sizedToGridArea" data-expected-width="30" data-expected-height="10">XXX</div>
    47         </div>
    48 XX
    49 XXXXX
    50 XXX
    51 FAIL:
    52 Expected 10 for height, but got 4.
    53 Expected 10 for height, but got 4.
    54 Expected 10 for height, but got 4.
    55 
    56 <div class="grid absolutelyPositioned">
    57             <div class="firstRowFirstColumn sizedToGridArea" data-expected-width="20" data-expected-height="10">XX</div>
    58             <div class="firstRowSecondColumn sizedToGridArea" data-expected-width="50" data-expected-height="10">XXXXX</div>
    59             <div class="firstRowThirdColumn sizedToGridArea" data-expected-width="30" data-expected-height="10">XXX</div>
    60         </div>
     38PASS
    6139XX
    6240XXXXX
    … …  
    6745XXX
    6846PASS
     47XX
     48XXXXX
     49XXX
     50PASS

  • trunk/LayoutTests/fast/css-grid-layout/percent-track-breadths-regarding-container-size.html

    r202974 r206253  
    120120    </div>
    121121
    122     <!-- FIXME: The height of the row is wrong calculated in the following 2 examples because of a bug in
    123          RenderBox::hasDefiniteLogicalHeight() that considers that any positioned element has a definite height.
    124          See: https://bugs.webkit.org/show_bug.cgi?id=159251 -->
    125122    <div class="fit-content indefiniteHeight">
    126123        <div class="grid absolutelyPositioned">

  • trunk/PerformanceTests/ChangeLog

    r205751 r206253  
     12016-09-19  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-grid] Remove the x2 computation of row sizes with indefinite heights
     4        https://bugs.webkit.org/show_bug.cgi?id=162150
     5
     6        Reviewed by Darin Adler.
     7
     8        Added a new test case which checks the layout performance of grids inside other grids, i.e,
     9        grids acting both as grid container and grid item.
     10
     11        * Layout/nested-grid.html: Added.
     12
    1132016-09-09  Simon Fraser  <simon.fraser@apple.com>
    214

  • trunk/Source/WebCore/ChangeLog

    r206252 r206253  
     12016-09-19  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-grid] Remove the x2 computation of row sizes with indefinite heights
     4        https://bugs.webkit.org/show_bug.cgi?id=162150
     5
     6        Reviewed by Darin Adler.
     7
     8        On r192154, among other things, we added a second pass of the track sizing algorithm for
     9        rows in order to properly compute row sizes when the height was indefinite. We did that in
     10        order to have a symmetrical implementation for columns and rows, but unfortunatelly that was
     11        not correct.
     12
     13        Apart from issuing incorrect results in some cases it created a huge performance issue in
     14        the case of having nested grids because we were exponentially increasing the amount of
     15        executions of the track sizing algorithm. The attached performance test shows a 200%
     16        improvement with the patch (26 vs 80 runs/s).
     17
     18        Test: fast/css-grid-layout/nested-grid.html
     19
     20        * rendering/RenderGrid.cpp:
     21        (WebCore::RenderGrid::layoutBlock):
     22        (WebCore::RenderGrid::computeIntrinsicLogicalHeight):
     23        (WebCore::RenderGrid::layoutGridItems):
     24
    1252016-09-22  Youenn Fablet  <youenn@apple.com>
    226

  • trunk/Source/WebCore/rendering/RenderGrid.cpp

    r205977 r206253  
    463463    setLogicalHeight(0);
    464464    updateLogicalWidth();
    465     bool logicalHeightWasIndefinite = !computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), Nullopt);
    466465
    467466    placeItemsOnGrid(TrackSizing);
    … …  
    476475    computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
    477476
    478     if (logicalHeightWasIndefinite)
     477    // FIXME: We should use RenderBlock::hasDefiniteLogicalHeight() but it does not work for positioned stuff.
     478    // FIXME: Consider caching the hasDefiniteLogicalHeight value throughout the layout.
     479    bool hasDefiniteLogicalHeight = hasOverrideLogicalContentHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), Nullopt);
     480    if (!hasDefiniteLogicalHeight)
    479481        computeIntrinsicLogicalHeight(sizingData);
    480482    else
    … …  
    485487    updateLogicalHeight();
    486488
    487     // The above call might have changed the grid's logical height depending on min|max height restrictions.
    488     // Update the sizes of the rows whose size depends on the logical height (also on definite|indefinite sizes).
    489     LayoutUnit availableSpaceForRows = contentLogicalHeight();
    490     if (logicalHeightWasIndefinite)
    491         computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
    492 
    493489    // 3- If the min-content contribution of any grid items have changed based on the row
    494490    // sizes calculated in step 2, steps 1 and 2 are repeated with the new min-content
    495491    // contribution (once only).
    496     repeatTracksSizingIfNeeded(sizingData, availableSpaceForColumns, availableSpaceForRows);
     492    repeatTracksSizingIfNeeded(sizingData, availableSpaceForColumns, contentLogicalHeight());
    497493
    498494    // Grid container should have the minimum height of a line if it's editable. That does not affect track sizing though.
    … …  
    634630void RenderGrid::computeIntrinsicLogicalHeight(GridSizingData& sizingData)
    635631{
     632    ASSERT(sizingData.isValidTransition(ForRows));
    636633    ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData));
    637634    sizingData.setAvailableSpace(Nullopt);
    … …  
    655652
    656653    ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData));
     654    sizingData.advanceNextState();
     655    sizingData.sizingOperation = TrackSizing;
    657656}
    658657
    … …  
    18891888void RenderGrid::layoutGridItems(GridSizingData& sizingData)
    18901889{
     1890    ASSERT(sizingData.sizingOperation == TrackSizing);
    18911891    populateGridPositionsForDirection(sizingData, ForColumns);
    18921892    populateGridPositionsForDirection(sizingData, ForRows);
Note: See TracChangeset for help on using the changeset viewer.