Closed Bug 396732 Opened 14 years ago Closed 13 years ago
Serious performance regression in dealing with binary-data-as-text document on Mac
149.47 KB, text/plain
560 bytes, text/html
1.43 KB, text/html
4.81 KB, patch
|Details | Diff | Splinter Review|
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.
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.
Flags: blocking1.9? → blocking1.9-
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.
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....
renominating for the same reasons as the linux bug. As comment 3 says, real users WILL hit this.
Flags: blocking1.9- → blocking1.9?
i don't think we would stop and not ship the release due to this bug. do you really think we would?
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.
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.
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.
I sort of hope that John's work to port Stuart's Windows font selection code to Mac will fix this.
(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.
Minusing and adding wanted-1.9. Hoping that the core text stuff will speed this up.
Flags: blocking1.9? → blocking1.9-
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.
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.
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.
Flags: blocking1.9- → blocking1.9?
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.
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.
Depends on: 396137
Loads the testcase file and reports the load time, then pauses 1 second before loading the page.
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);
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.
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.
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 18.104.22.168 (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 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?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.