Changeset 291952 in webkit

Timestamp:

Mar 27, 2022 1:24:03 PM (4 months ago)

Author:

Matt Woodrow

Message:

Lazily allocate backing store for grid columns.
​https://bugs.webkit.org/show_bug.cgi?id=212201

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

• web-platform-tests/css/css-grid/grid-definition/grid-limits-001-expected.txt: Added.

Add passing expectations file for previously skipped test.

Source/WebCore:

Existing WPT test grid-limits-001.html marked as passing.

• rendering/Grid.cpp:

(WebCore::Grid::ensureGridSize):
(WebCore::Grid::ensureStorageForRow):
(WebCore::Grid::insert):
(WebCore::Grid::cell const):

Only grow the first row's column storage (since we use this
to remember the column count) and lazily grow the others when
we try write to them.

(WebCore::GridIterator::GridIterator):
(WebCore::GridIterator::nextGridItem):
(WebCore::GridIterator::isEmptyAreaEnough const):
(WebCore::GridIterator::nextEmptyGridArea):

• rendering/Grid.h:

Changes GridIterator to use the Grid API rather than directly
accessing the inner Vector using 'friend'. This makes it easier to
ensure that we never access areas outside of our lazy allocations.

LayoutTests:

• TestExpectations:

Remove skip annotation for test grid-limits-001 that now passes.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-limits-001-expected.txt (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/Grid.cpp (modified) (8 diffs)
• Source/WebCore/rendering/Grid.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2022-03-27  Matt Woodrow  <mattwoodrow@apple.com>
+
+        Lazily allocate backing store for grid columns.
+        https://bugs.webkit.org/show_bug.cgi?id=212201
+
+        Reviewed by Dean Jackson.
+
+        * TestExpectations:
+
+        Remove skip annotation for test grid-limits-001 that now passes.
+
 2022-03-27  Alan Bujtas  <zalan@apple.com>
 

trunk/LayoutTests/TestExpectations

@@ -4251 +4251 @@
 webkit.org/b/209460 imported/w3c/web-platform-tests/css/css-grid/abspos/descendant-static-position-002.html [ ImageOnlyFailure ]
 webkit.org/b/209460 imported/w3c/web-platform-tests/css/css-grid/abspos/descendant-static-position-003.html [ ImageOnlyFailure ]
-webkit.org/b/212201 imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-limits-001.html [ Skip ]
 webkit.org/b/212246 imported/w3c/web-platform-tests/css/css-grid/alignment/grid-baseline-align-cycles-001.html [ ImageOnlyFailure ]
 webkit.org/b/231021 imported/w3c/web-platform-tests/css/css-grid/alignment/grid-inline-baseline.html [ ImageOnlyFailure ]

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2022-03-27  Matt Woodrow  <mattwoodrow@apple.com>
+
+        Lazily allocate backing store for grid columns.
+        https://bugs.webkit.org/show_bug.cgi?id=212201
+
+        Reviewed by Dean Jackson.
+
+        * web-platform-tests/css/css-grid/grid-definition/grid-limits-001-expected.txt: Added.
+
+        Add passing expectations file for previously skipped test.
+
 2022-03-26  Noam Rosenthal  <noam@webkit.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-03-27  Matt Woodrow  <mattwoodrow@apple.com>
+
+        Lazily allocate backing store for grid columns.
+        https://bugs.webkit.org/show_bug.cgi?id=212201
+
+        Reviewed by Dean Jackson.
+
+        Existing WPT test grid-limits-001.html marked as passing.
+
+        * rendering/Grid.cpp:
+        (WebCore::Grid::ensureGridSize):
+        (WebCore::Grid::ensureStorageForRow):
+        (WebCore::Grid::insert):
+        (WebCore::Grid::cell const):
+
+        Only grow the first row's column storage (since we use this
+        to remember the column count) and lazily grow the others when
+        we try write to them.
+
+        (WebCore::GridIterator::GridIterator):
+        (WebCore::GridIterator::nextGridItem):
+        (WebCore::GridIterator::isEmptyAreaEnough const):
+        (WebCore::GridIterator::nextEmptyGridArea):
+        * rendering/Grid.h:
+
+        Changes GridIterator to use the Grid API rather than directly
+        accessing the inner Vector using 'friend'. This makes it easier to
+        ensure that we never access areas outside of our lazy allocations.
+
 2022-03-27  Alan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/rendering/Grid.cpp

@@ -52 +52 @@
     if (maximumRowSize > oldRowSize) {
         m_grid.grow(maximumRowSize);
-        for (size_t row = oldRowSize; row < maximumRowSize; ++row)
-            m_grid[row].grow(oldColumnSize);
-    }
-
-    if (maximumColumnSize > oldColumnSize) {
-        for (size_t row = 0; row < numTracks(ForRows); ++row)
-            m_grid[row].grow(maximumColumnSize);
-    }
+    }
+
+    // Just grow the first row for now so that we know the requested size,
+    // and we'll lazily allocate the others when they get used.
+    if (maximumColumnSize > oldColumnSize && maximumRowSize) {
+        m_grid[0].grow(maximumColumnSize);
+    }
+}
+
+void Grid::ensureStorageForRow(unsigned row)
+{
+    m_grid[row].grow(m_grid[0].size());
 }
 
@@ -74 +78 @@
 
     for (auto row : clampedArea.rows) {
+        ensureStorageForRow(row);
         for (auto column : clampedArea.columns)
             m_grid[row][column].append(child);
@@ -80 +85 @@
     setGridItemArea(child, clampedArea);
     return clampedArea;
+}
+
+const GridCell& Grid::cell(unsigned row, unsigned column) const
+{
+    // Storage for rows other than the first is lazily allocated. We can
+    // just return a fake entry if the request is outside the allocated area,
+    // since it must be empty.
+    static const NeverDestroyed<GridCell> emptyCell;
+    if (row && m_grid[row].size() <= column)
+        return emptyCell;
+
+    return m_grid[row][column];
 }
 
@@ -192 +209 @@
 
 GridIterator::GridIterator(const Grid& grid, GridTrackSizingDirection direction, unsigned fixedTrackIndex, unsigned varyingTrackIndex)
-    : m_grid(grid.m_grid)
+    : m_grid(grid)
     , m_direction(direction)
     , m_rowIndex((direction == ForColumns) ? varyingTrackIndex : fixedTrackIndex)
@@ -198 +215 @@
     , m_childIndex(0)
 {
-    ASSERT(!m_grid.isEmpty());
-    ASSERT(!m_grid[0].isEmpty());
-    ASSERT(m_rowIndex < m_grid.size());
-    ASSERT(m_columnIndex < m_grid[0].size());
+    ASSERT(m_grid.numTracks(ForRows));
+    ASSERT(m_grid.numTracks(ForColumns));
+    ASSERT(m_rowIndex < m_grid.numTracks(ForRows));
+    ASSERT(m_columnIndex < m_grid.numTracks(ForColumns));
 }
 
 RenderBox* GridIterator::nextGridItem()
 {
-    ASSERT(!m_grid.isEmpty());
-    ASSERT(!m_grid[0].isEmpty());
+    ASSERT(m_grid.numTracks(ForRows));
+    ASSERT(m_grid.numTracks(ForColumns));
 
     unsigned& varyingTrackIndex = (m_direction == ForColumns) ? m_rowIndex : m_columnIndex;
-    const unsigned endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size();
+    const unsigned endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.numTracks(ForRows) : m_grid.numTracks(ForColumns);
     for (; varyingTrackIndex < endOfVaryingTrackIndex; ++varyingTrackIndex) {
-        const auto& children = m_grid[m_rowIndex][m_columnIndex];
+        const auto& children = m_grid.cell(m_rowIndex, m_columnIndex);
         if (m_childIndex < children.size())
             return children[m_childIndex++].get();
@@ -223 +240 @@
 bool GridIterator::isEmptyAreaEnough(unsigned rowSpan, unsigned columnSpan) const
 {
-    ASSERT(!m_grid.isEmpty());
-    ASSERT(!m_grid[0].isEmpty());
+    ASSERT(m_grid.numTracks(ForRows));
+    ASSERT(m_grid.numTracks(ForColumns));
 
     // Ignore cells outside current grid as we will grow it later if needed.
-    unsigned maxRows = std::min<unsigned>(m_rowIndex + rowSpan, m_grid.size());
-    unsigned maxColumns = std::min<unsigned>(m_columnIndex + columnSpan, m_grid[0].size());
+    unsigned maxRows = std::min<unsigned>(m_rowIndex + rowSpan, m_grid.numTracks(ForRows));
+    unsigned maxColumns = std::min<unsigned>(m_columnIndex + columnSpan, m_grid.numTracks(ForColumns));
 
     // This adds a O(N^2) behavior that shouldn't be a big deal as we expect spanning areas to be small.
     for (unsigned row = m_rowIndex; row < maxRows; ++row) {
         for (unsigned column = m_columnIndex; column < maxColumns; ++column) {
-            auto& children = m_grid[row][column];
+            auto& children = m_grid.cell(row, column);
             if (!children.isEmpty())
                 return false;
@@ -244 +261 @@
 std::unique_ptr<GridArea> GridIterator::nextEmptyGridArea(unsigned fixedTrackSpan, unsigned varyingTrackSpan)
 {
-    ASSERT(!m_grid.isEmpty());
-    ASSERT(!m_grid[0].isEmpty());
+    ASSERT(m_grid.numTracks(ForRows));
+    ASSERT(m_grid.numTracks(ForColumns));
     ASSERT(fixedTrackSpan >= 1);
     ASSERT(varyingTrackSpan >= 1);
 
-    if (m_grid.isEmpty())
+    if (!m_grid.hasGridItems())
         return nullptr;
 
@@ -256 +273 @@
 
     unsigned& varyingTrackIndex = (m_direction == ForColumns) ? m_rowIndex : m_columnIndex;
-    const unsigned endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size();
+    const unsigned endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.numTracks(ForRows) : m_grid.numTracks(ForColumns);
     for (; varyingTrackIndex < endOfVaryingTrackIndex; ++varyingTrackIndex) {
         if (isEmptyAreaEnough(rowSpan, columnSpan)) {

trunk/Source/WebCore/rendering/Grid.h

@@ -60 +60 @@
     GridSpan gridItemSpan(const RenderBox&, GridTrackSizingDirection) const;
 
-    const GridCell& cell(unsigned row, unsigned column) const { return m_grid[row][column]; }
+    const GridCell& cell(unsigned row, unsigned column) const;
 
     unsigned explicitGridStart(GridTrackSizingDirection) const;
@@ -87 +87 @@
 
 private:
-    friend class GridIterator;
+    void ensureStorageForRow(unsigned row);
 
     OrderIterator m_orderIterator;
@@ -129 +129 @@
 
 private:
-    const GridAsMatrix& m_grid;
+    const Grid& m_grid;
     GridTrackSizingDirection m_direction;
     unsigned m_rowIndex;