Closed Bug 1176070 Opened 5 years ago Closed 5 years ago

Optimize calls to nsTableFrame::FirstInFlow() via GetColumnISize(), in nsTableFrame::SetColumnDimensions().

Categories

(Core :: Layout: Tables, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dholbert, Assigned: jfkthame)

Details

Attachments

(1 file)

Right now, nsTableFrame::SetColumnDimensions has a nested loop, which walks all of the column frames and calls GetColumnISize for each of them.

Each of these GetColumnISize() calls (on "this") involves one or more virtual function calls:
 (1) We call FirstInFlow() (which is virtual), and if that's not us, then we redirect the call to that frame.
 (2) (If we redirected the call, then that frame has to do the same thing to make sure it's really the FirstInFlow. So, that's another virtual function call.)

We can optimize all of these calls away by doing something like:
 (1) Call FirstInFlow() *once*; e.g. "nsIFrame* fif = FirstInFlow();"
 (2) Instead of calling GetColumnISize() on |this|, we should instead use a helper-method named, say, "fif->GetColumnISizeAssumingFIF()" which assumes it's being called on the first in flow.

So then we end up only making one virtual function call, instead of one per column. (for the purpose of GetColumnISize at least)

(Noticed this while reviewing part 2 in bug 1174700, which rewrites SetColumnDimensions.)
Summary: Optimize nsTableFrame's FirstInFlow() lookups from calls to GetColumnISize → Optimize calls to nsTableFrame::FirstInFlow() via GetColumnISize(), in nsTableFrame::SetColumnDimensions().
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8625340 [details] [diff] [review]
Optimize nsTableFrame's FirstInFlow() lookups from calls to GetColumnISize()

Looks good!

One further request:
> int32_t
>-nsTableFrame::GetColumnISize(int32_t aColIndex)
>+nsTableFrame::GetColumnISizeFromFirstInFlow(int32_t aColIndex)
>+{
>+  MOZ_ASSERT(this == FirstInFlow());
>+  nsTableColFrame* colFrame = GetColFrame(aColIndex);
>+  return colFrame ? colFrame->GetFinalISize() : 0;
> }


While you're reworking this function, could you change the return type to 'nscoord', too? There's no reason for this to return int32_t. (Note that the thing we're returning, nsTableColFrame::GetFinalISize(), is a nscoord; and almost all of the callers use our return-value into an nscoord as well. 

> nscoord
> nsTableFrame::GetCollapsedISize(const WritingMode aWM,
>                                 const LogicalMargin& aBorderPadding)
> {
>   NS_ASSERTION(!GetPrevInFlow(), "GetCollapsedISize called on next in flow");
>   nscoord iSize = GetColSpacing(GetColCount());
[...]
>-        int32_t colISize = GetColumnISize(colX);
>+        int32_t colISize = fif->GetColumnISizeFromFirstInFlow(colX);
>         if (!collapseGroup && !collapseCol) {
>           iSize += colISize;

Similarly: this caller should use nscoord to store our return value, not int32_t. (I think this is the only caller that actually handles the rv as an int32_t.)
Attachment #8625340 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/e66ff38867c5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.