Closed Bug 64146 Opened 19 years ago Closed 18 years ago

Delay nsCharsetMenu initialization to avoid consuming ~2% of startup time

Categories

(Core :: Internationalization, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: dbaron, Assigned: jbetak)

References

Details

(Keywords: perf, Whiteboard: [br][nav+perf] needs sr)

Attachments

(1 file, 9 obsolete files)

35.16 KB, patch
jbetak
: review+
jbetak
: superreview+
Details | Diff | Splinter Review
The constructor for nsCharsetMenu seems, according to both a jprof profile and
printfs that I put at the beginning and end of the constructor, to take about
1/20 of our startup time (about half a second on my machine).  It is called once
during startup.

This seems a bit excessive, considering that it looks like what it is doing is
sorting a bunch of menus that don't need to be shown yet (7 of the 10 jprof
stack traces end up within the call to NS_QuickSort in
nsCharsetMenu::ReorderMenuItemArray).  If it can't be made faster (which I
certainly hope it could be), perhaps this initialization could be delayed until
a menu is shown?  For most runs of the browser this probably won't happen at
all, since most of the time seems to deal with the "More" menu (if I understand
the code correctly).
Reassign to cata, cc to ftang.
Assignee: nhotta → cata
Status: NEW → ASSIGNED
move all cata's bug to ftang
Assignee: cata → ftang
Status: ASSIGNED → NEW
cc yokoyama and bstell
I think the sorting of charset menu items is currently not storing sort keys.
Changing it to keep sort keys around may help speeding it up.
Blocks: 7251
Retaining sort keys would (I think) also fix bug 54746.

This delay could be taken out of startup time by implementing bug 10999.
We should do the following thing
1. lazy init untill method in public interface access it
2. improve the sorting by changing the sorting model- store sort key into the 
menu entry
mark as P2 moz0.9
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9
nhotta- can you help ?
Assignee: ftang → nhotta
Status: ASSIGNED → NEW
I ran Quantify, F+D time of nsCharsetMenu::ReorderMenuItemArray decreased from 
38149.77 to 6438.71 (microseconds).
Attached patch Patch, fixed a memory leak (obsolete) — Splinter Review
r=ftang
Status: NEW → ASSIGNED
*** Bug 68378 has been marked as a duplicate of this bug. ***
sr=erik for 2nd patch
Changed QA contact to nhotta@netscape.com.
QA Contact: teruko → nhotta
The sorting change was checked in.
Is this still a bottleneck for Linux start up?
Currently, nsCharsetMenu constructor spends 323,675.98 microseconds at stat up
(my machine is PIII 500MHz, Window2000).
I did another test on slower machine (PII 233MHz, 128MB, WinNT4).
The start up time is the same (about 16 seconds) with or without the menu item
initialization code in nsCharsetMenu contructor. I used debug builds.

I got a suggestion by waterson that the menu construction can be delayed like below.
For now, I retarget rest of the change as future.


<menu datasources="rdf:null" oncreate="loadCharsetMenu(event.target);"
...>

and then, in the JS,

  function loadCharsetMenu(menu)
  {
     var RDF =
Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(nsIRDFService);
    
menu.datasources.AddDataSource(RDF.GetDataSource("rdf:charset-menu"));
     menu.builder.rebuild();
  }
Target Milestone: mozilla0.9 → Future
nsCharsetMenu's ctor is still showing up as 2.2% of startup time for me, which 
seems like too much given that it's init'ing menus for seemingly no reason.  
Was the second patch (id 26223) ever checked in?  It doesn't look like it.
Whiteboard: [br]
The patch to delay charset menu creation, that's not checked in.
One problem I saw was that it made submenus very slow to appear (took several
seconds before showing up).
Can we just build the menus on a timer some time after startup?
Clear the milestone.
Roy, does your changes for the converter registration would affect this? Do you 
have a bug number?
Changed the summary from 1/20 to 2.2% based on the comment by Blake Ross 
2001-05-05 09:48.
Summary: nsCharsetMenu::nsCharsetMenu takes 1/20 of startup time → nsCharsetMenu::nsCharsetMenu takes 2.2% of startup time
Target Milestone: Future → ---
My bug number is 74815.  I think my first patch contributed a bit.
Okay, what is the remaining change? Which part of nsCharsetMenu.cpp will be changed?
yokoyama- please also take care this one. 
mark it as moz0.9.3
Assignee: nhotta → yokoyama
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.3
Whiteboard: [br] → [br][nav+perf]
mark it as future.  We already did some work here. We may improve it faster later. 
Target Milestone: mozilla0.9.3 → Future
I think most users would rather wait a couple seconds the first time they want 
to open the menu, given that many users may not use this feature.
Accepting.
Status: NEW → ASSIGNED
Filed bug 91231 on not initializing at startup.
No longer blocks: 7251
Blocks: 7251
assign Linux performance work to bstell. consider for m94
Assignee: yokoyama → bstell
Status: ASSIGNED → NEW
Target Milestone: Future → ---
move to 94
Target Milestone: --- → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
bstell: I'd like to help out with this, since Frank wants me to focus on 
performance. Please let me know, how we can coordinate this work or possibly 
reassign this bug to me.
re-assign to Juraj
Assignee: bstell → jbetak
Blocks: 103175
Target Milestone: mozilla0.9.5 → mozilla0.9.6
modifying summary and accepting - I'll post a preliminary patch this weekend. 

