26291 - string bundle shouldn't be using using OpenInputStream(), block UI thread

Status: RESOLVED INACTIVE

(Core :: Internationalization, defect, P3)

Description

[Judson Valeski]

http://lxr.mozilla.org/seamonkey/source/intl/strres/src/nsStringBundle.cpp#274 
shows a line using the netUtil function that wraps an OpenInputStream() call. 
The result is that the user is handed an input stream to read data from. The 
inputStream->Read() call is a blocking call and can/will block the thread it's 
called on. If this steam is guaranteed to be read from a thread other than the 
UI thread, evaluate whether or not the blocking behavior is ok, and if so, close 
this bug. Otherwise, your code need to do an asynchronous load of the URI. If 
you don't want to implement nsIStreamListener, please look at 
nsIStreamLoader.idl which will call a callback function you implement when all 
the data has arrived (it does all the asyncronous work for you).

Comment 1

[Simon Fraser [no longer active]]

OK, this is pretty serious, and is causing bug 26265. The profile manager is 
trying to load a string bundle on startup, but because of this attempt at a 
synchronous load, no data is loaded from the .properties file.

Comment 2

[Judson Valeski]

If someone needs help understanding an async load, just let me know.

Comment 3

[tao]

Hmm..., that would be me!

Jud,

Would you give an instruction of how this is going to work? I imagine that the 
stringBundle would pause after requesting an async load of the property file 
and wait to be called when the stream is available. Is this the way it works?

Also, would you point me to a sample code for reference?


Thanks

Comment 4

[Erik van der Poel]

OK, if there is a rule that states that nobody is allowed to block on the UI
thread, then that must mean that XUL files are also loaded on a different
thread. Then the UI thread spins in its event loop, and when the XUL file has
finished loading (or has loaded partially?), the XUL loader sends an event to
the UI thread's event loop, which then processes that data.

We will probably need to do something similar for StringBundle. We could have
the caller of the StringBundle set up a callback, similar to nsIStreamLoader's
callback mentioned above. StringBundle would itself implement nsIStreamLoader's
callback, which would call the app back, which can then do whatever it needs to
do with the StringBundle (e.g. getting strings from it). This adds a bit of
complexity to the caller, since they can't just get a bundle and then get
strings from it. Oh, well. I guess we can't get around this limitation due to
our UI thread policy.

Comment 5

[(not reading, please use [e-mail] instead)]

I'm seeing this on linux,too.

try running -installer on linux, and you see the problem.  if it is happening,
you'll see dump statements that say:  "please fix bug 26291" and buttons will
say "Migrate *" and "New Profile *" instead of what's in the string bundle.

Comment 6

[tao]

http://bugzilla.mozilla.org/show_bug.cgi?id=26265 contains more comments from
Jud and Erik.

For now, we do not seem to have a concrete solution for the async loading.
Unless the problem on Mac and Linux persists after my patch for
26265, I am pushing it to M16. 

Comment 7

[tao]

Blocking stream loading in UI therad is a known problem in Seamonkey. Unless
we plan to address them all, I will not fix this particular problem in this
release.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

just be aware that if this bug is causing profile manager misbehaviour on Mac, 
we will have to investigate another method for JS localization. 

Comment 9

[tao]

.

Comment 10

[Alec Flett]

I just saw the mail from tao on this - and noticed this is closed as VERIFIED
REMIND... should we reopen it, and at the very least mark as Future?

So how is this expected to work? the string bundle API is not asynchronous, and
I don't think it should be. It sounds like we basically need to start a url load
when someone calls nsIStringBundleService.CreateBundle(), and then block
nsIStringBundle.GetStringFromName() until the bundle is loaded.

Comment 11

[tao]

yes, let's reopen it. Should we discuss this in the newsgroup or in this bug report?

Comment 12

[Erik van der Poel]

When we originally designed the String Bundle API, we thought that it would be
useful to allow the caller to specify a URL for the property file. This is
basically at the root of this problem. Perhaps it is not so reasonable to open
a separate TCP/IP connection just for a property file. Perhaps apps would be
packaged as JAR files that contain XUL/DTD/JS/property files, in the same way
that Java applets are often packaged in zip/jar files.

Perhaps we can agree to make the String Bundle less general by allowing callers
to only refer to the contents of such packages. E.g. jar: URLs only, or also
allow file: URLs...

Just throwing some ideas out there...

Comment 13

[Alec Flett]

no, I think specifying the url is reasonable.... and it doesn't preclude us from
loading it asynchronously.. 

the trick is basically to make sure there's enough time between the
CreateBundle() call and the GetStringFromName() call to allow the
bundle to load asynchronously. This mainly involves fixing callers to  make sure
there is enough time between the two calls
(for instance, the CreateBundle could happen in the onload= handler, and the
GetStringFromName could happen during normal processing)

Comment 14

