Lazy loading of string bundles

RESOLVED FIXED in mozilla0.9.6

Status

()

defect
P2
normal
RESOLVED FIXED
18 years ago
5 years ago

People

(Reporter: alecf, Assigned: alecf)

Tracking

(Blocks 1 bug)

Trunk
mozilla0.9.6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in hand)

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

18 years ago
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.
Assignee

Comment 1

18 years ago
reassign to me, mark stuff appropriately.
Assignee: nhotta → alecf
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla0.9.1
Assignee

Comment 2

18 years ago
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
Assignee

Comment 3

18 years ago

Comment 4

18 years ago
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

Assignee

Comment 5

18 years ago
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

18 years ago
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

Comment 7

18 years ago
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.

Updated

18 years ago
Blocks: 26291
Assignee

Comment 8

18 years ago
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

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

Updated

18 years ago
Keywords: patch
Whiteboard: fix in hand
Assignee

Comment 10

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

Comment 11

18 years ago
Assignee

Comment 12

18 years ago
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

18 years ago
r=valeski.

Updated

18 years ago
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. 
Assignee

Updated

18 years ago
Target Milestone: mozilla0.9.2 → mozilla1.0

Updated

18 years ago
Blocks: 71781

Updated

18 years ago
Blocks: 88583

Comment 15

18 years ago
nav triage team:

Not a 0.9.4 stopper, leaving at mozilla1.0

Comment 16

18 years ago
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??

Updated

18 years ago
Target Milestone: mozilla1.0 → mozilla0.9.6

Updated

18 years ago
Blocks: 7251
Assignee

Comment 17

18 years ago
Posted patch new fix plus code cleanups (obsolete) — Splinter Review
Assignee

Comment 18

18 years ago
Assignee

Updated

18 years ago
Attachment #31650 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #33120 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #55952 - Attachment is obsolete: true
Assignee

Comment 19

18 years ago
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

18 years ago
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+
Assignee

Comment 22

18 years ago
Assignee

Updated

18 years ago
Attachment #55953 - Attachment is obsolete: true
Assignee

Comment 23

18 years ago
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

18 years ago
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)
Assignee

Comment 25

18 years ago
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!
Assignee

Comment 27

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

Comment 28

18 years ago
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+
Assignee

Comment 31

18 years ago
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: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.