Closed Bug 97175 Opened 23 years ago Closed 23 years ago

startup perf- remove the need of loading of charsetalias.properties files at startup

Categories

(Core :: Internationalization, defect, P3)

Product:
Core
Component:
Internationalization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: ftang, Assigned: jbetak)

References

Details

(Keywords: perf, Whiteboard: requested sr by email on 11/02/2001)

Attachments

(1 file, 10 obsolete files)

final patch candidate - this assumes that bug 64146 will be resolved soon
2.58 KB, patch
nhottanscp
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
we currently load charsetalias.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.
Blocks: 7251
Keywords: perf
Summary: startup perf- remove the need of loading of charsetalias.properties files at startup → startup perf- remove the need of loading of charsetalias.properties files at startup
per offline discussion with ftang - tentatively reassigning to myself
Assignee: ftang → jbetak
list of charset aliases requested during startup in sequential order (US trunk build): windows-1252 x-mac-roman us-ascii x-u-escaped ISO-8859-2 ISO-8859-3 ISO-8859-4 ISO-8859-5 ISO-8859-6 ISO-8859-6-I ISO-8859-6-E ISO-8859-7 ISO-8859-8 ISO-8859-8-I ISO-8859-8-E ISO-8859-9 ISO-8859-10 ISO-8859-13 ISO-8859-14 ISO-8859-15 ISO-IR-111 windows-1250 windows-1251 windows-1253 windows-1254 windows-1255 windows-1256 windows-1257 windows-1258 TIS-620 IBM866 KOI8-R KOI8-U x-mac-ce x-mac-greek x-mac-turkish x-mac-croatian x-mac-romanian x-mac-cyrillic x-mac-ukrainian x-mac-icelandic armscii-8 x-viet-tcvn5712 VISCII x-viet-vps UTF-7 x-imap4-modified-utf7 UTF-16BE UTF-16LE UTF-32BE UTF-32LE T.61-8bit x-user-defined x-mac-arabic x-mac-devanagari x-mac-farsi x-mac-gurmukhi x-mac-gujarati x-mac-hebrew IBM850 IBM852 IBM855 IBM857 IBM862 IBM864 IBM864i Shift_JIS EUC-JP ISO-2022-JP GB2312 windows-936 x-gbk HZ-GB-2312 gb18030 Big5 Big5-HKSCS x-euc-tw EUC-KR x-johab x-windows-949 windows-1252 x-mac-roman us-ascii x-u-escaped ISO-8859-2 ISO-8859-3 ISO-8859-4 ISO-8859-5 ISO-8859-6 ISO-8859-6-I ISO-8859-6-E ISO-8859-7 ISO-8859-8 ISO-8859-8-I ISO-8859-8-E ISO-8859-9 ISO-8859-10 ISO-8859-13 ISO-8859-14 ISO-8859-15 ISO-IR-111 windows-1250 windows-1251 windows-1253 windows-1254 windows-1255 windows-1256 windows-1257 windows-1258 TIS-620 IBM866 KOI8-R KOI8-U x-mac-ce x-mac-greek x-mac-turkish x-mac-croatian x-mac-romanian x-mac-cyrillic x-mac-ukrainian x-mac-icelandic armscii-8 x-viet-tcvn5712 VISCII x-viet-vps UTF-7 x-imap4-modified-utf7 UTF-16BE UTF-16LE UTF-16 UTF-32BE UTF-32LE T.61-8bit x-user-defined Adobe-Symbol-Encoding x-zapf-dingbats x-mac-arabic x-mac-devanagari x-mac-farsi x-mac-gurmukhi x-mac-gujarati x-mac-hebrew IBM850 IBM852 IBM855 IBM857 IBM862 IBM864 IBM864i Shift_JIS EUC-JP ISO-2022-JP jis_0201 jis_0208-1983 jis_0212-1990 GB2312 windows-936 x-gbk x-gbk-noascii HZ-GB-2312 gb_2312-80 gb18030.2000-0 gb18030.2000-1 gb18030 Big5 x-x-big5 Big5-HKSCS hkscs-1 x-euc-tw x-cns-11643-1 x-cns-11643-2 x-cns-11643-3 x-cns-11643-4 x-cns-11643-5 x-cns-11643-6 x-cns-11643-7 EUC-KR ks_c_5601-1987 x-x11johab x-johab x-johab-noascii x-windows-949 ISO-8859-15 armscii-8 ISO-8859-4 ISO-8859-14 ISO-8859-2 GB2312 Big5 KOI8-R windows-1251 KOI8-U ISO-8859-7 ISO-2022-JP EUC-KR ISO-8859-10 ISO-8859-3 TIS-620 ISO-8859-9 VISCII iso-8859-15 ibm850 x-mac-roman windows-1252 iso-8859-14 iso-8859-7 x-mac-greek windows-1253 x-mac-icelandic iso-8859-10 iso-8859-3 iso-8859-4 iso-8859-13 windows-1257 ibm852 iso-8859-2 x-mac-ce windows-1250 x-mac-croatian ibm855 iso-8859-5 iso-ir-111 koi8-r x-mac-cyrillic windows-1251 ibm866 koi8-u x-mac-ukrainian x-mac-romanian gb2312 x-gbk gb18030 hz-gb-2312 big5 big5-hkscs x-euc-tw euc-jp iso-2022-jp shift_jis euc-kr x-windows-949 x-johab armscii-8 tis-620 ibm857 iso-8859-9 x-mac-turkish windows-1254 x-viet-tcvn5712 viscii x-viet-vps windows-1258 x-mac-devanagari x-mac-gujarati x-mac-gurmukhi iso-8859-6 windows-1256 ibm864 x-mac-arabic x-mac-farsi iso-8859-8-i windows-1255 iso-8859-8 ibm862 x-mac-hebrew I just spoke to ftang and we shouldn't be needing all of these during startup. I suspect that there is some enumeration going on somewhere and we should defer that to later. I'm still in the first stage of investigation, but crude preliminary data indicate that this might be a worthwhile effort...
OK, I believe we are loading all these aliases because of the charset menu construction. I'm marking dependency to bug 64146.
Status: NEW → ASSIGNED
Depends on: 64146
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5
confirming my last assumption: removing the charset menu loader reduces the number of charset alias resolutions (in a US trunk build) to two: UTF-8 ISO-8859-1 Looks like we'll have to address both issues together. I'll run the same check with a Japanese build as Frank suggested and enhance the list of hard-coded charset aliases. However, as it seems the delayed loading of the charset menu would help greatly in addressing this issue.
here is a list of non-canonical charset designators. We need to fix these in our internal prefs to avoid the need for alias resolution and subsequent loading of charsetalias.properties. Two patches coming up - one will defer charset alias resolution to debug builds only, the other will address the need for strictly canonical charset designators in our internal prefs. Resolving charset alias: UTF-16 = UTF-16BE Resolving charset alias: gb_2312-80 = GB2312 Resolving charset alias: x-x-big5 = Big5 Resolving charset alias: ks_c_5601-1987 = x-windows-949 Resolving charset alias: iso-8859-1 = ISO-8859-1 Resolving charset alias: iso-8859-1 = ISO-8859-1 Resolving charset alias: iso-8859-1 = ISO-8859-1 Resolving charset alias: iso-8859-1 = ISO-8859-1 Resolving charset alias: iso-8859-15 = ISO-8859-15 Resolving charset alias: ibm850 = IBM850 Resolving charset alias: iso-8859-14 = ISO-8859-14 Resolving charset alias: iso-8859-7 = ISO-8859-7 Resolving charset alias: iso-8859-10 = ISO-8859-10 Resolving charset alias: iso-8859-3 = ISO-8859-3 Resolving charset alias: iso-8859-4 = ISO-8859-4 Resolving charset alias: iso-8859-13 = ISO-8859-13 Resolving charset alias: windows-1257 = windows-1257 Resolving charset alias: ibm852 = IBM852 Resolving charset alias: iso-8859-2 = ISO-8859-2 Resolving charset alias: ibm855 = IBM855 Resolving charset alias: iso-8859-5 = ISO-8859-5 Resolving charset alias: iso-ir-111 = ISO-IR-111 Resolving charset alias: koi8-r = KOI8-R Resolving charset alias: ibm866 = IBM866 Resolving charset alias: koi8-u = KOI8-U Resolving charset alias: gb2312 = GB2312 Resolving charset alias: hz-gb-2312 = HZ-GB-2312 Resolving charset alias: big5 = Big5 Resolving charset alias: big5-hkscs = Big5-HKSCS Resolving charset alias: euc-jp = EUC-JP Resolving charset alias: iso-2022-jp = ISO-2022-JP Resolving charset alias: shift_jis = Shift_JIS Resolving charset alias: euc-kr = EUC-KR Resolving charset alias: tis-620 = TIS-620 Resolving charset alias: ibm857 = IBM857 Resolving charset alias: iso-8859-9 = ISO-8859-9 Resolving charset alias: viscii = VISCII Resolving charset alias: iso-8859-6 = ISO-8859-6 Resolving charset alias: ibm864 = IBM864 Resolving charset alias: iso-8859-8-i = ISO-8859-8-I Resolving charset alias: iso-8859-8 = ISO-8859-8 Resolving charset alias: ibm862 = IBM862
cc'ing bobj and cathleen, first rough measurements show that this could save around 3% (0.25s) of startup time.
Review: So I guess we are completely eliminating the charset alias ? In that case, can we remove GetCharsetAtom() and GetCharsetAtom2() If not, atleast we can make GetCharsetAtom() call GetCharsetAtomInternal() r=dp
> So I guess we are completely eliminating the charset alias ? No, we are forcing all "internal" charset designators to use the canonical names, but we still need to support the aliases for external charsets (e.g., MIME headers). Nominating nsbranch for 0.9.4.
Keywords: nsbranch
Target Milestone: mozilla0.9.5 → mozilla0.9.4
dp, thanks for looking at this! I guess bobj already addressed your question. I might only add that I too believe that we might be able to increase the number of instances using GetCharsetAtomInternal(). I felt I had to create a new method, since the old one is used for both - internal and external charset designator resolution. The need for internal chasert aliasing could be eliminated thoroughtout the whole application, so this could be just a first step. I'm not entirely sure, how much we could benefit from going all the way, I'd have to investigate some more.
there is still a small problem left to resolve. After changing navigator.properties, I'm still left with the following non-canonical charset designators. I'm investigating, where they are coming from and suspect the source could be individual charset decoders. Resolving charset alias: UTF-16 = UTF-16BE Resolving charset alias: gb_2312-80 = GB2312 Resolving charset alias: x-x-big5 = Big5 Resolving charset alias: ks_c_5601-1987 = x-windows-949
I just verified that the remaining four non-canonical references (UTF-16, gb_2312-80, x-x-big5 and ks_c_5601-1987) are not coming from our registered charset decoders and I'm 100% certain they are not coming from the charset menu either, which was at the center of my optimization effort here. I'll give it a more careful look in the debugger and come up with a recommendation how to proceed from here. Please bear with me.
Attachment #48645 - Flags: review+
Attachment #48645 - Flags: needs-work+
Attachment #48655 - Attachment is obsolete: true
cc'ing some more i18n folks to solicit feedback on the proposed changes. As it turned out the encoding designators requiring aliasing came from four encoders: nsUnicodeToBIG5NoAscii, nsUnicodeToUTF16, nsUnicodeToKSC5601, nsUnicodeToGB2312GL. I'd like to change the _To argument in the NS_UCONV_REG_UNREG macro. It is my belief that it should cause no harm, since the NS_UNICODEDECODER_CONTRACTID_BASE and the DECODER_NAME_BASE remains unchanged. Another more aggressive change involves making GetCharsetAtom2() call GetCharsetAtomInternal(). Unlike GetCharsetAtom(), the "2" method is being used exclusively for internal purposes and we could easily avoid the penalty of encoding name aliasing: http://lxr.mozilla.org/seamonkey/search?string=GetCharsetAtom2 I'm still concerned that we might run into trouble, when QT/GTK/Xlib refer to different encoding names requiring aliasing. I might whip up GetCharsetAtomInternal2() to make this approach less aggressive, but I'd rather not. I do believe there is some measurable benefit in avoid internal encoding aliasing throughout the whole application as dp pointed out and the current patch is a first in that direction. Before we push for a branch checkin, I'd also like to make a binary drop for the QA to confirm the estimated startup time savings of 2.5-3%.
I'm afraid the following patch to nsUCvKoModule.cpp has some problem. -------------- RCS file: /cvsroot/mozilla/intl/uconv/ucvko/nsUCvKoModule.cpp,v retrieving revision 1.17 diff -u -w -b -r1.17 nsUCvKoModule.cpp --- nsUCvKoModule.cpp 2001/07/18 21:48:43 1.17 +++ nsUCvKoModule.cpp 2001/09/09 00:38:27 @@ -74,7 +74,7 @@ NS_UCONV_REG_UNREG(nsEUCKRToUnicode, "EUC-KR", "Unicode" , NS_EUCKRTOUNICODE_CID); NS_UCONV_REG_UNREG(nsUnicodeToEUCKR, "Unicode", "EUC-KR", NS_UNICODETOEUCKR_CID); -NS_UCONV_REG_UNREG(nsUnicodeToKSC5601, "Unicode", "ks_c_5601-1987", NS_UNICODETOKSC5601_CID); +NS_UCONV_REG_UNREG(nsUnicodeToKSC5601, "Unicode", "EUC-KR", NS_UNICODETOKSC5601_CID); NS_UCONV_REG_UNREG(nsUnicodeToX11Johab, "Unicode", "x-x11johab", NS_UNICODETOX11JOHAB_CID); NS_UCONV_REG_UNREG(nsJohabToUnicode, "x-johab", "Unicode" , NS_JOHABTOUNICODE_CID); NS_UCONV_REG_UNREG(nsUnicodeToJohab, "Unicode", "x-johab", NS_UNICODETOJOHAB_CID); ---------------------------------------- 'ks_c_5601-1987' plays *two* roles in Mozilla and this is a source of confusion. One is an alias name for the canonical (MIME) charset name 'x-windows-949'. Microsoft came up with this standard-violating MIME charset name (it's a long story) and began to use it in their mail/news clients. As their clients are so widespread that Mozilla has to be generous and accept it as an alias to either EUC-KR or x-windows-949. (the latter is MS's proprieatary extension of the former). For this role, the canonical name 'x-windows-949' can be used internally in place of 'ks_c_5601-1987'. However, to deal with Korean mail messages and news articles with MIME charset name 'ks_c_5601-1987', this has to be kept as an alias for x-windows-949 (the canonical name in Mozilla). The other is an internal charset name within Mozilla for KS C 5601 GL encoding (used ONLY by X11 *fonts* for chararacters in KS C 5601-1987. That's why there's no nsKSC5601ToUnicode.cpp) It is this role that 'ks_c_5601-1987' is playing in nsUnicodeToKSC5601.cpp and this CANNOT and SHOULD NOT be regarded as identical to 'EUC-KR'. Neither can/should it be regarded as identical to 'x-windows-949'. So, what has to be done? Unless there's some restriction on what format to be used for 'charset' name(e.g. only one '-' can be used or '.' cannot be used), we can just come up with a *distinct* name to use for this second case (i.e. in nsUnicodeToKSC5601 converter). I suggest we use 'ksc5601-1987-0' or 'ksc5601.1987-0' in intl/uconv/ucvko/nsUCvKoModule.cpp. I prefer the latter because that's exactly the same as used in the last two fields of XLFD of the fonts Unicode -> KSC5601 converter deals with. In case '.' cannot be used, we should use 'ksc5601-1987-0'. In case only one '-' can be used (which I don't think is the case), what about 'ksc5601_1987-0'. In short, intl/uconv/ucvko/nsUCvKoModule.cpp should have the following (assuming we can use '.' ) NS_UCONV_REG_UNREG(nsUnicodeToKSC5601, "Unicode", "ksc5601.1987-0", NS_UNICODETOKSC5601_CID); in place of NS_UCONV_REG_UNREG(nsUnicodeToKSC5601, "Unicode", "ks_c_5601-1987", NS_UNICODETOKSC5601_CID); If we decide on what to use for designation of KSC5601 GL encoding for X11 fonts, we also have to make changes in gfx/src/gtk/nsFontMetricGTK.cpp. 'ks_c_5601-1987' in the following part of the file has to be replaced by what we decide to use for KS C 5601 GL encoding. static nsFontCharSetInfo KSC5601 = { "ks_c_5601-1987", DoubleByteConvert,1); should be changed to static nsFontCharSetInfo KSC5601 = { "ksc5601.1987-0", DoubleByteConvert,1); I suspect gb_2312-80 has a similar problem. One may wonder why JIS 0208 and JIS 0212 don't have this problem. That's because for Japanese, there's been no confusion between coded character set name (JIS X 0208, JIS X 0212) on the one hand and character set encoding scheme (EUC-JP, ISO-2022-JP). For Korean, Microsoft mixed all these up (although EUC-KR had been very well established before MS shipped their first internet mail clients) and Mozilla, in its effort to be generous, made a little blunder by failing to distinguish between ks_c_5601-1987 (the standard-violating MIME charset name used by MS products) and ksc5601.1987-0 (the last two fields of XLFD for Korean X11 fonts with KSC5601 GL encoding). The situation for simplified Chinese is even worse (GB2312 and variants are used to mean at least two different things. It would be nice if a much better name EUC-CN had caught on as MIME charset)
Exactly like ks_c_5601-1987 plays dual roles in Mozilla source, x-x-big5 and gb_2312-80 play two roles as 'MIME charset' name (non-canonical) and the font encoding name in X11. The same principle can be applied to distinguish one from the other. I'm attaching a patch to fix all three cases together. In this patch, I use 'ksc5601.1987-0' for KS C 5601-1987 GL encoding, 'x-big5-noascii' for X11 Big5 font without US-ASCII part (similar names have been used for X11 JOHAB fonts and X11 GBK fonts), and 'gb2312.1980-0' for GB 2312-1980 GL encoding. I found that there's nothing to prevent '.' from being used in Mozilla's internal charset name and '.' is used for 'gb18030.2000-0'. Besides, I think using 'gb2312.1980-0' and 'ksc5601.1987-0' as Mozilla's internal charset names for Mozilla's encoders to deal with the X11 fonts with the same XLFD name fields is a good idea. 'ksc5601.1987-0', 'x-big5-noascii' and 'gb2312.1980-0' are used only by encoders for X11 fonts and they are never used as MIME charset name (so they should not be in charsetalias.properties file as commented there). Like jis_0208-1983 and jis_0212-1990 don't cause any alias resolution problem, these three names wouldn't cause any alias resolution problem.
I forgot to look into QT and Xlib part of gfx. I'm going to attach a new patch that take care of QT and Xlib as well as gtk.
Attachment #48747 - Attachment is obsolete: true
Attachment #48775 - Attachment is obsolete: true
I'm sorry I keep forgetting a thing or two. I believe the latest attachment I've just added (timed 9:43 US PDT) has everything I want to change. BTW, most of my changes can be regarded as merely 'aesthetic' (not affecting the way Mozilla runs but simply making the source code clearer). However, I think it's better this way because it will prevent potential confusion in the future.
thanks for all your input and the hard work Jungshik! At a first glance the changes look good, I'll roll up a new hopefully all-encompassing patch including your changes. mkaply - do we need to worry about nsCSIDtoCharsetName.cpp? What is your opinion on the proposed course of action?
I like this fix a lot. I was investigating some Mac encoding problems and was very confused to discover that aliases are required for all charsets even when the alias is the same as the original charset. I don't think you need to mess with nCSIDtoCharsetName.cpp - this is really just a 4.x compatibility thing. Frank would have more info though.
marking nsbranch+ since it is a performance win
Keywords: nsbranch → nsbranch+
I spent several hours working with Juraj on this yesterday. It is not clear to me that we will actually save much startup time by not loading this file. Juraj put a high accuracy time around the code that loads this file and while it takes about 250 milli-seconds to build the charset menu he reported it takes about 30 milli-seconds to load this file.
30 ms out of ? An easy way to time startup is - create 'Default User' - time ./mozilla -P 'Default User' file:///usr/tmp/quit.html quit.html: <html><body onLoad="window.close()"></body></html> There are no big wins; we got to be doing a whole bunch of small wins to get a cumulative performance improvement. Maybe small improvements need not be taken in for the branch.
bstell, I've just learned from jrgm that the timer resolution on W2K is 16ms. This would explain why I was measuring either 16 or 32 ms gain using the built- in timer method. It also explains some of the "noise" I was picking up when using the method described in bug 97712, which I continue to use in parallel. I will also apply dp's method on Linux. So far it looks like the lower boundary is at ~ 30 ms and the upper boundary at ~ 250 ms. After completing the patch, we could perhaps apply it to the trunk and have the QA officially record the performance gain using their metrics. If deemed significant enough, we could take the patch into the branch.
I'd like to wait for ftang to have a look at this before proceeding any futher. I saw some asserts with a Linux test build (http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsGTK.cpp#3692), but they seem to be unrelated to this change.
Whiteboard: have patch, need r/sr
> I'd like to wait for ftang to have a look at this before proceeding any futher. > I saw some asserts with a Linux test build > (http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsGTK.cpp#3692), > but they seem to be unrelated to this change. If it's unrelated, can you back out your change and still see the asserts?
>If it's unrelated, can you back out your change and still see the asserts? Yes, I've tried on two separate build machines (thanks Brian!). Although, the asserts are quite irritating, they seem to unrelated. I just wanted to have it recorded here, to have the benefit of a wider audience. Although I'm not sure how to resolve those asserts at this point, I believe we could proceed with this patch a that's why I posted it.
Juraj: please set the environment variable NS_FONT_DEBUG to 4 and find out what charset is complaining. Dp: Building the charset menu seems to take around 250-300mS. Not reading in the file (which is the stated plan of this bug) would appear to save about 30 mS which is not much of the 250+ mS.
0.9.4 is out the door.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Brian: I believe it's hpbig5, but there might be others. I'll have to rerun all of my converter test with the font debug turned on. I'll follow up on that soon. I followed up with Chris McAfee in bug 97712 and it seems that the "official" startup metrics can achieve a 3% margin of error, which is roughly 3 times better than what I have been seeing. If the performance improvement is small, it could be lost in the "noise". I believe the 30 mS for the file access is correct, although the overall improvement could be better due to reduced number of hash table lookups, smaller bloat etc.
from the startup log: ###!!! ASSERTION: failed to get converter type: 'mDocConverterType!=nsnull', file nsFontMetricsGTK.cpp, line 3692 ###!!! Break: at file nsFontMetricsGTK.cpp, line 3692 failed to get converter type for zh-TW, nsFontMetricsGTK.cpp 3693 The zh-TW charset is for traditional Chinese.
> The zh-TW charset well, that would be a language group, wouldn't it? Am I right in assuming that "mDocConverterType = first_font->mCharSetInfo->Convert" failed?
taking the nsbranch+ off this. it should bake on the trunk for awhile and then only come to the branch if it's extreme low risk and high reward at this point.. let me know if it turns out that way and it happens in the next week or so.
Keywords: nsbranch+ → nsbranch-
OS: Windows 95 → All
Attachment #49623 - Attachment is obsolete: true
Attachment #49402 - Attachment is obsolete: true
Attachment #48776 - Attachment is obsolete: true
Attachment #48744 - Attachment is obsolete: true
Attachment #48721 - Attachment is obsolete: true
Attachment #48645 - Attachment is obsolete: true
ftang: would you be able to review tha most recent patch? The changes now affect the registry enumeration, which is in turn used only by the UI (charset menu, mail encoding menu etc.). This would greatly minimize any regression risk as discussed offline. I did some preliminary verification and it looks good. I'd like to finally close this and move on to "bigger fish" like bug 64146.
another comment: we really need to resolve both "x-sjis" and "shift_jis" for Japanese builds. I was quite surprised at this finding.
> another comment: we really need to resolve both "x-sjis" and "shift_jis" for > Japanese builds. I was quite surprised at this finding. Probably a bug. Historically, when shift_jis was not registered with IANA we created x-sjis. We still need to alias x-sjis for old webpages, but we should not use this internally. Cc'ing rchen who might know why the Japanese builds are using x-sjis.
Blocks: 103175
this one is more complex than I thought. jbetak said it is still ok for m0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Attachment #50337 - Attachment is obsolete: true
ftang - could you please review this again? I removed all attempts to optimize charset aliasing to reduce risk of regressions. When bug 64146 is checked in, there is no need to enumerate 180+ converters, which simplifies this problem quite a bit. By accident I noticed that we didn't report any error when a charset designator was empty - mozilla.org returns an empty HTTP header charset and we end up loading the string bundle to look it up - I found it quite amusing. Also cc'ing blizzard to put this on his radar. It would be nice, if he could sr after ftang approves of this approach.
nhotta - would you feel comfortable with and have time to review this? I'd really appreciate it. Hopefully Chriss Blizzard will sr soon, so that I can get it checked in.
NS_TIMELINE_STOP_TIMER("nsCharsetAlias2:GetPreferred"); is that for permance profiling?
nhotta - thanks for looking at this! Yes, NS_TIMELINE_STOP_TIMER is for performance profiling, it only gets built in debug builds with the the enviroment variable MOZ_TIMELINE set. I was surprised to see that lxr is full of these macros - someone has been putting them all over :-)
So is this just adding checks for two cases ("shift_jis" and "x-sjis")?
Comment on attachment 53808 [details] [diff] [review] final patch candidate - this assumes that bug 64146 will be resolved soon r=nhotta
Attachment #53808 - Flags: review+
blizzard - could I pester you with one more sr? This one is on i18n performance (startup) tuning. I'd really appreciate it, as M096 is drawing near :-)
cc'ing waterson for potential sr
Blocks: 107067
Keywords: nsbranch-
cc'ing brendan for potential i18n sr
Whiteboard: have patch, need r/sr → needs sr
Whiteboard: needs sr → requested sr by email on 11/02/2001
Comment on attachment 53808 [details] [diff] [review] final patch candidate - this assumes that bug 64146 will be resolved soon sr=kin@netscape.com
Attachment #53808 - Flags: superreview+
I forgot to ask why the "x-sjis" and "shift_jis" cases can't be combined into a single if statement like ... + if (aKey.Equals(NS_LITERAL_STRING("x-sjis") || + aKey.Equals(NS_LITERAL_STRING("shift_jis")))
Netcenter needs to be evangelized and x-sjis should be dropped from their Japanese site. The only reason I kept them separate was my vain hope to have them do it before I can check this patch in. Kin, thanks so much for your review - I'll correct this and land the patch in a few minutes.
patch has just landed... Marking fixed - thanks everyone!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Changed QA contact to ftang@netscape.com.
QA Contact: andreasb → ftang
Kat, > Netcenter needs to be evangelized and x-sjis should be dropped from their > Japanese site. The only reason I kept them separate was my vain hope to have > them do it before I can check this patch in. Can you help on this?
> Kat, >> Netcenter needs to be evangelized and x-sjis should be dropped from their >> Japanese site. I guess it is finally time to mothball Netscape 2.x or earlier & IE3 completely. >>The only reason I kept them separate was my vain hope to have >> them do it before I can check this patch in. NetCenter isn't the only site using x-sjis, but it should now be safe for other web sites to drop x-sjis also. As long as external charset handling for Mozilla can deal with x-sjis, this should not be a problem. >Can you help on this? Sure.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: