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>
Assignee: tao → dveditz
Dan is on vacation. Reassigning to Roy Yokoyoma for evaluation. Adding intl keyword.
Assignee: dveditz → yokoyama
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.
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?
Adding nsbeta1 keyword.
Created attachment 34439 [details] [diff] [review] declare encoder/decoder member data in nsCharsetMenu
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?
Created attachment 34554 [details] [diff] [review] Oops. I should allow for RefreshMailviewMenu() to read the list.
ftang: can you review?
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).
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?
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. :)
Whiteboard: Waiting for /r= → Waiting for /sr=
brendan: can you /sr = ?
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
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.
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.
> 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.
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.
>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?
Marking as P2, since this hits startup Perf.
Priority: -- → P2
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?
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.
unfortunately dveditz is on sabatical... :-( anyone else can help figure this out?
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
mark it as future.
Target Milestone: mozilla0.9.3 → Future
as frank suggests, assign to jbetak.
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
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 → ---
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.
jbetak's contract is up. Bulk move bugs to ftang
Assignee: jbetak → ftang
Status: ASSIGNED → NEW
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
You need to log in before you can comment on or make changes to this bug.