Issue 129633004: Avoid unnecessary forced layout of flex items when stretching

Issue 129633004: Avoid unnecessary forced layout of flex items when stretching (Closed)

Created:
6 years, 11 months ago by leviw_travelin_and_unemployed

Modified:
6 years, 11 months ago

Reviewers:
ojan, dsinclair, esprehn, tony

CC:
blink-reviews, bemjb+rendering_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Avoid unnecessary forced layout of flex items when stretching An old optimization to RenderFlexibleBox layout considered whether or not an element can stretch in the orthogonal direction to flow to determine whether or not to force layout. This case is already handled properly in applyStretchAlignmentToChild, so this was unnecessarily causing extra layouts. Fixing this speeds up our performance on the Holiday Lites benchmark: http://ie.microsoft.com/testdrive/Performance/HolidayLites/ TEST=PerformanceTests/flexbox-with-stretch-layout.html BUG=331848 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164728

Patch Set 1 #

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+67 lines, -15 lines)			Patch
M	LayoutTests/css3/flexbox/repaint-on-layout-expected.txt	View	1 chunk	+0 lines, -2 lines	0 comments	Download
A	PerformanceTests/flexbox-with-stretch-layout.html	View	1 chunk	+65 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderFlexibleBox.h	View	1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderFlexibleBox.cpp	View	2 chunks	+2 lines, -12 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

ojan

LGTM Dan, FYI, this happens to "fix" your repaint test because it gets rid of ...

6 years, 11 months ago (2014-01-08 23:38:39 UTC) #2

LGTM

Dan, FYI, this happens to "fix" your repaint test because it gets rid of
the 2-pass layout in that test case. I think we should be able to recreate
the bug by just invalidating the layout on the nested flex item as well.


On Wed, Jan 8, 2014 at 3:19 PM, <leviw@chromium.org> wrote:

> Reviewers: dsinclair, ojan, tony, esprehn,
>
> Description:
> Avoid unnecessary forced layout of flex items when stretching
>
> An old optimization to RenderFlexibleBox layout considered whether
> or not an element can stretch in the orthogonal direction to flow
> to determine whether or not to force layout. This case is already
> handled properly in applyStretchAlignmentToChild, so this was
> unnecessarily causing extra layouts. Fixing this speeds up our
> performance on the Holiday Lites benchmark:
> http://ie.microsoft.com/testdrive/Performance/HolidayLites/
>
> TEST=PerformanceTests/flexbox-with-stretch-layout.html
> BUG=331848
>
> Please review this at https://codereview.chromium.org/129633004/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+67, -15 lines):
>   M LayoutTests/css3/flexbox/repaint-on-layout-expected.txt
>   A PerformanceTests/flexbox-with-stretch-layout.html
>   M Source/core/rendering/RenderFlexibleBox.h
>   M Source/core/rendering/RenderFlexibleBox.cpp
>
>
> Index: LayoutTests/css3/flexbox/repaint-on-layout-expected.txt
> diff --git a/LayoutTests/css3/flexbox/repaint-on-layout-expected.txt
> b/LayoutTests/css3/flexbox/repaint-on-layout-expected.txt
> index b5c2a1071608a10ab5e0f0fa58eb7c7567fbce48..
> c8680652ba474edb509b1b869226b43907178ac1 100644
> --- a/LayoutTests/css3/flexbox/repaint-on-layout-expected.txt
> +++ b/LayoutTests/css3/flexbox/repaint-on-layout-expected.txt
> @@ -3,7 +3,5 @@
>    (rect 712 8 80 24)
>    (rect 712 8 80 24)
>    (rect 712 8 80 24)
> -  (rect 8 0 200 600)
> -  (rect 8 0 200 600)
>  )
>
> Index: PerformanceTests/flexbox-with-stretch-layout.html
> diff --git a/PerformanceTests/flexbox-with-stretch-layout.html
> b/PerformanceTests/flexbox-with-stretch-layout.html
> new file mode 100644
> index 0000000000000000000000000000000000000000..
> 7be0d079ec4df3a537b5dad167ae704926c3d7af
> --- /dev/null
> +++ b/PerformanceTests/flexbox-with-stretch-layout.html
> @@ -0,0 +1,65 @@
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<script src="../resources/runner.js"></script>
> +<style>
> +
> +#container {
> +    display: flex;
> +    flex-wrap: wrap;
> +}
> +
> +.byteBox {
> +    width: 9px;
> +    font-size: 9px;
> +}
> +
> +.inlineBlock {
> +    font-family: Ahem;
> +    width: 100%;
> +    display: inline-block;
> +}
> +
> +</style>
> +</head>
> +<body>
> +
> +<div id="container" style="">
> +</div>
> +
> +</body>
> +<script>
> +
> +var nbsp = '\u00A0';
> +
> +function setupTest()
> +{
> +    var container = document.getElementById("container");
> +    for (var i = 0; i < 200; i++) {
> +        var div = document.createElement("div");
> +        div.className = "byteBox";
> +        var span = document.createElement("span");
> +        span.className = "inlineBlock";
> +        span.innerText = nbsp;
> +        div.appendChild(span);
> +        container.appendChild(div);
> +    };
> +};
> +
> +setupTest();
> +
> +var switcher = true;
> +
> +function runTest()
> +{
> +    var inlineBlocks = document.getElementsByClassName("inlineBlock");
> +    for (var i = inlineBlocks.length - 1; i >= 0; i--) {
> +        inlineBlocks[i].innerText = switcher ? " " : nbsp;
> +        inlineBlocks[i].offsetTop;
> +    };
> +    switcher = !switcher;
> +};
> +
> +PerfTestRunner.measureRunsPerSecond({run: runTest });
> +</script>
> +</html>
> Index: Source/core/rendering/RenderFlexibleBox.cpp
> diff --git a/Source/core/rendering/RenderFlexibleBox.cpp
> b/Source/core/rendering/RenderFlexibleBox.cpp
> index 6fec4ff1add1a2298a3171ec1fd520d4cdbaa820..
> d8dfa98834e550388609261a09d348f2df5d60f6 100644
> --- a/Source/core/rendering/RenderFlexibleBox.cpp
> +++ b/Source/core/rendering/RenderFlexibleBox.cpp
> @@ -1044,15 +1044,6 @@ size_t RenderFlexibleBox::
> numberOfInFlowPositionedChildren(const OrderedFlexItem
>      return count;
>  }
>
> -bool RenderFlexibleBox::needToStretchChild(RenderBox* child)
> -{
> -    if (alignmentForChild(child) != AlignStretch)
> -        return false;
> -
> -    Length crossAxisLength = isHorizontalFlow() ?
> child->style()->height() : child->style()->width();
> -    return crossAxisLength.isAuto();
> -}
> -
>  void RenderFlexibleBox::resetAutoMarginsAndLogicalTopInCrossAxis(RenderBox*
> child)
>  {
>      if (hasAutoMarginsInCrossAxis(child))
> @@ -1084,10 +1075,9 @@ void
RenderFlexibleBox::layoutAndPlaceChildren(LayoutUnit&
> crossAxisOffset, cons
>
>          LayoutUnit childPreferredSize = childSizes[i] +
> mainAxisBorderAndPaddingExtentForChild(child);
>          setLogicalOverrideSize(child, childPreferredSize);
> -        // FIXME: Can avoid laying out here in some cases. See
> https://webkit.org/b/87905.
> -        if (needToStretchChild(child) || childPreferredSize !=
> mainAxisExtentForChild(child))
> +        if (childPreferredSize != mainAxisExtentForChild(child)) {
>              child->setChildNeedsLayout(MarkOnlyThis);
> -        else {
> +        } else {
>              // To avoid double applying margin changes in
> updateAutoMarginsInCrossAxis, we reset the margins here.
>              resetAutoMarginsAndLogicalTopInCrossAxis(child);
>          }
> Index: Source/core/rendering/RenderFlexibleBox.h
> diff --git a/Source/core/rendering/RenderFlexibleBox.h
> b/Source/core/rendering/RenderFlexibleBox.h
> index 34f5187dccc3ce8d1c259007c9ecfa1ef1e69690..
> 0dc737bee245b2db047fe5d931ab658e754cab53 100644
> --- a/Source/core/rendering/RenderFlexibleBox.h
> +++ b/Source/core/rendering/RenderFlexibleBox.h
> @@ -145,7 +145,6 @@ private:
>      void freezeViolations(const Vector<Violation>&, LayoutUnit&
> availableFreeSpace, double& totalFlexGrow, double& totalWeightedFlexShrink,
> InflexibleFlexItemSize&, bool hasInfiniteLineLength);
>
>      void resetAutoMarginsAndLogicalTopInCrossAxis(RenderBox*);
> -    bool needToStretchChild(RenderBox*);
>      void setLogicalOverrideSize(RenderBox* child, LayoutUnit
> childPreferredSize);
>      void prepareChildForPositionedLayout(RenderBox* child, LayoutUnit
> mainAxisOffset, LayoutUnit crossAxisOffset, PositionedLayoutMode);
>      size_t numberOfInFlowPositionedChildren(const OrderedFlexItemList&)
> const;
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leviw@chromium.org/129633004/1

6 years, 11 months ago (2014-01-08 23:39:32 UTC) #3

Message was sent while issue was closed.

Change committed as 164728

Expand Messages | Collapse Messages