[Erik van der Poel]

That sounds good to me.

Comment 15

[tao]

I have a patch sitting in my local tree (MN6 branch though) does something
similar to what is suggested here and a few others:

  1. I added a new api

     nsIStringBundle CreateAsyncBundle([const] in string aURLSpec);


   to nsIStringBundleService. This new api returns a bundle that loads string
   resource asynchronously.

  2. I added to nsIStringBundle:

    +  // init version
    +  nsStringBundle();
    +  nsresult Init(const char* aURLSpec);

  The default contructor does only basic data member initialization and leave
  the real bundling to init() which returns nsresult.

  Also the strinbundle now becomes an observer of StreamLoader. In Init(), it
  opens the stream loader and waits for OnStreamComplete to be called. When
  that happens, it loads the bundle and notify its observer (from JS/C++) or
  registered callback (from C).


To use this new scheme, the caller need to wait until the notification comes
or check the status of the stream loading via a new api, GetLoaded().

In JS environment, the async bundle creation, as alecf suggested, should be
somewhere in onLoad() or the like. In C++ object, Init() or constructor is a
good place to initialize the async bundle.

How does this sound?


I'll attach the patch later. (This patch contains other works, though)

Comment 16

[Alec Flett]

no, I think that is the wrong approach

To reiterate:
1) we should not be adding new APIs - the existing CreateBundle should switch to
an asychronous model
2) The creation of the string bundle via CreateBundle() should kick of the URL
load
3) when someone calls GetStringFromName(), then we should block until the string
bundle is done loading. (and if the string bundle is already loaded, this will
return immediately with the new string)

Comment 17

[Judson Valeski]

alec's got it.

Comment 18

[tao]

OK, blocking the calling threading until property file is loaded sounds
reasonable.


Jud,


What's the safest way to block the calling thread until OnStreamComplete() is
called and executed?


thx

Comment 19

[Judson Valeski]

something like...

::CreateBundle(){
...
nsAutoMonitor mon(mMon /*create a monitor in the bundle's init method*/);
nsIStreamLoader loader;
NS_NewStreamLoader(getter_AddRefs(loader), mURI, this /*the load observer*/);
mon.Wait(); // this blocks the current thread.
...
}

The above will kick off an async load of mURI, your
nsIStreamLoaderObserver::OnStreamComplete() impl will be called once all the
data has arrived (or in case something went wrong, be sure to check the result
code in that method callback). If you want to gather status, bytes transfered,
etc. you should use nsIStreamListener|Observer instead of
nsIStreamLoaderObserver. Once OnStreamComplete() is called you can suck all the
data out of the stream.

::OnStreamComplete(...) {
...
nsAutoMonitor mon(mMon);
// suck all teh data out of the stream
mMon.Notify(); this will unblock the CreateBundle() call's thread.
...
}

I like alec's suggestion of lazily blocking/sleeping the caller's thread only if
all the data hasn't arrived yet, but that adds a level of complexity, and we're
more vulnerable to race conditions, thus, I'd just block everytime in
CreateBundle().

Comment 20

[Erik van der Poel]

Jud, I'm just trying to understand your concern about not waiting for the URL
in CreateBundle(). The app currently loads many different types of URLs
asynchronously, and it doesn't wait for them. E.g. HTML docs, images, etc. Are
you saying that StringBundles are fundamentally different, so they shouldn't
be treated the same way? For example, are you concerned that StringBundles might
be used from the very thread(s) that are doing some of the related work, so that
there might be deadlock?

Comment 21

[Judson Valeski]

