Bug 221339 - [css-grid] Cache definite height detection
Summary: [css-grid] Cache definite height detection
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2021-02-03 09:28 PST by zsun
Modified: 2022-05-13 03:25 PDT (History)
12 users (show)

See Also:

Patch (6.03 KB, text/plain)
2021-02-03 11:55 PST, zsun
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description zsun 2021-02-03 09:28:58 PST
It might be worthy of storing the definite/indefinite height detection in a new attribute
in RenderGrid. By doing so, we only need to call RenderBlock::hasDefiniteLogicalHeight() once 
from RenderGrid::RenderBlock(). Then we can use the cached value in other places.
Comment 1 zsun 2021-02-03 11:52:37 PST
Correcting above comment: RenderGrid::RenderBlock() -> RenderGrid::layoutBlock()
Comment 2 zsun 2021-02-03 11:55:20 PST
Created attachment 419167 [details]
Comment 3 Radar WebKit Bug Importer 2021-02-10 09:29:14 PST
Comment 4 Javier Fernandez 2021-02-15 13:30:47 PST
Comment on attachment 419167 [details]

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

The change looks good in general, but since it only makes sense to improve performance, I'd rather justify it with some data from the perf bots. I think we've recently added some changes in the height definiteness logic that could have impacted negatively, so we could monitor the perf tests we have since that change and evaluate the benefit of this patch.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1119
> +    const bool indefiniteHeight = m_direction == ForRows && !m_renderGrid->cachedHasDefiniteLogicalHeight();

Theoretically, it's possible to call the GridTrackSizingAlgorithm method of RenderGrid instance during the execution of its LayoutBlock method (eg, computing the grid's intrinsic size). If I understood correctly this patch, we are initializing the cache during the Layout, so it'd be possible that we call the cachedHasDefiniteLogicalHeight() when the cache Optional value is uninitialized. Shouldn't we add some defensive code to prevent this scenario ?