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

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 1

[nhottanscp]

Reassign to cata, cc to ftang.

Comment 2

[Frank Tang]

move all cata's bug to ftang

Comment 3

[Frank Tang]

cc yokoyama and bstell

Comment 4

[nhottanscp]

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.

Comment 5

[Matthew T (active 1999-2002)]

Retaining sort keys would (I think) also fix bug 54746.

This delay could be taken out of startup time by implementing bug 10999.

Comment 6

[Frank Tang]

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

Comment 7

[Simon Fraser [no longer active]]

See also bug 68075

Comment 8

[Frank Tang]

nhotta- can you help ?

Comment 9

[nhottanscp]

Attachment: Patch, changed to store collation key

Comment 10

[nhottanscp]

I ran Quantify, F+D time of nsCharsetMenu::ReorderMenuItemArray decreased from 
38149.77 to 6438.71 (microseconds).

Comment 11

[nhottanscp]

Attachment: Patch, fixed a memory leak

Comment 12

[nhottanscp]

r=ftang

Comment 13

[nhottanscp]

Attachment: Patch, delay charset menu creation.

Comment 14

[nhottanscp]

*** Bug 68378 has been marked as a duplicate of this bug. ***

Comment 15

[Erik van der Poel]

sr=erik for 2nd patch

Comment 16

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 17

[nhottanscp]

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).

Comment 18

[nhottanscp]

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();
  }

Comment 19

[Blake Ross]

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.

Comment 20

[nhottanscp]

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).

Comment 21

[Simon Fraser [no longer active]]

Can we just build the menus on a timer some time after startup?

Comment 22

[nhottanscp]

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.

Comment 23

[Roy Yokoyama]

My bug number is 74815.  I think my first patch contributed a bit.

Comment 24

[nhottanscp]

Okay, what is the remaining change? Which part of nsCharsetMenu.cpp will be changed?

Comment 25

[Frank Tang]

yokoyama- please also take care this one. 
mark it as moz0.9.3

Comment 26

[Frank Tang]

mark it as future.  We already did some work here. We may improve it faster later. 

Comment 27

[Blake Ross]

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.

Comment 28

[Roy Yokoyama]

Accepting.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Filed bug 91231 on not initializing at startup.

Comment 30

[Frank Tang]

assign Linux performance work to bstell. consider for m94

Comment 31

[Frank Tang]

move to 94

Comment 32

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

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.

Comment 33

[kill this account]

re-assign to Juraj

Comment 34

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

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.

Comment 35

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

Attachment: patch v1.0 - still need some work, although it appears to be functionally complete, reduces charset menu startup time to <0 ms

Comment 36

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

Attachment: finalized patch - soliciting early review feedback, working on XP verification

Comment 37

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

Attachment: tweaked some things related to observer registration

Comment 38

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

cc'ing dp for review/architectural feedback.

Comment 39

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

Attachment: updated patch - the "SetCurrentCharset" extension is still missing from this version

Comment 40

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

Attachment: Finalized patch - this should address all issue nhotta and I were able to see

Comment 41

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

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.

Comment 42

[Suresh Duddi (gone)]

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");

Comment 43

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

Attachment: updated patch - thanks dp!

Comment 44

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

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.

Comment 45

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

cc'ing brendan for potential i18n super-review

Comment 46

[Alec Flett]

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.

Comment 47

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

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...

Comment 48

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

Attachment: revised patch - this hopefully addresses the issues alecf has raised

Comment 49

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

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

Comment 50

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

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.

Comment 51

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

fixed

Comment 52

[jag (Peter Annema)]

*** Bug 91231 has been marked as a duplicate of this bug. ***