blake sent me, varada, and ducarroz a patch to improve compose window perf. I'll attach it here (and add comments.)

any ideas on what kind of improvement this gives?

I'm having problems estimating how much improvement this patch gets us. I was trying to stop watch from when I hit "new msg" to when I could type in the address field, but my win2k box is too fast.

Have you try to turn on prlog for msgcompose?

You probably wouldn't see much anyways. At this point, this is easily the biggest win after the editor has already been focused once. I'm still looking into why it's not as fast the first time, but I suspect it's editor doing some init'ing.

blake wrote privately: "If we can figure out why editor has no onfocus or onblur, we can speed up compose opening a LOT. Right now, we currently update tons of commands every time focus changes in the compose window, regardless of whether the editor is involved. And we update 66 commands upon opening the window." blake, thanks for starting this work.

Anyone can feel free to come down here and see the improvement I'm seeing because a nightly and my optimized builds (which are typically even slower than nightlies for me)

taking this

What's left doing with the attached patch?

I havent had much time before to play around with this patch but I am going to be doing that now along with some more work on compose window perf and memory leaks.

This patch can be checked in but I would like to keep the bug open for further improvements instead of reopening it again or a new bug. r=varada.

<BenGoodger> sr=ben

Varada and I took a look at this the other day, and I revisited it just a few minutes ago. Looks really promising in terms of tabbing/focus issues. One thing we're still really bad on is enabling the HTML formatting toolbar in msg compose (if your pref is set to HTML compose). You can see this when tabbing from subject to the body, and then cycling back to the subject again. Are we making extra calls to that Editor shell that we don't need? (I should probably profile tabbing forward and back between subject and body).

How thoroughly has it been tested? What are the risks? Do we really want this for 0.9.3?

Blake has an updated patch that I'm testing now. I'll post the patch here, then I'll need to export my build to the lab, to compare against baseline current trunk speeds.

Seth, Blake and I worked on this, and we got rid of some other factors like biff that might have affected the results of the 1st comparisons, summaries, etc that I posted. Only 7/26/01 20:11 and 7/26/01 20:12 are what we need to look at.

I just got a demo from blake / stephend. it's looking like a good win, for both reply and new window compose. excellent work blake. blake, is the attached patch the final version of the patch? once varada does a review of the final patch, I'll can sr. I suggest 0.9.4 as the approprate milestone.

Not yet the final, but probably close. I'm investigating some more areas that we may be able to improve upon. Currently this looks to shave about a second off of reply and new window time. I'll post a final patch tomorrow that hopefully stephen can bang on for awhile.

Okay, let's just stick with this patch for now (same as before, minor update). Seth, can you take a look? Why do we need the pref about updating the titlebar when you type? This doesn't make any sense to me, to pref something like that.

> Why do we need the pref about updating the titlebar when you type? > This doesn't > make any sense to me, to pref something like that. that's 4.x speaking from the dead. When I added the "update title as you type" feature to 4.x UNIX, people complained, so I make it so you could disable it. But I'm ok with removing the pref to make our compose .js simpler, cleaner, and faster. (especially since there is no UI for it.) If you do remove it, make sure to remove "mail.update_compose_title_as_you_type" from mailnews.js as well. I'm a little nervous about your change to remove this code: //This migrates the LDAPServer Preferences from 4.x to mozilla format it looks like that was added for a reason. in removing it, are you going to break this scenario: 1) user migrates from 4.x to 6.x / mozilla 2) before starting up addrbook or mailnews, the user starts up msg compose, ldap auto complete prefs are not migrated. cc'ing srilatha. she should know. we might have to move that code to the 4.x messenger migrator, which will get called first in that scenario.

Removing LDAPPrefsService will break migrating ldap prefs from 4.x format to 6.x format in the scenario Seth pointed out.And putting it in the messenger migrator is not good idea because migration of ldap prefs has to be done if the user profile has migrated the profile using mozilla 0.9.2( which does not have ldap prefs) or earlier versions of it and now using later version of mozilla(which has ldap prefs). Also ldapSession needs to be a global variable and it has to be initialized only once per composewindow session. So, you can move this to ComposeStartup()

