386122 - Performance of loading large pages of text has regressed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Adam Guthrie]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070622 Minefield/3.0a6pre

The time it takes to load large pages of text; e.g. full Tinderbox logs, has regressed.

Steps to reproduce:
1. Load http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1182983866.1001.gz&fulltext=1

In a build BEFORE the textframe landing I see the following memory usage:

start browser: 30K
load page: 60K
navigate away to a few other pages: 40K

and with a build AFTER the textframe landing:

start browser: 30K
load page: 400K (slowly drops off back to 70K)
navigate away to a few other pages: 40K

The textframe build also seems to use more CPU and pretty much hangs my browser when it's loading. I think this occurs on Linux, too, but my testing was done only on Windows XP.

Comment 1

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

Attachment: patch

This should help a bit. Peak memory usage will still be pretty high as it loads but it should be less, and you should see lower memory usage while you're scrolling around. Maybe I should add some textrun stats...

Comment 2

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

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

This is completely bogus

Comment 3

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

Local version of the testcase in my debug build, so these are rough numbers:

Before patch:
Elapsed time: 19003 (ms)
Textrun storage high water mark=286860357 (bytes)

After patch:
Elapsed time: 7126 (ms)
Textrun storage high water mark=23970784 (bytes)

So I think this upcoming patch helps a lot...

Comment 4

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

Attachment: fix

The basic idea here is to allow text runs to end at preformatted newline boundaries, instead of forcing the entire file of preformatted text into one textrun. I change StylesMatchForTextRun to ContinueTextRunAcrossFrames and add the newline check in there. Also:

-- Clean up HasTerminalNewline and provide it as a static function as well as a virtual method

-- Don't pack next-in-flows into the same mMappedFlow if there's a preformatted newline present (otherwise ContinueTextRunAcrossFrames is not called)

-- In CharacterDataChanged and AdjustOffsetsForBidi, we now need to explicitly clear text runs for all in-flows: their text runs may no longer correspond to reality and calling ClearTextRun on the first-in-flow will only kill its textrun, not any other textruns which may be owned by next-in-flows.

Comment 5

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

Attachment: textrun memory-usage instrumentation

This lets us capture the (approximate) high-water-mark of textrun memory usage, which is very useful for measuring this sort of testcase.

Comment 6

[Stuart Parmenter]

Comment on attachment 270278 [details] [diff] [review]
textrun memory-usage instrumentation

please add 2 more spaces to the second line here:
+    if (aTextRun->GetFlags() & gfxTextRunFactory::TEXT_IS_PERSISTENT) {
+      bytesPerChar += (aTextRun->GetFlags() & gfxTextRunFactory::TEXT_IS_8BIT) ? 1 : 2;

Comment 7

[Jesse Ruderman]

Please make that DEBUG_roc or something.  Most developers don't need to see textrun stats.

Comment 8

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

OK I will, although it's only 1 line of output.

Comment 9

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

Checked in the gfx patch accidentally as part of the patch for bug 9101...

Comment 10

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

Checked in the layout patch.

Comment 11

[Steve England [:stevee]]

Could this have caused bug 386584?

Comment 12

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

Yes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

roc, would it make sense to only clear some text runs in the append case?  Or is figuring out what the first impacted frame is too much pain?

Comment 14

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

Actually I think clearing all the text runs is usually a good idea. During a big load we won't need most of those text runs again; clearing them early helps free up memory. (Some of them won't be released because they're associated with cached words, but others will go.)