Bug 98566 - Make the common table-layout cases a little faster with inlining
Summary: Make the common table-layout cases a little faster with inlining
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel
URL:
Keywords:
Depends on:
Blocks: 98798
  Show dependency treegraph
 
Reported: 2012-10-05 15:41 PDT by Eric Seidel
Modified: 2012-10-09 11:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.08 KB, patch)
2012-10-05 15:43 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Patch for landing (7.65 KB, patch)
2012-10-08 15:30 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2012-10-05 15:41:50 PDT
Make the common table-layout cases a little faster with inlining
Comment 1 Eric Seidel 2012-10-05 15:43:11 PDT
Created attachment 167400 [details]
Patch
Comment 2 Julien Chaffraix 2012-10-05 18:42:08 PDT
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).
Comment 3 Eric Seidel 2012-10-08 15:16:58 PDT
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.
Comment 4 Eric Seidel 2012-10-08 15:30:40 PDT
Created attachment 167631 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-10-08 15:59:30 PDT
Comment on attachment 167631 [details]
Patch for landing

Clearing flags on attachment: 167631

Committed r130698: <http://trac.webkit.org/changeset/130698>
Comment 6 WebKit Review Bot 2012-10-08 15:59:34 PDT
All reviewed patches have been landed.  Closing bug.