my original suggestion was to have string bundles load data asynchronous, but I
received a ton of resistance. Apparently most string bundle users aren't setup
to consume data asyncronously (ie. string bundles are used all over the place
and therefore we can't go in and change the control flow of all the usage sites).

Comment 22

[Erik van der Poel]

I may be missing something, but why would we need to change any of the callers
when we make CreateBundle() load the URL asynchronously? The callers are
currently calling CreateBundle() and then GetStringFromName(). In the new world,
they would continue to call these methods. The only difference would be that
CreateBundle() returns immediately, while GetStringFromName() might block,
depending on whether or not the bundle has arrived at that point in time. I
guess I was assuming that CreateBundle() would spin a new thread and load the
URL in that thread. GetStringFromName() would then synchronize with that thread,
so there shouldn't be any deadlock worries. What am I missing?

Of course, to take advantage of this new design, the callers ought to call
CreateBundle() at start-up time or soon thereafter, especially if they expect to
make use of the bundle. Callers that might not ever use the bundle may wish to
load lazily, waiting until some situation arises before calling CreateBundle().
Of course, these users will then have to wait for the bundle to arrive. That's
the penalty they pay for loading lazily.

Comment 23

[Alec Flett]

I think we should eventually make CreateBundle() return immediately and have
GetStringFromName block, but I think jud's suggestion is a good first step... 

Comment 24

[Judson Valeski]

"I may be missing something, but why would we need to change any of the callers
when we make CreateBundle() load the URL asynchronously?
"

If you bury the async load using wait/notify on monitors behind the createbundle
impl, and onstreamcomplete, you don't have to change callers. There's some
communication issue here, not sure what it is. if you want to provide a truly
async model (no blocking/waiting), you have to change all the call sites. using
nsIStreamLoaderObserver allows you to get around this (though it's not *really*
async as you're blocking the thread). This bug is about getting rid of the sync
api usage of necko; and nsistreamloaderobserver will do that. I'm not familiar
w/ the bundle api or it's usage, if someone wants to make sure the wait/notify
can occur (maybe it's  a no-brainer, i was just expecting other methods off the
string bundle being called and the order wouldn't be known) w/ the correct
waiting, great. My point is that whether you block in CreateBundle, or Get*(),
because the calling code is synchronous, it really doesn't matter, and you could
avoid any race cond. issues by just blocking in CreateBundle.

Comment 25

[tao]

How about using a condition var in createbundle() and check the state change
in GetStringFromName()? Will this work?

Jud's suggestion sounds workable. But, blocking the thread in createbundle()
seems to defeat the advantage of async laoding.

I'll try both and let you guys know what tunrs out. Thanks for the valuable
input.

Comment 26

[Judson Valeski]

again, this change isn't happening to gain the advantages of async loading (it's
just not possible w/ out a ton of rewrite of string bundle consumers). This
change is to avoid a broken api. Changing to the new api will do nothing but
cause the string bundle code to use a supported api (and one that's proven to
work). The condition var was what I was thinking about using, but I chickened
out because you'd have to do atomic lock/unlocks which aren't cheap, *and*
because things aren't really async anyway, there's no gain to moving the
blocking code out of CreateBundle().

Comment 27

[tao]

IMO, we should not try to change str bundle's usages. Instead, either hide the
synchronization in current APIs or provide a set of new APIs (as I originally
planned) as an alternative to the callers that *really* need them. While the
former will affect all current usages, the latter should affect only new usages
or serve as a fix to problems caused by using the OpenInputStream() version.

Comment 28

[tao]

Adding wtc to the CC list.

Comment 29

[Erik van der Poel]

I would prefer not to add new methods to the APIs. I like Alec's idea of
starting with Jud's suggestion (make CreateBundle() block), moving to the other
idea later (CreateBundle() returns immediately, while GetStringFromName() may
block).

Comment 30

[Alec Flett]

this really isn't that complicated! If someone doesn't attach a patch this soon, 
I'm going to take this bug, implement it, and show you all how easy this is :)

Comment 31

[Judson Valeski]

I don't think anyone's struggling w/ the work, we're just bickering over what we
want.

Tao/Erik, if we want a new async API, sounds great. We might be able to get new
folks to use it and benefit from the fruits of async operations. I wouldn't
spend time here though unless we do the extra work of determining how usages can
be restructured (pass control flow out to the innermost event loop) to take
advantage of it.

As for the "where to block" impl detail, Erik, I suspect the majority of
CreateBundle() usages look like this
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookie.cpp#205 , and
note that the GetString() call occurs on the same thread, right after the
CreateBundle call. Therefore, blocking in GetString*() has *no* gain and adds an
atomic lock/unlock overhead on some condition var (if I'm wrong here let me
know), as well as the potential for a race condition if a new
GetStringFromJud'sHouse() method is added and the lock is misplaced or
forgotten. Of course, you'd know more about the usage landscape of strings, and
maybe we have instances of CreateBundle() being called on one thread (then that
thread drops into an event loop), while the GetString*() is called on another
thread.

Comment 32

[Erik van der Poel]

I wouldn't work on a new async API (yet), if I were the one assigned to this.
It's up to you, Tao.

As for current usage patterns, I wouldn't be at all surprised if a lot or even
most of the call sites called GetStringFromName() right after CreateBundle().
However, returning immediately from CreateBundle() would allow callers to call
that during start-up, so that GetStringFromName() might return quickly too,
depending on when it is called.

I think I've said what I wanted to say, several times now. Over and out.

Comment 33

[Judson Valeski]

erik, I hear what you're saying, but "returning quickly" doesn't matter if the
two calls are made in sequence on the same thread.

Comment 34

[tao]

Jud's suggestion of blocking in CreateBundle() seems to be safer and
accepted by all. I am going to implement that first. Although, I like Alec's
idea of blocking in the GetStringFromxxx() since it has the potential of
performance gain from async loading. I will look into alecf's idea in the
background.

As to the status of the patch, I believe that the only thing left is to
implement Jud's pseudo code in the CreateBundle() & onStreamComplete().

I probably will also attach the new async apis version for later reference.
It is based on the MN6 branch, though.

Comment 35

[Alec Flett]

well, in C++, we often do a CreateBundle/GetStringFromName quickly in sequence,
but in JavaScript, there I've noticed this pattern all over:

var bundle = srGetBundle(...)

function foo() {
   window.alert(bundle.GetStringFromName(...));
}

This pattern will benefit from async loading when blocking in GetStringFromName()

Also, Ben Goodger has a new <stringbundle/> XUL tag implemented in XBL, which
will call CreateBundle at initialization time (actually, when the CSS is
evaluated, which is after the onload handler)

and then you can say

function foo() {
  window.alert(document.getElementById("mybundle").getString(...));
}

this pattern also benefits.. I have a feeling that we do alot more
localization-related stuff in JS anyway, so I think there are more of these
patterns than the one that Jud just pointed out.

Comment 36

[tao]

Attachment: async loading using a new set of api; MN6 version. Will not checkin

Comment 37

[tao]

Attachment: async loading v2: applied to trunk and then cleanup; callers need to block itself until stream loaded

Comment 38

[tao]

hi, Jud:


Is nsIStreamLoader truely async? I try to block the calling thread in
createBundle() as suggested and found that Mozilla hangs at mon.wait()
and never wake up after :-\

Comment 39

[Judson Valeski]

yes, it's async. One of two things must be happening.

1. streamloader::Init() is failing. is the return code from your streamloader
creation ok?
2. you're not getting your OnStreamComplete() callback hit. Do you see it get
called?

Comment 40

[tao]

>yes, it's async. One of two things must be happening.

> 1. streamloader::Init() is failing. is the return code from your streamloader
> creation ok?

I assume it is since it is the same code segment from the first attached
patch (20370)which was tested against MN6 branch.

I'll double check.

> 2. you're not getting your OnStreamComplete() callback hit. Do you see it get
> called?

No. This was the first thing I checked. I'll look into it further once I got a
turn-around.

Comment 41

[Judson Valeski]

regarding the 12/08/00 15:17 patch.

nsStringBundle::Init() - please use a nsCOMPtr for the uri variable. Also use on
for the loader variable (which is currently leaked).

::onstreamcomplete() - not sure why you're releaseing aLoader. Looks like you're
compensating for the leak in the Init() method. You don't need to do this
though.

mLoaded - Not sure why you need this member var if you're blocking in
CreateBundle().

I'm not sure what you're doing w/ the nsIObserver stuff. Is that seperate from
the async loading?

Comment 42

[tao]

>regarding the 12/08/00 15:17 patch.
>
>nsStringBundle::Init() - please use a nsCOMPtr for the uri variable.
> Also use on for the loader variable (which is currently leaked).

I was mimicing NS_NewStreamLoader()'s usage in nsXULDocument.cpp.

>::onstreamcomplete() - not sure why you're releaseing aLoader. Looks like
> you're compensating for the leak in the Init() method. You don't need to do
> this though.
same reason above. I'll correct it.

>mLoaded - Not sure why you need this member var if you're blocking in
>CreateBundle().
That patch was done before adopting your suggestion of blocking in
createbundle(). That's why you don't see mon.wait() or mon.notify() in that
patch. The caller needs to call bundle->GetLoaded() to check if the resource
bundle is complete.

> I'm not sure what you're doing w/ the nsIObserver stuff. Is that seperate
> from the async loading?
The observer stuff is to notify the caller when the stream loading is complete.
It's like what StreamLoader does. With the implementation in this patch, the
caller gets to choose how the string bundle is loaded: synchronuously or
asynchronously. The caller calls CreateAsyncBundle() to 'benefit' from  async
loading.

Comment 43

[tao]

Jud,

>1. streamloader::Init() is failing. is the return code from your streamloader
> creation ok?

Just to confirm that the return status of streamloader creation is OK. But,
none of the streamloader's member funtions are called after mon.wait().

Comment 44

[Judson Valeski]

you should break in streamloader's OnStopRequest() method to see if it's even
being hit.

Comment 45

[tao]

I inserted printf in all of StreamLoader's member functions and the only one I
got is nsStreamLoader::Init() which returns '0'. In VC++ debugger, none of the
other member functions got called.

Comment 46

[Judson Valeski]

well, I'm a moron for two reasons:
1. I didn't recall the fact that nsIStreamLoader requires an event loop on the
calling side; and I wrote the stupid thing.
2. I pushed us down this road for days w/ out realizing it. 

I'm sorry.

nsIStreamLoader is a simplification interface so you don't have to muck w/
nsIStreamListener|Observer to do asynchronous I/O. It *doesn't* allow the caller
to get the async i/o benefits w/ out dropping into an event loop (duh! that
would redefine the notion of "async" :-/). Unfortunately, there are many string
bundle users that make top level string bundle calls synchronously (CreateBundle
or GetSTring*). Without reworking those callers to handle string loads via
asynchronous callbacks (prolly a boat load of work), they cannot be helped by
nsIStreamLoader|Observer(). 

Result: nsIStreamLoader is useless here, unless you want to provide *new* async
i/o functionality for string bundle users.

Comment 47

[Alec Flett]

wait, so why can't we use the nsIStreamLoaderObserver mechanism?

Comment 48

[Judson Valeski]

because nsIStreamLoaderObserver is just nsIStreamListener|Observer underneath,
and that requires the calling thread to process events (which it obviously isn't
doing if we've mon.wait()ed the thread :-/).

Comment 49

[tao]

Jud,

Per your comments, should we take the patch which you reviewed earlier?
This patch requires caller explicitly call CreateAsyncBundle() to load
resource bundle asynchronously. We will notify the caller upon loading
completed instead of blocking thread in StringBundle.

The callers of the new api will use the new api when the sync version does
not work well for them (in their env.)

Comment 50

[Judson Valeski]

hmm, well. now that I understand a little more about what you're using topics
for, I'd suggest just using nsIStreamListener|Observer to do the notification
(the topics nsIObservers aren't needed). Also, it's not clear to me how that
test works because it doesn't even have an event loop (which is required for
asynchronous operations).

Comment 51

[tao]

Attachment: Add async api, CreateAsyncBundle(), to strbundleservice

Comment 52

[tao]

Hi, Alec, Jud:


Would you review and sr the aptch attached above? To sum up:

 1. Code cleanup.
  * I removed the old style constructor of nsStringBundle() and use Init() or
    InitSync() to return the status/error code.
  * Use nsCOMPtr as suggested.

 2. New apis.
  * I add a new api, CreateAsyncBundle(), to nsIStrinBundleService so callers
    have an option to load the resource asynchronuously. CreateAsyncBundle()
    returns right away after creation of the new bundle and nsIStreamLoader.

    It will notify the caller upon completion of resource loading. In C++/JS,
    the caller should register as an observer of NS_STRBUNDLE_LOADED_TOPIC.

  * Added a new api, RegisterCallback(), to nsIStrinBundle for C caller. The
    callback will be called upon the completion of stream loading.
  * Added a new attribute to nsIStringBundle so callers can test if the resource
    binding is complete.

No current callers of strres need to change their usage unless they want to
take advantage of async loading or the sync version does not work well for them.

thx

Comment 53

[tao]

Attachment: add srGetAsyncBundle() to strres.js

Comment 54

[Alec Flett]

The patch looks good, a few things though:
- you should never directly compare anything to NS_OK, you should use
NS_SUCCEEDED() (I know the old code was already there, but since you're touching
that line, you might as well fix it)
- instead of using a callback, which is not scriptable, you should use the
nsIObserver interface, and notify callers via nsIObserver::Observe (note that
this does not imply going through the observer service)

honestly, I'm doubtful that anyone is ever going to use this... it's going to be
so much work just to be notified that a string bundle is loaded that everyone is
going to be lazy and just use the non-async version. Not to mention the
complexity it adds to every call site of CreateAsyncBundle - the value gained
from loading the bundle asyncronously seems lost to me just in the amount of
code you have to write and maintain just to make sure that it's loaded.

How much work would it be to put an event loop in the string bundle code so that
it can pump the events to load the string bundle? I still see value in that..
basically we make CreateBundle() return immediately (like we talked about
before) and then wait for the bundle to load using nsIStreamLoaderObserver.. if
the bundle still hasn't loaded when GetStringFromName is called, then we pump
the event queue until it arrives.

Comment 55

[Judson Valeski]

gotta love soul searching discussions regarding async operations, coupled w/
observer notification callbacks :-).

I'm skipping a code review until we resolve what we want (I suspect Tao's about
to INVALIDATE this bug if we go through another iteration of what it "should" do
:-)).

