74815 - Charset converters' use of libreg is causing lots of disk reads at startup

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Simon Fraser [no longer active]]

The charset converters make pretty heavy use of the Components Registry. The call 
to nsCharsetConverterManager::GetRegistryEnumeration2() causes about 190 10K disk 
reads from the registry on startup.

For stack traces of each fread() in libreg, see
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=29755>

Comment 1

[Simon Fraser [no longer active]]

To Dan.

Comment 2

[Jaime Rodriguez, Jr.]

Dan is on vacation. Reassigning to Roy Yokoyoma for evaluation.

Adding intl keyword.

Comment 3

[Frank Tang]

Yes, we use libreg to register a available charset so we can iterate throuh 
available charset eoncverter. Is there a better way to do that. This should only 
happen one time for the first time. 

Comment 4

[Chris Waterson]

ftang: couldn't we go to the registry when we needed to find a particular 
converter? (Instead of eagerly pre-populating it on startup?) How is this 
information used? Can you point me to the file?

Comment 5

[Jaime Rodriguez, Jr.]

Adding nsbeta1 keyword.

Comment 6

[Roy Yokoyama]

Attachment: declare encoder/decoder member data in nsCharsetMenu

Comment 7

[Roy Yokoyama]

Instead of getting the encoder/decoder list every 
Browser/Mail/Composer/Other::Init() and menu refresh,
I declared member data so that we only need to
query the Registry once and keep them around.

ftang/sfraser: can you /r= for the patch?

Comment 8

[Roy Yokoyama]

Attachment: Oops. I should allow for RefreshMailviewMenu() to read the list.

Comment 9

[Roy Yokoyama]

ftang: can you review?

Comment 10

[Simon Fraser [no longer active]]

With these changes, we still trawl through the registry once on startup, right? I 
was thinking more along the lines of not using the registry at all for this data 
(store it in a flat text file or something).

Comment 11

[Roy Yokoyama]

sfraser: you are right for my patch. We read the registry entries once
at the start-up.   I believe, however, the static data file
may not be a good idea.

As I understand correctly and correct me if I am wrong, that
we are allowing to add the unicode converters on the fly.
(ie. copying a new unicode converter dll into \dist\coomponents will
 display the new menu item in the apps.  Personally, I'd like to see
 Klingon unicode coverter.... )
I need confirmation from ftang on this, though.....

Frank, any comments?

Comment 12

[Frank Tang]

sfraser- Is registry a text file?
Can we check in the current patch and improve the perofrmance first. And we keep 
improve other idea as you suggest later. :)

Comment 13

[Roy Yokoyama]

got /r=ftang

Comment 14

[Roy Yokoyama]

brendan: can you /sr = ?

Comment 15

[Marek Z. Jeziorek]

TM to 0.9.2 per PDT triage (it's OK to check it in by Friday or after 0.9.1
branch is made).

Comment 16

[Brendan Eich [:brendan]]

Looks ok to me, except for pre-existing misspelling:

nsresult nsCharsetMenu::InitSecodaryTiers()

Fix if you can, whenever convenient.

But this patch does nothing about the bug's summary complaint: lots of libreg
disk reads at startup.  It just avoids non-startup repetition of that work, if I
understand correctly.  The bug should therefore stay open even after this patch
goes in, so that we can do something in a future milestone to cut back on eager
disk reads.

For example, instead of changing to a flat file format and not trawling through
the registry on startup, could we not trawl only on first activation of the
charset menu?  I.e., make the user wait the first time that menu item is
selected, but do not make all users wait at startup?

/be

Comment 17

[Brendan Eich [:brendan]]

I mistyped an extra "not" -- I meant "could we trawl only on first activation of
the charset menu?"

/be

Comment 18

[Frank Tang]

brendan- you are right. We should keep this bug open even after we check in the 
current patch. We want to do improvement step-by-step. Thanks for your 
suggestion. One thing we are not clear about sfraser's suggestion about the 
registry and the flat file. 
Also, I belive this patch also will reduce the startup time call to libreg. We 
agree there are still some space to improve this bug. And we think the right 
thing to do is to done one thing at a time step-by-step to reduce the risk. 

Comment 19

[Frank Tang]

roy- check in the code and leave this bug open. 
After you check in, please clear the status whiteboard . Thanks

Comment 20

[Simon Fraser [no longer active]]

> One thing we are not clear about sfraser's suggestion about the
> registry and the flat file. 

What's not clear? Libreg is not a good database; it's inefficient and slow. You'd 
get much better performance, if you just need persistence between runs, to roll 
your own on-disk backing store for the data you want to save.

Comment 21

[Roy Yokoyama]

Patch is checked-in with typo fix (InitSecodaryTiers).
Leaving the bug OPEN/ASSIGNED since not all the issues are resolved.

Comment 22

[Frank Tang]

>Libreg is not a good database; it's inefficient and slow. 
should we then make it fast instead creat yet another thing?
Why is it slow? How many other places using it? If the libreg is not efficient, 
then we should fix it - for this particular issue and also for other caller.

> if you just need persistence between runs, to roll 
> your own on-disk backing store for the data you want to save
Any suggestion what class should we use?

Comment 23

[Jaime Rodriguez, Jr.]

Marking as P2, since this hits startup Perf.

Comment 24

[Frank Tang]

yokoyama, I think there are two parallel improvement could be done in here.
1. change the file format and API as simon suggest- which I personally disagree. 
2. reduce the number of calls to find the list-
I think these two are parallel to each other, there are no reason we only want 
to do one without the other, 
can you start to look at 2 and maybe sfraser can give us a good suggestion of 1?

Comment 25

[Simon Fraser [no longer active]]

I can't give a good suggestion about how to remove your dependency on libreg. 
Talk to dveditz about its performance as a database, see if you can make fewer 
libreg calls, profile it, and maybe then make a decision about whether another 
persistence format would be better for you.

Comment 26

[Cathleen]

unfortunately dveditz is on sabatical... :-(

anyone else can help figure this out?

Comment 27

[Frank Tang]

It's nice to gain performance by this bug but not critical for moz0.9.2. move it 
to moz0.9.3 

Comment 28

[Frank Tang]

mark it as future. 

Comment 29

[Roy Yokoyama]

as frank suggests, assign to jbetak.

Comment 30

[[e-mail] (away - not reading bugmail)]

accepting - we has some initial discussion with dp on how we could eliminate 
some XPCOM overhead. Major part of our initial effort will be a fix for bug 
64146, which will eliminate a good deal of registry and disk reads.

Comment 31

[Cathleen]

We are planning on getting rid of storing registry data in memory, because it's
taking 500K.

Once that's reverted to where we were 6 mo. ago, we will need charset converters
to be more efficient, not causing too much registry reads at startup.

Resetting FUTURE.

ftang and jbetak, can you guys help?

Comment 32

[[e-mail] (away - not reading bugmail)]

I'll try to get some profiling  data for this as well - charset registration
should have some wiggle room, which could be used to further improve startup.

Comment 33

[Frank Tang]

jbetak's contract is up. Bulk move bugs to ftang

Comment 34

[Frank Tang]

I believe with jbetak's recent performance work, such usage already been delay .
Mark this future. mark this as "---" if you still think it is a huge startup
problem 

Comment 35

[Daniel Veditz [:dveditz]]

I'm not seeing this anymore, the registry hit comes later when you open the
menu. Calling this FIXED