76944 - Lazy loading of string bundles

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Alec Flett]

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.

Comment 1

[Alec Flett]

reassign to me, mark stuff appropriately.

Comment 2

[Alec Flett]

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.

Comment 3

[Alec Flett]

Attachment: lazily load, plus lots of cleanup

Comment 4

[Judson Valeski]

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

Comment 5

[Alec Flett]

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 :))

Comment 6

[Andreas Becker]

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.

Comment 7

[tao]

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.

Comment 8

[Alec Flett]

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

Comment 9

[Erik van der Poel]

Plan sounds good, and this patch looks good too. sr=[e-mail]

Comment 10

[Alec Flett]

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

Comment 11

[Alec Flett]

Attachment: remove all refs to the old sync code

Comment 12

[Alec Flett]

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=

Comment 13

[Judson Valeski]

r=valeski.

Comment 14

[Viswanath Ramachandran]

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. 

Comment 15

[Paul Chen]

nav triage team:

Not a 0.9.4 stopper, leaving at mozilla1.0

Comment 16

[Cathleen]

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??

Comment 17

[Alec Flett]

Attachment: new fix plus code cleanups

Comment 18

[Alec Flett]

Attachment: take two - #if 0 out the async stuff

Comment 19

[Alec Flett]

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 20

[tao]

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 21

[Daniel Veditz [:dveditz]]

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

sr=dveditz

Comment 22

[Alec Flett]

Attachment: ok, so I got inspired - major cleanup!

Comment 23

[Alec Flett]

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 24

[tao]

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)

Comment 25

[Alec Flett]

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!

Comment 26

[Alec Flett]

Attachment: patch where we remove ns*AcceptLang

Comment 27

[Alec Flett]

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

Comment 28

[Alec Flett]

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

Comment 29

[hacker formerly known as [e-mail]]

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

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 56142 [details] [diff] [review]
patch where we remove ns*AcceptLang

sr=dveditz, adding in tao's earlier r=

Comment 31

[Alec Flett]

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.