The bug was originally borne out of the use of a potentially broken API. I was
suggesting that we convert callers over to use an async API instead. I got shot
down quickly on that as string bundles are used at low levels and, I
think/thought, before we even have an event loop up in some cases. The bug was
left open in hopes that necko would provide honest sync i/o at some point
(hasn't happened).

If we're not going to convert old usage of sync string bundles to the new api
provided by Tao, or we don't have a scenario that can help drive the new api
usage, we should probably put this bug to bed.

alec, you commented that the win is lost w/ the overhead incurred w/ the new
async api impl. It may have room for optimization (I haven't looked), but the
whole idea is that the *current* model blocks the thread. Blocking a thread
versus slow/heavy code are two different things and I'd vote for the async api
just to get rid of the current blocking scenario.

Regarding the event loop addition... I like it! :-). We could use Tao's original
nsIStreamLoaderObserver patch w/ slight modifications (I've said that before
:-/). I would still make CreateBundle block though for two reasons:
1. I still argue that the callers aren't async anyway, so they're going to block
somewhere at sometime.
2. To package an event loop in the string bundle code means we're sticking a
while(1) in somewhere, and it might as well be in the root call (CreateBundle())
so we can avoid race conditions and flag member variables.

Here's what you'd replace the mon.Wait() call w/ 

...
NS_WITH_SERVICE(nsIEventQueueService, eventQService, kEventQueueServiceCID,
&rv);
if (NS_FAILED(rv)) return rv;
 
// create is only necessary if there's the potential that 
// CreateBundle() is being called before anyone else has 
//created a queue for the thead (I believe this is the case)
rv = eventQService->CreateThreadEventQueue();
if (NS_FAILED(rv)) return rv;
 
eventQService->GetThreadEventQueue(NS_CURRENT_THREAD, &eventQ);

while(mRunning) {
  PLEvent *gEvent;
  rv = gEventQ->WaitForEvent(&gEvent);
  //if failed return
  rv = gEventQ->HandleEvent(gEvent);
  //if failed return
}

stringbundle::OnStreamComplete(...) {
  mRunning = PR_FALSE;
....
}

...

Comment 56

[tao]

i'll try it out when I got a trunaround later... thx

Comment 57

[tao]

Attachment: async loading which blocks in Init().

Comment 58

[tao]

OK, here is the proposed async version of streamloading. I tried it out on both
win32 and linux. I haven't got a Mac build to test it, yet.

However, I might be halucinating, I think it is a bit slower than the sync
version. Therefore, please give it a try and let me know if you observe the same
thing. Since the sync apis still there, we can switch back and forth at will,
I'd like to check it in asap so Alec and I can add more stuff to strres module.

Let me know what you think?

thx

Comment 59

[Judson Valeski]

processing events isn't free. it may very well be slower. any chance you can
quantify it w/ printfs and times? Slow or not, we're now no longer blocking the
main thread (or any thread using string bundles for that matter) which is a
*good* thing.

Comment 60

[tao]

Measuring the loading time could only give us the information about strres bundle.
I am wondering if the introduced eventqueue stack would slow down tasks on other
thread.

To make the swtiching (sync v.s. async), I add a flag which is set via env. var.
I am attaching it below.

Comment 61

[tao]

Attachment: intl/strres/src/nsStringBundle.cpp: async when STRRES_ASYNC is set

Comment 62

[Judson Valeski]

now I'm having second thoughts about this (don't kill me tao :-/). So, the async
load indeed incurs overhead. If string bundles are ever planning on accessing
remote bundles (on a server, nfs or otherwise) the async impl makes sense. But,
if the data is always local (is it?) maybe we should stick to sync?

