Last Comment Bug 538378 - Mass delete of messages resulted in massive memory usage caused by adding offline undo recording code path in v3
: Mass delete of messages resulted in massive memory usage caused by adding off...
Status: RESOLVED FIXED
[bulkoperations][gs]
: perf, regression
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- major with 2 votes (vote)
: Thunderbird 14.0
Assigned To: David :Bienvenu
:
Mentors:
http://getsatisfaction.com/mozilla_me...
: 296453 616224 644333 (view as bug list)
Depends on:
Blocks: 398684 583365 610551 693659 616224 644333
  Show dependency treegraph
 
Reported: 2010-01-07 07:48 PST by David :Bienvenu
Modified: 2013-07-05 06:51 PDT (History)
12 users (show)
dmose: wanted‑thunderbird+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
wip (28.60 KB, patch)
2011-11-10 08:15 PST, David :Bienvenu
no flags Details | Diff | Review
de-bitrotted patch (28.58 KB, patch)
2012-01-09 15:10 PST, David :Bienvenu
no flags Details | Diff | Review
re-de-bitrotted patch (23.24 KB, patch)
2012-03-26 20:43 PDT, David :Bienvenu
standard8: review+
Details | Diff | Review

Description David :Bienvenu 2010-01-07 07:48:57 PST
+++ 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, NI for questions) 2010-02-15 19:17:37 PST
would this have happened also on TB2?
Comment 2 Dan Mosedale (:dmose) 2010-02-16 13:30:29 PST
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 2010-02-16 13:46:24 PST
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 (:dmose) 2010-02-16 15:59:41 PST
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 2010-02-16 16:20:06 PST
(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 2010-10-25 16:00:27 PDT
Setting dependency of Bug 398684 and Bug 583365 to this bug, for ease of analysis and tracking.
Comment 7 WADA 2010-11-04 18:50:14 PDT
(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 8 Vik 2010-12-03 07:57:42 PST
*** Bug 616224 has been marked as a duplicate of this bug. ***
Comment 9 David :Bienvenu 2011-11-10 08:15:00 PST
Created attachment 573519 [details] [diff] [review]
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 2011-11-17 14:15:37 PST
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.
Comment 11 David :Bienvenu 2012-01-03 08:38:32 PST
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 2012-01-09 15:10:06 PST
Created attachment 587173 [details] [diff] [review]
de-bitrotted patch

I've requested a try server build with this patch - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-5912f98025ee
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2012-01-24 13:40:35 PST
(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
Comment 14 Wayne Mery (:wsmwk, NI for questions) 2012-02-03 05:09:45 PST
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 2012-02-03 07:46:05 PST
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, NI for questions) 2012-02-07 03:47:54 PST
no results so far from anyone in mozillazine build forum http://forums.mozillazine.org/viewtopic.php?p=11666219
Comment 17 Mark Banner (:standard8) 2012-03-26 01:27:22 PDT
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 2012-03-26 12:52:54 PDT
ugh, all the important parts of that patch have bit-rotted. I'll try to update it.
Comment 19 David :Bienvenu 2012-03-26 16:22:06 PDT
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 2012-03-26 16:33:17 PDT
bug 739467 filed for db batching removal.
Comment 21 David :Bienvenu 2012-03-26 20:43:19 PDT
Created attachment 609600 [details] [diff] [review]
re-de-bitrotted patch
Comment 22 David :Bienvenu 2012-03-27 10:52:45 PDT
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 2012-03-27 15:46:02 PDT
http://hg.mozilla.org/comm-central/rev/223c36fe5d85
Comment 24 David :Bienvenu 2012-03-28 06:51:53 PDT
*** Bug 296453 has been marked as a duplicate of this bug. ***
Comment 25 Wayne Mery (:wsmwk, NI for questions) 2012-06-18 09:30:37 PDT
*** Bug 644333 has been marked as a duplicate of this bug. ***
Comment 26 Wayne Mery (:wsmwk, NI for questions) 2013-07-05 06:51:10 PDT
*** Bug 296453 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.