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

Status: RESOLVED EXPIRED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Cavin Song]

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

Comment 1

[(not reading, please use [e-mail] instead)]

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.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch for the blur problem.

Comment 3

[Cavin Song]

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

r=cavin.

Comment 4

[Cavin Song]

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

[David :Bienvenu]

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

sr=bienvenu

Comment 6

[(not reading, please use [e-mail] instead)]

ok, that patch is in. 

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

Comment 7

[Cavin Song]

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.

Comment 8

[Blake Ross]

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.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: patch, for this bug and #102180 (close stand alone msg window when you delete if it is the last message)

Comment 10

[(not reading, please use [e-mail] instead)]

taking

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: new patch, update comments. patch also includes spelling fix (suppress, not supress)

Comment 12

[David :Bienvenu]

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

sr=bienvenu

Comment 13

[(not reading, please use [e-mail] instead)]

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.

Comment 14

[Cavin Song]

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

r=cavin.

Comment 15

[(not reading, please use [e-mail] instead)]

fix landed.

Comment 16

[(not reading, please use [e-mail] instead)]

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

checked in, so marking obsolete.

Comment 17

[(not reading, please use [e-mail] instead)]

moving to 0.9.7, not a blocker for 0.9.6

Comment 18

[scottputterman]

moving to future re: conversation I had earlier with Seth.

Comment 19

[Aleksander Adamowski]

Has this bug been fixed? I have a problem with Mozilla hanging when entering a
folder and am looking for appropriate bug.

Comment 20

[Robert Kaiser]

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

Comment 21

[Robert Kaiser]

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

Comment 22

[Robert Kaiser]

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

Comment 23

[Robert Kaiser]

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