Comment 63

[Alec Flett]

theres alot here that I don't understand:
- are you allowed to call PR_GetEnv from a global scope like that? I'm surprised
that compiles :) - in any case, you should do it from the string bundle
service's constructor.
- why are you not always addref'ing on return of getStringBundle()? We should
always be addrefing the bundle before giving it back to the caller
- I thought we had agreed that we would not be doing a seperate API - if this is
true, why do we have callbacks? I'm glad to see the user of the observer
mechanism, but it seems like we should instead drop the c-function style
callback mechanism and instead just use the nsIObserver interface, and not go
through the observer service. (i.e. have a
nsIStringBundle::registerListener(nsIObserver *aListener))
But again, I don't think we should have a seperate API.
- we should not be flushing the string bundles when we do a cache flush, those
should be seperate operations.. and again I don't see any reason to go through
the observer service.

Comment 64

[tao]

>now I'm having second thoughts about this (don't kill me tao :-/). So, the async
>load indeed incurs overhead. If string bundles are ever planning on accessing
>remote bundles (on a server, nfs or otherwise) the async impl makes sense. But,
>if the data is always local (is it?) maybe we should stick to sync?
Most likely, it will be local. Even in the remote case, the property files
are packaged in *.jar.

The benefit of using async occurs when you know you definitely need str bundle
and request the bundle in something like JS onLoad() or C++'s object
constructor. When you call GetStringFromXXX(), mostlikely, the bundle is ready.