First (very rogue) atempts show a savings of ~70ms (from 100ms down to 30ms), 
which is ~1.4% of my local start-up time. I'm quite certain I can optimize this 
even more once I shuffled more code around in nsCharsetMenu.
Status: NEW → ASSIGNED
Summary: nsCharsetMenu::nsCharsetMenu takes 2.2% of startup time → Delay nsCharsetMenu initialization to avoid consuming ~2% of startup time
Attachment #53675 - Attachment is obsolete: true
Attachment #26223 - Attachment is obsolete: true
Attachment #25959 - Attachment is obsolete: true
Attachment #25947 - Attachment is obsolete: true
cc'ing dp for review/architectural feedback.
Attachment #53879 - Attachment is obsolete: true
Attachment #53978 - Attachment is obsolete: true
Attachment #55025 - Attachment is obsolete: true
nhotta, ftang, dp: 

would one of you guys be able to spend some time looking over the proposed 
changes and help to get this rolling with a review? I'd really like to have it 
checked in ASAP to get some QA exposure in M096.
The following should be NS_TIMELINE_STOP and MARK I think. Otherwise the changes
look ok.  r=dp

@@ -757,20 +818,30 @@
   nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(mPrefs);
   if (pbi)
     res = pbi->AddObserver(kBrowserStaticPrefKey, mCharsetMenuObserver, PR_FALSE);
+  }
+
+  mBrowserMenuInitialized = NS_SUCCEEDED(res);
+
+  NS_TIMELINE_START_TIMER("nsCharsetMenu::InitBrowserMenu");
+  NS_TIMELINE_START_TIMER("nsCharsetMenu::InitBrowserMenu");

oh I think I misunderstood your comment, I changed the two alleged macros in 
the most recent patch to NS_TIMELINE_STOP_TIMER and NS_TIMELINE_MARK_TIMER.
Attachment #55200 - Attachment is obsolete: true
cc'ing brendan for potential i18n super-review
Whiteboard: [br][nav+perf] → [br][nav+perf] needs sr
Attachment #55274 - Attachment description: updated patch - thanks dp! BTW NS_TIMELINE_START_TIMER is the only thing I could find, I'll keep using it for now: http://lxr.mozilla.org/seamonkey/search?string=NS_TIMELINE_START → updated patch - thanks dp!
Comment on attachment 55274 [details] [diff] [review]
updated patch - thanks dp! 

seems like this is a perfect opportunity for PRPackedBool..

in nsCharsetMenuObserver::Observe, your NS_TIMELINE stuff says "nsCharsetMenu:Init" - is that supposed to be "...::Observe"?

You're initializing lots of variables to static values in nsCharsetMenu::nsCharsetMenu() - you could speed that up by using the initialization syntax

nsCharsetMenu::nsCharsetMenu() : mInitialized(PR_FALSE), mOthersInitialized(PR_FALSE), etc

indenting is messed up in mCCManager->GetEncoderList


- there are other menus in the project besides the charset menu - CreateMenu should be called CreateCharsetMenu()

Actually, what I don't get about any of this charset stuff is why we don't lazily create the resources when we get a GetTargets() on a well-known resource.
it seems like all this Observer stuff could be completely avoided.
thanks alec! I will repost a modified patch tonight. Meanwhile - your 
proposition sounds quite intriguing. Could you sketch it out a bit more? I'm 
not that familiar with this part of the code base. 

If adding one observer represents additional overhead which could easily be 
avoided, I'd go for it. However I'm most concerned with getting the charset 
menu monster out of startup sequence ASAP, so if the approach you are proposing 
is not incompatible with the currently discussed changes and could be done at a 
later stage, I'd definitely prefer that...
Attachment #55274 - Attachment is obsolete: true
Comment on attachment 55958 [details] [diff] [review]
revised patch - this hopefully addresses the issues alecf has raised

dp's review should extend to this revision
Attachment #55958 - Flags: review+
alec:

I made an attempt to comply with all requested changes except for the 
indentation (line 840, mCCManager->GetEncoderList) - seems like this spot 
appears distorted in the patch due to the unidiff option.
Attachment #55958 - Flags: superreview+
fixed
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 91231 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.