Closed Bug 505456 Opened 14 years ago Closed 3 years ago

move/copying multiple imap messages to local folder bypasses offline store and redownloads messages. Need to preflight the move/copy.

Categories

(MailNews Core :: Backend, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 74.0

People

(Reporter: Bienvenu, Assigned: gds)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

If you move/copy multiple imap messages to a local folder, we run it as a single imap url with multiple messages. This bypasses the offline store cache checking, which means we'll fetch the messages from the server unnecessarily if we have the messages in the offline cache.

To fix this, I think we need to preflight the move/copy, and check if we have all the messages offline - if so, just copy the offline copies to the local folder. If we have a mix of offline and not offline messages, we could break it into two operations, but the homogeneous case is the low-hanging fruit.
hmm, that explains slowness (I frequently do this)
Keywords: perf
This sure would be nice.
Flags: wanted-thunderbird3+
OS: Windows Vista → All
Hardware: x86 → All
blocking1.9.1: needed → ---
Severity: normal → major
if Bug 579520 doesn't get closed for odd reasons, perhaps this would help him.
Summary: move/copying multiple imap messages to local folder bypasses offline store → move/copying multiple imap messages to local folder bypasses offline store and redownloads messages
Summary: move/copying multiple imap messages to local folder bypasses offline store and redownloads messages → move/copying multiple imap messages to local folder bypasses offline store and redownloads messages. Need to preflight the move/copy.
See Also: → 538375

Yes, I just tested this and emails are fetched from server before they are copied of a folder in Local Folders, even if the source imap folder has offline storage. However, if the copy is to another imap server's folder, the messages are just read from offline store and then imap appended to the destination folder. I didn't know this and it may be relevant to Bug 538375
Note also that if you go offline and do the copy to Local Folders, the source data is obtained from offline store.

See Also: → 1525033
Attached patch 505456-proposed-change.patch (obsolete) — Splinter Review

With this change IMAP messages copied to Local Folders are now obtained from offline storage if all messages being copied reside in offline storage. Otherwise, they are fetched from the IMAP server.

Previously, if more than one IMAP messages is copied to Local Folders and if TB is online, the messages are always fetched from the IMAP server.

So this fixes the "homogeneous, low-hanging fruit" case described in Comment 0.

This greatly impacts Bug 1566717 and other similar bugs regarding copying from IMAP folders to Local Folders (See Bug 1566717 comment 53).

Assignee: nobody → gds
Attachment #9124922 - Flags: review?(mkmelin+mozilla)
See Also: → 1566717
Comment on attachment 9124922 [details] [diff] [review]
505456-proposed-change.patch

Review of attachment 9124922 [details] [diff] [review]:
-----------------------------------------------------------------

Nice low hanging fruit! r=mkmelin

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1321,5 @@
>    nsCString protocolType;
>    rv = srcFolder->GetURI(protocolType);
>    protocolType.SetLength(protocolType.FindChar(':'));
>  
> +  // If TB is offline and the source folder is imap or news, to do the

nit: we usually comment in the "we" form.
If we're offline....

@@ +1343,5 @@
>        */
>        totalMsgSize += msgSize + 200;
>  
> +      // Check if each source folder message has offline storage regardless
> +      // of whether TB is online or offline.

here too

@@ +1349,5 @@
> +      bool hasMsgOffline = false;
> +      srcFolder->HasMsgOffline(key, &hasMsgOffline);
> +      allMsgsHaveOfflineStore = allMsgsHaveOfflineStore && hasMsgOffline;
> +
> +      // If TB is offline and not all messages are in offline storage, the copy

and here

@@ +1489,5 @@
>        (void)OnCopyCompleted(srcSupport, false);
>      }
>    } else {
> +    // This obtains the source messages from local/offline storage to do the
> +    // copy. Note: CopyMessageTo() actually handles one or more messages.

Maybe as a follow-up we could split up into two arrays and do both cases: first copy what we got locally, and only after that go and copy the ones we didn't have locally yet.
Attachment #9124922 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED

Changed "TB is" to "we're" in 3 places.

Maybe as a follow-up we could split up into two arrays and do both cases: first copy what we got locally, and only after that go and copy the ones we didn't have locally yet.

Good idea. I'll keep this in mind.

Attachment #9124922 - Attachment is obsolete: true
Attachment #9125139 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9125139 [details] [diff] [review]
505456-proposed-change(fixup1).patch

Review of attachment 9125139 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gene! Looks good. One thing: the commit message should start "Bug 505456 - ", not "Bug 505456:"
I can fix that before landing.
Attachment #9125139 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/9185bf0f28e2
Make imap folders with offline store not fetch from IMAP when copying to local folders. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 74.0
Blocks: 1566717
See Also: → 462156
You need to log in before you can comment on or make changes to this bug.