That's what I said, keep two sets of APIs and leave the call to the consumers.

Comment 65

[tao]

> theres alot here that I don't understand:
> - are you allowed to call PR_GetEnv from a global scope like that? I'm
> surprised that compiles :) - in any case, you should do it from the string
>  bundle service's constructor.
It's a quick hack before we are sure the eventqueue blocking async loading is
really what we want. It's a defect as it stands now; I'll move it to the constr
as suggested.

> - why are you not always addref'ing on return of getStringBundle()? We should
> always be addrefing the bundle before giving it back to the caller
I am always addrefing it.. In the async case, I did it right after the creation
of the new bundle (set addre to true); otherwise (addref == false), it will be
added before return.
> - I thought we had agreed that we would not be doing a seperate API -
In fact, there could be 3 sets of apis:
  1. sync (OpenInputStream()) -- the old style
  2. async w/o blocking -- true async: StreamLoader + observing consumer)
     I need add a new api to reuse part of current patch.
  3. async but blocking in Init() -- transparent to current consumers. This is
     what you are seeing now in the patch. The observer/callback stuff are not
     necessary for this.

We all like (3), but, it turns out there is an overhead for doing blocking
async. So, I am leaving the consumer a choice; use async when they can,
otherwise, stay with the sync one.

I will explain the rest later; I am late for my apt. BUt, could we get the
current patch in; we will decide how to deal with the non-used part of the patch.

