353455 - [FIX]TableBackgroundPainter::PaintRowGroup shouldn't loop over all rows

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 241796 comment 22.  With the patch for that bug checked in, PaintRowGroup  is the remaining major problem.  I performed the following steps:

1)  Took the testcase from bug 241796.
2)  Cut it down by a factor of 10 so I could load it in sane time.
3)  Ran the following URL on it:

    javascript:void(function(){for (var x=0; x<1000; x++)
                                 window.scrollTo(0,x*10)})()

    and profiled it.

The results were (in a build with the patch for bug 241796):

Total hit count: 423386
Count %Total  Function Name
229729   54.3     nsTableRowFrame::GetNextRow() const
102218   24.1     TableBackgroundPainter::TableBackgroundData::SetFrame(nsIFrame*)
22518   5.3     TableBackgroundPainter::PaintRowGroup(nsTableRowGroupFrame*, int)
21335   5.0     nsTableRowFrame::GetType() const
4481   1.1     __i686.get_pc_thunk.bx
1575   0.4     nsStyleContext::GetStyleData(nsStyleStructID)

The results for the same test with the patch I'm about to attach are:

Total hit count: 38081
Count %Total  Function Name
1472   3.9     .bss
1087   2.9     nsStyleContext::GetStyleData(nsStyleStructID)

So I think we win.  ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 239305 [details] [diff] [review]
Fix

nice

Comment 3

[Bernd]

+  // Sadly, it seems like there may be non-row frames in there... or something?
+  // There are certainly null-checks in GetFirstRow() and GetNextRow().  :(

Thats bug 317876 and all its siblings....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for checking, with a minor comment change

The comment change was:

@@ -499,4 +499,7 @@
                    // their originating row.  We do care about overflow below,
                    // however, since that can be due to rowspans.
+
+  // Note that mDirtyRect is guaranteed to be in the row group's coordinate
+  // system here, so passing its .y to GetFirstRowContaining is ok.
   nsIFrame* cursor = aFrame->GetFirstRowContaining(mDirtyRect.y, &ignored);

because I realized that it was not quite completely obvious what coord systems were in use where...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Checked in.