I don't know why I removed that ldap stuff. I thought I lxr'd for it and found no uses of it, but now I'm finding them. I'll keep it.

Oops, that latest patch adds the ldap stuff back, but now I see what I did -- I moved ldapSession into setupLdapAutocompleteSession() so that new patch has it in both places. But lxr shows that setupLdapAutocompleteSession() is the only place that's used. So srilatha, why does that need to be a global variable? (If it doesn't have to be, I'll still need to move it out of the |if| that it's in now)

ldapSession issue aside for now (until I hear back from srilatha), can we continue review on this?

yes, ldapSession is used only in setupLDAPAutocompleteSession. In setupLdapAutocompleteSession we are doing an addsession and removesession based on if the user has selected a directory to autocomplete against or not. If it is a global variable then we know what session we have added and we know what session we need to remove. It's not so much that it needs to be a global variable but it needs to have the lifetime which is same as the compose window.

I've got some great results from the Win32 test machine. Running my normal tests on a 2K message on a 266mhz, Blake's patch cuts total reply time: Last week's trunk build - 7.08 5.22 5.22 seconds to open a 2K HTML message, quote the data, and insert the cursors' focus at the top of the document. With Blake's patch applied to my trunk build, we have: 5.23 3.38 and 3.36 seconds! I'm sure there's more improvement to go in this and other areas, but so far, so good.

Compare those numbers with the same methodology and using NSPR_LOG to generate the times automatically to: http://www.mozilla.org/mailnews/win_performance_results.html

I just wanted to add that if those numbers hold that these are fantastic results. Thanks for looking into this.

Run # Patched | Unpatched | Delta ---------------------------------- 1 5.24 | 6.64 | -1.40 2 3.39 | 4.66 | -1.27 3 3.37 | 4.64 | -1.27 Pentium II, 266 megahertz, 128 MB of RAM, 2K HTML message.

Just a note about the results: as impressive as they are, Blake's patch really addresses tabbing performance and new window performance. It's very difficult to get accurate numbers for tabbing performance, but Seth, Blake and I (and anyone who applies the patch) can tell you that tabbing back and forth between the addressing widget, subject and body is *much* faster.

varada, can you re-review / test and I'll super review? blake, thanks again for working on this. compose / reply window opening performance is one of the warts of mailnews.

+ try { + if (!ggLDAPSession) + ggLDAPSession = Components.classes["@mozilla.org/autocompleteSession;1?type=ldap"].createInstanc e() + .QueryInterface(Components.interfaces.nsILDAPAutoCompleteSession); + } catch (ex) { + dump ("ERROR:" + ex + "\n"); + } blake, what is ggLDAPSession for. Did you put an extra g by mistake!

varada said he tested this out, but the ggLdap typo seems like it would cause problems. blake, can you attach a new patch?

Yeah, that's what happens when you replace two instances manually and then do a global find-and-replace, combined with the fact that I'm a moron.

this patch has cvs diff logs in lots of places - can you attach a clean one +<<<<<<< MsgComposeCommands.js +======= //Create message window object var msgWindowContractID = "@mozilla.org/messenger/msgwindow;1"; var msgWindow = Components.classes[msgWindowContractID].createInstance(); +>>>>>>> 1.199

there are cvs conflicts in the last patch.

r=varada

sr=sspitzer

varada, don't forget to remove "mail.update_compose_title_as_you_type" from mailnews.js

There is a copy-paste error here: - <menu id="formatMenu" label="&formatMenu.label;" accesskey="& formatMenu.accesskey;" observes="cmd_format"> + <menu id="formatMenu" label="&formatMenu.label;" accesskey="& formatMenu.accesskey;" command="cmd_insert">

my bad! that one slipped by me - I will check in the fix first thing monday morn

I can check this in now, if nobody objects?

rs=sfraser

No, that's actually right, the command is just poorly named. Since Insert and Format go together and need no special handling (other than disabling at the right time), they use the same command. Perhaps it would be better named cmd_InsertFormat or something.

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

Just a note: Blake is keeping this open for further investigation/improvements.

I'm going to close this for now, we'll file specific bugs as we see fit but I think we've done most of the work we can in this area.

Seth and Varada have done some work on this, in other bugs. Verified FIXED (see my previous comments/results).