397510 - Scrolling in large file is very slow due to textrun reconstruction

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk Linux build

STEPS TO REPRODUCE:

1)  Start the browser
2)  Load http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1190683140.1190683950.6780.gz&fulltext=1
3)  Scroll to the bottom (e.g. by hitting the end key)
4)  Wait 30 seconds.
5)  Scroll up about halfway (e.g. middle-click on the scrollbar there).
6)  Wait a few seconds.
7)  Scroll up to about a quarter way (same as step 5)

This happens when trying to, e.g., read the trace-malloc output (or more precisely to find where it starts).

EXPECTED RESULTS: Reasonable scrolling

ACTUAL RESULTS: Every time I move by a reasonable distance, the browser freezes up for 10 seconds or so.  Profiling indicates that pretty much all the time is spent under BuildTextRuns(nsIRenderingContext*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*).

It looks like when we go to paint we reconstruct all the textruns for the huge block instead of limiting ourselves to the dirty area.

This is also a problem if you mouse out of the window, wait a few seconds, then mouse back in.  10-second hang.

Comment 1

[Boris Zbarsky [:bzbarsky]]

And actually... even if I make sure to scroll pretty continuously (to prevent the textruns from timing out), scrolling up hangs like this.  Scrolling down is reasonably fast.  Do we construct the textrun from the beginning of the dirty rect to the end of the file or something?

Comment 2

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

This should work, something must be wrong with the backwards-scanner that finds where to start constructing textruns.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Doesn't seem like there's anything wrong with that, actually.  We start at the beginning of the dirty rect or so, like I said.  But we seem to keep going past the end of the dirty rect.

Comment 4

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

Attachment: fix

Okay, this seems to fix it completely for me.

The idea's very simple; we just stop building textruns after some number of lines (currently 50) after we encountered aForFrame and after we've completed a textrun.

Comment 5

[Simon Montagu :smontagu]

Why 50? If it's intended to be the maximum number of lines on screen, it seems a bit small. If it's derived from some other calculation, perhaps you can document it?

Comment 6

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

It's arbitrary. The goal is merely to avoid the overhead of calling BuildTextRuns too many times. Calling it at most once every 50 lines, (and constructing textruns for at most 50 lines too many, seems like a reasonable tradeoff.

Comment 7

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

checked in

Comment 8

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

I backed this out to see if it was responsible for a Tp regression. If it was, we may want to try uping the line limit from 50 to 500 or something.

Comment 9

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

As far as I can tell, backing this out had no effect whatsoever on relevant performance metrics. I'll reland it when the tree opens.

Comment 10

[Dietrich Ayala (:dietrich)]

relanding backed-out blockers:

Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.111; previous revision: 3.110
done

Comment 11

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

I backed it out again because we're still suspicious it's causing a Tp issue and I don't want to deal with it before M9 is done. After M9 I'll find a nice quiet time to reland it and get a few good perf runs. I may also try tweaking the line limit up to 500 or something.

Comment 12

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

Attachment: updated for checkin

Updated for checkin, with the lines to process in BuildTextRuns upped from 50 to 200 to hopefully reduce any overhead in Tp.

Comment 13

[Reed Loden [:reed]]

Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.125; previous revision: 3.124
done

Comment 14

[Reed Loden [:reed]]

Ugh, don't forget there's a separate bustage fix that needs to go with this patch or things turn ugly. :(