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

RESOLVED FIXED in Future

Status

()

Core
Internationalization
P2
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: Simon Fraser, Assigned: Frank Tang)

Tracking

({footprint, intl, perf})

Trunk
Future
PowerPC
Mac System 8.5
footprint, intl, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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>
(Reporter)

Updated

17 years ago
Keywords: perf
(Reporter)

Comment 1

17 years ago
To Dan.
Assignee: tao → dveditz

Updated

17 years ago
Blocks: 27510

Comment 2

17 years ago
Dan is on vacation. Reassigning to Roy Yokoyoma for evaluation.

Adding intl keyword.
Assignee: dveditz → yokoyama
Keywords: intl

Updated

17 years ago
Target Milestone: --- → mozilla0.9.1
(Assignee)

Comment 3

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

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

17 years ago
Adding nsbeta1 keyword.
Keywords: nsbeta1

Updated

17 years ago
Status: NEW → ASSIGNED

Comment 6

17 years ago
Created attachment 34439 [details] [diff] [review]
declare encoder/decoder member data in nsCharsetMenu

Comment 7

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

17 years ago
Created attachment 34554 [details] [diff] [review]
Oops.  I should allow for RefreshMailviewMenu()  to read the list.

Comment 9

17 years ago
ftang: can you review?

(Reporter)

Comment 10

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

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


Updated

17 years ago
Whiteboard: Waiting for /r=
(Assignee)

Comment 12

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

17 years ago
got /r=ftang
Whiteboard: Waiting for /r= → Waiting for /sr=

Comment 14

17 years ago
brendan: can you /sr = ?

Comment 15

17 years ago
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).
Target Milestone: mozilla0.9.1 → mozilla0.9.2
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
I mistyped an extra "not" -- I meant "could we trawl only on first activation of
the charset menu?"

/be
(Assignee)

Comment 18

17 years ago
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. 
(Assignee)

Comment 19

17 years ago
roy- check in the code and leave this bug open. 
After you check in, please clear the status whiteboard . Thanks
Whiteboard: Waiting for /sr= → Got sr. Wait for tree to open to land the first patch.
(Reporter)

Comment 20

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

17 years ago
Patch is checked-in with typo fix (InitSecodaryTiers).
Leaving the bug OPEN/ASSIGNED since not all the issues are resolved.
Whiteboard: Got sr. Wait for tree to open to land the first patch.
(Assignee)

Comment 22

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

17 years ago
Marking as P2, since this hits startup Perf.
Priority: -- → P2
(Assignee)

Comment 24

17 years ago
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?
(Reporter)

Comment 25

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

17 years ago
unfortunately dveditz is on sabatical... :-(

anyone else can help figure this out?
(Assignee)

Comment 27

17 years ago
It's nice to gain performance by this bug but not critical for moz0.9.2. move it 
to moz0.9.3 
Target Milestone: mozilla0.9.2 → mozilla0.9.3
(Assignee)

Comment 28

17 years ago
mark it as future. 
Target Milestone: mozilla0.9.3 → Future

Updated

17 years ago
Hardware: All → Macintosh

Updated

17 years ago
Assignee: yokoyama → jbetak
Status: ASSIGNED → NEW

Comment 29

17 years ago
as frank suggests, assign to jbetak.
(Assignee)

Updated

17 years ago
Blocks: 103175

Updated

17 years ago
Blocks: 104166
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.
Status: NEW → ASSIGNED

Comment 31

17 years ago
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?
Target Milestone: Future → ---

Updated

17 years ago
Blocks: 76329
Keywords: footprint
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.
(Assignee)

Comment 33

16 years ago
jbetak's contract is up. Bulk move bugs to ftang
Assignee: jbetak → ftang
Status: ASSIGNED → NEW
(Assignee)

Comment 34

16 years ago
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 
Status: NEW → ASSIGNED
Target Milestone: --- → Future
I'm not seeing this anymore, the registry hit comes later when you open the
menu. Calling this FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
QA Contact: andreasb → kasumi
You need to log in before you can comment on or make changes to this bug.