538378 - Mass delete of messages resulted in massive memory usage caused by adding offline undo recording code path in v3

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

+++ This bug was initially created as a clone of  +++

1. clicked on an IMAP folder
2. searched by subject
3. selected all messages that matched (16000+)
4. clicked the delete button
Result: tb3.0b4 started eating memory like there was no tomorrow. Around 100MB/sec or so of virtual memory, so I killed it.

See https://bugzilla.mozilla.org/show_bug.cgi?id=525646#c4 for remaining work of creating a single undo object instead of 1000's.

Comment 1

[Wayne Mery (:wsmwk)]

would this have happened also on TB2?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I suspect this is not a regression from Tb2, but bienvenu can say for sure.  Given that, marking as blocking-, wanted+ for now.

Comment 3

[David :Bienvenu]

This would be a regression from TB 2, in the sense that TB 2 did its imap deletes online, and TB 3 does them as offline operations, and it's the offline undo recording code path that was allocating the huge number of objects.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Is it also the case that a user would have to press ctrl-z 16,000 times in order to undo this delete?

In either case, bienvenu, I'll leave the final call to you: if you feel this should block, please override the minus that I gave this bug earlier.

Comment 5

[David :Bienvenu]

(In reply to comment #4)
> Is it also the case that a user would have to press ctrl-z 16,000 times in
> order to undo this delete?

No, the operations were supposed to be coalesced. 
> 
> In either case, bienvenu, I'll leave the final call to you: if you feel this
> should block, please override the minus that I gave this bug earlier.

No, but there was a regression apparently caused by partially fixing this. I may have to fix this to fix the regression because I think backing out the partial fix would result in a blocker...

Comment 6

[WADA:World Anti-bad-Duping Agency]

Setting dependency of Bug 398684 and Bug 583365 to this bug, for ease of analysis and tracking.

Comment 7

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #3)
> TB 3 does them as offline operations, and it's the offline undo recording code path
> that was allocating the huge number of objects.

IMAP folder of offline-use=on only issue? No such problem if local mail folder?

I observed phenomenon of Bug 583365 Comment #1 by bulk move between local mail folders. I guess that freed large real memory(and virtual memory too) by "open new Tb window then close old Tb window" is huge number of objects allocated for undo delete. Is it wrong? The freed large memory is UI resource?

Comment 9

[David :Bienvenu]

Attachment: wip

this vastly improves performance of deletion of large numbers of messages. It allocates a lot less memory, and is a lot faster. I still have to work out some kinks in undoing the delete, however. This patch contains both the coalescing of undo actions and adds batching for the notifications that we do synchronously.

Comment 10

[David :Bienvenu]

I've requested try server builds with this patch, if anyone wants to try them - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-c655d2697693

it seems to be working for me here. My main concern has to do with whether the offline store is correctly populated after an undo or an undo/redo, or at least, is it as correctly populated as w/o the patch.

Comment 11

[David :Bienvenu]

This fix would improve performance of archive and moving to the junk folder, basically, any message move that's performed as an offline operation and played back later, which means any message move within an imap account that's undoable.

I think I de-bitrotted this patch on my desktop machine; I'll check.

Comment 12

[David :Bienvenu]

Attachment: de-bitrotted patch

I've requested a try server build with this patch - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-5912f98025ee

Comment 13

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #12)
> 
> I've requested a try server build with this patch -
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> [e-mail]-5912f98025ee

bienvenu, looks like the try build succeeded, at least for windows. had you posted asking for testers?

This should help drop the stats of bug 610551 according to crash comments like bp-76d08864-8cb4-49fd-a3cf-dd86f2120120 and bp-65b1b8bf-1050-43d1-a836-afc902120120

Comment 14

[Wayne Mery (:wsmwk)]

bienvenu, one report of success at http://getsatisfaction.com/mozilla_messaging/topics/thunderbird_crashing_when_copying_mail_via_imap#reply_7941489

Comment 15

[David :Bienvenu]

Comment on attachment 587173 [details] [diff] [review]
de-bitrotted patch

OK, cool, thx, Wayne.

Standard8, the high level summary is that instead of having separate undo objects for each message that we move "offline", we have a single undo object, which cuts down hugely on memory allocations. I also added some batching to cut down on notifications, which slow us down in places.

Comment 16

[Wayne Mery (:wsmwk)]

no results so far from anyone in mozillazine build forum http://forums.mozillazine.org/viewtopic.php?p=11666219

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 587173 [details] [diff] [review]
de-bitrotted patch

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

Generally this looks reasonable. I've not tested the patch yet as unfortunately it has suffered some bitrot. Some comments below.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +2860,5 @@
>        nsCOMPtr<nsIMsgFolderNotificationService> notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
>        if (notifier)
>          notifier->NotifyMsgsDeleted(hdrsToDelete);
>      }
> +    EnableNotifications(nsIMsgFolder::allMessageCountNotifications, false, false);

Wouldn't it be better to do dbBatching here? (I'm comparing with deleting messsages in nsMsgLocalMailFolder)

@@ +7368,5 @@
>    }
>  
>    if (isMove && NS_SUCCEEDED(rv) && (deleteToTrash || deleteImmediately))
> +  {
> +    srcFolder->EnableNotifications(nsIMsgFolder::allMessageCountNotifications, false, false);

I think you possibly want dbBatching here as well.

::: mailnews/imap/src/nsImapUndoTxn.cpp
@@ +705,5 @@
> +      else if (!WeAreOffline())
> +      {
> +        // couldn't find offline op - it must have been played back already
> +        // so we should redo the transaction online.
> +        return nsImapMoveCopyMsgTxn::RedoTransaction();

Presumably this will still work fine with handling multiple messages in m_srcHdrs?

Comment 18

[David :Bienvenu]

ugh, all the important parts of that patch have bit-rotted. I'll try to update it.

Comment 19

[David :Bienvenu]

db batching doesn't do anything anymore, since the pluggable store landing. It used to cache the file stream for a berkeley mailbox, so that updating the mozilla-status headers for multiple local messages would be fast. That doesn't apply for imap messages. I'll file a bug for removing db batching.

Comment 20

[David :Bienvenu]

bug 739467 filed for db batching removal.

Comment 21

[David :Bienvenu]

Attachment: re-de-bitrotted patch

Comment 22

[David :Bienvenu]

redo is a bit broken, though it was broken w/o this patch. The reason it's broken is this:

When we move a message, we remember its UID in the source folder. But undo doesn't "undelete" the original message, it moves the moved message from the target to the source, which creates a new message. Redo stores the deleted flag on the original UID in the source folder, but it needs to store the deleted flag on the moved back copy's UID.

Comment 23

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/223c36fe5d85