Closed Bug 76944 Opened 24 years ago Closed 23 years ago

Lazy loading of string bundles

Categories

(Core :: Internationalization, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: alecf, Assigned: alecf)

References

Details

(Whiteboard: fix in hand)

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nhotta → alecf
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla0.9.1
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.
Status: NEW → ASSIGNED
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.
QA Contact: andreasb → 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.
Blocks: 26291
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
Keywords: patch
Whiteboard: fix in hand
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.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
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. 
Target Milestone: mozilla0.9.2 → mozilla1.0
Blocks: 71781
Blocks: 88583
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??
Target Milestone: mozilla1.0 → mozilla0.9.6
Blocks: 7251
Attached patch new fix plus code cleanups (obsolete) — Splinter Review
Attachment #31650 - Attachment is obsolete: true
Attachment #33120 - Attachment is obsolete: true
Attachment #55952 - Attachment is obsolete: true
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
Attachment #55953 - Flags: superreview+
Attachment #55953 - Attachment is obsolete: true
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
Attachment #56087 - Attachment is obsolete: true
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=
Attachment #56142 - Flags: superreview+
Attachment #56142 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: