396732 - Serious performance regression in dealing with binary-data-as-text document on Mac

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This is the Mac version of bug 396726.

BUILD: Current trunk Mac build

STEPS TO REPRODUCE:
1)  Download https://bugzilla.mozilla.org/attachment.cgi?id=281427 and save it
    to a text file locally.
2)  Create an HTML document with a single <iframe width="100%" height="100%">
    in it that includes the text file (this allows timing the load in the HTML
    document).
2)  Open that HTML document in the browser.

EXPECTED RESULTS: Fairly quick rendering and painting

ACTUAL RESULTS: Initial rendering takes about 80 seconds on trunk (as compared to under a second on the 1.8 branch).  I cannot reproduce the tab/window switching issue reported in bug 390051 comment 12.

I profiled part of the load in the wonderful thing that is Shark.  The top-down profile says that 99.9% of the time is spent under gfxAtsuiFontGroup:InitTextRun and that 97.1% is spent under ATSUMatchFontsToText, called from InitTextRun.  The bottom-up profile says that:

  60.6% of the time is spent in TFontFallbacks::AddCharacterToCache
  20.3% of the time is spent in CFCharacterSetIsLongCharacterMember
   7.1% is spent in TFontFallbacks::LoadAllUncoveredChars
   3.3% is spent in CFCharacterSetIsCharacterMember

Basically, ATSUMatchFontsToText calls TfontFallbacks::FindFontFor which calls TFontFallbacks::LoadAllUncoveredChars, which splits as the above bottom-up data.

Comment 1

[Stuart Parmenter]

as filed, i don't believe this is really a blocker -- don't load binary documents as text.  if you have certain unicode documents that are really slow due to font fallback, those should probably block.

Comment 2

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

Try
http://mxr.mozilla.org/seamonkey/search?string=CreateWindowEx
which tries to output some binary data into HTML, and hangs the browser for a short time even though it's produced a relatively small document.

In an ideal world authors wouldn't do this, but we all know how far from an ideal world the Web is... I think this should block.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

There's an awful problem (still) where lots of servers are misconfigured to send .dmg (and sometimes .tar.gz) as text/plain, so we still run into an awful lot of cases where "download this Mac app by clicking here" ends up loading the entire .dmg as text instead, which kills our apps....

Comment 4

[Boris Zbarsky [:bzbarsky]]

renominating for the same reasons as the linux bug.  As comment 3 says, real users WILL hit this.

Comment 5

[Stuart Parmenter]

i don't think we would stop and not ship the release due to this bug.  do you really think we would?

Comment 6

[Boris Zbarsky [:bzbarsky]]

It's a hang regression that users will be hitting reasonably often.  Heck yes, I think we should stop and not ship due to this.  It's a much more serious user-facing issue (esp. for Mac users due to the .dmg mess) than a lot of things we're blocking on now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Put another way, I think hangs and crashes are pretty much equivalent from a user's perspective (if the user knows about force-quit; if not, a hang is worse).  If this were a crash regression, would you be blocking on it?

The binary file in this case is tiny, by the way.  For something more like a typical .dmg file, I wouldn't be suprised if the hang is effectively permanent, not just for a minute or two as here.

Comment 8

[Justin Dolske [:Dolske]]

FWIW, in the past I've run across this on servers sending media files with a bogus mimetype; you learn to see an AVI or RM header and desperately stab at the Stop button before it gets too far.

Comment 9

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

I sort of hope that John's work to port Stuart's Windows font selection code to Mac will fix this.

Comment 10

[philippe (part-time)]

