Bug 98566 – Make the common table-layout cases a little faster with inlining

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make the common table-layout cases a little faster with inlining

[Eric Seidel (no email)]

Created attachment 167400 [details]
Patch

[Julien Chaffraix]

Comment on attachment 167400 [details]
Patch

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

I think the change makes sense from a performance perspective. It also cleans up this code a bit which is nice.

r=me with comments.

> Source/WebCore/ChangeLog:3
> +        Make the common table-layout cases a little faster with inlining

It's probably the no-column case that you are talking about here.

> Source/WebCore/rendering/RenderTable.cpp:-780
> -

Nit: I like those extra spaces before returns as it helps see the different blocks so I would rather keep this one.

> Source/WebCore/rendering/RenderTable.h:276
> +    RenderTableCol* slowColElement(unsigned col, bool* startEdge = 0, bool* endEdge = 0) const;

Not default arguments as you only call it from colElement with all the arguments.

> Source/WebCore/rendering/RenderTableCell.cpp:153
> +    Length colWidthSum = Length(0, Fixed);

While touching this code, you may just end up making colWidthSum an int and explicitly use Length::intValue() as we only ever add fixed length to it. This would save on the Length creation and copying.

> Source/WebCore/rendering/RenderTableCell.cpp:176
> +    if (colWidthSum.isFixed() && colWidthSum.value() > 0)

AFAICT the isFixed() check will always be true as we explicitly make colWidthSum a Fixed Length.

> Source/WebCore/rendering/RenderTableCell.h:248
> +    Length logicalWidthFromTableColumn(RenderTableCol* firstColForThisCell, Length widthFromStyle) const;

I feel that the name could be improved a bit. You find your logical widths from your columns so it should probably be logicalWidthFromColumns (I would drop the 'Table' part but it's a matter of preference).

[Eric Seidel (no email)]

Comment on attachment 167400 [details]
Patch

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

>> Source/WebCore/rendering/RenderTable.cpp:-780
>> -
> 
> Nit: I like those extra spaces before returns as it helps see the different blocks so I would rather keep this one.

Fixed.

>> Source/WebCore/rendering/RenderTable.h:276
>> +    RenderTableCol* slowColElement(unsigned col, bool* startEdge = 0, bool* endEdge = 0) const;
> 
> Not default arguments as you only call it from colElement with all the arguments.

Fixed.

>> Source/WebCore/rendering/RenderTableCell.cpp:153
>> +    Length colWidthSum = Length(0, Fixed);
> 
> While touching this code, you may just end up making colWidthSum an int and explicitly use Length::intValue() as we only ever add fixed length to it. This would save on the Length creation and copying.

Great suggestion.  It made obvious that we're not applying border/padding in teh negative-width case.  I added a FIXME.

>> Source/WebCore/rendering/RenderTableCell.cpp:176
>> +    if (colWidthSum.isFixed() && colWidthSum.value() > 0)
> 
> AFAICT the isFixed() check will always be true as we explicitly make colWidthSum a Fixed Length.

Yeah, and the > 0 doesn't make much sense, nor does the max?  Unless it's specified that border/padding can never make width negative?

>> Source/WebCore/rendering/RenderTableCell.h:248
>> +    Length logicalWidthFromTableColumn(RenderTableCol* firstColForThisCell, Length widthFromStyle) const;
> 
> I feel that the name could be improved a bit. You find your logical widths from your columns so it should probably be logicalWidthFromColumns (I would drop the 'Table' part but it's a matter of preference).

Done.

[Eric Seidel (no email)]

Created attachment 167631 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167631 [details]
Patch for landing

Clearing flags on attachment: 167631

Committed r130698: <http://trac.webkit.org/changeset/130698>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.