spun off of bug 26291.. we should be lazy about loading string bundles, and not actually load the file until the first string is requested. Eventually we'll try to load them asynchronously, but that's what bug 26291 is all about. patch forthcoming.

reassign to me, mark stuff appropriately.

from the other bug: I'm attaching patches which do the following: - in CreateBundle(), all we do is store the URL of the bundle that the user asked for - adding LoadProperties() which loads the property using the event loop mechanism - on the entry point to GetStringFromName(), GetStringFromID(), and FormatStringFromName(), I'm calling LoadProperties, which will load the bundle if necessary - removes the defunct RegisterCallback mechanism what this essentially accomplishes is the ability to arbitrarily create bundles very quickly, but have them loaded as soon as you ask for the first string. I'm also keeping track of whether the bundle was ever loaded, so that if you keep trying to access the same string over and over, it doesn't keep trying to load the bundle. this improves startup performance because on startup we create 8 bundles, but only ask for strings from 7 of them.. so one of them won't even be loaded until it's necessary.

in InitSyncStream() you're now setting mLoaded to true, where we weren't before. I'm assuming we neglected to set it to true before and you're fixing that. r=valeski

right.. it was never actually necessary to set it to true before, but now it is, since the sync and non-sync cases will be calling LoadProperties.. once this is in and running well for a few days, I'm going to remove the "sync" versions entirely, and just make "async" the default (continuing to put it in quotes, because it's not really async :))

Changing QA contact to tao since this is a spun off for bug 26291 and the qa contact there is set to tao. Tao, please reassign to me or ftang in case you disagree.

So now you are assuming the overhead of blocking w/ eventloop wouldn't be more than the straight OpenInputStream()? I thought we were convinced that OpenInputSteam() is a bit faster than the eventloop blocking and therefore decided not to move forward to this blocking async? On the other hand, lazy loading has proved its value in stringbundle overlay. Extending it to general stringbundle certainly is the right thing to do. You might want to see how it interacts with the stringbundle overlay, though.

re: stringubndle overlays - already have, it works fine. I should add that this only affects you if you have STRRES_ASYNC turned on... I'd like to switch to this mechanism in a seperate patch, so if something goes wrong, we can back out the 2nd patch instead of this one. Actually, we probably could do lazy string bundle loading with OpenInputStream, but I'd like to switch to the event loop model eventually so we can eventually have true async loading

Plan sounds good, and this patch looks good too. sr=erik@netscape.com

alright, THAT fix is in. Now I'm finally going to turn it on for everyone in a seperate patch.

rather than change the status, I finished the patch. I'm going to run with these for a day or three, make sure things are continuing to work with no funky event race conditions or anything... looking for people to try this and/or sr=

r=valeski.

nav triage: not clear why this is needed for m0.9.2, i.e. what's the user impact or startup perf improvement? would like to defer to mozilla1.0.

nav triage team: Not a 0.9.4 stopper, leaving at mozilla1.0

alecf is on sabbatical... startup performance is extremely important for 0.9.4. I see a proposed patch is here, and it has first round of review... Can someone help pick this up and get it in for 0.9.4??

So I tried the async stuff, as it stands now, and thanks to our use of JARs, this no longer works.. I get lots of threadsafe assertions trying to read a .properties file out of a .jar. So I've #if 0'ed out that code right now, so that we're not bloating the string bundle code with it, and I've isolated it in one function..we can fix the async stuff in the async bug 26291 With this patch, we avoid loading 2 out of the 16 string bundles that get loaded at startup.

Comment on attachment 55953 [details] [diff] [review] take two - #if 0 out the async stuff Looks good. Two minor points: 1. Since we are taking out the async stuff for now, why not take out the all the nsIStreamLoaderObserver stuff as well? 2. Can we use something like "#if ASYNC_LOADING" instead of "#if 0"?

Comment on attachment 55953 [details] [diff] [review] take two - #if 0 out the async stuff sr=dveditz

