128572 - CPU usage jumps to 100% when loading a page with wrong charset

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Koike Kazuhiko]

Large HTML file with wrong charset specification consumes CPU too much.

Testcase:
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=587

This file is in Shift_JIS but specified as "charset=iso-2022-JP".


Original report in Bugzilla-jp:
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=1939

Comment 1

[Dimitrios]

It seems that you must have Japanese fonts installed to use the testcase.
Otherwise, I get the download font prompt and, if I press cancel, nothing is
loaded. I ll try to testcase in my languagae.

Comment 2

[Dimitrios]

Well, I tried and failed to reproduce a charset depended behaviour on Win98,
2002030103. Unless there is a specific situation with Japanese fonts, I think
the problem you see is due to the png image background in the "mozillagumi" div
(bugs 64401 and 64188). IE 5.5 is even more slow responding than Mozilla on this
testcase. Suggesting to remove the png from it and test again.

Comment 3

[Yuying Long]

I can not reproduce it on linux RH7.2/02-26 trunk build.

Comment 4

[Roy Yokoyama]

It is indeed very hard to find out what's really happening.
I've visited mozilla.gr.jp site for the original report to
see if I can dig up more info.

While I was monitoring the CPU usage on my 
WinXP-Simplified Chinese (Auto-Detect OFF), 
I observed the followings:

Test 1) http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=587
The given test case (iso-2022-JP) peaked out to about 80%.

Test 2) http://www.mozilla.gr.jp/docs/mozilla_de_go.html
Incidently, I tried the page with correct charset (Shift_JIS)
and peaked out to about 70%

It appears that having a wrong charset increases the CPU usage.

FYI, http://bugzilla.mozilla.org/show_bug.cgi?id=128572
_this_ bugzilla page (no charst meta) peaked out to 100% for
about 0.5 sec.   Whyyyyyyyyyyy ???

assign to smontagu and cc'ing shanjian

Comment 5

[Simon Montagu :smontagu]

A few months ago I saw comparative page-loading data that suggested that
iso-2022-JP conversion is much slower than shift_JIS. At the time I didn't
succeed in getting a handle on the reason for this, but I can go back to it.

As a test for this hypothesis, could someone examine the reverse case? i.e. a
page in iso-2022-JP with the charset incorrectly specified as shift_JIS.

Comment 6

[Koike Kazuhiko]

Attachment: Testcase #1

written in ISO-2022-JP, specified as Shift_JIS

Comment 7

[Koike Kazuhiko]

Attachment: Testcase #2

written in Shift_JIS, specified as EUC-JP

Comment 8

[Yuying Long]

I can reproduce the test case #2 in comment #7 on my linux 7.2 with default
locale set to ja_JP.eucJP.

Comment 9

[Frank Tang]

>peaked out to 100% for about 0.5 sec.   Whyyyyyyyyyyy ???
because it convert to some unicode code point which do not have a font on the
system. 
ylong said she saw 1-5 second hang (which is not too bad).

Comment 10

[Shanjian Li]

accept. 

Comment 11

[Frank Tang]

katakai, can you take this bug? it look a performance tuning issue for linux and
solaris in general.

Comment 12

[Masaki Katakai]

Attachment: patch, just return mWesternFont at UCS2_NOMAPPING char [CHECKED-IN]

Comment 13

[Masaki Katakai]

I've attached proposed patch, can anyone review?

Even when such invalid char (UCS2_NOMAPPING 0xfffd)
is passed to FindFont(), nsFontMetricsGTK::FindFont() tries
to search by the following,

  nsFontGTK* font = FindUserDefinedFont(aChar);
  if (!font) {
    font = FindStyleSheetSpecificFont(aChar);
    if (!font) {
      font = FindStyleSheetGenericFont(aChar);

Checking UCS2_NOMAPPING codes are now located in the end of 
FindStyleSheetGenericFont(). So FindFont() always tries to
find possible font in FindUserDefinedFont()->FindStyleSheetSpecificFont()
->FindStyleSheetGenericFont() even if aChar is UCS2_NOMAPPING.
That is the reason of this bug.

I'm thinking the checking should be the top of the FindFont().

Comment 14

[kill this account]

Katakai: I believe your analysis is correct on the cause. I put the test later
in the code so that a test for an invalid page did not slow down valid pages. 

Of course I never measured the cost, it just seemed like the right thing to do.
Beyond this I don't have strong feeling either way.

Do you have any idea if this patch will have a measurable effect on speed?
 

Comment 15

[kill this account]

Specifically, my question is what performance impact this will have on "normal" 
(correct charset) cases. I trust you to have tested that this helps "abnormal" 
cases.

Comment 16

[Masaki Katakai]

I understand what you're saying.  How about this? Put the following codes
to Init(). We can get time of FindFont('a') executing 1000*1000 times.
It simply uses 'a' but I believe it's good enough because we need to get
the cost of "if (aChar == UCS2_NOMAPPING) {" line.

  static long started_time = 0, current_time ; // in msec
  struct timeval now;
  gettimeofday(&now, NULL);
  started_time = now.tv_sec*1000 + now.tv_usec/1000;

  //mWesternFont = FindFont('a');
  for(int i=0;i<1000;i++) {
    for(int j=0;j<1000;j++) {
      FindFont('a');
    }
  }

  gettimeofday(&now, NULL);
  current_time = now.tv_sec*1000 + now.tv_usec/1000;

printf("%ld\n", (current_time-started_time));

Here are the results on my linux box. I run the problem
30 times and ignore the fastest two and slowest two,
then take average.

2154.275862 msec - before patch
2156.862069 msec - after patch

These are results of 1000*1000 times and we can find that
there is no critical difference between before/after the patch.

Comment 17

[Masaki Katakai]

Brian,

Any comments?

Could you r= please?

Comment 18

[kill this account]

does this patch affect the page-loader test times?

http://www.mozilla.org/performance/startup-perf-brownbag.html
Pageload: http://cowtools.mcom.com/page-loader/loader.pl

Comment 19

[Masaki Katakai]

I think I don't have permission to access mcom.com.

Can I get file from you? or should I ask Netscape people?

Comment 20

[kill this account]

Comment on attachment 92664 [details] [diff] [review]
patch, just return mWesternFont at UCS2_NOMAPPING char [CHECKED-IN]

I checked and it does not appear to slow page loading

r=[e-mail]

Comment 21

[kill this account]

Katakai: 

I have been thinking about this and have a couple of questions:

1) Would it be easier just to add UCS2_NOMAPPING to the tranliterator?
This way it would "found" be in the font list we would not call FindFont 
everytime we see it. If this works we might open a follow up bug to
put "not found" chars into the transliterator so we only look for them once.

2) If we decide to return mWesternFont could you check that it does not get
inserted into the font list more than once?

