357637 - Loading time (Tp) of pages with Chinese text is unbearable

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Adam Guthrie]

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.

Comment 1

[Adam Guthrie]

Attachment: testcase (causes hang)

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 8

[Jungshik Shin]

(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. 

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Caching glyphs and fonts in text run rv1.0

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.

Comment 11

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

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

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Roc:

Can you attach the patch without the new text frame?

Comment 14

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

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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Thanks, Roc.

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

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

somebody:

how about the performance on your system?

Comment 18

[Bjorn]

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

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv1.1

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.)

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv1.2

Oops, sorry, I removed the unused variable.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv1.3

I found a bug.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv1.4

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.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv1.4.1

This is more readable than the previous patch.

Comment 26

[Justin Kerk]

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.

Comment 27

[Jungshik Shin]

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.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv1.5

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,À" in my environment.)

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv2.0

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.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Justin and Jungshik:

Do you have some opinions for latest patch?

Comment 32

[Justin Kerk]

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 33

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

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.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 35

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

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

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv2.1

Thank you, Justin, vlad and roc.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: removing the redundant shaping for many languages patch rv2.1.1

Oops, sorry for the spam.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in the second approach.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

dbaron:

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

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv1.0

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?

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

And the patch fixes a memory leak.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Are there any provisions for eviction from said cache?

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #44)
> Are there any provisions for eviction from said cache?

I cannot understand what you said...

Comment 46

[L A Walsh]

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.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv1.0.1

fix some nits. (not changed the logic.)

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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...

Comment 49

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Excellent.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv1.1

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

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv2.0

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.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv2.0.1

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

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 251375 [details] [diff] [review]
caching the pango font rv2.0.1

I'll attach the new patch for latest trunk.

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv3.0

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.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv3.1

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.

Comment 59

[Mats Palmgren (inactive)]

+    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.

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv3.2

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

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 62

[Simon Montagu :smontagu]

(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?

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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...

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv3.3

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

Comment 65

[Mats Palmgren (inactive)]

+        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.

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv3.4

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

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.0

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

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

6. fix the leaks at shutdown.

Comment 70

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.0.1

fix a nit.

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.0.2

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.

Comment 72

[Karl Tomlinson (:karlt)]

pango_glyph_string_free should probably be called when AddGlyphRun fails in AppendSegment.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.1

You're right. Thank you.

Comment 74

[Karl Tomlinson (:karlt)]

(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.

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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 76

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

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.

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.1.1

Comment 78

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in the patch.

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

backed-out the patch.

Comment 80

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 81

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.2

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.

Comment 82

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.2

Oops, sorry, the previous patch is wrong file.

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.3

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 85

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

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).

Comment 86

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

thank you, checked-in.

the patch cleared the reftests in my environment.

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

backed-out.

Why the reftests are failed???

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

vlad:

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

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 90

[Michael Ventnor]

(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

Comment 91

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 92

[Robert Sayre]

(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.

Comment 93

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 94

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.4

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

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.5

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.

Comment 96

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 97

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: caching the pango font rv4.5.1

fix the wrong indent.

Comment 98

[Jonathan Watt [:jwatt]]

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 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 263898 [details] [diff] [review]
caching the pango font rv4.5.1

carry over r+ from prev patch.

Comment 100

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 101

[Damon Sicore (:damons)]

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

Comment 102

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 103

[Damon Sicore (:damons)]

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.

Comment 104

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

-> FIXED

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