Closed
Bug 74815
Opened 23 years ago
Closed 23 years ago
Charset converters' use of libreg is causing lots of disk reads at startup
Categories
(Core :: Internationalization, defect, P2)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: sfraser_bugs, Assigned: ftang)
References
Details
(Keywords: intl, memory-footprint, perf)
Attachments
(2 files)
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 2•23 years ago
|
||
Dan is on vacation. Reassigning to Roy Yokoyoma for evaluation. Adding intl keyword.
Assignee: dveditz → yokoyama
Keywords: intl
Updated•23 years ago
|
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 3•23 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•23 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?
Updated•23 years ago
|
Status: NEW → ASSIGNED
Comment 6•23 years ago
|
||
Comment 7•23 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•23 years ago
|
||
Comment 9•23 years ago
|
||
ftang: can you review?
Reporter | ||
Comment 10•23 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•23 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•23 years ago
|
Whiteboard: Waiting for /r=
Assignee | ||
Comment 12•23 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 14•23 years ago
|
||
brendan: can you /sr = ?
Comment 15•23 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
Comment 16•23 years ago
|
||
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•23 years ago
|
||
I mistyped an extra "not" -- I meant "could we trawl only on first activation of the charset menu?" /be
Assignee | ||
Comment 18•23 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•23 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•23 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•23 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•23 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?
Assignee | ||
Comment 24•23 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•23 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•23 years ago
|
||
unfortunately dveditz is on sabatical... :-( anyone else can help figure this out?
Assignee | ||
Comment 27•23 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
Updated•23 years ago
|
Assignee: yokoyama → jbetak
Status: ASSIGNED → NEW
Comment 29•23 years ago
|
||
as frank suggests, assign to jbetak.
Comment 30•23 years ago
|
||
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•23 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•23 years ago
|
Comment 32•23 years ago
|
||
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•23 years ago
|
||
jbetak's contract is up. Bulk move bugs to ftang
Assignee: jbetak → ftang
Status: ASSIGNED → NEW
Assignee | ||
Comment 34•23 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
Comment 35•23 years ago
|
||
I'm not seeing this anymore, the registry hit comes later when you open the menu. Calling this FIXED
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•22 years ago
|
QA Contact: andreasb → kasumi
You need to log in
before you can comment on or make changes to this bug.
Description
•