Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061022 Minefield/3.0a1 After the patch for bug 339513 landed, it's basically impossible to load pages that have large amounts of Chinese (and other?) text in them. Steps to reproduce: 1. Load testcase in a trunk cairo build. The exact regression range is between 2006-09-25-04 and 2006-09-26-04, so I'm not 100% bug 339513 is to blame, just pretty sure. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-25+03&maxdate=2006-09-26+05&cvsroot=%2Fcvsroot In a 2006-09-25 build, loading the attached testcase takes < 30 seconds. In a 2006-09-26 build it takes over 3 minutes with CPU at 100% the entire time.

So the profile sez: Total hit count: 341650 292109 of them under FontSelector::FontSelector, of which 261546 are under FontSelector::AppendCJKPrefFonts. Looking at the code in bug 339513, let me see if I understand this correctly: For every single text measurement operation that fails to come out with a lang in MeasureOrDrawFast or comes out with a negative width, we bail over to MeasureOrDrawItemizing, where we construct a brand-new FontSelector every single time? Shouldn't we at least cache FontSelector objects? Or something? Note that this might be a worse problem if the OS has a lot of fonts available, if I understand the code correctly...

(In reply to comment #2) > For every single text measurement operation that fails to come out with a lang > in MeasureOrDrawFast or comes out with a negative width, we bail over to > MeasureOrDrawItemizing, where we construct a brand-new FontSelector every > single time? Yes, you're right. > Shouldn't we at least cache FontSelector objects? Or something? I think that the caching of FontSelector is not easy. Because that has optimized font list for each segment of the text. However, now, we have textrun cache, I think that we can cache the computed family and glyph id for each grapheme clusters for the string. See bug 356695 comment 2. > Note that this might be a worse problem if the OS has a lot of fonts available, > if I understand the code correctly... Yes. Same issue has on Windows. But it's not only for thebes. The same issue already has old gfx for Win. I have some plans for better performance for intl text rendering on Linux. I'm working on it. Please wait for two or three months for finishing my work.

Um... The problem is that we want to ship an alpha of Gecko 1.9 in the next month at most. And this is bad enough to possibly be an alpha blocker.... This is definitely a release blocker, imo. Out of curiousity, does it really take multiple minutes of completely locked up browser to load the testcase for this bug on Windows without Thebes GFX? Another thought. Even if you can't cache the FontSelector, perhaps you can cache the parts that the FontSelector gets from the OS, since those seem to be the slow ones here? Well, that, and pref caching, etc, etc. This code is on the critical path performance-wise; every single instruction here counts.

Yeah, we need to fix the performance regressions, but the current code is not correct for the font selection. See bug 352174. I'm working on this, and I'll work for the better performance after current work.

Is bug 352174 more of a blocker than this bug for alpha testing?

bug 352174 is changing the font selection mechanism. I think that the 'correct' font listing mechanism is needed before this. The current code breaks alias font names that is default settings of Fx.

(In reply to comment #4) > Out of curiousity, does it really take multiple minutes of completely locked up > browser to load the testcase for this bug on Windows without Thebes GFX? On my 6-year old machine (P750, 512MB RAM), it takes firefox 1.5/2.0 about 25 seconds (or shorter) to load and render the test case uploaded in comment #1. While loading it, both Windows and firefox (other tabs) are functional and responsive. I guess I have more fonts than ordinary Windows users (300 TTFs and 200 FON fonts). However, I don't have that many East Asian fonts (about 30), which may make the difference between me and Masayuki.

*** Bug 362997 has been marked as a duplicate of this bug. ***

This patch doesn't fix the perf issue as completely. But this patch wins in many cases. I hope that we use this until coming the new text frame.

I would prefer that we land my textrun patch with the new textframe disabled, which has the same effect as this patch.

(In reply to comment #11) > I would prefer that we land my textrun patch with the new textframe disabled, > which has the same effect as this patch. > If we can do it, I agree to the idea. Because the new cache of the patch is better than my patch.

Roc: Can you attach the patch without the new text frame?

With the patches in bug 333659, you can select between old textframe and new textframe just by changng MOZ_ENABLE_NEW_TEXT_FRAME in configure.in. Actually the latest patch attached there doesn't do this because I left out a couple of files from the diff :-(. I'll attach a new patch there shortly when I update the new textframe after the reflow branch landing.

Thanks, Roc. Vlad: We should use this patch between the short time but for some pseudo hanging up.

checked-in the patch.(attachment 247983 [details] [diff] [review]) Should we open this bug? The patch doesn't help in *all* cases.

somebody: how about the performance on your system?

It's so slow that it's useless for normal surfing. it completely halts the browser for several seconds, even up to a minute on some pages. pages with only english characters loads pretty much instantly

I measured the performance. |pango_shape| is very expensive if the font doesn't have all glyph. I think that we should cache the lang info for each fonts. And we should check the font before calling the pango_shape. I'll create the patch.

This patch initializes the segments without the pango_shape if it is possible. I believe that this patch makes faster rendering. But I could not confirm it. Because pango may be caching the shaping information at first time. So, the testing is difficult. And this patch removes the |MeasureOrDrawFast|. Because the function fails in very many cases, the cause is bug 352174 that resolves the generic family name to multi fonts. So, in many cases, the first font doesn't have non-ASCII characters. (The ASCII only text is not drawn by gfxPangoTextRun.)

Oops, sorry, I removed the unused variable.

I'm testing the patch. I think that this patch may not make faster rendering in daily use. But in special cases, such as the case of hung up, this patch makes faster rendering in current build.

I found a bug.

This patch has great performance if non-UTF-8 contents are rendered as UTF-8 document by Auto Detect. When such a case, there are very many U+FFFD. But the previous patch decides that the case needs shaping. This patch checks it correctly.

This is more readable than the previous patch.

I don't think restricting shaping to certain scripts is a good idea (assuming that's what the patch is actually doing), for one thing the list in the patch omits a number of scripts that require shaping (Syriac, Mongolian, Phags-pa, N'Ko...) and more will surely be added with each new Unicode version, and even scripts that don't normally shape can have fonts with ligatures.

In principle, I agree with Justin. In practice, we need to do something now to speed things up. Btw, instead of using multiple if-statements, using 'compressed charmap' may save us some cycles (while increasing the memory footprint). In addition to scripts named by him, U+1100-U+11FF (and U+AC00 ... when combined with them) also need shaping (ok. at this point, Pango doesn't support shaping them). So do Latin diacritic marks. A recent version of Pango supports diacritic marks with Latin, Cyrillic and Greek.

I think that we can ignore the 'simple' complex that is ligatures. Because in the final step before rendering, the pango_shape is called for *all* text. But I'll add the other languages for you added them, thanks.

Updating. I have a question. Does this patch break any testcases? I want the testcase for non-complex script. (This patch doesn't break "data:text/html,A&#x300;" in my environment.)

I think that the previous patch was misread by someone, that is not good thing. Therefore, I rewrite the patch more simply. And this patch boost up in complex script too.

Justin and Jungshik: Do you have some opinions for latest patch?

It's still missing Syloti Nagri and Thaana. Why is Hebrew listed? If it's just because it's RTL, there are other non-complex RTL scripts like Phoenician and Cypriot. If it's because there can be combining accents, then many other scripts like Latin would need to be included as well (or just include the Combining Diacritical Marks blocks). There are also some typos: Kannda should be Kannada, Malayalm should be Malayalam, and Presentaion should be Presentation (in two places). I can't really evaluate the code part of things.

Comment on attachment 250158 [details] [diff] [review] removing the redundant shaping for many languages patch rv2.0 This looks OK, but please check with roc -- he's going to try to land his textrun code soon, and I'm not sure how your work and his will conflict.

roc: This patch changes the initialization of FontSelector, can I check-in this patch before the new text frame? I think that this patch fixes some hung-up cases, so, this is not only for perf regressions. ( And I think that the patch can be imported to your patch easily.) Justin: Thanks for the comment. I'll fix the nits before checking-in.

Merging it is going to be a little bit of work but not a big problem. So go ahead and land.

Thank you, Justin, vlad and roc.

Oops, sorry for the spam.

checked-in the second approach.

Was that patch supposed to fix what I reported in bug 362997? It is not fixed.

(In reply to comment #39) > Was that patch supposed to fix what I reported in bug 362997? It is not fixed. Really? The patch boost up the page rendering if the font is not matching in many fonts in the font list. E.g., if the 20th font is used for the rendering, the 1st font to 19th font testing are processed in very short time. If this patch doesn't affect in your environment, we need to boost up the 'normal' page rendering speed. I'm still thinking it. But I don't have idea, now.

dbaron: Can you attach the new Jprof Profile Report for current code?

dbaron said, the |pango_context_load_font| is expensive. I confirmed it. Sometimes, the |pango_context_load_font| is very slow. For suppressing it, I cache the pango font in hash table. Because PangoFont doesn't depend on the context. dbaron: Can you test this patch?

And the patch fixes a memory leak.

Are there any provisions for eviction from said cache?

(In reply to comment #44) > Are there any provisions for eviction from said cache? I cannot understand what you said...

This may be un-useful information, but going to the full-page display of the attachment (https://bugzilla.mozilla.org/attachment.cgi?id=243159), which supposedly hangs on some people's system (or at least is quite slow), take ~ 2 seconds (according to "fasterfox" timer). Charset is set for Universal auto-detect with UTF-8 being autoselected. I have 856 fonts (including severarl asian fonts). What am I doing 'wrong', to not get the delay? I am running on a 2GHz Core Duo processor.

fix some nits. (not changed the logic.)

law: thank you for you test. (In reply to comment #46) > This may be un-useful information, but going to the full-page display of > the attachment (https://bugzilla.mozilla.org/attachment.cgi?id=243159), which > supposedly hangs on some people's system (or at least is quite slow), take ~ 2 > seconds (according to "fasterfox" timer). How much the current trunk score? > Charset is set for Universal > auto-detect with UTF-8 being autoselected. I think that the universal auto-detect has performance problem, because it is rendered with non-conclusion encoding once. The broken rendering is very slow in old gfx and Win32 too. > I have 856 fonts (including severarl asian fonts). The current linux code doesn't depend on the count of fonts. But if the alias name (e.g., "sans") links to very many fonts in fontconfig, the performance is very bad...

> I cannot understand what you said... Are things ever removed from the cache? Or does it just grow for the lifetime of the application as new font combinations (sizes, etc) are encountered?

(In reply to comment #49) > > I cannot understand what you said... > > Are things ever removed from the cache? Or does it just grow for the lifetime > of the application as new font combinations (sizes, etc) are encountered? If the cache entry is over 5,000, then the cache is cleared. This is same as Mac.

Ah, ok. Excellent.

We can use PangoFont cache in other places. And we can remove the itemizing in GetSize mothod.

This patch has better performance. This patch has sample font cache that is cached per font-family. Therefore, it independents from size, weight and style. But I think that it is enough for checking glyphs. But in principle, it is *not* correct. I think that we should create pango independent glyph checking. (e.g., ccmap) But it is not a scope of this bug.

Oops, sorry, the previous patch has a debug code.

Vlad: I'm testing the new gfxPangoFonts. I think that we need the latest patch for it too. Because the new gfxPangoFonts still has this issue. Would you review the patch? I'll work for merging in bug 333659.

Comment on attachment 251375 [details] [diff] [review] caching the pango font rv2.0.1 I'll attach the new patch for latest trunk.

This patch includes the attachment 250580 [details] [diff] [review]. Because the patch is kicked out by bug 333659. Note: 1. gfxPangoFont should cache the PangoFont* in mPangoFont. 2. gfxPangoFont should unref mPangoFont at destroying. 3. gfxPangoFont::GetSize should not use pango_itemize. We should init the PangoAnysis ourselves. (Because it loads the PangoFont, it's expensive.) 4. CreateGlyphRunsFast should not be used for non-ASCII text. Because by bug 339513, the first font of the generic family fails in many languages. But for ASCII text, this fast pass is good way. It will be enabled by nsTextFrameThebes. 5. Now, we should always check whether the glyph is not missing glyph after shaping in FontSelector. By this we can cut the cost for checking the string has complex. We have only a little cost for the checking in SetGlyphs. 6. The sample font for gfxPangoFont::HasGlyph is not best way. HasGlyph should use FontConfig which should be cached in gfxPlatformGtk. It should be improved after bug 366285 and bug 366664.

Updating to latest trunk. And removing the CreateGlyphRunsFast, because we are using XFT for ASCII text, therefore, the Fast method fails in many languages if the font family is generic family.

+ PRBool AppendMissingSegment(PRUint32 aUTF8Offset, PRUint32 aUTF8Length) { + if (aUTF8Length == 0) + return PR_TRUE; + + PangoGlyphString *glyphString = pango_glyph_string_new(); + if (!glyphString) + return PR_FALSE; + const char *start = mString + aUTF8Offset; + const char *last = start + aUTF8Length; + + PRUint32 numGlyphs = 0; + for (const char *c = start; c < last; numGlyphs++) + UTF8CharEnumerator::NextChar(&c, last, nsnull); + pango_glyph_string_set_size(glyphString, numGlyphs); Please be aware that glyphString->glyphs and/or glyphString->log_clusters might be NULL after pango_glyph_string_set_size() is called. Behold: void pango_glyph_string_set_size(PangoGlyphString *string, gint new_len) { g_return_if_fail (new_len >= 0); while (new_len > string->space) { if (string->space == 0) string->space = 1; else string->space *= 2; if (string->space < 0) g_error("%s: glyph string length overflows maximum integer size", "pango_glyph_string_set_size"); } string->glyphs = g_realloc(string->glyphs, string->space * sizeof(PangoGlyphInfo)); string->log_clusters = g_realloc(string->log_clusters, string->space * sizeof (gint)); string->num_glyphs = new_len; } gpointer g_realloc(gpointer p, gulong size) { gpointer n; if (size == 0) { gm_free(p); return NULL; } n = gm_realloc(p, size); if (n) return n; g_error("re-allocation of %lu bytes failed", size); return NULL; } Nice, huh? Note how they still set 'num_glyphs' even if the allocation fails. Note how they fail to check "string->space * sizeof(PangoGlyphInfo)" for integer overflow.

updating to latest trunk, this patch will die. But some testers may need this patch until then...

the latest patch checking the issue of comment 59. But it will crash at using the cached glyphs...

(In reply to comment #60) > Created an attachment (id=255240) [details] > caching the pango font rv3.2 > > updating to latest trunk, this patch will die. What about the dependent bug 368996?

(In reply to comment #62) > What about the dependent bug 368996? I don't find the gap in the testcase of bug 368996 even if I don't apply the patch. But in Arabic CNN, I can find the gaps in heading text with patched build...

updating to latest trunk. and for the nightly testers who builds it themselves.

+ if (!glyphString->glyphs || !glyphString->log_clusters) + return PR_FALSE; Nit: add "pango_glyph_string_free(glyphString);" before return +gfxPangoFontCache::Put(const PangoFontDescription *aFontDesc, ... + gfxPangoFontWrapper *value = new gfxPangoFontWrapper(aPangoFont); + mPangoFonts.Put(key, value); I think you need to return before Put if 'value' is null. Otherwise gfxPangoFontCache::Get will crash for this key. Ditto for gfxSamplePangoFonts::Put.

Updating for latest trunk. (and changing the points which was suggested in previous comment.)

Comment on attachment 260418 [details] [diff] [review] caching the pango font rv3.4 This patch is obsolete. And I found some bugs in this patch.

changing from rv3.x: 1. fix a crash bug at setting the missing glyphs 2. speed up the setting of the missing glyphs 3. fix a wrong separation at the run needs multiple fonts 4. remove to check whether the shaping is failed, I think that they should be always succeeded. Because each characters checking whether the font has the glyph before shaping. I think that the checking should work fine for complex scripts too. 5. now, using gnome APIs for UTF-8 processing

6. fix the leaks at shutdown.

fix a nit.

Vlad: I'm still waiting your review... We can get the many merits by this patch: 1. simple code for font switching. 2. faster performance for the mixed fonts text. 3. fix some crash at shaping the complex scripts.

pango_glyph_string_free should probably be called when AddGlyphRun fails in AppendSegment.

You're right. Thank you.

(In reply to comment #58) > removing the CreateGlyphRunsFast, because we are > using XFT for ASCII text, therefore, the Fast method fails in many languages if > the font family is generic family. At some stage we may wish to use Pango instead of Xft for ASCII/8BIT text (kerning, ligatures), so I think it would be good to leave an option for a Pango 8BIT path, assuming its faster than Itemizing.

I don't think so. Because the |CreateGlyphRunsFast| can treat the non-ASCII text, but the process fails many times if the first font doesn't have all characters. And we can treat the generic alias font name (e.g., san, serif), therefore, the method is not good solution for most languages. I think that if ASCII users need the fast path in option, we should create the new method only for ASCII, it should not try to process the non-ASCII text.

Comment on attachment 263238 [details] [diff] [review] caching the pango font rv4.1 Looks good, sorry this review took a while! Only thing I'd change is the naming of 'gfxSamplePangoFonts'; maybe call it 'gfxPangoFontNameMap' or something, and maybe change mSampleFont to mGlyphTestingFont to make it clear what it's being used for. Other than that, r=me if all reftests pass.

checked-in the patch.

backed-out the patch.

the patch has a problem for rendering the zero width characters. I'll retry to fix it.

The zero-width characters will be missing glyphs at *shaping*. I don't know the reason. But we can override the glyph ID and the width after shaping. This clears the 5 reftests.

Oops, sorry, the previous patch is wrong file.

Er, I found the code in pango. pango set the 'empty glyph(0xFFFFFFFF)' to zero width characters which is defined in pango_is_zero_width. Therefore, the SetGlyphs doesn't work fine. I'll attach better patch, please wait a moment.

I think that this is a best way. We cannot use pango_is_zero_width, because it is implemented after 1.10. I think that if the glyph ID is empty glyph and the width is zero, it is a zero width characters in pango_is_zero_width. Therefore, we should replace the glyph in such case.

Comment on attachment 263598 [details] [diff] [review] caching the pango font rv4.3 Hmm, ok.. at some point soon the minimum pango version will jump (to at least 1.14, if not 1.16), so we may be able to take advantage of the new pango function. I assume this fixes the reftest failures for you locally as well? If so, r+.. if not, it would be good to have you figure out why you're still seeing failures (either before or after the patch).

thank you, checked-in. the patch cleared the reftests in my environment.

backed-out. Why the reftests are failed???

vlad: I want to know the version of pango of "Linux qm-rhel02 dep unit test".

er, the old pango (1.11 and before) returns 0 for ZW*. but it cannot be cause of the failure...

(In reply to comment #88) > I want to know the version of pango of "Linux qm-rhel02 dep unit test". From the build logs, qm-rhel02 uses pango 1.0

(In reply to comment #90) > (In reply to comment #88) > > I want to know the version of pango of "Linux qm-rhel02 dep unit test". > > From the build logs, qm-rhel02 uses pango 1.0 > No, we cannot use pango 1.0 for gecko.

(In reply to comment #91) > > No, we cannot use pango 1.0 for gecko. > Hi, I filed bug Bug 379712 to fix pango on qm-rhel02.

> From the build logs, qm-rhel02 uses pango 1.0 Er... which part of the build log would this be? The only mention of pango version in the build log is -I/usr/include/pango-1.0, and that would be true for any 1.x.x pango. If that machine is our reference platform (which would be RHEL4.0), it has pango 1.6.0. That's the pango version we currently need to support.

I think that this patch is correct. but this needs pango 1.10.

I think that this patch should work fine with old pango. This patch renders the zero width character as zero width. The definition of zero width characters are same as pango 1.16.

I confirm that the patch works fine on CentOS4(pango 1.6.0). Let's land it until bug 379712, because this bug blocks my next work and roc's current work.

fix the wrong indent.

I've just checked in the patch for bug 379823, which may or may not help you. You could now use 'skip-if' on the reftests that failed on qm-rhel02 to only run the tests on windows and mac. See: http://lxr.mozilla.org/seamonkey/source/layout/tools/reftest/README.txt

Comment on attachment 263898 [details] [diff] [review] caching the pango font rv4.5.1 carry over r+ from prev patch.

the reftests are cleared. but remains: 1. Remove gfxPangoFontNameMap after bug 366664. 2. Use the pango_is_zero_width API instead of MOZ_pango_is_zero_width after bug 379712.

Is this going to make A5? If not, let's move please.

This was fixed a while ago, no? Should separate bugs be filed on comment 100, if needed?

Please update the status of this bug, RESOLVED FIXED? Moving this to A6. If this is actually fixed and in A5, please set the milestone to the correct value.

-> FIXED # I'll file a new bug for comment 100.