Ok, I got inspired by tao's cleanup request, and did a major cleanup..tao, dveditz, do you mind re-reviewing? Sorry the patch ended up being big, but I've been wanting to do this cleanup for a while: - fixed all uses of string classes to reduce the number of copies of strings that we make, and changed some stack-based nsStrings to nsAutoString. Also switched some strings to nsDependentString, where we weren't writing to the string - removed [const] from some interfaces - "const" is implied by "in" and is redundant - i.e. this doesn't change the call signature of the C++ headers - removed redundant "copyUnicode" - this functionality already exists in ToNewUnicode() - removed the old GetEnumeration - the only consumer was xpinstall, so I fixed that too, and switched it over to using nsCOMPtr

Comment on attachment 56087 [details] [diff] [review] ok, so I got inspired - major cleanup! >Index: intl/strres/public/nsIAcceptLang.idl >=================================================================== >RCS file: /cvsroot/mozilla/intl/strres/public/nsIAcceptLang.idl,v >retrieving revision 1.5 >diff -u -r1.5 nsIAcceptLang.idl >--- nsIAcceptLang.idl 2001/09/26 00:40:13 1.5 >+++ nsIAcceptLang.idl 2001/11/01 16:51:14 >@@ -53,7 +53,7 @@ > [scriptable, uuid(383F6C16-2797-11d4-BA1C-001083344DE7)] > interface nsIAcceptLang : nsISupports > { >- wstring getAcceptLangFromLocale([const] in wstring aLocale); >- wstring getLocaleFromAcceptLang([const] in wstring aName); >- wstring acceptLang2List([const] in wstring aName, [const] in wstring aList); >+ wstring getAcceptLangFromLocale(in wstring aLocale); >+ wstring getLocaleFromAcceptLang(in wstring aName); >+ wstring acceptLang2List(in wstring aName, in wstring aList); > }; Are we deprecating "[const]" from XPIDL? Also, I suspect we are not using nsIAcceptLang now. It was created for a miscarried feature. I couldn't find any usage in LXR. Perhaps, we can take out the references (don't remove it, though) in nsStringBundle.cpp & makefiles? It should cut a few fat. >Index: intl/strres/src/nsStringBundle.cpp >=================================================================== >RCS file: /cvsroot/mozilla/intl/strres/src/nsStringBundle.cpp,v >retrieving revision 1.113 >diff -u -r1.113 nsStringBundle.cpp >--- nsStringBundle.cpp 2001/10/30 08:48:49 1.113 >+++ nsStringBundle.cpp 2001/11/01 16:51:16 >@@ -44,7 +44,6 @@ > #include "nsIPersistentProperties2.h" > #include "nsIStringBundle.h" > #include "nscore.h" >-#include "nsILocale.h" > #include "nsMemory.h" > #include "plstr.h" > #include "nsNetUtil.h" >@@ -72,8 +71,10 @@ > #include "nsAcceptLang.h" // for nsIAcceptLang > Comment this out? > // for async loading >+#ifdef ASYNC_LOADING > #include "nsIBinaryInputStream.h" > #include "nsIStringStream.h" >+#endif The rest of them are fine. Many thanks for cleanup! r=tao after one more cleanup of the nsAcceptLang)

const isn't deprecated, its just already implied by "in string" or "in wstring" I've yanked nsAcceptLang from the build - thanks for pointing that out!

And I'll actually remove nsIAcceptLang.idl and nsAcceptLang.* from the tree once I've cleaned up the mac projects.

Comment on attachment 56087 [details] [diff] [review] ok, so I got inspired - major cleanup! got the ok from bhuvan and tao to rip out the ns*AcceptLang* from the build

Where did NS_GetURLFromFile() come from? My current tip build doesn't like it.

Comment on attachment 56142 [details] [diff] [review] patch where we remove ns*AcceptLang sr=dveditz, adding in tao's earlier r=

arg, that NS_GetURLFromFile was leftover from another bug.. and it got checked in! doh! anyway, I backed that part out, but everything else here went in.