[css-grid] Avoid the HashMap for the override containing block size (issue 2176633002 by jfernandez@igalia.com)

[jfern...@igalia.com]

Reviewers: svillar, cbiesinger, Manuel Rego
CL: https://codereview.chromium.org/2176633002/

Description:
[css-grid] Avoid the HashMap for the override containing block size

The change made in r406557 caused a 12% regression on the grid layout
performance tests. The root cause is that as part of the changes
required to implement the orthogonal flows support we decided to use
physical directions to determine whether a grid item overflows its
grid area or not. Since we store the logical sizes in a HashMap, we
implemented some utility methods that, based on the grid container's
writing mode, retrieve the stored width or height from the HashMap.

Since the previous logic was reusing the already computed logical
value, the change introduced a considerable performance regression.

This patch address the issue by removing the utility methods and
determine the physical sizes directly inside the layout logic.


BUG=630448

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+12, -50 lines):
M third_party/WebKit/Source/core/layout/LayoutBox.h
M third_party/WebKit/Source/core/layout/LayoutBox.cpp
M third_party/WebKit/Source/core/layout/LayoutGrid.h
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp


Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 3b8d7d7531e7ed02ccd4b6d555f64b960ae7cd43..650314c54d4fb2cdec190cf1fe753ff3cb7d5356 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -1099,26 +1099,26 @@ LayoutUnit LayoutBox::overrideContainingBlockContentLogicalWidth() const
return gOverrideContainingBlockLogicalWidthMap->get(this);
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
LayoutUnit LayoutBox::overrideContainingBlockContentLogicalHeight() const
{
ASSERT(hasOverrideContainingBlockLogicalHeight());
return gOverrideContainingBlockLogicalHeightMap->get(this);
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
bool LayoutBox::hasOverrideContainingBlockLogicalWidth() const
{
return gOverrideContainingBlockLogicalWidthMap && gOverrideContainingBlockLogicalWidthMap->contains(this);
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
bool LayoutBox::hasOverrideContainingBlockLogicalHeight() const
{
return gOverrideContainingBlockLogicalHeightMap && gOverrideContainingBlockLogicalHeightMap->contains(this);
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
void LayoutBox::setOverrideContainingBlockContentLogicalWidth(LayoutUnit logicalWidth)
{
if (!gOverrideContainingBlockLogicalWidthMap)
@@ -1126,7 +1126,7 @@ void LayoutBox::setOverrideContainingBlockContentLogicalWidth(LayoutUnit logical
gOverrideContainingBlockLogicalWidthMap->set(this, logicalWidth);
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
void LayoutBox::setOverrideContainingBlockContentLogicalHeight(LayoutUnit logicalHeight)
{
if (!gOverrideContainingBlockLogicalHeightMap)
@@ -1134,7 +1134,7 @@ void LayoutBox::setOverrideContainingBlockContentLogicalHeight(LayoutUnit logica
gOverrideContainingBlockLogicalHeightMap->set(this, logicalHeight);
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
void LayoutBox::clearContainingBlockOverrideSize()
{
if (gOverrideContainingBlockLogicalWidthMap)
@@ -1142,33 +1142,13 @@ void LayoutBox::clearContainingBlockOverrideSize()
clearOverrideContainingBlockContentLogicalHeight();
}

-// TODO (lajava) Now that we have implemented these functions based on physical direction, we'd rather remove the logical ones.
+// TODO (lajava) Shouldn't we implement these functions based on physical direction ?.
void LayoutBox::clearOverrideContainingBlockContentLogicalHeight()
{
if (gOverrideContainingBlockLogicalHeightMap)
gOverrideContainingBlockLogicalHeightMap->remove(this);
}

-LayoutUnit LayoutBox::overrideContainingBlockContentWidth() const
-{
- return containingBlock()->isHorizontalWritingMode() ? overrideContainingBlockContentLogicalWidth() : overrideContainingBlockContentLogicalHeight();
-}
-
-LayoutUnit LayoutBox::overrideContainingBlockContentHeight() const
-{
- return containingBlock()->isHorizontalWritingMode() ? overrideContainingBlockContentLogicalHeight() : overrideContainingBlockContentLogicalWidth();
-}
-
-bool LayoutBox::hasOverrideContainingBlockWidth() const
-{
- return containingBlock()->isHorizontalWritingMode() ? hasOverrideContainingBlockLogicalWidth() : hasOverrideContainingBlockLogicalHeight();
-}
-
-bool LayoutBox::hasOverrideContainingBlockHeight() const
-{
- return containingBlock()->isHorizontalWritingMode() ? hasOverrideContainingBlockLogicalHeight() : hasOverrideContainingBlockLogicalWidth();
-}
-
LayoutUnit LayoutBox::extraInlineOffset() const
{
return gExtraInlineOffsetMap ? gExtraInlineOffsetMap->get(this) : LayoutUnit();
Index: third_party/WebKit/Source/core/layout/LayoutBox.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.h b/third_party/WebKit/Source/core/layout/LayoutBox.h
index c1571cbdf8195fff29c4c6def26801d7cb8284e6..c1938cd2d57be1dcf50062eb9afc9b17243722da 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.h
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.h
@@ -552,10 +552,6 @@ public:
void setOverrideContainingBlockContentLogicalHeight(LayoutUnit);
void clearContainingBlockOverrideSize();
void clearOverrideContainingBlockContentLogicalHeight();
- LayoutUnit overrideContainingBlockContentWidth() const;
- LayoutUnit overrideContainingBlockContentHeight() const;
- bool hasOverrideContainingBlockWidth() const;
- bool hasOverrideContainingBlockHeight() const;

LayoutUnit extraInlineOffset() const;
LayoutUnit extraBlockOffset() const;
Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index e9d00961ae56359570e6530bdc8e647c2c108593..e055a8bd24bd63a9140b6e12ec14b54c0e3e9927 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -1825,20 +1825,6 @@ void LayoutGrid::applyStretchAlignmentToTracksIfNeeded(GridTrackSizingDirection
availableSpace = LayoutUnit();
}

-bool LayoutGrid::isChildOverflowingContainingBlockHeight(const LayoutBox& child) const
-{
- // TODO (lajava) We must consider margins to determine whether it overflows or not (see https://crbug/628155)
- LayoutUnit containingBlockContentHeight = child.hasOverrideContainingBlockHeight() ? child.overrideContainingBlockContentHeight() : LayoutUnit();
- return child.size().height() > containingBlockContentHeight;
-}
-
-bool LayoutGrid::isChildOverflowingContainingBlockWidth(const LayoutBox& child) const
-{
- // TODO (lajava) We must consider margins to determine whether it overflows or not (see https://crbug/628155)
- LayoutUnit containingBlockContentWidth = child.hasOverrideContainingBlockWidth() ? child.overrideContainingBlockContentWidth() : LayoutUnit();
- return child.size().width() > containingBlockContentWidth;
-}
-
void LayoutGrid::layoutGridItems(GridSizingData& sizingData)
{
populateGridPositionsForDirection(sizingData, ForColumns);
@@ -1884,10 +1870,12 @@ void LayoutGrid::layoutGridItems(GridSizingData& sizingData)
#endif
child->setLogicalLocation(findChildLogicalPosition(*child, sizingData));

- // Keep track of children overflowing their grid area as we might need to paint them even if the grid-area is
- // not visible.
+ // Keep track of children overflowing their grid area as we might need to paint them even if the grid-area is not visible.
// Using physical dimensions for simplicity, so we can forget about orthogonalty.
- if (isChildOverflowingContainingBlockHeight(*child) || isChildOverflowingContainingBlockWidth(*child))
+ // TODO (lajava): Child's margins should account when evaluating whether it overflows its grid area (http://crbug.com/628155).
+ LayoutUnit childGridAreaHeight = isHorizontalWritingMode() ? overrideContainingBlockContentLogicalHeight : overrideContainingBlockContentLogicalWidth;
+ LayoutUnit childGridAreaWidth = isHorizontalWritingMode() ? overrideContainingBlockContentLogicalWidth : overrideContainingBlockContentLogicalHeight;
+ if (child->size().height() > childGridAreaHeight || child->size().width() > childGridAreaWidth)
m_gridItemsOverflowingGridArea.append(child);
}
}
Index: third_party/WebKit/Source/core/layout/LayoutGrid.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.h b/third_party/WebKit/Source/core/layout/LayoutGrid.h
index 63d0ec611fa861ba590309f99d360a4bb4c409dc..fb9ae43eaf3c8a0ca704987e3add8ebc39c9531c 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.h
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.h
@@ -166,8 +166,6 @@ private:
const GridTrackSize& rawGridTrackSize(GridTrackSizingDirection, size_t) const;
GridTrackSize gridTrackSize(GridTrackSizingDirection, size_t, SizingOperation = TrackSizing) const;

- bool isChildOverflowingContainingBlockWidth(const LayoutBox&) const;
- bool isChildOverflowingContainingBlockHeight(const LayoutBox&) const;
bool updateOverrideContainingBlockContentSizeForChild(LayoutBox&, GridTrackSizingDirection, GridSizingData&) const;
LayoutUnit logicalHeightForChild(LayoutBox&, GridSizingData&) const;
LayoutUnit minSizeForChild(LayoutBox&, GridTrackSizingDirection, GridSizingData&) const;

[svi...@igalia.com]

Never liked those methods :)

lgtm

https://codereview.chromium.org/2176633002/

[cbies...@chromium.org]

lgtm

(longer term it may also be helpful to store the override heights in
LayoutBoxRareData -
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.h?sq=package:chromium&rcl=1469186210&l=58)

https://codereview.chromium.org/2176633002/

[jfern...@igalia.com]

On 2016/07/22 16:36:38, cbiesinger wrote:
> lgtm
>
> (longer term it may also be helpful to store the override heights in
> LayoutBoxRareData -
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.h?sq=package:chromium&rcl=1469186210&l=58)

It's an interesting suggestion, indeed. I'll add a TODO comment.

https://codereview.chromium.org/2176633002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2176633002/1


https://codereview.chromium.org/2176633002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/262125)

https://codereview.chromium.org/2176633002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #1 (id:1)

https://codereview.chromium.org/2176633002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 1 (id:??) landed as
https://crrev.com/2cf9f05031414616d3d1eb1e5ffa5013e396d61b
Cr-Commit-Position: refs/heads/master@{#407762}

https://codereview.chromium.org/2176633002/

[rob...@chromium.org]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2180153003/ by rob...@chromium.org.

The reason for reverting is: Speculative Revert for Win10 Failing Tests:
fast/text/emphasis-combined-text.html
svg/W3C-SVG-1.1/filters-composite-02-b.svg.

https://codereview.chromium.org/2176633002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 1 (id:??) landed as

https://crrev.com/e2bf9ba77a6b9add6c7a703be1fa85814c79c12e
Cr-Commit-Position: refs/heads/master@{#408203}

https://codereview.chromium.org/2176633002/