112253 - Reduce number of outliner row repaints and speed up painting of rows

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[David :Bienvenu]

Profiling loading a mail folder shows that we're spending a lot of our time
repainting outliner rows. This is both because we're painting rows multiple
times, and painting cells is expensive because
nsXULTemplateBuilder::ParseAttribute is expensive and (it seems to me), inneficient.

I'll attach a patch that cuts the startup row paints by 75%. The patch
eliminates the background paint of each cell after drawing the background rect,
and eliminates some invalidations when scroll bar and focus state haven't
changed. I don't know if these changes are the right thing to do or not, but I'm
trying them out and asking for guidance.

Comment 1

[David :Bienvenu]

Attachment: reduce the number of paints

Comment 2

[David :Bienvenu]

Dave, what about the other changes, to not invalidate on SetFocus or
ShowScrollbars if the focus/scroll bar state hasn't really changed? This seems
to cut down row paints down significantly on startup by itself.

Comment 3

[David Hyatt]

The focus change looks ok, although fix the indentation on it.  I need more
context for the "else return NS_OK" portion of the patch.

Comment 4

[David :Bienvenu]

Attachment: same diff, with more context (and don't bail out on background)

the routine with the else return NS_OK is
nsOutlinerBodyFrame::SetVisibleScrollbar

Comment 5

[David :Bienvenu]

Attachment: make InvalidateCell invalidate just the cell

Comment 6

[David :Bienvenu]

Attachment: fixes for outliner image cache

I've made three changes here - the first is that in
nsOutlinerBodyFrame::GetImage, when we've found out we already have a pending
request for an image, we should return after adding the row to the list of rows
to be notified, instead of falling through to the code that makes another
request to fetch the image. Often (always?), the second request cancelled the
notification for the first request, because it overwrote the hash table entry
for the request. (I believe this might explain the problems we've sometimes
seen with images not getting painted in the thread/folder pane on startup).
The second change is to increase the hashtable size to 64 - mailnews seems to
use more than 32, and it's very cheap to set the hash table size to 64 instead.

The third change is to AddRow, to start mMin at -1, so that we don't invalidate
from 0 to mMax all the time.

Comment 7

[David :Bienvenu]

OK, I'm pretty much done for now with the outliner changes - the number of row
paints on startup has been cut from 750 to 125 for my test case, so I'm
satisfied for now. Can I get some r and sr's for the patches? Thx!

Comment 8

[Jan Varga [:janv]]

I applied all 3 patches and tested a little bit.
Works great, changes make sense to me.
r=varga

Comment 9

[David :Bienvenu]

Attachment: Fix nsOutlinerBodyFrame::RowCountChanged to deal with partial last row

One last change - because SetVisibleScrollbar no longer invalidates the whole
outliner if the visibility of the scrollbar hasn't changed (see my first
patch), a slight error in nsOutlinerBodyFrame::RowCountChanged was exposed - it
doesn't account for the last, partial row. GetLastVisibleRow doesn't return
partial rows (perhaps it should - I don't really know), so I just incremented
last and everything works fine. The problem I was seeing before this last fix
was that if a new message arrived and put at the bottom of the thread pane (and
was partially clipped), it wasn't painted at all. And if you deleted it, it
wasn't repainted either.  Now everything seems to work fine.

Comment 10

[Jan Varga [:janv]]

The change to SetVisibleScrollBar() undiscovered another bug.
Just what David already said, but I think this could be done a different way.
Attaching patch

Comment 11

[Jan Varga [:janv]]

Attachment: patch

Comment 12

[David :Bienvenu]

Comment on attachment 59957 [details] [diff] [review]
patch

the change to GetLastVisibleRow makes sense - I thought about doing it that
way, but I wasn't sure what other callers (it's a public method) might expect.
The second change, I'm a little confused by - if you change GetLastVisibleRow
as you did, and take out the last++ that I put in, then last == mTopRowIndex +
mPageCount, not mTopRowIndex + mPageCount + 1 (or is that intentional?)

Comment 13

[Jan Varga [:janv]]

originally GetLastVisibleRow() was implemented as:
  mTopRowIndex + mPageCount + 1
then it was changed to:
  mTopRowIndex + mPageCount - 1

now I recall that I did review for this change, mea culpa I didn't notice that

and I think |mTopRowIndex + mPageCount + 1| in InvalidateRange() 
is actually GetLastVisibleRow(), but it was not fixed

There are only 2 callers of this method I know. One in /layout/base/src/outliner
and one in patch for autoscrolling in thread pane (don't know if it was checked in)

Comment 14

[David :Bienvenu]

OK, I'll apply your patch, and remove my last patch, and run with it for a bit.
Thx for the help.

Comment 15

[David Hyatt]

Can I get a single patch with all the changes in it?  I'm getting confused about
which is the real patch.

Comment 16

[David :Bienvenu]

Attachment: all in one patch, with my changes and Jan's change

Comment 17

[David Hyatt]

Fix the indentation in SetFocused.  You need to indent everything inside the new
if statement you're wrapping everything in.

sr=hyatt

Comment 18

[David :Bienvenu]

thanks, Dave. It really is indented correctly in my tree; it's just not showing
up correctly in cvs -uw diff.

Comment 19

[David :Bienvenu]

fix checked in.