Opening folder: Doing UpdateCommands("mail-toolbar") several times

RESOLVED EXPIRED

Status

RESOLVED EXPIRED
18 years ago
5 years ago

People

(Reporter: cavin, Unassigned)

Tracking

({perf})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 obsolete attachments)

(Reporter)

Description

18 years ago
This is filed against folder loading performance.

Scenario: Start up mailnews program and then open( click on) a local folder
with 1000 msgs.

I started out by adding some timing code to java function ChangeFolderByURI() in
base\resource\contgent\commandglue.js to see where time was spent when opening
a folder and here is what I got:

++++++++++++++++++++++++++++++++++++++++++++++++++++++
In ChangeFolderByURI uri = mailbox://nobody@Local%20Folders/Big1000Msgs sortType
 = 18
*** Performance check #2: time used = 10
*** Performance check #3: time used = 10
*** Performance check #4: time used = 10
*** Performance check #5: time used = 20
*** Performance check #6: time used = 20
It takes 237 milliseconds for nsMsgLocalMailFolder::UpdateFolder() to finish
*** Performance check #7: time used = 260
WEBSHELL- = 4
 (150 milliseconds is used here)
*** Performance check #11: time used = 410
++++++++++++++++++++++++++++++++++++++++++++++++++++++

It takes 150 milliseconds between 'Performance check #7' and 'Performance check 
#11' and the time was spent on the following statement:

  document.commandDispatcher.updateCommands('mail-toolbar');

where 'updateCommands' is defined in 
content/xul/document/src/nsXULCommandDispatcher.cpp (as 
nsXULCommandDispatcher::UpdateCommands()).

Note that 'Performance check #11' gets printed out before UpdateCommands() 
returns (ie, the last statement of the function).

Then I added some more code to nsXULCommandDispatcher::UpdateCommands() to 
figure out what it's doing and here is what I got:

++++++++++++++++++++++++++++++++++++++++++++++++++++++
In ChangeFolderByURI uri = mailbox://nobody@Local%20Folders/Big1000Msgs sortType
 = 18
*** Performance check #2: time used = 10
*** Performance check #3: time used = 10
*** Performance check #4: time used = 20
*** Performance check #5: time used = 30
*** Performance check #6: time used = 30
It takes 260 milliseconds for nsMsgLocalMailFolder::UpdateFolder() to finish
*** Performance check #7: time used = 291
Doing UpdateCommands("mail-toolbar")
  UpdateCommands point #1: 4
WEBSHELL- = 4
  UpdateCommands point #2: 152
  UpdateCommands point #3: 153
*** Performance check #11: time used = 451
Doing UpdateCommands("focus")
  UpdateCommands point #1: 2
Doing UpdateCommands("mail-toolbar")
  UpdateCommands point #1: 1
  UpdateCommands point #2: 91
  UpdateCommands point #3: 92
  UpdateCommands point #2: 206
  UpdateCommands point #2: 241
  UpdateCommands point #3: 242
Doing UpdateCommands("focus")
  UpdateCommands point #1: 1
Doing UpdateCommands("mail-toolbar")
  UpdateCommands point #1: 1
  UpdateCommands point #2: 107
  UpdateCommands point #3: 108
  UpdateCommands point #2: 204
  UpdateCommands point #2: 241
  UpdateCommands point #3: 242
Doing UpdateCommands("focus")
  UpdateCommands point #1: 1
  UpdateCommands point #2: 7
  UpdateCommands point #2: 41
  UpdateCommands point #3: 42
Doing UpdateCommands("focus")
  UpdateCommands point #1: 1
  UpdateCommands point #2: 7
  UpdateCommands point #2: 55
  UpdateCommands point #3: 56
++++++++++++++++++++++++++++++++++++++++++++++++++++++

Here, we can tell that between 'Performance check #7' and 'Performance check 
#11' we issued 1 UpdateCommands("mail-toolbar") call and immediately after 
'Performance check #11' another 2 UpdateCommands("mail-toolbar") calls were 
invoked along with 4 UpdateCommands("focus") calls. The first 
UpdateCommands("mail-toolbar") call is invoked by the java function 
ChangeFolderByURI() so this is expected. But the two extra 
UpdateCommands("mail-toolbar") calls which were triggered by the associated 
UpdateCommands("focus") calls was somehting I did not expect to see and they 
take up 242*2= 484 milliseconds!

The question is do we really need all 3 UpdateCommands("mail-toolbar") calls? If 
we can just save one or two such calls here we'll be gaining a big percentage in 
folder opening time.

One thing to note is that the execution time for UpdateCommands("mail-toolbar") 
calls seem to be very stable and is not affected by the size of the folder.

Another side note here: UpdateCommands("focus") triggers 
UpdateCommands("mail-toolbar") call because in 
nsXULCommandDispatcher::UpdateCommands() it calls content->HandleDOMEvent() 
which eventually invokes UpdateCommands("mail-toolbar") (ie, recursive).
Keywords: perf
OS: Windows NT → All
QA Contact: esther → stephend

Updated

18 years ago
Keywords: nsbeta1+
Priority: -- → P1
Target Milestone: --- → mozilla0.9.6
cavin's found good stuff.

here's the worst case scenario:

you've got a message selected in your imap inbox, focus in your thread pane, you
click on your imap sent folder.

here's what happens:

