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

RESOLVED FIXED in mozilla0.9.6

Status

()

defect
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: ftang, Assigned: jbetak)

Tracking

({perf})

Trunk
mozilla0.9.6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Status: NEW → ASSIGNED
Keywords: perf
Blocks: 7251
per offline discussion with ftang - tentatively reassigning to myself
Assignee: ftang → jbetak
Status: ASSIGNED → NEW
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
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
I would expect these to be:

  en_US

  ja_JP.eucJP

  de_DE

not what is in the patch.
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.
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
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.


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 ?

Blocks: 103175
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 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)
Attachment #51981 - Flags: review+
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.

cc'ing Chris Blizzard for potential i18n sr.
change to m0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Use foo.Assign(NS_LITERAL_STRING("bar")) instead of
foo.AssignWithConversion("bar"). Make those changes, and sr=waterson.
Attachment #52292 - Flags: superreview+
Attachment #52292 - Flags: review+
Attachment #51981 - Attachment is obsolete: true
fix checked in - thanks everyone!
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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.
QA Contact: andreasb → ftang
You need to log in before you can comment on or make changes to this bug.