thx

Comment 66

[tao]

> - I thought we had agreed that we would not be doing a seperate API -
> if this is true, why do we have callbacks?

> I'm glad to see the user of the
>  observer mechanism, but it seems like we should instead drop the
> c-function style callback mechanism and
The C-style callback is for consumers using async loading (2) from C function.
Mailnews has such example:
http://lxr.mozilla.org/mozilla/source/mailnews/mime/cthandlers/vcard/mimevcrd.cpp#1974

> instead just use the nsIObserver interface, and not go
> through the observer service. (i.e. have a
> nsIStringBundle::registerListener(nsIObserver *aListener))
Is there a strong reason (great benefit) of using registerListenser mechanism?
The nsIObserver approach is easy to implement in strres and probably as easy to
implement for the consumer. Besides, I don't need to introduce another new
intereface.
> - we should not be flushing the string bundles when we do a cache flush,
> those should be seperate operations.. and again I don't see any reason to
> go through the observer service.
I will take out the notification part in flushcachebundle().

Comment 67

[tao]

Attachment: add new async loading api: set env. var. STRRES_ASYNC to always do async

Comment 68

[tao]

In lacking of another review on the patch, I am marking this bug as remind. I
will submit another patch to change the nsStringBundle creation so that the
constructor does only initialization and a new function, Init(), does the
loading and bundling stuff.

Comment 69

[tao]

OK, I got a last minute review from erik based on discussion with danm.
reopening and checking it in.

Comment 70

[tao]

fixed checked in!

Comment 71

[tao]

.

Comment 72

[Pete Collins (MDG)]

*** Bug 76682 has been marked as a duplicate of this bug. ***

Comment 73

[Judson Valeski]

re-opening as perf. Pete (guy who just duped another bug w/ this one) mentioned
he's seeing a couple second bottleneck in here on startup. cc'ing him.

Comment 74

[tao]

Are you suggesting not to use OpenInputStream() which is what people get unless
they "setenv STRRES_ASYNC 1".

Comment 75

[Judson Valeski]

OpenInputStream() blocks the UI thread which is the perf issue that pete says
he's seeing.

Comment 76

[Pete Collins (MDG)]

I've waded through the labrynth of funct calls to here

nsNetUtil.h:

NS_OpenURI

ioService->NewChannelFromURI

Is where i see the hiccup begin. I'm still investigating.

--pete

 

Comment 77

[Pete Collins (MDG)]

Well, i wrapped the beginning and end of 

nsIOService::NewChannelFromURI  

w/ some printf's:
nsIOService::NewChannelFromURI

printf("BEGIN: NewChannelFromURI\n\n");
. . . 
printf("END: NewChannelFromURI\n\n");

Which in turn i wrapped the caller in NS_OpenURI

    printf("\n\nHRMMM 1. . .\n\n");
    rv = ioService->NewChannelFromURI(uri, &channel);
    printf("\n\nHRMMM 2. . .\n\n");


My output is this:

#
#
#
************ NS_OpenURI NEW!! ************
#
#
#


HRMMM 1. . .

BEGIN: NewChannelFromURI

BEGIN: NewChannelFromURI

END: NewChannelFromURI

BEGIN: NewChannelFromURI

END: NewChannelFromURI

BEGIN: NewChannelFromURI

END: NewChannelFromURI

BEGIN: NewChannelFromURI

END: NewChannelFromURI

BEGIN: NewChannelFromURI

END: NewChannelFromURI

BEGIN: NewChannelFromURI

END: NewChannelFromURI

END: NewChannelFromURI



HRMMM 2. . .





************ OUT #### NS_OpenURI NEW!! ************


This seems like erratic output. 

So i thought this looks a bit strange.

--pete

Comment 78

[tao]

Hi, Jud:


One way to fix this problem is to use non-blocking async bundling: 

  1. caller "new" an async bundle which kicks off an async loading and
     register itself as an observer of this loadingCompleteTopic; 
     non-eventloop is involved. 
  2. the caller runs off to do something eles
  3. in the meantime, the stringbundle works on property file loading and 
     bundling. When the bundling is done, StringBundle notifies the caller
     that bundle is ready for use.
  4. Caller calls bundle->GetStringFromName("foo").
 
