116030 - provide a way to identify langGroup/rang for a unicode char

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Shanjian Li]

In mozilla, if the glyph of a unicode character could not be found in local font and 
generic font, we will try global fonts (and preference fonts after bug 115392) rather
randomly. Loading each font and getting its cmap is very expensive, it really hurts 
our performance. We can narrow the font search range by identifying the langGroup 
(or range).

Comment 1

[Brendan Eich [:brendan]]

What milestone should this bug target?

/be

Comment 2

[Shanjian Li]

Target at 0.9.8. Patch on my local tree is almost done. 

Comment 3

[Frank Tang]

so you want a 
Unicode -> list of langGroup 
operation ?

Will this approach help performance of normal page after we fix 115392 ?
the FindPrefFont should already shorter the search list for most of the page we
care. If we want some tune base on the PRUnichar, we should use it to change the
order of using gCharsetInfo in the FindPrefFont
for example, if the PRUnichar is between 0x3400-0x9FFF we should try "ja",
"zh-CN", "zh-TW", "ko" first before other pref. 
if the PRUnichar is between 0xAC00-0xD7FF we should try "ko" first. 

Comment 4

[Frank Tang]

shanjian, I am not sure which page (give me the URL) will be benefit from this
tuning. (I think we may find some, but probably won't be the common case in the
top 100 eng and top 100 intl page). I think 115393 is more important. Because I
saw it on every (including English) page. 

Comment 5

[Shanjian Li]

Frank, 
It does not make sense to try cyrillic font for a chinese characters. Bug 115392
might reduce font search list to 50%, this bug should reduce it further. The 
range information can also be used to help reduce global font search list. We 
have language coverage information with each font. We should utilize this information
to narrow our search. 

I will address bug 115393 in 0.9.8. 

Comment 6

[Shanjian Li]

Attachment: patch

Comment 7

[Shanjian Li]

rbs, could you review my patch? thx!

Comment 8

[rbs]

Comment on attachment 67137 [details] [diff] [review]
patch

+  PRUint8 unicodeRange = FindCharUnicodeRange(aChar);
+  if (kRangFrequentNum > unicodeRange) { 

|kRangFrequentNum| is a rather uninformative variable name to be
tested the way you are doing

+    // spcific language is identified
typo: specific

+    AppendGenericFontFromPref(font.name,
LangGroupFromUnicodeRange(unicodeRange), 
+				 NS_ConvertUCS2toUTF8(mGeneric).get());
+  } else if (kRangSetLatin == unicodeRange) { 
+    // latin language set, try western and central european
+    // It will be a surprise if it omes here when mLangGroup is western or
central euro

typo: s/if it omes/to come/
(leaving other typos to brendan :-)

Index: nsUnicodeRange.cpp
===================================================================
RCS file: nsUnicodeRange.cpp

+ * Contributor(s):
+ *   Roger B. Sidje <[e-mail]>
+ *   Pierre Phaneuf <[e-mail]>

remnants of copy-paste in both .h/.cpp to be removed

+
+static PRUint8 UnicodeSubrangeTable[NUM_OF_SUBTABLES][SUBTABLE_SIZE] =

customary to prefix global variables with 'g'

+{ 
+  { // table for X---
+    kRangTableBase+1,	//u0xxx
+    kRangTableBase+2,	//u1xxx
+    kRangTableBase+3,	//u2xxx
+    kRangTableBase+4,	//uaxxx
+    kRangTableBase+5,	//udxxx  
+    kRangTableBase+6	//ufxxx
[...]
uncommented genuflexions here..., yet important to document what is going on:
+  if (range < kRangTableBase)
+    return range;
+  table = range - kRangTableBase;

might be more readable to just s/kRang/kRange/g everywhere

r=rbs

Comment 9

[Shanjian Li]

Attachment: update patch,

Comment 10

[Shanjian Li]

Comment on attachment 67436 [details] [diff] [review]
update patch,

carry over review

Comment 11

[Shanjian Li]

brendan, could you sr?

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 67436 [details] [diff] [review]
update patch,

>+  PRUint8 unicodeRange = FindCharUnicodeRange(aChar);
>+  if (kRangeSpecificItemNum > unicodeRange) {

Style nit: this makes visualizing a number line hard -- why not put the
constant on the right and use <?  Your call, I'm just wondering.  The dogma
that constants must go on the left of all operators doesn't work for me (it
comes from the fear that someone may typo variable == kFoo as variable = kFoo
in a condition, but we have compiler warnings for such things, and they're so
unlikely in practice, from what I can tell, that it seems a shame to obfuscate
*all* relational and equality ops).

>+    // a single language is identified
>+    AppendGenericFontFromPref(font.name, LangGroupFromUnicodeRange(unicodeRange), 
>+                                NS_ConvertUCS2toUTF8(mGeneric).get());

Style nit: here and below, maybe make the second line of actual parameters
indent so the params line up with the first param (start in the same column)?

>+  } else if (kRangeSetLatin == unicodeRange) { 
>+    // latin language set, try western and central european
>+    // It will be a surprise to come here when mLangGroup is western or central euro
>+    AppendGenericFontFromPref(font.name, "x-western",
>+                                NS_ConvertUCS2toUTF8(mGeneric).get());
>+    AppendGenericFontFromPref(font.name, "x-central-euro",
>+                                NS_ConvertUCS2toUTF8(mGeneric).get());
>+  } else if (kRangeSetCJK == unicodeRange) { 
>+    // CJK, we have to be careful about the order, use locale info as hint
>+    
>+    // try user locale first, if it is CJK
>+    if ((gUsersLocale != mLangGroup) && IsCJKLangGroupAtom(gUsersLocale)) {
>+      nsAutoString usersLocaleLangGroup;
>+      gUsersLocale->ToString(usersLocaleLangGroup);
>+      AppendGenericFontFromPref(font.name, NS_ConvertUCS2toUTF8(usersLocaleLangGroup).get(), 
>                                 NS_ConvertUCS2toUTF8(mGeneric).get());

Why the need for an nsAutoString here?	Why not just NS_ConvertUCS2toUTF8 on
the shared (not copied) result of gSystemLocale->GetUnicode?

>     }
>-    NS_IF_RELEASE(langGroup);
>-  }
>+    
>+    // then system locale (os language)
>+    if ((gSystemLocale != mLangGroup) && (gSystemLocale != gUsersLocale) && IsCJKLangGroupAtom(gSystemLocale)) {
>+      nsAutoString systemLocaleLangGroup;
>+      gSystemLocale->ToString(systemLocaleLangGroup);
>+      AppendGenericFontFromPref(font.name, NS_ConvertUCS2toUTF8(systemLocaleLangGroup).get(), 
>+                                NS_ConvertUCS2toUTF8(mGeneric).get());

Same question here.

>+PRUint8 FindCharUnicodeRange(PRUnichar ch)
>+{
>+  PRUint8 range;
>+  PRUint32 table = 0;
>+
>+  //search the first table
>+  range = gUnicodeSubrangeTable[table][ch >> 12];

Why not use 0 here instead of table?

>+  
>+  if (range < kRangeTableBase)
>+    // we got a specific range 
>+    return range;

(Nice < number-line order of operands here! :-)

>+
>+  // otherwise, we have one more table to look at
>+  table = range - kRangeTableBase;
>+  range = gUnicodeSubrangeTable[table][(ch & 0x0f00) >> 8];

Ok, I see this in the table of 7 sub-tables:

+  { //table for 0X--
+    kRangeSetLatin,	      //u00xx
+    kRangeSetLatin,	      //u01xx
+    kRangeSetLatin,	      //u02xx
+    kRangeTableBase+8,       //u03xx
+    kRangeCyrillic,	      //u04xx
+    kRangeTableBase+9,       //u05xx

Why won't those +8 and +9 terms cause an out-of-bounds array index?

>+const PRUint8   kRangeUnassgined           = 35;

Unassigned misspelled here.

/be

Comment 13

[Shanjian Li]

Attachment: update patch as suggested by brendan

Comment 14

[Shanjian Li]

Attachment: repost of last patch, (forgot to correct a misspelling there.)

Comment 15

[Brendan Eich [:brendan]]

Shanjian, those last two patches are identical to the one I reviewed.

/be

Comment 16

[Shanjian Li]

Attachment: repost patch, the last two are outdated ones.

Comment 17

[Shanjian Li]

Sorry brendan. I always copy patches to a spefic directory before I post it. 
A typo in my copy command cause it to be put to a different directory. Now it 
should be OK. 

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 67507 [details] [diff] [review]
repost patch, the last two are outdated ones.

I think using PRUint8 for the type of range, and for the return type of
FindCharUnicodeRange, may result in extra instructions being generated to widen
(and narrow).  I think unsigned int (or PRUintn for naming consonance) is
better -- same goes for the PRUint32 table; variable, which you could eliminate
as it is set once and used once.

sr=[e-mail], make the above change if it helps shrink compiled code
(compare object file sizes before and after).

/be

Comment 19

[Grey Hodge (jX)]

Attachment: comment clarification

As per Timeless in #mozillazine, the following comment:
+ // latin language set, try western and central european
+ // It will be a surprise if it omes here when mLangGroup is western or
central euro

has been changed to read more clearly, and now reads as follows:

+ // Latin language is now set, so we'll try western and central european
+ // If mLangGroup is western or central european, this most probably will not
be
+ // used, but is here as a fallback scenario.

Comment 20

[Shanjian Li]

Attachment: more comment clarification

Comment 21

[Shanjian Li]

Previous comment modification has some misunderstanding (caused by my poor 
English). Rewrote it.

to brendan,
Changing from PRUint8 to PRUint32 will increase the size from 110592 bytes to 
111104 bytes. The reason of choosing PRUint8 is to shrink the size of 
gUnicodeSubrangeTable. Comparison of optimized dll file shows that does make 
some difference. 

Comment 22

[Brendan Eich [:brendan]]

shanjian, I was *not* proposing that you change the type of any table from
PRUint8 to unsigned int.  I was *only* proposing that you change the type of
local variables loaded from such table elements.  Using narrow types for local
variables and arguments can be suboptimal and have undesired "wraparound" effects.

/be

Comment 23

[Shanjian Li]

Attachment: update, change locale variable from int8 to int32

Comment 24

[Shanjian Li]

brendan, is that what you suggested? There is no difference made with windows
compiler. This might not be true for other platform/compiler. Since there is 
no negative impact, I made the change. 

Comment 25

[Grey Hodge (jX)]

Attachment: comment-edited patch, code identical to last patch

While the code is identical to the previous patch, as per request from
Timeless, the comments are cleaned up to try to be as clear and correct as
possible.

Note to Shanjian: I wanted to make clear that I meant no insult towards your
english. I understand it's not your native language, and it's near impossible
to learn, but you seem to have an excellent grip on it. I was just
asked/volunteered to help clear it up a bit more. :)

Comment 26

[Shanjian Li]

Comment on attachment 67855 [details] [diff] [review]
comment-edited patch, code identical to last patch

carry over r/sr

Comment 27

[Shanjian Li]

fix checked in.

Comment 28

[Teruko Kobayashi]

Changed QA contact to [e-mail]. Shanjian, please verify this.

Comment 29

[rbs]

An errata was checked in for this bug in bug 184848.