(In reply to comment #7)
...
> The binary file in this case is tiny, by the way.  For something more like a
> typical .dmg file, I wouldn't be suprised if the hang is effectively permanent,
> not just for a minute or two as here.
> 
The other day, while Mozilla's FTP servers were on the move/reorganising/..., the .htaccess had gone missing (tracked in bug 397904). Both Camino trunk and Minefield chocked badly when trying to download the latest nightly: hang requiring force quit.

Comment 11

[Damon Sicore (:damons)]

Minusing and adding wanted-1.9.  Hoping that the core text stuff will speed this up.  

Comment 12

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

Core Text will only work on Leopard so that's not an option for 1.9, even if it made sense risk and schedule wise, which I don't think it does.

Is jdaggett still working on getting the Windows font selection code ported to Mac?

I still think this should block. It will be a source of frequent hangs for our users.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Just to make it clear, I agree 100% with everything roc said in comment 12.  In fact, I'd been going to say more or less the same thing.

As I said in comment 6, I think this is a much more serious problem than a lot of things that we _are_ blocking on.  I'm not going to renominate this yet again, but I think that not fixing this is a serious mistake.

Comment 14

[Mike Schroepfer]

If I take comment 3 at face value this means that real users, on the mac (which tends imho to be a vocal part of our users), will click on links out in the real world and their browser will hang.   That, combined with the fact that this is new and different from 1.8 means that imho we should block.   So I'm re-nominating.

Comment 15

[Stuart Parmenter]

schrep: if you feel this should block, please mark it as such.  vlad and I both feel that we would not hold the release for this bug so otherwise we'll just - it during our next triage session.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 17

[John Daggett (:jtd)]

Will try and deal with this as part of porting Windows font selection code to Mac, bug 396137, since the code that seems to be causing the slowdown is the ATSUI font matching code.  

Comment 19

[John Daggett (:jtd)]

Attachment: testpage to repeated load binary file, for profiling performance

Loads the testcase file and reports the load time, then pauses 1 second before loading the page.

Comment 20

[John Daggett (:jtd)]

Attachment: loadtest - load binary files of different sizes

Steps to use:

1. Download large files and save them as blah.dmg and blah.mov:

dmg file:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006/12/2006-12-01-05-trunk/firefox-3.0a1.en-US.mac.dmg

mov file:
http://www.crooksandliars.com/Media/Download/24617/2/abc_gma_cigna_health_protest_071221a.mov

2. Chop the files up into 1MB, 8MB and 16MB test files:

dd if=blah.dmg bs=1024 of=dmg1MB.txt count=1024
dd if=blah.dmg bs=1024 of=dmg8MB.txt count=8192
dd if=blah.dmg bs=1024 of=dmg16MB.txt count=16384
dd if=blah.mov bs=1024 of=mov1MB.txt count=1024
dd if=blah.mov bs=1024 of=mov8MB.txt count=8192
dd if=blah.mov bs=1024 of=mov16MB.txt count=16384

3. Download loadtest.html into the same directory as the test files

4. Click each button to perform a test load

Results:

trunk with 396137 patch:

dmg 1MB 4248 ms
dmg 8MB 30759 ms
dmg 16MB 62597 ms

mov 1MB 4891 ms
mov 8MB 34092 ms
mov 16MB 68157 ms

Comparison:

Firefox 2 dmg 1MB 33311 ms
Firefox 2 mov 1MB 30909 ms
Firefox 2 mov 16MB ** infinite loop ** (!!!)

Safari 3 mov 16MB 6134 ms (!!!)

44.1%	XUL	gfxAtsuiFontGroup::InitTextRun(gfxTextRun*, unsigned short const*, unsigned, int, unsigned, unsigned)	
19.1%	XUL	AppendPrefFonts(nsTArray<nsRefPtr<gfxFont> >*, eFontPrefLang, unsigned&, gfxFontStyle const*)	
6.3%	XUL	gfxQuartzFontCache::FindFontForChar(unsigned, gfxAtsuiFont*)	
5.3%	XUL	gfxAtsuiFontGroup::MakeTextRun(unsigned short const*, unsigned, gfxTextRunFactory::Parameters const*, unsigned)	
3.6%	XUL	gfxTextRun::SortGlyphRuns()	
3.3%	XUL	gfxTextRun::BreakAndMeasureText(unsigned, unsigned, int, double, gfxTextRun::PropertyProvider*, int, double*, gfxFont::RunMetrics*, int, gfxContext*, int*, unsigned*)	
1.2%	XUL	nsBlockFrame::ComputeCombinedArea(nsHTMLReflowState const&, nsHTMLReflowMetrics&)	
1.2%	XUL	TextRunWordCache::MakeTextRun(unsigned short const*, unsigned, gfxFontGroup*, gfxTextRunFactory::Parameters const*, unsigned)	
1.0%	XUL	nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&)	

