464126 - imap should load and show newest headers first, especially in the new client account with existing server account case

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, enhancement)

Description

[David :Bienvenu]

If you set up a new imap account in TB or SM, we fetch all the headers, in order, from oldest to newest, and then start fetching message bodies for offline use. We would like to fetch the newest headers first, and display those in the thread pane, and then start fetching message bodies and older headers, so that the user can more quickly read their new mail.

Right now we chunk the header fetches - I thought we were doing 200 headers at a time, but that's not true anymore - we limit the length of the command we send to the imap server, not the number of headers we fetch, and since in this case, it will tend to be something like fetch 1:5000 headers, since missing uids in ranges are ignored by the imap server, and we're smart enough to know which ones are missing. So the chunking is just based on command length.

It's easy enough to change the chunking so that we fetch the 200 newest headers, followed by the next 200 older headers, etc. But this doesn't buy us anything, since this process currently has to run to completion before the user can click on a message, or we can start downloading message bodies. So we would need to chain the fetches, and interleave message body fetches, and/or the user's attempt to fetch a message by clicking on the new header. This is all doable, but a non-trivial amount of work. It might be significantly easier to do something a bit more pragmatic - make the first fetch do just the newest messages, and then let the auto sync stuff download the newest messages, and when it's done, just re-update the folder, which will download all the rest of the headers, and allow auto sync to fetch older messages at its leisure. The thinking is that the user is most likely to want to read one of the 10 newest messages and the bang for the buck of fetching msg header 1000-1200 before 0-200  in a 5000 message folder is minimal.

Comment 1

[hansen]

This will fix Bug 122391 (and Bug 252644 which is the same) right?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

I like the pragmatic approach here, sounds doable and likely to solve the problem in a sufficient enough way.

Comment 3

[David :Bienvenu]

I'm close to getting a first pass at this working. The general approach is as follows:

Instead of only tellng the imap protocol code about all the msg hdrs needed, the backend code will tell the imap protocol about some headers (newest ones first) along with a flag that says if there are more headers besides those. If there are more headers need, the imap protocol code will suspend the url when done (i.e,, put it at the end of the queue) and run queued urls first. When it's the suspended url's turn to run again, it will again ask the backend for which headers it should download, and repeat as needed. Only when there are no more headers to download will the url send an OnStopRunningUrl notification, so any code that assumes all headers are downloaded when the url is done won't get confused.

The idea is that if a user clicks on a message while the headers are getting downloaded, that url will get run when the current batch of headers are finished downloading, because that url will get queued before the next batch of headers. Autosync will also likely kick in and download some messages well. And downloading the newest headers first will give a much better experience.

The downside is that it will take us quite a bit longer to fetch the headers, because doing the headers in chunks will involve a lot more roundtrips, and increase server load to some extent. I intend to make the chunk size configurable, and it's conceivable that the backend code could adjust the chunk size dynamically. For example, if autosync is *not* turned on, and the user hasn't tried to read any messages, then we could go for a larger chunk size. But I suspect the downside is relatively minor, and not worth worrying much about.

Comment 4

[David :Bienvenu]

Attachment: wip

This seems to work pretty well. I'd like to add a pref to control this (and by setting the pref to a large number, you'd essentially turn off this behavior), and I'd like to get rid of NotifyHdrsToDownload - it doesn't make sense anymore. UpdateImapMailboxInfo can just return the headers to download, and skip the monitor notifying, and the imap protocol code can skip the waiting on the monitor.

Comment 5

[David :Bienvenu]

Attachment: proposed fix

this is pretty much ready for review. It passes all the existing unit tests (not surprising, since none of them try to download > 200 headers) but I'd like to write a unit test just to confirm that the chaining works correctly.

This patch adds a pref ("mail.imap.hdr_chunk_size", default 200) and only does chunking if the folder is open in a window. I also got rid of the monitor wait+notification for header download, which simplifies that code a bit.

Comment 6

[David :Bienvenu]

Attachment: fix with xpcshell test

This adds a unit test.

Neil, the basic idea is that we want to download the newer headers first, and give the user the chance of reading them while older headers get downloaded in the background. This should improve the initial user experience when setting up an account with a large imap inbox.

Comment 7

[[e-mail]]

Comment on attachment 533448 [details] [diff] [review]
fix with xpcshell test

>+  // should make this a pref
You did ;-)

>+  if (folderOpen && m_keysToFetch.Length() > hdrChunkSize)
Sanity-check hdrChunkSize; if it's <= 0 then you want to disable chunking.

>+  *keys = (nsMsgKey *) PR_Malloc(numKeysToFetch * sizeof(nsMsgKey));
>+  NS_ENSURE_TRUE(*keys, NS_ERROR_OUT_OF_MEMORY);
>+  for (PRUint32 i = 0, keyIndex = startIndex; i < numKeysToFetch; i++)
>+    (*keys)[i] = m_keysToFetch[keyIndex++];
nsMemory::Clone(&m_keysToFetch[startIndex], numKeysToFetch * sizeof(nsMsgKey))
(and NS_Free to free it again, of course)
[Shouldn't use PR allocator for XPCOM methods]

>-      UpdatedMailboxSpec(new_spec);
Where did this line go?

>+  do_check_eq(gIMAPInbox.msgDatabase.dBFolderInfo.numMessages, 10);
I guess you can't actually test that chunking is working, given that its operation is supposed to be transparent ;-)

Comment 8

[David :Bienvenu]

Attachment: fix addressing comments

fix addressing comments. UpdatedMailboxSpec was a two line function that was only called once, so I got rid of it and move its one remaining line of code to the caller.

I took your last comment as a challenge :-), and changed the xpcshell test to verify that I could stream a message in the middle of running the url to fetch the hdrs.

Comment 9

[[e-mail]]

Comment on attachment 533790 [details] [diff] [review]
fix addressing comments

I tried this out by doing a "Repair Folder" several times, with a variety of chunk values. I wasn't pleased that the chunking affects the progress notifications ;-) Once I didn't even get any notifications for a chunk (the previous and following chunks did at least count up to 200). Also this assertion filled my terminal after one test:

###!!! ASSERTION: downloading hdrs for hdr we already have: 'Error', file nsImapMailFolder.cpp, line 3008

I wasn't able to reproduce the problems though.

Comment 10

[David :Bienvenu]

I'm not happy about the progress notifications just showing the current chunk's progress either - I'd kinda talked myself into it but I think I'll fix it.

I'm not sure about the assertion - I think that implies that you got two downloads going at once, or else the logic to avoid refetching the same headers is broken. I haven't seen it myself.

Comment 11

[David :Bienvenu]

Attachment: fix total counts

this fixes the total count...I don't think this needs re-review since it only affects the progress messages/percents...

Comment 12

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/e545b6accf99

fix checked in - had to add new xpcshell test to imap/test/unit/xpcshell.ini

Comment 13

[johny why]

what happened to this? current version of tbird still downloads oldest headers first.