1)  clicking on the folder pane does a focus switch, bluring focus from thread
pane to folder pane.
2)  during the blur, we set the focused element to null (update toolbars #1),
then we set it to the folder pane (update toolbars #2)
3)  the selection in the thread pane goes from 1 to 0, (update toolbars #3).

nsMsgDBView.cpp, SelectionChanged()
mCommandUpdater->UpdateCommandStatus();

4)  folder loads, (update toolbars #4)commandglue.js, ChangeFolderByURI():
document.commandDispatcher.updateCommands('mail-toolbar');

5) folder is done loading, we switch focus to the thread pane automatically.
that's another blur.  (update toolbars #5, from focused element to null, and #6
when focused element goes to the thread pane)

that's 6 times we update the toolbar on a single user action.

cavin and I have a fix in hand for 2 of these, we've fixed the blur problem.

we'll work on the others next.  

I think we can get down to 2.  

two focus switches: thread pane to folder pane, and then folder pane back to
thread pane.
Summary: Opening folder: Doing UpdateCommands("mail-toolbar") 3 times? → Opening folder: Doing UpdateCommands("mail-toolbar") several times
(Reporter)

Comment 3

18 years ago
Comment on attachment 55850 [details] [diff] [review]
patch for the blur problem.

r=cavin.
Attachment #55850 - Flags: review+
(Reporter)

Comment 4

18 years ago
With the patch, we save one updateCommands('mail-toolbar') call which is about 
250 millisceconds for the scenario I've been testing against. A very nice gain 
here.

Update on the scenario I've been using: Don't have any imap or pop account. The 
only thing the profile has is the local mail folders.  So I normally start up 
the mailnews program and then click on the folder with 1000 (or 20000/4000) 
msgs.

Comment 5

18 years ago
Comment on attachment 55850 [details] [diff] [review]
patch for the blur problem.

sr=bienvenu
Attachment #55850 - Flags: superreview+
ok, that patch is in. 

I'll continue working on this, to supress our updates of the toolbar even further.
(Reporter)

Comment 7

18 years ago
With this 'blur problem' patch, we save 2 UpdateCommands("mail-toolbar") calls 
for the following (more normal) scenario (which is what seth described earlier 
as the worst case scenario):

1. Start program and log onto an imap acct so the focus is on INBOX thread pane.
2. Then click on a local folder. 

Without the patch we invoke 6 UpdateCommands("mail-toolbar") calls and only 4 
with the patch.

Updated

18 years ago
Blocks: 74955

Comment 8

18 years ago
For what it's worth, command updaters have a |targets| attribute that is 
specifically for this purpose -- designating for what elements you want to watch 
certain events (e.g. blur) for, so you don't have to do it yourself.  The 
attribute takes a comma-delimited list of ids.
Created attachment 56027 [details] [diff] [review]
patch, for this bug and #102180 (close stand alone msg window when you delete if it is the last message)
taking
Assignee: cavin → sspitzer
Blocks: 102180
Created attachment 56029 [details] [diff] [review]
new patch, update comments.  patch also includes spelling fix (suppress, not supress)

Comment 12

18 years ago
Comment on attachment 56029 [details] [diff] [review]
new patch, update comments.  patch also includes spelling fix (suppress, not supress)

sr=bienvenu
Attachment #56029 - Flags: superreview+
after that patch,  here's what's left:

task                                         # of calls to updating toolbar

startup                                      2
switching local folders, nothing selected    2
switching local folders, message selected    2
switching imap folders, nothing selected     2
switching imap folders, message selected     2
switching folders if switching
folder to an account that ends up
loading the start page                       3

after this lands, I'll worry about reducing the updates even further.

but before I start that, I'll go investigate what blake said, to simplfy the
first patch.
Status: NEW → ASSIGNED
(Reporter)

Comment 14

18 years ago
Comment on attachment 56029 [details] [diff] [review]
new patch, update comments.  patch also includes spelling fix (suppress, not supress)

r=cavin.
Attachment #56029 - Flags: review+
Comment on attachment 56029 [details] [diff] [review]
new patch, update comments.  patch also includes spelling fix (suppress, not supress)

checked in, so marking obsolete.
Attachment #56029 - Attachment is obsolete: true
moving to 0.9.7, not a blocker for 0.9.6

Target Milestone: mozilla0.9.6 → mozilla0.9.7

Updated

18 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8

Comment 18

17 years ago
moving to future re: conversation I had earlier with Seth.
Target Milestone: mozilla0.9.8 → Future

Updated

17 years ago
Keywords: nsbeta1+ → nsbeta1-
Has this bug been fixed? I have a problem with Mozilla hanging when entering a
folder and am looking for appropriate bug.
Product: Browser → Seamonkey

Updated

14 years ago
Assignee: sspitzer → mail
Status: ASSIGNED → NEW
Assignee: mail → nobody
Priority: P1 → --
QA Contact: stephend → message-display
Target Milestone: Future → ---

Comment 20

10 years ago
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614
Status: NEW → UNCONFIRMED

Comment 21

10 years ago
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614
Group: core-security

Comment 22

10 years ago
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Updated

10 years ago
Group: core-security

Comment 23

9 years ago
MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago.

Because of this, we're resolving the bug as EXPIRED.

If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component.

Query tag for this change: EXPIRED-20100420
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → EXPIRED
You need to log in before you can comment on or make changes to this bug.