Within InitTextRun:

80.1% 1682 ATSUGetGlyphBounds(layout, 0, 0, headerChars, aSegmentLength, kATSUseFractionalOrigins, 1, &trap, &trapCount); 			
5.0% 1505 ATSUCopyAttributes (mainStyle, subStyle);			
4.8% 1688 ATSUDisposeStyle(stylesToDispose[i]);			
4.6% 1514 ATSUSetRunStyle (layout, subStyle, offset, length);			
2.0% 1504 ATSUCreateStyle (&subStyle);			
1.3% 1511 ATSUSetAttributes (subStyle, 1, fontTags, fontArgSizes, fontArgs);			
0.5% 1684 ATSUDisposeTextLayout(layout);

Comment 21

[John Daggett (:jtd)]

I think there are two distinct issues here.  One is the overall performance of InitTextRun, something that will be addressed more in bug 409342.  But I also ran into one case where the browser completely hung on a 16MB file.  Sure, that's huge, but my concern is that there's a deadlock issue lurking around that really needs to get addressed.  If that's not reproducible, then I think 396137 and 409342 will resolve the performance regression issue.  Doing some more testing to see if I can reproduce the hang.

Comment 22

[John Daggett (:jtd)]

Attachment: patch, v.0.1, cache codepoints with no fonts

Using the cycle count patch attached to bug xxx, looked at the cycle times for various font matching routines.  With an optimized build, these are the cycle times for matching a single character with a font:

   font group font ==> 120cycles
   pref font ==> 1,400 cycles
   system-wide font search ==> 100,000cycles (!!!)

So any way we can avoid a system-wide search helps performance.  In the case of binary-as-text documents, there are lots of control characters and 0xFFFD's (unknown, caused transcoding error), so before searching system fonts, explicitly test and exclude control characters.  If the system-wide search fails, add that to the bitmap of codepoints that should fail immediately.

Comment 23

[John Daggett (:jtd)]

Testing with attached patch, along with additional patch from bug 409342.  Ran through tests described in comment 20, comparing current trunk to my patched perf build.  Times are all in ms. 

Western (ISO-8859-1)   
           trunk   patch speedup
dmg 1MB    4,943   1,986  2.49
    8MB   36,616  12,056  3.04
    16MB  75,438  18,551  4.07
mov 1MB    5,807   2,033  2.86
    8MB   42,408  11,545  3.67
    16MB  85,234  17,893  4.76

Japanese (Shift-JIS)
           trunk   patch speedup
dmg 1MB    8,553   7,383  1.16
    8MB   55,544  43,843  1.27
    16MB 112,621  87,591  1.29
mov 1MB    8,925   7,232  1.23
    8MB   59,160  43,278  1.37
    16MB 116,616  88,436  1.32

The patch drastically improves load time when the default encoding is set to Western, and makes it a tad better when the default encoding is set to Shift-JIS.  With the patch, our load time is better than FF2:

FF 2.0.0.11 (Western)
dmg 16MB 30,364
mov 16MB 28,791

We still lag behind Safari 3:

Safari 3 (Western)
dmg 16MB  8,000
mov 16MB  7,000

With the patch, font matching is roughly 10% of the total time spent in InitTextRun, so there's not much more to be gained by tweaking the font matching code for this case, it's probably better to poke around in the rest of the text rendering code to find more speedups.

I also tried a version of this patch using the LastResort font instead of just specifying a missing glyph range but this took twice as long to render so I don't think that makes sense.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 301630 [details] [diff] [review]
patch, v.0.1, cache codepoints with no fonts

Looks good, though this looks like the kind of thing we'd want to do on all platforms, no?

Comment 25

[John Daggett (:jtd)]

checked in