after fixing bug 157993, I think I'll have a way to shrink the i18n libraries significantly The problem is basically that almost all of the converter classes contain no actual code - they just map to a generated in-memory table. We have the overhead of a class which derives from a shared base class basically we have something like this: class nsUnicodeToFoo : nsBaseEncoder { nsUnicodeToFoo() : nsBaseEncoder(table1, ...) {} } and then later declare a constructor NS_GENERIC_FACTORY_CONSTRUCTOR(nsUnicodeToFoo); and then put nsUnicodeToFooConstructor in the generic module header. Here's what we can do instead: for each "class" we instead ditch the actual class, and just write a custom constructor: nsUnicodeToFooConstructor() { return CreateBaseEncoder(g_FooTable, ...); } and allow all constructors to share a common constructor: CreateBaseEncoder(table, ...) { return new nsBaseEncoder(table,...); } then nsUnicodeToFooConstructor will be much smaller.

looks a good plan. but they are dervied from different classes. But in general your plan will work.

code issue, QA to yokoyama@netscape.com for now.

yes, I've noticed they derive from about a dozen different classes...so this will consolidate the constructors down to 12 from a total of about 80 or more.

ok, just ran through some analysis.. check this out: 81 of 85 decoders are basic (95%) 97 of 112 encoders are basic (86%) I tweaked TestUConv to be able to detect which classes are based off of any of the base classes (where "basic" means "derived from one of these base classes like nsEncoderSupport or something).. interesting results, eh? This means that there are roughly 178 classes that are in some way table-based. I suspect that 10-20 of those classes have specialized converter methods, but the rest (lets say 160 classes) should be easily converted to static constructors..

mixed news here - after developing a patch for bug 157993, I was able to start on this one.. I converted 147 classes over, which isn't as many as I had hoped, but still 75% of the available classes. On my debug build, this saved only 16k in ucvasia (2%) but it saved 56k on ucvwest - over 28% of the dll! This is mostly because many of the asain charset converters require specialized C++ to make the conversion, and couldn't be done in a simple table... I'm still doing uconv.dll, and I suspect similar, though not as dramatic, results as ucvwest.dll

turns out there were only 8 converters in uconv.dll, and I could convert 6 of them..saving a grand total of 5k However, I think there is overlap between uconv.dll and ucvwest - namely the MacRoman converters, and some odd discrepancy between the various ISO8859-* decoders.

Ok, here's the conversion - note that you'll have to apply the patch from bug 157993, or wait until that bug is fixed to apply this patch.

ok, another decent test - on a linux release build, this reduced the total library size from 668k/220k (888k total) to 656k/160k (816k total) or about 72k - not spectacular, but also not bad considering no loss in functionality and in-memory footprint.

ok, this converts most converters over to static constructors.. you can see the classes that were removed in the patch to intl/uconv/src/nsUConvModule.cpp looking for reviews...

I'm looking for reviews here... Roy? or maybe Naoki?

Comment on attachment 94994 [details] [diff] [review] convert to static constructors that's a huge patch. r=shanjian. Off topic, iso88596E, iso88596I, iso88598E, iso88598I seem unnecessary. If so we should resolve them in charset alias. ftang?

defer to smontagu about iso88596E, iso88596I, iso88598E, iso88598I I think we still need different charset here. The conversion will be the same, but other stuff (logical / visual order etc) may still depend on a different charset name inside Mozilla

The only one of those charset names that we need to preserve for different behaviour is iso88598i; the others are all either obsolete or make no practical difference.

A number of the .h files seem to have an unneeded include of nsUCSupport.h (eg ucvibm/nsCP855ToUnicode.h, ucvibm/nsCP864ToUnicode.h and others like that). Come to think of it, looks like every single header involved includes nsUCSupport.h, and I doubt they need to. I don't like the use of NS_IMETHODIMP for these global (non-method) functions.... I'd think that something like "NS_EXPORT nsresult" would be better, no? These are certainly not methods. I realize that you're doing this to match what NS_GENERIC_FACTORY_CONSTRUCTOR does, but it still seems very odd to me... +nsUnicodeToBIG5HKSCSConstructor(nsISupports *aOuter, REFNSIID aIID, + void **aResult) + { That's indented oddly, as is the body of this function as a result. +nsUnicodeToEUCTWConstructor(nsISupports *aOuter, REFNSIID aIID, + void **aResult) { + return CreateMultiTableEncoder( 8, The space before that "8" can go. The rest is good!

Ok, so I was going to protest against the NS_IMETHODIMP stuff, but I discovered, poking around nscore.h, that NS_METHOD has the same definition, so I will just use that. I'll also fix the nsUCSupport.h thing, and the spacing..patch coming up soon

Ok, here's an updated patch that addresses all of Boris's comments. the good news is that switching from nsUCSupport.h to nsISupports.h (I had to, in order to get the basic type definitions for the file) dropped the build time of this library in half! the files fly by now.

Comment on attachment 97044 [details] [diff] [review] convert to static constructors v1.1 transferring r=shanjian

Hmm... so how come you're not including nsISupports.h in some of those headers but have to in others? See, eg, ucvibm/nsUnicodeToCP864i.h compared to ucvja/nsUnicodeToEUCJP.h

I've gotta agree with bz here, using any of the *METHOD* macros for non-methods (i.e. C functions) seems really really wrong to me. I know we do it in some places, but that doesn't make it any more right IMO. From what I can tell, NS_METHOD expands to |__stdcall nsresult| on windows and just |nsresult| on other platforms, and AFAIK __stdcall is the default calling convention on windows so there's no need for that. Why not just make those global functions return nsresult instead of hiding the return type it in a macro (that's intended for XPCOM methods) when there's no need for that? Or did I miss something?

ok, finally a patch which uses nsresult, not NS_*METHOD, has #include "nsISupports" in each header file, and is generally just cleaned up... hopefully everyone will be happy with this one :) Can I get an sr=bz now? :)

aaaauuuugggghghhhh this is driving me bonkers. so the one file that doesn't compile (drum roll please..) is nsUConvModule.cpp, with over 100 errors (stopping compilation..) because none of the methods are __stdcall... which is appearently not the default on windows. Ignore that last patch, I'm switching back to NS_METHOD, and nobody is allowed to complain about that any more. I'm using NS_METHOD, its the only way to get what I need. We shouldn't have made generic factories use NS_IMETHOD but now its too late.

Comment on attachment 98234 [details] [diff] [review] convert to static constructors v1.11 Yes, indeed. sr=bzbarsky, rolling the r=. Thanks for your patience, Alec!

ugh. Just read comment 21. OK. NS_METHOD it is, the sr still applies.

thanks a ton, boris!

Comment on attachment 98234 [details] [diff] [review] convert to static constructors v1.11 a=roc+moz for trunk

:-( (re NS_METHOD)

jst: I know.. :( feel free to file another bug on nsIGenericFactory - its in glue code so its technically not frozen...but maybe we just need a macro with a new name.. anyway, this is now fixed...