97171 - startup perf- remove the need of loading of langGroups.properties files at startup time to speed up startup performance

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Frank Tang]

we currently load langGroups.properties at startup time, we should find a way to 
remove the need of loading it at startup time to speed up startup performance
we could 
1. build in common used pair in the cpp code, and/or
2. cache the resolved result in registry.

Comment 1

[[e-mail] (away - not reading bugmail)]

per offline discussion with ftang - tentatively reassigning to myself

Comment 2

[Suresh Duddi (gone)]

Stack trace for when this gets loaded: LookupLanguage("en-US"); en-US should be
probably hardcoded into the app.

(gdb) bt
#0  stat (
    __path=0x82255d8 "/home/dp/moz/mozilla/dist/bin/res/langGroups.properties",
__statbuf=0x8225714) at /usr/include/sys/stat.h:346
#1  0x401d5e2c in nsLocalFile::FillStatCache (this=0x8225708)
    at nsLocalFileUnix.h:100
#2  0x40177949 in nsLocalFile::IsDirectory (this=0x8225708, _retval=0xbfffdef8)
    at nsLocalFileUnix.cpp:1226
#3  0x40980ce0 in nsFileIO::GetInputStream (this=0x822b590, 
    aInputStream=0xbfffdfa8) at nsFileStreams.cpp:238
#4  0x40982be8 in nsFileTransport::OpenInputStream (this=0x822b4f0, 
    aTransferOffset=0, aTransferCount=4294967295, aFlags=0, aResult=0xbfffdfa8)
    at nsFileTransport.cpp:457
#5  0x409d7233 in nsFileChannel::Open (this=0x8225630, result=0xbfffdfa8)
    at nsFileChannel.cpp:245
#6  0x415b4962 in NS_OpenURI (result=0xbfffe020, uri=0x82265b0, ioService=0x0, 
    loadGroup=0x0, notificationCallbacks=0x0, loadAttributes=0)
    at ../../../dist/include/nsNetUtil.h:177
#7  0x415aed88 in nsLanguageAtomService::InitLangGroupTable (this=0x8212500)
    at nsLanguageAtomService.cpp:130
#8  0x415af282 in nsLanguageAtomService::LookupLanguage (this=0x8212500, 
    aLanguage=0x8101f20, aResult=0xbfffe380) at nsLanguageAtomService.cpp:177
#9  0x415aff15 in nsLanguageAtomService::GetLocaleLanguageGroup (
    this=0x8212500, aResult=0xbfffe5b0) at nsLanguageAtomService.cpp:285
#10 0x415af81c in nsLanguageAtomService::LookupCharSet (this=0x8212500, 
    aCharSet=0xbfffe698, aResult=0x820dbac) at nsLanguageAtomService.cpp:230

Comment 3

[[e-mail] (away - not reading bugmail)]

Attachment: patch v1.0 - the locale strings need to be verified on all supported major platforms

Comment 4

[[e-mail] (away - not reading bugmail)]

dp,

I apologize for dragging my feet with this. It seems like a small change, but 
I'll have to verify the platform locale strings for all major platforms and 
possibly make the ifs a little smarter. Dumping all language-language group 
pairs seems to be not a real option at this point, as i18n desires run-time 
extensibility. Looks like the goal is to remove the largest performance penalty 
and if that proves not to be enough continue with another push at the expense 
of runtime extensibility.

Comment 5

[kill this account]

I would expect these to be:

  en_US

  ja_JP.eucJP

  de_DE

not what is in the patch.

Comment 6

[[e-mail] (away - not reading bugmail)]

Attachment: correcting my "guesstimate" for the Japanese locale string - Japanese NT is returning ja-jp

Comment 7

[[e-mail] (away - not reading bugmail)]

Brian,

the locale string is already being converted to lowercase by the existing code, 
which makes only "ja_JP.eucJP" disputable. Japanese NT is returning ja-jp and I 
haven't checked Japanes Linux and Mac yet. Is it reasonably safe to assume that 
the locale string will be uniform across platforms. I'd like to avoid parsing 
the language ID out to minimize the performance penalty we are introducing for 
other language groups.

Cc'ing nhotta for the question on uniformness of platform locale strings.

Comment 8

[[e-mail] (away - not reading bugmail)]

oh, and I verified on German Linux (SuSe distribution) that the separator is in
fact a hyphen and not an underscore. Similarly based on the existing code I
think it's reasonable to expect an hyphen:

http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLanguageAtomService.cpp#197

Comment 9

[nhottanscp]

Platform locale is platform specific. For Macintosh, platform locale is not a
string but a collection of numbers (scriptCode, langCode, regionCode).
XP locale is cross platform, it is lang and country separated by a hyphen.

Comment 10

[kill this account]

If this is going into cross platform code then the current patches 
leave me very uncomfortable.

What if the locale is de_DE@Euro or de_DE.UTF8 or de_DE.UTF@EURO ?

Comment 11

[Frank Tang]

bstell the function jbetak is changing is
157 nsLanguageAtomService::LookupLanguage(const PRUnichar* aLanguage,
158   nsILanguageAtom** aResult)

The input is NOT a locale string, but a Language String, which should be in the 
format of "xx-yy" where "xx" is the two letter language code and "yy" is the two 
letter region code. This part is ensure by the platform specific locale code. If 
the input is not in that format, then it mean our platform specific locale code 
(which convert from platform locale name to xp language name) have bug and we 
should fix it there. 

The patch simply shortcut our tier one language to by passed those operation. 
The risk is later we may introduce bug in the non shortcut part without knowing 
them since most of the time we won't hit those code. 

Comment 12

[Frank Tang]

Comment on attachment 51981 [details] [diff] [review]
correcting my "guesstimate" for the Japanese locale string - Japanese NT is returning ja-jp

r=ftang
I know you are using -w -b to show the diff so
I won't see the indent differences
please make sure while you check in, the
else part is iindent correctly (which you probably already did in your tree)

Comment 13

[[e-mail] (away - not reading bugmail)]

thanks for explaining this Frank. I was looking into the locale code to
understand fully how it works, my feeling was that in LookupLanguage() we are
expecting a language-locale pair. 

Brian here is the only caller of LookupLanguage():

http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLanguageAtomService.cpp#300

and here is nsLocale.cpp, which I was trying to understand better:

http://lxr.mozilla.org/seamonkey/source/intl/locale/src/nsLocale.cpp#121

I apologize for all the confusion.

Comment 14

[[e-mail] (away - not reading bugmail)]

cc'ing Chris Blizzard for potential i18n sr.

Comment 15

[Frank Tang]

change to m0.9.6

Comment 16

[Chris Waterson]

Use foo.Assign(NS_LITERAL_STRING("bar")) instead of
foo.AssignWithConversion("bar"). Make those changes, and sr=waterson.

Comment 17

[[e-mail] (away - not reading bugmail)]

Attachment: Thanks waterson! Attaching revised patch to maintain audit trail.

Comment 18

[[e-mail] (away - not reading bugmail)]

fix checked in - thanks everyone!

Comment 19

[Andreas Becker]

Switching qa cantact to ftang for now. Frank, can this one be verified within
development or can you provide QA with a test case? Thanks.