The drawback is that should the caller needs the string before it is ready,
some fallback mechanisms are desired to prevent runtime error.

Comment 79

[Alec Flett]

I've said this a million times - I still think this is too complicated for the
caller. Nobody is ever going to use that API...

The real way this should work is this:
- caller creates string bundle
  - string bundle kicks off load in the background
- caller requests a string
  - if the bundle hasn't finished loading, the string bundle manually pumps the
events until the bundle is loaded
  - bundle returns the string

Comment 80

[tao]

I'm 100% for your proposal. But, is there a clean way blocking the calling
thread without introducing overhead? If yes, feel free to take this bug over.

If not, could we attack such bottleneck problems case by case instead of 
one cure for all? One obvious example is using XUL caching to lazily load 
bundle and prevent reloading the same bundle.

Comment 81

[Alec Flett]

the clean way is to put an event loop in there, but if jud knows of a way to
process just a single kind of necko message, that would be better...

as for caching, we already do caching of string bundles - it sounds like what
pete is encountering is the load of the first string bundle..

pete, the bundle has to be loaded SOMETIME, so really it's just a matter of
figuring out if we can offload it onto a seperate thread or not.

do you know what .properties file file is being loaded?

taking this bug, one of these days I'll fix this, just not now

Comment 82

[Pete Collins (MDG)]

Yea, Alec, I'm looking specifically at nsEmbedAPI.cpp. It is loading the
`necko.properties' file.

> pete, the bundle has to be loaded SOMETIME, so really it's just a matter of
> figuring out if we can offload it onto a seperate thread or not.

Agree, but the initial load that registers all of the chrome takes about 10
seconds. That is completely unacceptable. I'm not even addressing *that* issue.

;-)

I'm just stepping through things and this was the first performance hit i
encountered so i figured i'd investigate.

If we can shave off a second here a second there and a few milliseconds
somewhere else, that will begin to add up to greatly reducing mozilla's startup
time.

I think i understand now that by going through nsIInputStream we put everything
on hold until the thread can move forward. This wasn't clear to me before.

--pete

Comment 83

[Judson Valeski]

it's still async, but you can use a stream loader 
http://lxr.mozilla.org/mozilla/source/netwerk/base/public/nsIStreamLoader.idl if 
 all you want is a single callback. you'll still need to break the caller apart 
from data retrieval (and fire up an event loop), it's just easier to handle the 
single callback using the stream loader.

I agree w/ alec that wedging this model in at the string bundle caller level 
would be quite hard. I reopened this bug because it is obviously showing up on 
perf radar somewhere. if we want to resolve it WONTFIX (or future), that's fine, 
but we need to be clear that we're paying a heavy penalty using 
OpenInputStream() here.

Comment 84

[Pete Collins (MDG)]

You might want to look at some findings i came accross here.
It is related to some extent.

http://bugzilla.mozilla.org/show_bug.cgi?id=76865

--pete

Comment 85

[Alec Flett]

the current code is very broken.. I've just discovered that this whole callback
mechanism doesn't work at all.. why? Because if you are using the "async" stuff
(which is not really async) then what happens is this:

var bundle = stringBundleService.CreateBundle("chrome://foo/bar.properties") 
// creates the bundle, using an event loop.. returns when the bundle has
finished loading

bundle.RegisterCallback(foo, closure);
// callback is never called, because the bundle has already been loaded!

so 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.

This does NOT fix the actual issue of async bundle loading, but it at least
makes them lazy. 

Once I attach the patch, I'll be looking for reviewers!

Comment 86

[Alec Flett]

at valeski's request, spun off into bug 76944

Comment 87

[Paul Chen]

nav triage team:

We want this code, but it's not a beta stopper. Marking nsbeta1-

Comment 88

[Viswanath Ramachandran]

Nav triage team:  not needed for 0.9.4 eMojo, leaving in 1.0.

Comment 89

[Cathleen]

discussed in perf triage meeting today.  agreed to put this one off... for later.

Comment 90

[Cathleen]

performance triage meeting, changing milestone 0.9.6

Comment 91

[benc]

Can some look at bug 76682 and see if the bugs describe the same problem? TIA.

Comment 92

[Alec Flett]

moving out to 0.9.7, I never said I had time to do this, and it's not even clear
there is any way to do this in a way that would improve performance.

Comment 93

[Alec Flett]

this just isn't buying us any performance win right now. pushing out to 0.9.8

Comment 94

[Alec Flett]

pushing out non-critical bugs past moz 1.0

Comment 95

[basic]

if this isn't giving any performance improvements why does it block bug 7251 ?

Comment 96

[Alec Flett]

it doesn't now.

Comment 97

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 98

[Zibi Braniecki [:zbraniecki][:gandalf]]

We're not going to invest in StringBundle anymore.