+++ 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.

would this have happened also on TB2?

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

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.

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.

(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...

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

(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?

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.

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/bienvenu@nventure.com-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.

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.

I've requested a try server build with this patch - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-5912f98025ee

(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/ > bienvenu@nventure.com-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

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

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.

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

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?

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

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.

bug 739467 filed for db batching removal.

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.