Comment 22

[Simon Montagu :smontagu]

Removing nsbeta1+ per intl bug triage meeting. This seems like a minor
performance issue which will only arise on pages with errors.

Comment 23

[rbs]

Comment on attachment 92664 [details] [diff] [review]
patch, just return mWesternFont at UCS2_NOMAPPING char [CHECKED-IN]

sr=rbs

Comment 24

[Masaki Katakai]

patch checked into Trunk, thank you for review and super-review.

Comment 25

[rbs]

Did you verify point 2 in comment 21? You might possibly also need to set the
UCS2_NOMAPPING char as representable in mWesternFont
(nsCompressedCharMap::SetChar()) to avoid that multiple insertion problem -- if
the problem is there.

Comment 26

[Masaki Katakai]

2) I understand multiple insertion problem does not occur because
the font is added to mLoadedFonts after UCS2_NOMAPPING()
in FindFont().

1) I'm sorry I don't understand well what the transliterator is and
how to follow up. Brian, could you explain more detail or example?

Comment 27

[kill this account]

Add UCS2_NOMAPPING to the western font really means adding to
any (or every) real font.

While it is possible to add UCS2_NOMAPPING to every real font
this is solving the problem at the wrong protocol layer. This
solution would make the encoding converters solve an efficiency
issue in the X font system.

The "right" place is in the substitute (transliterator) font but
this is only loaded after all other fonts are tried. There might
still be an efficiency issue so this would need to be tested

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsGTK.cpp#6029
6029 nsFontMetricsGTK::FindSubstituteFont(PRUnichar aChar)
6030 {
6031   if (!mSubstituteFont) {
6032     for (int i = 0; i < mLoadedFontsCount; i++) {
6033       if (CCMAP_HAS_CHAR(mLoadedFonts[i]->mCCMap, 'a')) {
6034         mSubstituteFont = new nsFontGTKSubstitute(mLoadedFonts[i]);
6035         break;
6036       }
6037     }
6038     // Currently the substitute font does not have a glyph map.
6039     // This means that even if we have already checked all fonts
6040     // for a particular character the mLoadedFonts will not know it.
6041     // Thus we reparse *all* font glyph maps every time we see
6042     // a character that ends up using a substitute font.
6043     // future work:
6044     // create an empty mCCMap and every time we determine a
6045     // character will get its "glyph" from the substitute font
6046     // mark that character in the mCCMap.
6047   }
6048   // mark the mCCMap to indicate that this character has a "glyph"
6049 
6050   // If we know that mLoadedFonts has every font's glyph map loaded
6051   // then we can now set all the bit in the substitute font's glyph map
6052   // and thus direct all umapped characters to the substitute
6053   // font (without the font search).
6054   // if tried all glyphs {
6055   //   create a substitute font with all bits set
6056   //   set all bits in mCCMap
6057   // }
6058 
6059   return mSubstituteFont;
6060 }

A lower quality (but more efficient) alternate place would be in
the double byte special chars font. This class prevents wide
versions (eg: Japanese) of certian chars (eg: smart quotes) from
being used in western documents since the visual match is so
poor.

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsGTK.cpp#5927
5927       // add the early transliterator
5928       // to avoid getting Japanese "special chars" such as smart
5929       // since they are very oversized compared to western fonts
5930       nsFontGTK* sub_font = FindSubstituteFont(aChar);
5931       NS_ASSERTION(sub_font, "failed to get a special chars substitute font");
5932       if (sub_font) {
5933         sub_font->mCCMap = gDoubleByteSpecialCharsCCMap;
5934         AddToLoadedFontsList(sub_font);

To add the UCS2_NOMAPPING to this add it to the
gDoubleByteSpecialCharsCCMap ccmap.

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsGTK.cpp#635
635 static const PRUnichar gDoubleByteSpecialChars[] = {
636   0x0152, /* LATIN CAPITAL LIGATURE OE */

Comment 28

[Yuying Long]

I don't see the CPU jump to 100% on 12-26 trunk build / linux RH7.2, mark this
as verified.  Please re-open or open a new bug if there still some problems.

Comment 29

[Roland Mainz]

Somehow we missed to port this to gfx/src/xlib... ;-(

Comment 30

[Roland Mainz]

Taking myself...

Comment 31

[Roland Mainz]

Attachment: Patch for 2003-01-10-08-trunk (attachment 92664 ported to gfx/src/xlib) [CHECKED-IN]

Comment 32

[Roland Mainz]

Patch checked-in
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1042686480&maxdate=1042686900&who=timeless%25mozdev.org),
marking bug as FIXED.