Bug 98221 – Make tables which don't use col/row span much faster to layout

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Make tables which don't use col/row span much faster to layout

[Eric Seidel (no email)]

Created attachment 166791 [details]
Patch

[Eric Seidel (no email)]

I'm open to other ways of doing this.  I suspect this is just getting back a "regression" from http://trac.webkit.org/changeset/97691.

[Early Warning System Bot]

Comment on attachment 166791 [details]
Patch

Attachment 166791 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14131463

[WebKit Review Bot]

Comment on attachment 166791 [details]
Patch

Attachment 166791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14137401

[Early Warning System Bot]

Comment on attachment 166791 [details]
Patch

Attachment 166791 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14130522

[Eric Seidel (no email)]

Comment on attachment 166791 [details]
Patch

Updating now.

[Eric Seidel (no email)]

Created attachment 166794 [details]
Patch

[Julien Chaffraix]

Comment on attachment 166794 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        It also looks like we could consider removing m_hasHTMLTableCellElement since that appears
> +        to have been a previous attempt at such an optimization from http://trac.webkit.org/changeset/97691.

m_hasHTMLTableCellElement is basically determining if we are an HTML table cell's renderer as CSS doesn't define spanning rows / columns. With your optimization, I think it should be removed, folded into m_hasColSpan and m_hasRowSpan.

> Source/WebCore/rendering/RenderTableCell.cpp:56
> +    , m_hasColSpan(false)
> +    , m_hasRowSpan(false)

Not a huge fan of those names, how about m_hasColumnSpanSet or m_hasColumnSpanAttribute.

> Source/WebCore/rendering/RenderTableCell.cpp:221
> +    // The vast majority of table cells do not have a colspan or rowspan,
> +    // so we keep a bool to know if we need to bother reading from the DOM.
> +    m_hasColSpan = parseColSpanFromDOM() != 1;
> +    m_hasRowSpan = parseRowSpanFromDOM() != 1;

You should do it on colSpanOrRowSpanChanged instead of forcing this cost for every layout.

> Source/WebCore/rendering/RenderTableCell.h:245
> +    unsigned m_column : 28;
>      bool m_cellWidthChanged : 1;
>      bool m_hasHTMLTableCellElement : 1;
> +    bool m_hasColSpan: 1;
> +    bool m_hasRowSpan: 1;

If we want those fields to be packed on Windows, they should have the same type. See https://lists.webkit.org/pipermail/webkit-dev/2012-March/020134.html

(we probably want to keep table cells small and add a COMPILE_ASSERT).

[WebKit Review Bot]

Comment on attachment 166794 [details]
Patch

Attachment 166794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14129449

New failing tests:
css1/cascade/important.html
css1/basic/class_as_selector.html
css1/box_properties/border_color.html
css1/box_properties/border_bottom.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/classification/list_style_position.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border.html
css1/color_and_background/background_position.html
WebFrameTest.FindInPageMatchRects
css1/color_and_background/background_attachment.html
css1/classification/list_style_image.html
css1/cascade/cascade_order.html
css1/color_and_background/background.html
css1/classification/list_style.html
css1/conformance/forward_compatible_parsing.html
css1/box_properties/border_inline.html
css1/color_and_background/background_color.html
css1/color_and_background/background_image.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/basic/contextual_selectors.html
css1/basic/comments.html
css1/color_and_background/background_repeat.html
css1/classification/white_space.html
css1/classification/display.html
css1/classification/list_style_type.html

[Build Bot]

Comment on attachment 166794 [details]
Patch

Attachment 166794 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14124784

New failing tests:
css1/cascade/important.html
css1/basic/class_as_selector.html
css1/box_properties/border_color.html
css1/box_properties/border_bottom.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/classification/list_style_position.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border.html
css1/color_and_background/background_position.html
css1/color_and_background/background_attachment.html
css1/classification/list_style_image.html
css1/cascade/cascade_order.html
css1/color_and_background/background.html
css1/classification/list_style.html
css1/conformance/forward_compatible_parsing.html
css1/box_properties/border_inline.html
css1/color_and_background/background_color.html
css1/color_and_background/background_image.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/basic/contextual_selectors.html
css1/basic/comments.html
css1/classification/white_space.html
css1/font_properties/font.html
css1/classification/display.html
css1/classification/list_style_type.html

[Eric Seidel (no email)]

Created attachment 167134 [details]
Addressed Julien's feedback, but still fails tests

[Eric Seidel (no email)]

Created attachment 167186 [details]
Patch

[Eric Seidel (no email)]

I may need to change bool to unsigned or the compile may fail on Windows.  If it does, I'll fix it.

[Julien Chaffraix]

Comment on attachment 167186 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        so I stole another 2 bits from RenderTableCell::m_column to avoid

you stole 1 bit actually ;)

> Source/WebCore/rendering/RenderTableCell.cpp:55
> +COMPILE_ASSERT(sizeof(RenderTableCell) == sizeof(SameSizeAsRenderTableCell), RenderTableCell_should_stay_small);

As you pointed out, this will likely fail on Windows due to the different member's type.

> Source/WebCore/rendering/RenderTableCell.cpp:81
> +    if (!node())
> +        return 1;

This could be moved around the call to updateColAndRowSpanFlags() in the constructor and removed from both parse function. You are guaranteed to have the right type if you get called from colSpanOrRowSpanChanged.

> Source/WebCore/rendering/RenderTableCell.cpp:123
> +    // FIXME: I suspect that we could return early here if !m_hasColSpan && !m_hasRowSpan.

I don't think this is true. You could probably bail out if the old / new values of rowSpan and colSpan were identical though but we don't have enough information in this function to check that.

[Build Bot]

Comment on attachment 167186 [details]
Patch

Attachment 167186 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14168470

[Eric Seidel (no email)]

Created attachment 167359 [details]
ready

[Eric Seidel (no email)]

I'll mark cq+ once the bots like it.

[WebKit Review Bot]

Comment on attachment 167359 [details]
ready

Clearing flags on attachment: 167359

Committed r130548: <http://trac.webkit.org/changeset/130548>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.