Closed Bug 163964 Opened 22 years ago Closed 3 years ago

speedup imap startup by skipping folder discovery and other niceties, for many hundreds to thousands of folders

Categories

(MailNews Core :: Networking: IMAP, defect)

defect

Tracking

(thunderbird_esr78 wontfix, thunderbird_esr91+ fixed, thunderbird92 wontfix)

RESOLVED FIXED
93 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird_esr91 + fixed
thunderbird92 --- wontfix

People

(Reporter: Bienvenu, Assigned: gds)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 10 obsolete files)

Our startup time will be faster if we don't discover folders (i.e., list subscriptions), and also if we don't refresh ACL every time we select a folder, selecting folders will be faster (especially the first acl fetch, which seems to be especially slow). Other clients have a "refresh folders" menu item and people have to explicitly pick it to discover folders.
this is just a snapshot to save this work, since I need to use my tree for other work now.
Attached patch work in progress patch (obsolete) — Splinter Review
whoops, that patch was for something else
Attachment #98137 - Attachment is obsolete: true
I also forgot this change the default prefs: Index: mailnews.js =================================================================== RCS file: /cvsroot/mozilla/modules/libpref/src/init/mailnews.js,v retrieving revision 3.160 diff -u -w -r3.160 mailnews.js --- mailnews.js 7 Sep 2002 01:55:13 -0000 3.160 +++ mailnews.js 10 Sep 2002 20:32:58 -0000 @@ -330,6 +330,8 @@ pref("mail.server.default.canCreateFolders", true); pref("mail.server.default.canFileMessages", true); +pref("mail.server.default.refresh_folder_info", true); + pref("mail.smtpserver.default.auth_method", 1); // auth any pref("mail.smtpserver.default.try_ssl", 0);
Status: NEW → ASSIGNED
QA Contact: huang → gchan
David: this sounds cool. Did this ever get checked in?
No, it was never finished - I'll get back to it some day.
Product: MailNews → Core
Product: Core → MailNews Core
QA Contact: grylchan → networking.imap
Severity: normal → enhancement
Keywords: perf
Whiteboard: [patchlove][has draft patch]
I'm sure this patch won't apply anymore, because adding condstore support changed a lot of the related code. And condstore is a much nicer way of doing this, and I think it's getting pretty good traction with server vendors.
There's some value in delaying folder discovery for existing profiles, however.
Comment on attachment 98141 [details] [diff] [review] work in progress patch (In reply to comment #6) > I'm sure this patch won't apply anymore, because adding condstore support > changed a lot of the related code. And condstore is a much nicer way of doing > this, and I think it's getting pretty good traction with server vendors. Yup, it doesn't apply, confirming patch's obsolete status to clean it out of bugzilla queries..
Attachment #98141 - Attachment is obsolete: true
david, is this a candidate to block bug 487832 startup performance meta bug?
(In reply to comment #9) > david, is this a candidate to block bug 487832 startup performance meta bug? I think this is pretty far down the list, unless you have many hundreds or thousands of folders.
(In reply to comment #10) > I think this is pretty far down the list, unless you have many hundreds or > thousands of folders. agreed. probably not a hit for most people. OTOH I'm sure i've read some even in the past month where people had in the mid to high hundreds of folders. I can't put my finger on an example just now
This makes us discover folders after downloading the new headers for the first folder we load. For normal usage, this should be fine, and could make the app feel a bit snappier. On the downside, this seems to break some of the unit tests, and it doesn't allow message bodies to get fetched before folder discovery. We'd like folder discovery to run as a separate url a certain amount of time later, though that increases the time that our picture of the folder structure on the server is stale.
Whiteboard: [patchlove][has draft patch]
Whiteboard: [patchlove][has draft patch]
startup is so IO intensive that I'm going to say this is important enough to call it a bug, not ENH. Some users with many folders for example no doubt get Unresponsive Script on startup, and this would help.
Assignee: mozilla → nobody
Severity: enhancement → normal
Status: ASSIGNED → NEW
Whiteboard: [patchlove][has draft patch] → [patchlove][breaks unit tests?]
Severity: normal → minor
Summary: add option to speedup imap startup by skipping folder discovery and other niceties → speedup imap startup by skipping folder discovery and other niceties, for many hundreds to thousands of folders
(In reply to David :Bienvenu from comment #0) > .... Other clients have a "refresh folders" menu item and people > have to explicitly pick it to discover folders. Gene, are you aware of other clients that do this?
Flags: needinfo?(gds)

(In reply to Wayne Mery (:wsmwk) from comment #14)

(In reply to David :Bienvenu from comment #0)

.... Other clients have a "refresh folders" menu item and people
have to explicitly pick it to discover folders.

Gene, are you aware of other clients that do this?

Sorry a bit late on reply. But I've not looked that close at other clients in a while and don't know if they do that.
Anyhow, I think this might be the problem Richard Leger is seeing: Bug 1660672

Edit: The collapse/expand account folder tree widget in tb does what a "refresh folders" menu item would do.

Flags: needinfo?(gds)
Blocks: 1660672

(In reply to gene smith from comment #15)

(In reply to Wayne Mery (:wsmwk) from comment #14)

Edit: The collapse/expand account folder tree widget in tb does what a "refresh folders" menu item would do.

I think this is true only for the first mailbox setup at the top (as seen in TB 82 branch). If you have additional mailboxes, expanding them does not trigger a "refresh folders" for that mailbox but could be nice to have.

It would still be nice to have a "folder refresh" trigger at startup but either in a worker not to block main UI and other main processing (like navigating msg and liadin preview of content) or at TB iddle time... as well as when expanding a folder.

As an end-user, if I start TB I would still expect it to show an updated folder list in my mailboxes witout having to trigger such refresh manually while still getting propper performance and responsivness while that is happening in the background... there must certainly be a way to make both happen at the same time ;-)

A visual progress bar shall show (or visual aid) shall indicate to end-user progress of updating "folder list" especially if it would take a long time...

If the above were to be implemented "update folder"
button may not be necessary... instead of a button one could imagine than right click on a folder you would get the "update folder(s)" in context menu... to reduce cluttering of intetface... or within folder properties along dide Repair that would suffice...

Refreshing folder shall not be something end-users would need to do manually unless there is a specific issue with it or need to force a refresh...

As noticed during my testing, one side effect of disabling discovery of folders at startup is that when you add a new IMAP mail account, only the Inbox folder appears but none of the other primary folders and subfolders... unless you collapse/expand the mailbox tree to trigger the "folder refresh" manually.

Adding an account shall trigger the "folder refresh" automatically... just to keep in mind if the discovery of folder would be disabled at startup in the future...

Also noted the higher the number of folder is and the slower the network connection is, the longer the "Looking for folders" (and getting ACL) run... It may seems evident but just thought to highlight it...

I think this is true only for the first mailbox setup at the top (as seen in TB 82 branch). If you have additional mailboxes, expanding them does not trigger a "refresh folders" for that mailbox but could be nice to have.

It's only true for the "account/server name" line at the top as in the "richard@..." line in your attachment 9175188 [details]. Collapse/expand on a folder or sub-folder tree doesn't trigger the discovery.

Richard, with the try build I provided, does collapse/expand on the account/server line slow down your system while (re)discovering folders? It did help with startup time, right?

I'm sure there a lot of issue that need to be considering when implementing this improvement.

(In reply to gene smith from comment #19)

...
Richard, with the try build I provided, does collapse/expand on the account/server line slow down your system while (re)discovering folders? It did help with startup time, right?

Flags: needinfo?(richard.leger)

(In reply to Wayne Mery (:wsmwk) from comment #20)

(In reply to gene smith from comment #19)

...
Richard, with the try build I provided, does collapse/expand on the account/server line slow down your system while (re)discovering folders? It did help with startup time, right?

Gene, Mery,

Would you be able to provide again the link with the target build so I can retry and assess in terms of speed? Sorry I have removed it from my laptop and cannot find the link to download from...

Cheers,

Flags: needinfo?(richard.leger)

I suspect it would need to be rebuilt. I'm not even sure what patch was involved.

Flags: needinfo?(gds)

The old build expired and was deleted so I did a new one. You can get it here. It's finished when you see a green B.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ff30fc316d924d5768d4b3d80dcb21555ad4d56c
See bug 1660672 comment 20 for instructions.
This patches the current daily build.

Flags: needinfo?(gds)
Attached image installer_missing.png (obsolete) —

(In reply to gene smith from comment #23)

The old build expired and was deleted so I did a new one. You can get it here. It's finished when you see a green B.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ff30fc316d924d5768d4b3d80dcb21555ad4d56c
See bug 1660672 comment 20 for instructions.
This patches the current daily build.

Hi Gene,
I tried to download the installer but I cannot find it... is it because it has expired?
Regards,

Flags: needinfo?(gds)

(In reply to Richard Leger from comment #24)

...
I tried to download the installer but I cannot find it... is it because it has expired?

Yes, as before, it has expired when you wait too long. You need to commit to picking it up when it has been supplied for you.

Flags: needinfo?(richard.leger)
Flags: needinfo?(gds)

Gene, can you run the try again? (I tried to retrigger but wasn't able to)

Flags: needinfo?(gds)

(In reply to Wayne Mery (:wsmwk) from comment #25)

(In reply to Richard Leger from comment #24)

...
I tried to download the installer but I cannot find it... is it because it has expired?

Yes, as before, it has expired when you wait too long. You need to commit to picking it up when it has been supplied for you.

Sincere apology and mea-culpa... I am just a volunteer trying to do what I can, when I can...
This is not my job, but I am happy to help :-)

I am now using TB 87.0b2 (64-bit) so Gene you would only need to provide me with a new build if your patch did not yet make it into that version...
You can set me in the needinfo when published so I get reminders :-)

Flags: needinfo?(richard.leger)

Hi Richard, Sorry for the delay in getting back to you on this. There seems to be button on the "try" page to redo an expired build but I don't seem to have the right credentials. So I went ahead an resubmitted the change to rebuild for win64 and linux and run the unit test. I also included my patch for bug 1615064 in the try build so I will give you the honor of testing that too. :). If you see a problem with any headers it might be due to this.
Here it is:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7eb9936769ca8ba6e01b8766fca6847cebd4117e
Same instructions as in comment 23 above.

...if your patch did not yet make it into that version...

Just to be clear, the change I made to "speed up startup" is purely experimental. It needs a lot more testing and probably coding to be applied to a release, even a daily build. However, since you have a very large email setup, whatever you see with just this simple change will be helpful.

Flags: needinfo?(gds)

(In reply to gene smith from comment #28)

Just to be clear, the change I made to "speed up startup" is purely experimental.

Well in a nutshell: It seems to work!
It does drastically increase speed to read 1st email message but not without drawbacks :-)

I use setup a new profile (1 IMAP account - no keep messages - hundreds of folders) with the TARGET version provided in binary form by Wayne in Comment 29 and duplicated that profile for comparison in the normal DAILY both the same version TB 2021-03-15_64-bit on Windows 10.

Results:

With the TARGET version (Daily version patched) no lookup for IMAP folders occurs at startup...
It takes about ~25s to read 1st message (includes a 5s overhead as profile manager starting prior TB to allow me choose profile pre-selected, I just click immediately Start button as soon as the Window appears)
Perf profile: https://share.firefox.dev/3vv0PEV

With the DAILY version (not patched - https://ftp.mozilla.org/pub/thunderbird/nightly/2021/03/2021-03-15-10-40-12-comm-central/thunderbird-88.0a1.en-US.win64.zip) lookup for IMAP folders at startup (though no mention in status bar, just assume it does) - delaying reading first email
It takes about ~48s to read 1st message (same ~5s overhead included as above)
Perf profile: https://share.firefox.dev/38OZCi1

Drawbacks with TARGET:

  • when creating new account, mailbox folders are not populated e.g: primary folders such as Sent, Trash, etc... or any other folders and subfolders... only Inbox appear visible.
  • restarting TB does not populate the folders either, which is expected with TARGET including your patch...
  • only by folding then unfolding email address folder (root top folder in left column aka folder list), does the folders start to appear (effectively triggering the looking for folders feature). No the most convenient...

In both case TARGET and DAILY it takes ages to populate the full list of all the folders in TB... it is very very slow much slower than before... but likely not directly related to this bug/patch :-)

My gut feeling is that Looking for IMAP folders feature shall still occurs at startup (and upon account creation) but may need to be delegated to a webworker (background thread) - https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers
If that is feasible...

Flags: needinfo?(richard.leger)

(In reply to Richard Leger from comment #30)

...
My gut feeling is that Looking for IMAP folders feature shall still occurs at startup (and upon account creation) but may need to be delegated to a webworker (background thread) - https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers
If that is feasible...

Ben, you've been looking at worker threads? Would your work fix the above issue? And if not, does it sound reasonable to go in this direction for this issue?

Bug 324710 also suggests doing some operation in background.

Flags: needinfo?(benc)
See Also: → 324710

(In reply to Richard Leger from comment #30)

(In reply to gene smith from comment #28)

Just to be clear, the change I made to "speed up startup" is purely experimental.

Well in a nutshell: It seems to work!
It does drastically increase speed to read 1st email message but not without drawbacks :-)
...
Drawbacks with TARGET:

  • when creating new account, mailbox folders are not populated e.g: primary folders such as Sent, Trash, etc... or any other folders and subfolders... only Inbox appear visible.
  • restarting TB does not populate the folders either, which is expected with TARGET including your patch...
  • only by folding then unfolding email address folder (root top folder in left column aka folder list), does the folders start to appear (effectively triggering the looking for folders feature). No the most convenient...

ref comment 16

Gene, Is it possible to address these issues?

Flags: needinfo?(gds)

I'd have to look at this again to know since it's been a while since I studied it. I'll admit that the proposed addition of "worker threads" is outside of my area so if that's really what is required there may be a learning curve (or maybe just have Ben or someone who is already familiar with threading do this).
Keeping NI set and adding this to my "todo" list.

(In reply to Wayne Mery (:wsmwk) from comment #31)

Ben, you've been looking at worker threads? Would your work fix the above issue? And if not, does it sound reasonable to go in this direction for this issue?

If it's a case of asking the IMAP server to tell us about all the folders, then it should be totally network-bound. So farming stuff off to another thread shouldn't help at all.
My guess would be it's more a case of something blocking, and not being as async as it should be.
I'll try and set up some test cases and have a poke about (I'll leave my NI on this for now).
(I'm a little hazy on how IMAP folders persist between runs of TB, but it seems like the assumption should be that all the previous folders still exist, and a refresh should proceed incrementally in the background without really preventing the user from doing other things)

Good morning Ben,

(In reply to Ben Campbell from comment #34)

(In reply to Wayne Mery (:wsmwk) from comment #31)

Ben, you've been looking at worker threads? Would your work fix the above issue? And if not, does it sound reasonable to go in this direction for this issue?

If it's a case of asking the IMAP server to tell us about all the folders, then it should be totally network-bound.

The problem is not the time it takes for IMAP server to tell about the folders but the fact that the UI "freezes" during the process... eg cannot load an IMAP message while the folder list is being refreshed... likely an async issue...

Part of it may not necessarily be network bound but the fact that via IMAP protocol each folder may be checked in turn individually because it may check for flags or else status for each folder as well... not just get a full list of folders at once and compared to the cached list of folders...

So farming stuff off to another thread shouldn't help at all.

My though was that a different thread may help prevent the UI "freeze" during the folder lookup processing to allow main UI to remain available but maybe you know a better way to do it...

My guess would be it's more a case of something blocking, and not being as async as it should be.

Yes. That is exactly right, the UI is not totally available during Looking for Folders... at least the process is preventing loading message content in preview pane (IMAP online no cache)... if not the total UI unavailable... maybe because it is trying to do within the same IMAP connection already used to refresh the folders? So waiting for process to complete, prior triggering the next IMAP request to load the message content?

I'll try and set up some test cases and have a poke about (I'll leave my NI on this for now).

Have you had a chance to reproduce the issue at your end?
FYI, I have currently about 5480 folders in my main IMAP mailbox... and multiple mailbox setup... though others are much smaller in size...

(I'm a little hazy on how IMAP folders persist between runs of TB, but it seems like the assumption should be that all the previous folders still exist, and a refresh should proceed incrementally in the background without really preventing the user from doing other things)

The list of folders exists in the local cache of TB and within the TB profile of the Windows user on disk in the 'imapMail' folder along side the .msf files... but I don't know if it may keep a parallel list of folders in sqlite DB as well possibly all folder info is stored in .msf files... I am just and end-user having a rough guess here :-)

Hope that help identify the culprit and a possible solution because at the minute it is still happening in beta 91.x...

Cheers,
Richard

Here's another try based on the patch in comment 12 above. This still does the folder discovery at startup but does it after the inbox message status is fetched. I can't see a difference with this with my normal folder count but the original IMAP developer D. Bienvenu claimed that it might make the startup "snappier". So, Richard, if you could try this with your TB setup it would be interesting to see its effect.
D.B. also said this change breaks unit tests so that might be another issue, but can address that later if it improves your startup time.

The try build is in progress here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6709cde9b51a84bcb73ca947c6917d8437354bd8

Flags: needinfo?(gds)
See Also: → 1660672

Looking at the log again and re-reading bug 1660672 comment 8, I now have another change that may help. I would still like for Richard to try out the build in comment 36 first before this one.

Since multiple connections occur on startup to mailboxes with many folders, there seems to be some overlap, time-wise, on discovering the folders. The assumption in the code is that only the first connection will do the discover and it will be complete before subsequent connections occur. But since Richard has so many folders, the 2nd connection occurs before the first connection has finished doing the discover so both connection end up doing discover which increases the time and slows down the CPU since it is doing redundant work.

My proposal is to add a new flag that is common to the server that prevents a discover from starting if one is already in progress for a given server. (With the release, beta and daily code, additional discovers are blocked only when the the first discover has completely finished, but not if it is still in progress.) Again, with my normal number of folders there is no effect but maybe with Richard's large number of folders there may be.

This "try" build also includes the change from the other try build linked in comment 36:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f9969606e9ddc88affb091018193181326d0b317

(In reply to gene smith from comment #36)

Here's another try based on the patch in comment 12 above. This still does the folder discovery at startup but does it after the inbox message status is fetched. I can't see a difference with this with my normal folder count but the original IMAP developer D. Bienvenu claimed that it might make the startup "snappier". So, Richard, if you could try this with your TB setup it would be interesting to see its effect.
D.B. also said this change breaks unit tests so that might be another issue, but can address that later if it improves your startup time.

The try build is in progress here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6709cde9b51a84bcb73ca947c6917d8437354bd8

Hi Gene,

Thank you for looking into this...

I tried version above... which appears as TB 92.0a1 (2021-07-27) (64-bit)...

Result: the selected folder (e.g Inbox) message list appears fine as it was before... but I am still not able to load a message content, selecting a message in the message list (to see content in preview pane) has change the status to Loading Message... but has no effect while the Looking for folder process is on-going... the preview pane remain "blanked" and not showing the msg content (headers, body, attachment)...

At first setup of profile, it took between 5-10mn to be able to read my first message... after the second restart of TB, it seems most folder disappear and got repopulated which once again took 5-10mn during another reboot status showing Getting folder ACLs... but it could be because the indexing also was possibly running in the background...

Apart from the glitches above, with this version and after couple of reboot following first setup (only one IMAP mailbox account setup with not cached email), upon TB restart it takes ~35sec to read first message (select message in Inbox upon startup and wait for content to appear in preview pane).

Also noted, this version of TB seems to keep downloading messages while I did deselected upon profile setup the synchronisation option "Keep message in all folders...", the download of message started only after headers were loaded which happened only after first message was loaded in preview pane which happened only after the looking for folders process completed :-) It seems it was fixed after couple of TB restart and repairing the Inbox folder...

Hope that help.

Regards,
Richard

Hi Richard. One thing I might suggest is that when you setup the account in the new profile (which I think you are doing) is to, before completing, drop into "manual setup" and go to "Sync and Storage" and turn off "Keep messages on all folders". This will avoid having to download all the messages again and just the header will be fetched. Then just let the headers for Inbox download before trying to access and do timing on the message display. You might also later select some other folders and let headers be fetched for them before restarting TB and doing the time measurement.

Anyhow, sounds like the the change in the first try build didn't really help any. Hopefully, you can test the 2nd try build too. It is a bigger change and maybe will have more effect. Thanks again!

(In reply to gene smith from comment #37)

This "try" build also includes the change from the other try build linked in comment 36:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f9969606e9ddc88affb091018193181326d0b317

Same results with this version with a bit less glitch perhaps :-)

  • Appears as TB 92.0a1 (2021-07-28) (64-bit) version
  • First time setup new profile 5-10mn to read first email - my gut feeling is that TB is likely to be downloading messages from Inbox (despite have disabled caching of messages during account setup) and possibly start to index data... also possibly creating the folder on the local disk in TB profile imapMail folder may take more time the first time... The activity monitor blue status bar is progressing slowly at the bottom right corner but when clicking on it, it either does not show activity running or indexing...
  • Successive restart are much faster still takes about 30-35sec to be able to read first email... basically the Loading message in preview pane does not complete until the Looking for folder (or else background process) has completed first...

This result again is obtained by solely having only one IMAP mailbox setup (no cached/downloaded emails) - no other mailbox or network calendar/address-book setup.

Hope that help.

Regards,
Richard

(In reply to gene smith from comment #39)

One thing I might suggest is that when you setup the account in the new profile (which I think you are doing) is to, before completing, drop into "manual setup" and go to "Sync and Storage" and turn off "Keep messages on all folders". This will avoid having to download all the messages again

It is what I do but TB seems to ignore it because by the time I get to the settings I think it already started to sync the mailbox so the change of settings is not effective till next TB restart :-(
That is likely a separate bug that was already present in previous version of TB as I noticed it already in the past...

and just the header will be fetched. Then just let the headers for Inbox download before trying to access and do timing on the message display. You might also later select some other folders and let headers be fetched for them before restarting TB and doing the time measurement.

After first loading is completed, reboot and repair the Inbox to make sure I have no downloaded messages when testing...

Anyhow, sounds like the the change in the first try build didn't really help any. Hopefully, you can test the 2nd try build too. It is a bigger change and maybe will have more effect. Thanks again!

I can run some profiles for both testing version, would that help you spot progress or culprit?

Ok, according to the log you sent Wayne a while back, the time to discover folders was, I think, about 25 seconds but occurred at least twice so the startup time was about 50s. But I suggested that it might be variable depending on how many connections occur which somewhat depends on what you were doing before shutting TB down. The change in the try version you ran in comment 40 just makes the time, hopefully, more constant and dependent on the number of folders. So the time for the first message to appear should be less variable. You see 35 sec and I was hoping for 25 seconds.

Sounds like you are setting up without offline store so there should be no download of full messages from server until the message is opened.

This result again is obtained by solely having only one IMAP mailbox setup (no cached/downloaded emails) - no other mailbox or network calendar/address-book setup.

By "one IMAP mailbox" I assume you mean one IMAP account? Just to be exact, in IMAP terms each folder is considered a mailbox. So I assume you still seeing >5000 folders (a.k.a., mailboxes) when you run the new profile.

Another thing you might try is to enable CONDSTORE. This is something that is disabled by default due to some problem with gmail I think. But it can help with startup time especially when you have lots of messages in a folder. I think David Bienvenu implied it could affect this bug (comment 6 above) but I have my doubts. In advanced setting make this true: mail.server.default.use_condstore. Anyhow, worth a try. It requires a TB restart to take effect.

It is what I do but TB seems to ignore it because by the time I get to the settings I think it already started to sync the mailbox so the change of settings is not effective till next TB restart :-(
That is likely a separate bug that was already present in previous version of TB as I noticed it already in the past...

Yes, but I've only seen it start to download full messages if you click on folder like Inbox. But If you do a "advanced config" from the setup dialog you can disable storing and downloading messages so, even if you click a folder, it won't download messages, just headers. At least that's been what I've seen.

I can run some profiles for both testing version, would that help you spot progress or culprit?

I've never been able to tell much from those profiles. It usually just show stuff down in mozilla code that I don't really understand. I guess I'm mostly curious if the time for the message to appear is constant now with the last build? Thanks for the offer.

I wanted to do a test with 5000 folders like Richard has so I modified my locally built TB so that when I create a folder named something, say "gds" TB creates 5000 folders gds-0, gds-1, ...gds-4999. I didn't want to manually create 5000 folders one at a time! So now I have an existing folder, called Queue, with 5000 sub-folders.

After creating them, TB began the initial discovery process that was a bit slow but eventually all 5000 new folders were listed. The slowness is probably because each folder has a corresponding .msf file associated with it that must be created by TB and initialized. plus corresponding activities on the server (a local dovecot connected to via wifi). I noticed that each folder list response took maybe about 1/2 second while monitoring the IMAP:5 log during the init phase.

After initialization and TB restart it took about 14 to 16 seconds for a message in folder gds-0 to display and it only appears after the folder list discovery response finishes, so it is being blocked by the network activity. This is faster than what Richard reports, 35 seconds. My observation of the message appearing after 15 seconds seems very consistent over several restarts.

I did notice that if I make folder gds-0 have offline store that the appearance of the message is almost immediate even with intense network folder discovery going on in the background. So this may be a work-around that may be acceptable. Richard might just have to set Inbox to have offline store if that is where he usually wants to quickly look at messages immediately after TB startup.

Looking at the log with my 2nd try build changes applied, it does appear that the redundant LISTs are eliminated and only one occurs. Looking at the times with the unchanged code, the time to LIST the 5000 new sub-folders is about 16 or 17 seconds and it occurs twice but interleaved. With my proposed change in the 2nd try build applied, there is no redundant LIST occurring but the time only goes down to about 11 seconds to complete the single LIST. So this makes the message appear some faster on my setup.

I also see that if a message is not selected immediately after startup and if you just wait a while (15s for me, maybe 35s for Richard) that you can then select and open an existing message with no delay.

Hi Gene,

Yes by my mailbox I meant one IMAP account setup... with thousands of folders (aka mailboxes as you refer to)
I really appreciate your efforts in trying to help fix TB perf issues...

I don't think disabling the folder update list at startup in TB is the solution but anything that can help improve that process performance would be very beneficial anyway. Which should be the point of this bug...

But the main question remain why does any IMAP processing delay message content loading in preview pane at startup when working in online mode? And how we can make it happen in parallel? That was the point of Bug 1660672 that lead us here...

(In reply to gene smith from comment #43)

After creating them, TB began the initial discovery process that was a bit slow but eventually all 5000 new folders were listed. The slowness is probably because each folder has a corresponding .msf file associated with it that must be created by TB and initialized. plus corresponding activities on the server (a local dovecot connected to via wifi). I noticed that each folder list response took maybe about 1/2 second while monitoring the IMAP:5 log during the init phase.

Half a second for each folder seems a lot... would there be anyway to identify if that can be improved?
5-10mn to populate 5000 folders in Thunderbird seems quite long no? But that is a separate bug issue altogether for another day...

After initialization and TB restart it took about 14 to 16 seconds for a message in folder gds-0 to display and it only appears after the folder list discovery response finishes, so it is being blocked by the network activity.

This is great news as you managed to reproduce the issue!

By "blocked by network activity" I suppose you mean that TB IMAP connection is busy with processing the folder list update or listing/indexing emails in the folder... or what did you meant something else?
Surely network activity shall not prevent the selected message to load its content in preview pane in parallel of other IMAP processing... it should definitely be possible for TB to do both at the same time no? by multi-tasking maybe workers functionality could be used to load message content instead which may be a simpler processing and easier to move in a separate process than IMAP folder list update? Unless there is a better way to do it... e.g via async feature or prioritising message loading content over folder update list when IMAP processing is on-going and tasks queued?

Could it be because TB attempt to re-use the same IMAP connection already opened to load the message content but because it is busy with updating folder list, the loading has to wait? Can't TB open a new separate IMAP connection to deal with message content loading? This way both could happen in parallel instead of in series... as it seems...

Unless it is a JavaScript design issue where a function processing has not finished before the next one can starts... somehow...

Maximum number of server connection is set to 5 in server advance settings... so surely multiple network connection can happen at the same time... to remove the blockage by network activity... this is not a network loading issue, but it is a TB design/bug issue I believe...

I just don't know how to reveal the cuprite further to narrow it down...

This is faster than what Richard reports, 35 seconds. My observation of the message appearing after 15 seconds seems very consistent over several restarts.

For me as well the 35s is consistent... I have 1500 msg in my Inbox (which is where I attempt to load message content from) so could it be the difference come from the fact I may have more message in the selected folder or within my 5000 folders so TB takes more time to check differences to update unread count flags or else within each folders due to higher number of emails? Indexing may also play a role here perhaps...

I did notice that if I make folder gds-0 have offline store that the appearance of the message is almost immediate even with intense network folder discovery going on in the background. So this may be a work-around that may be acceptable. Richard might just have to set Inbox to have offline store if that is where he usually wants to quickly look at messages immediately after TB startup.

Yes if the you enable offline store, the message content load from the store without delay when I tick "Keep message in all folders...on this computer" option and the "sync the most recent" set to "3 days" (side note: when ticking the option TB is Nor Responding for minutes before the save setting dialogue appears - another bug for another day!) but that is not an acceptable workaround/solution nor the point... hiding dust under the carpet never is :-)

Because I don't want copy of my email made available offline (cached) on the computer, working online shall still be possible without causing more issues than it should (apart from the inevitable network transfer overhead) and loading of message shall still be happening in parallel of else IMAP processing... so that would still need to be fixed somehow... I am happy to help where I can to make this possible...

TB when in online mode shall be able to refresh the IMAP folder list, list message in folder, index and load messages content in preview pane ""in parallel**, some of those already do, but obviously something is preventing this to happen with the message content loading and that is not a network issue like message slow to load due to network bandwidth/speed... but a bug in TB that prevent the loading all together... maybe because it is waiting for the same IMAP connection to be re-used so queuing the loading message process till the connection is free... maybe that could be fixed by creating a separate individual connection to load message content?

If it takes 15-35s to read a message with just one IMAP mailbox setup, imagine what happens when you have multiple account setup for email, calendar & addressbook... then there would be more network activity... and more delay? Is that acceptable from end-user point of view? No...

What ever delay we can save, do count tremendously for the whole... if I can easily read emails (in online mode) while other things happen in the background upon TB startup is a big win for end-users and the project overall... because I am occupied while it happens...

Looking at the log with my 2nd try build changes applied, it does appear that the redundant LISTs are eliminated and only one occurs. Looking at the times with the unchanged code, the time to LIST the 5000 new sub-folders is about 16 or 17 seconds and it occurs twice but interleaved. With my proposed change in the 2nd try build applied, there is no redundant LIST occurring but the time only goes down to about 11 seconds to complete the single LIST. So this makes the message appear some faster on my setup.

I think it is quite possible that this improvement and possible other improvements in TB (like speedier parsing of network responses) may explain why speed improved from 50sec+ previously to 35s now for me... so it is still beneficial to improve folder list update processing... as obviously there is no need for redundant LIST occurring...

I also see that if a message is not selected immediately after startup and if you just wait a while (15s for me, maybe 35s for Richard) that you can then select and open an existing message with no delay.

That correspond to the delay you see when you select the msg immediately after and wait 15s for its content to appear...
In both case you await for background TB processing (e.g folder list update) to complete... before the message loading process can effectively starts... in all case it is started immediately as message loading appears in status bar... but something is blocking the content of message to be effectively loaded immediately in preview pane without waiting... and that what I wish would be fixed...

Do you reckon any next steps?

Regards,
Richard

Richard wrote:

Because I don't want copy of my email made available offline (cached) on the computer, working online shall still be possible without causing more issues than it should (apart from the inevitable network transfer overhead) and loading of message shall still be happening in parallel of else IMAP processing... so that would still need to be fixed somehow... I am happy to help where I can to make this possible...

Hi Richard. Thanks for the detailed reply. I understand what you are saying and mostly agree on your points but not quite sure I understand the above quote. It sounds like you are thinking my suggestion is to make all folders have offline store (cached). I was only suggesting that you make Inbox have offline store. It can be selected separately via advanced setting of "Synchronization and Storage". All other folder would remain un-cached with no offline storage. But this is just a temporary work-around that might help your situation while we continue to work on the root problem.

Anyhow, yes the folder discovery is definitely blocking the message fetch from inbox because they are occurring on the same IMAP thread, triggered by the first URL. Beinvenu suggested this in comment 12 above:

We'd like folder discovery to run as a separate url a certain amount of time later, though that increases the time that our picture of the folder structure on the server is stale.

Running a discovery URL explicitly would cause TB to spawn a new IMAP thread. Right now, a URL to trigger discovery is only triggered by account root folder collapse/expand. The discovery that is causing your issue (this bug) is not running as its own URL but it triggered by whatever other URL is running and once the discovery finishes one time it no longer occurs on every URL but it's always checked for the need to run with every URL.

So I'll see if I can find a way to implement the comment 12 suggestion.

Here's a new try build for my latest proposed fix:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=844778fe36027adeb1c6f672e392f1ca20f43234

I found a way to not check for and possibly trigger a discovery on every URL but instead cause the discovery to occur in its own URL and (usually) in its own thread. Every IMAP connection to a mailbox (folder) begins with the select URL. This triggers a chain of imap commands that occur before the actual imap select (capability, starttls, authenicate, id, select and, with the original code, list) and, if this is the first select, the discovery occurs from parsing imap list command responses. My proposed change is to trigger a discovery URL (list) right after the first select url and no longer trigger a list (discover) from within the select URL chain. This causes the discovery to occur in parallel with the select and fetch URLs, avoiding the delay in downloading the message that Richard sees. I've kept the lock-outs added in the previous try build so that a discovery URL is not done after the select URL if a discovery for the account has already occurred or if one is already in progress from another select to avoid redundant list responses and parsing.

This seems to work as I hoped and and a message can be fetched and read while the discovery is in progress on my 5000 folders/mailboxes, unlike before where the message would only appear after the discovery finished.

I haven't tried it yet, but this change should also work on initial folder discovery after a new account is created. This is because an initial imap select on Inbox will occur and, since no discovery has occurred yet, the discovery URL should occur at this time on a new thread/connection as it does on TB startup with my change. However, this change probably does not affect the new account creation time.

One possible issue for users who change the default number of cached imap connection from default 5 down to 1 is that it won't be possible for the discovery URL triggered after the initial select URL to obtain its own thread. It will have to wait for the select chain of commands to complete on the single thread before it can become active. So subsequent activity on the single imap thread will be blocked while discovery completes. I haven't tried or tested this yet either.

Richard, if you could test the new try build on your setup it would be great. Thanks!

Assignee: nobody → gds

Hi Gene,

(In reply to gene smith from comment #46)

Here's a new try build for my latest proposed fix:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=844778fe36027adeb1c6f672e392f1ca20f43234
This seems to work as I hoped and and a message can be fetched and read while the discovery is in progress on my 5000 folders/mailboxes,

Just tested it... TB version 92.0a1 (2021-07-29) (64-bit) as it appears...
...and one thing to say: Thank You! Thank You! Thank You!

I can see you modified .h files (c code) so it was well outside my reach from a tech point of you, to help fix :-)
You nailed it well done! This seem to really boost TB startup performance till you can read first email...

It works like a charm (at the speed of light):

  • I can load a message content as soon as TB opens - while list of folders/indexing and other goodies are processing in the background
  • Each message loading in preview pane do so at a much much faster speed than before even those with attachments - side effect of this patch or improvement in TB?
  • Updating the folder list is much much much faster now! Even at first setup of the account...

Side notes:

  • sometime when starting TB (possibly since beta 90.x version) - only two folders appears in the account before folders get re-populated again one by one... other time when starting TB all folders appears immediately so there is some sort of caching or view issue causing inconsistencies - but likely a separate issue to this bug or fix.

  • contrary to you, when setting up a new mailbox account, messages are download by default especially as the account is being setup as soon as I click advanced settings and populated prior I can reach accounts advance settings to disable the "Keep..." option which seems to modify folder settings immediately but only effectively apply after reboot of TB (if you don't reboot TB keep downloading messages). I can see msg being downloaded despite folder sync settings set not to keep copy of message on the computer - but likely a separate issue to this bug or fix.

Side question:

  • How did you debug IMAP stuff and found the issue? As nothing appearing in Activity Manager, Debugger or Network within TB? I was myself a bit at lost while trying to digging it out...

I haven't tried it yet, but this change should also work on initial folder discovery after a new account is created.

It does.

One possible issue for users who change the default number of cached imap connection from default 5 down to 1 is that it won't be possible for the discovery URL triggered after the initial select URL to obtain its own thread.

Testing confirms suspicion.

With connection set to 1, then back to square one, as Message Loading appears in status bar as message is selected in the list but without loading the message in preview pane till the update folder list has completed... but the folder update list being much much much faster than before, the message loads quite quickly anyway... and much less of an issue because then it is user's choice no longer a bug in TB :-)
TB default setting seems be set to 5 anyway so that shall be fine for most usage...

You make me very happy about those speed/perf improvements.
Have a fabulous WE!

Regards,
Richard

Hi Richard. Thanks for testing and and thanks for the nice comment! I'll address your side issues later but right now I'm seeing a bit of problem when I try to create a new account in a new profile to my 5000 folder test server. I have a self-signed certificate that I have to override and on account creation the first select and discovery URLs fail. This leaves the discovery URL in "in-progress" state so on the next select after overriding the cert no discovery happens (due to my new lockout variable) so I only see Inbox and not the 5000+ folders. I can only get the discovery and see all the folder by either collapse/expand of the root folder or restart tb. So I need to look at this.

Richard wrote about this side issue:

contrary to you, when setting up a new mailbox account, messages are download by default especially as the account is being setup as soon as I click advanced settings and populated prior I can reach accounts advance settings to disable the "Keep..." option which seems to modify folder settings immediately but only effectively apply after reboot of TB (if you don't reboot TB keep downloading messages). I can see msg being downloaded despite folder sync settings set not to keep copy of message on the computer - but likely a separate issue to this bug or fix.

Yes, I see the same problem when I setup the first IMAP account in a new profile. The reason I wasn't seeing it before was because I typically setup new accounts in an existing profile. When you add a new account to an existing profile you have to be selected to some existing folder or account. So when you add a new account, the selection point stays where it is and the Inbox of the newly added account is not automatically selected. This allows you choose "advanced" setting to change the settings on the new account before any URL occurs for that account so you can turn off offline store or do other changes before the default settings start being used.

When you add a new profile, TB automatically prompts for adding a new account. You add the new account and when you select "advanced" setting to change the setting, the selection goes to Inbox automatically of this first new account causing the folder discovery and auto-sync to occur which starts downloading full messages which is the default setting and your changes to disable offline store is not seen at this time. After a TB restart it is seen but now you probably have a lot of mbox or maildir files for folders that you really don't want and have to clean up somehow.

This is only an issue on the first IMAP account of a new profile but a possible workaround is this (pending a real fix):

  1. Create a new profile
  2. Enter info for a "fake" account, e.g.,
    a) Full Name: abc
    b) Email addr: abc@abc.abc
    c) Password: abc
  3. Continue
  4. Configure manually
  5. Advanced config
  6. Account actions -> Add mail account ...
  7. Add your real account but select "Manual config" and then "Advanced config" when it appears (don't click Done/Complete)
  8. Now fix up any setting, e.g., "Don't keep messages locally"
  9. Select the Inbox tab
  10. Note that abc account Inbox is selected and only Inbox is present on your new real account
  11. Select Inbox on your new real account
  12. First connection to new real account occurs and respects the setting you made.
  13. Remove the "fake" account.

I've submitted a new bug for this here: bug 1723324

See Also: → 1723324

(In reply to gene smith from comment #48)

Hi Richard. Thanks for testing and and thanks for the nice comment! I'll address your side issues later but right now I'm seeing a bit of problem when I try to create a new account in a new profile to my 5000 folder test server. I have a self-signed certificate that I have to override and on account creation the first select and discovery URLs fail. This leaves the discovery URL in "in-progress" state so on the next select after overriding the cert no discovery happens (due to my new lockout variable) so I only see Inbox and not the 5000+ folders. I can only get the discovery and see all the folder by either collapse/expand of the root folder or restart tb. So I need to look at this.

Could it be a port issue as encountered in Bug 1700044 Comment 5?

Could it be a port issue as encountered in Bug 1700044 Comment 5?

No, I don't think so. I'm just using the usual port 143 with STARTTLS. It could occur for any discovery URL error or timeout, not just with self-signed certs. Anyhow, I think I found a solution for that.

However, I have a bigger problem in that the imap and possibly other unit tests are failing due to my changes, as predicted in comment 12. I found a fix for one test (I think) but I need to look at other tests tomorrow to see if anything else is failing. (The rule is to fix the TB code without changing the test code itself.)

Blocks: 1649103
Attached patch discovery-fix.diff (obsolete) — Splinter Review

Here's a proposed fix. It does the same thing as the previous t2.diff that reporter Richard Leger tested via a "try" build. Its difference is that it now passes the imap related xpcshell-tests, unlike t2.diff (now mark obsolete).

Patch discovery-fix.diff still contains printf's and other stuff that needs to be cleaned up before a formal patch for review is submitted.

Here's the "try" build for patch discovery-fix.diff. Some tests still show failures but they don't seem to be caused by this patch, AFAIKT: Try build

Edit, P/S: Comment 46 has a reasonable explanation of the fix.

Attachment #9233429 - Attachment is obsolete: true
Attachment #9233852 - Attachment is obsolete: true
Attachment #9236112 - Flags: feedback?(benc)
Comment on attachment 9236112 [details] [diff] [review] discovery-fix.diff Review of attachment 9236112 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a nice solution to me. ::: mailnews/imap/src/nsImapHostSessionList.cpp @@ +302,5 @@ > + PR_EnterMonitor(gCachedHostInfoMonitor); > + nsIMAPHostInfo* host = FindHost(serverKey); > + if (host) result = host->fDiscoveryForHostInProgress; > + PR_ExitMonitor(gCachedHostInfoMonitor); > + return (host == NULL) ? NS_ERROR_ILLEGAL_VALUE : NS_OK; I'd probably make sure result was always set even if an error occurs. Your new code calling GetDiscoveryForHostInProgress() is responsible and pre-fills result with `false`, but I wouldn't count on anyone writing new code remembering to do that. I realise you're following the patterns already in nsImapHostSessionList, but I think all those patterns are wrong :-) ::: mailnews/imap/src/nsImapIncomingServer.cpp @@ +862,4 @@ > } > > NS_IMETHODIMP > nsImapIncomingServer::PerformExpand(nsIMsgWindow* aMsgWindow) { I'm just thinking of a user who clicks to expand a subfolder while the server-level DiscoverAllFolders() is already in progress. Would expanding that subfolder kick off an extra redundant discovery? There's nsImapFolder::PerformExpand(), which I guess handles expanding non-top-level folders. Should that function also check hostSessionList->GetDiscoveryForHostInProgress() to see if a server-level discovery is already in progress? The folder PerformExpand() uses imapService::DiscoverChilden() - I haven't dug through to see what IMAP commands that actually ends up issuing.
Attachment #9236112 - Flags: feedback?(benc) → feedback+
Flags: needinfo?(benc)
Attached patch imap-folder-discovery-fix.patch (obsolete) — Splinter Review

I'd probably make sure result was always set even if an error occurs.

I made the requested changed on my new Get...() function. I haven't made any changes on the others existing functions with the same pattern but if you think I should, I will in a revised patch.

I'm just thinking of a user who clicks to expand a subfolder while the server-level DiscoverAllFolders() is already in progress. Would expanding that subfolder kick off an extra redundant discovery?
There's nsImapFolder::PerformExpand(), which I guess handles expanding non-top-level folders. Should that function also check hostSessionList->GetDiscoveryForHostInProgress() to see if a server-level discovery is already in progress?
The folder PerformExpand() uses imapService::DiscoverChilden() - I haven't dug through to see what IMAP commands that actually ends up issuing.

I have always noticed that expanding a subfolder doesn't discover any new folders in it. I looked into it and the reason why is because nsImapFolder::PerformExpand() only calls imapService::DiscoverChilden() when subscriptions are not used, and they are by default (Advanced setting "Only show subscribed folders" is checked/true by default).
But when I uncheck this and test with my 5000 folders then I see a redundant discovery of my subfolder containing 5000 folders when I expand it during initial discovery at startup. However, I don't see any slow-down and I can still access messages quickly in any folder.
So I don't right off see a problem and haven't changed anything regarding this.

I also tested again with just one and two cached connection. This still delays being able to open an email until the discovery finishes, as I would expect. With 3 or more you can open the message while discovery progresses.

Attachment #9236112 - Attachment is obsolete: true
Attachment #9236392 - Flags: review?(benc)
Comment on attachment 9236392 [details] [diff] [review] imap-folder-discovery-fix.patch Review of attachment 9236392 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!
Attachment #9236392 - Flags: review?(benc) → review+

(In reply to gene smith from comment #54)

I made the requested changed on my new Get...() function. I haven't made any changes on the others existing functions with the same pattern but if you think I should, I will in a revised patch.

No, just for your new Get() function is fine - keeps the patch focused.
(I might go through and patch the others sometime, but I don't think there's any real urgency on that. It's the sort of thing that static analysis tools complain about :- )

I have always noticed that expanding a subfolder doesn't discover any new folders in it. I looked into it and the reason why is because nsImapFolder::PerformExpand() only calls imapService::DiscoverChilden() when subscriptions are not used, and they are by default (Advanced setting "Only show subscribed folders" is checked/true by default).
But when I uncheck this and test with my 5000 folders then I see a redundant discovery of my subfolder containing 5000 folders when I expand it during initial discovery at startup. However, I don't see any slow-down and I can still access messages quickly in any folder.
So I don't right off see a problem and haven't changed anything regarding this.

Cool - that sounds good. Thanks for checking it out!

Ready to land?

Status: NEW → ASSIGNED

Yes, been meaning to set the keyword. Thanks for reminder.

Looking forward to this.

In considering future releases and uplifts, a) it's good to see this landing now, because I'd like to see this patch "fully" ride the train, i.e. a couple weeks on nightly, and then several weeks on beta, before requesting uplift to ESR, and b) to simplify potential troubleshooting, do we want to delay the landing of imap bug 1708981 until after the next merge (sept 6)? (i.e. so that we avoid having these patches hit beta at the same time) Ditto for any other major imap patches.

Flags: needinfo?(gds)

On bug 1708981 I'm still in the process of getting feedback from BenC and haven't yet submitted the formal patch for review. So it may get delayed naturally. Otherwise, I have no problem with imposing a delay. (But not sure I'm the best one to ask about release scheduling.)

Flags: needinfo?(gds)
Whiteboard: [patchlove][breaks unit tests?]
Target Milestone: --- → 93 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/313d232c4038
Run initial folder discovery in own URL so messages open faster. r=benc

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

Unfortunately this is patch is causing a bunch of test failures. I'll back it out soon. Failing tests (at least) - https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=ImRzb6cHRmWkjqvwtJMgWA.0&revision=b4081109cf60e6de991469434fae5f35a9b9b196 -

comm/mail/components/extensions/test/xpcshell/test_ext_accounts.js
comm/mail/components/extensions/test/xpcshell/test_ext_folders.js
comm/mailnews/imap/test/unit/test_imapHighWater.js

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

(In reply to Magnus Melin [:mkmelin] from comment #63)

Unfortunately this is patch is causing a bunch of test failures.

Yes it is. I thought my try build checked this and showed no errors on my last "try" that was same as the patch:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&author=gds%40chartertn.net

I was doing a build without the patch and then with the patch and it appeared to show no errors. But I must have done something wrong.

Now when I run the the test locally I see the errors. Haven't been able to get a fix yet for all the tests (just the failing imap test now works by adjusting my c++ changes). The tests in the "extension" area is more difficult: I.e., don't know what this means (seems to be the primary error):

 0:02.65 FAIL test_folders - [test_folders : 176] undefined - Expected: 3, Actual: 2 - false == true

This is for the comm/mail/components/extensions/test/xpcshell/test_ext_folders.js. Is the 176 a line number in the file? What the Expected/Actual referring to and what's the "false == true"?

Also, for some reason I can't get a MOZ_LOG to appear, with or without my patch, when I run the "extensions" tests but it does with the "imap" unit tests. I can see some IMAP going on with wireshark on localhost running the extensions tests.

Attached patch discovery-fix-v2.diff (obsolete) — Splinter Review

I found the problem. Some of the tests in the extensions area and the highWater test in imap don't do imap SELECT first like happens when TB itself is running and sending URLs to imap protocol. With my defective patch this caused some tests to fail that don't do a SELECT until much later.

The fix in the attached diff now again does the mailbox discovery on the first URL if discovery is not in progress due to SELECT not occurring first. This allows the unit test to now pass. It also still allows discovery to run in its own URL when SELECT occurs first and allows quicker access by TB to the messages.

Here's the try build results for the discovery-fix-v2.diff:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1488461e7eea52b51883ff63958f721a45102157

I need to do some more testing on this but could someone (Magnus and/or Ben) verify that the test results actually are passing (I was fooled before). I do see an error "bct3" but it appears that other try builds have the same error. I want to be sure the test results are OK before submitting another formal patch.

Attachment #9236392 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #9237906 - Flags: feedback?(benc)

That try run looks good (the failing is bug 1727201).

Flags: needinfo?(mkmelin+mozilla)

Did some more testing and looks OK.
This is the same as discovery-fix-v2.diff except printf's removed and clang formatter applied.
Re-ran previously failing unit tests locally and all pass.

Attachment #9237906 - Attachment is obsolete: true
Attachment #9237906 - Flags: feedback?(benc)
Attachment #9238030 - Flags: review?(benc)
Attachment #505528 - Attachment is obsolete: true
Attachment #9203914 - Attachment is obsolete: true
Comment on attachment 9238030 [details] [diff] [review] imap-folder-discovery-fix-v2.patch Review of attachment 9238030 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +800,5 @@ > + // See if discovery in progress and not yet finished. > + hostSession->GetDiscoveryForHostInProgress(serverKey.get(), > + discoveryInProgress); > + } > + if (!foundMailboxesAlready && !discoveryInProgress) { Notionally, there's a path through the code where discoveryInProgress is read, but undefined at this point. It's safe because of the !foundMailBoxesAlready guard, but I'd probably just make it explicit by nesting this second if block, eg: ``` if (!foundMailboxesAlready) { discoveryInProgress = false; // See if discovery in progress and not yet finished. hostSession->GetDiscoveryForHostInProgress(serverKey.get(), discoveryInProgress); if(!discoveryInProgress) { nsCOMPtr<nsIMsgFolder> rootFolder; rv = GetRootFolder(getter_AddRefs(rootFolder)); if (NS_SUCCEEDED(rv) && rootFolder) { rv = imapService->DiscoverAllFolders(rootFolder, this, aMsgWindow, nullptr); if (NS_SUCCEEDED(rv)) hostSession->SetDiscoveryForHostInProgress(serverKey.get(), true); } } } ```
Attachment #9238030 - Flags: review?(benc) → review+

Thanks Ben. I made your suggested change which does seem better. Also clang formatted and re-tested some. So I think it is OK to set the checkin-tb keyword?

Attachment #9238030 - Attachment is obsolete: true
Attachment #9238206 - Flags: feedback?(benc)
Attachment #9238206 - Flags: review+

(In reply to gene smith from comment #71)

Thanks Ben. I made your suggested change which does seem better. Also clang formatted and re-tested some. So I think it is OK to set the checkin-tb keyword?

Absolutely :-)
(also, I found clang-format and js linting got a bit tedious after a while so I wrote myself a little shell script to just lint all the files that have been changed. In case it's any use to you: https://github.com/bcampbell/tb-notes/blob/master/scripts/cclint )

Attachment #9238206 - Flags: feedback?(benc) → feedback+

(In reply to Ben Campbell from comment #72)

Thanks, I set the checkin-tb keyword.

(also, I found clang-format and js linting got a bit tedious after a while so I wrote myself a little shell script to just lint all the files that have been changed. In case it's any use to you: https://github.com/bcampbell/tb-notes/blob/master/scripts/cclint )

I haven't looked at yours yet but I made one too over a year ago that does the clang format:

gene@smith:~/mozilla/comm $ ./cfs --help
Clang Format Script usage: ./cfs (-d | -i | -s) [filepath]...
Requires the option -d (diff), -i (inplace) or -s (stdout) followed by
one for more file paths. If no path, applies to all changed files.

but doesn't do lint. I'll take a look at yours.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/add2d0e3802b
Run initial folder discovery in own URL so messages open faster. r=benc

Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED

Thank you.

Comment on attachment 9238206 [details] [diff] [review]
imap-folder-discovery-fix-v3.patch

[Approval Request Comment]
Performance improvement on startup, landed in 93 milestone.

Attachment #9238206 - Flags: approval-comm-esr91?

Out of an abundance of caution, I'd like hold this longer on beta

(In reply to Wayne Mery (:wsmwk) from comment #78)

Out of an abundance of caution, I'd like hold this longer on beta

I agree, I think Bug 1733966 would need to be fixed before backporting this change/patch to esr91...

passing on this again for esr91 - 91.2.1

Severity: minor → S3
See Also: → 1733966

(In reply to Richard Leger from comment #79)

(In reply to Wayne Mery (:wsmwk) from comment #78)

Out of an abundance of caution, I'd like hold this longer on beta

I agree, I think Bug 1733966 would need to be fixed before backporting this change/patch to esr91...

Bug 1733966 is now resolved, so we can proceed to uplift to esr91. TBD is whether it goes in 91.3.x, or 91.4.x. I think the later is more likely, as we are still focused on resolving TB91 bugs/regressions. (plus it would be a nice seasonal present)

Comment on attachment 9238206 [details] [diff] [review]
imap-folder-discovery-fix-v3.patch

[Triage Comment]
(finally) approved for esr91

Attachment #9238206 - Flags: approval-comm-esr91? → approval-comm-esr91+

Thank you akl! +1

See Also: → 1745205
See Also: → 1768121
See Also: → 1862111
See Also: → 1617060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: