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

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Backend
--
major
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

(Blocks: 4 bugs, {perf, regression})

Trunk
Thunderbird 14.0
perf, regression
Dependency tree / graph
Bug Flags:
wanted-thunderbird +

Firefox Tracking Flags

(blocking-thunderbird3.1 -)

Details

(Whiteboard: [bulkoperations][gs], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
+++ 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.
(Assignee)

Updated

8 years ago
Keywords: perf
Flags: blocking-thunderbird3.1?
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.
blocking-thunderbird3.1: --- → -
Flags: blocking-thunderbird3.1? → wanted-thunderbird+
(Assignee)

Comment 3

7 years ago
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.
Keywords: regression
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.
(Assignee)

Comment 5

7 years ago
(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.
Blocks: 398684, 583365
(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?

Updated

7 years ago
Duplicate of this bug: 616224
Blocks: 616224
Severity: normal → major
Summary: Mass delete of messages resulted in massive memory usage → Mass delete of messages resulted in massive memory usage caused by adding offline undo recording code path in v3
Whiteboard: [bulkoperations]
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
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.
Whiteboard: [bulkoperations] → [bulkoperations][needs new try build]
Target Milestone: --- → Thunderbird 12.0
(Assignee)

Comment 11

6 years ago
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.
Blocks: 644333
Blocks: 693659
(Assignee)

Comment 12

5 years ago
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
Attachment #573519 - Attachment is obsolete: true
(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
Blocks: 610551
Whiteboard: [bulkoperations][needs new try build] → [bulkoperations]
bienvenu, one report of success at http://getsatisfaction.com/mozilla_messaging/topics/thunderbird_crashing_when_copying_mail_via_imap#reply_7941489
Whiteboard: [bulkoperations] → [bulkoperations][gs]
Target Milestone: Thunderbird 12.0 → ---
(Assignee)

Comment 15

5 years ago
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.
Attachment #587173 - Flags: review?(mbanner)
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?
Attachment #587173 - Flags: review?(mbanner)
(Assignee)

Comment 18

5 years ago
ugh, all the important parts of that patch have bit-rotted. I'll try to update it.
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
bug 739467 filed for db batching removal.
(Assignee)

Comment 21

5 years ago
Created attachment 609600 [details] [diff] [review]
re-de-bitrotted patch
Attachment #587173 - Attachment is obsolete: true
Attachment #609600 - Flags: review?(mbanner)
(Assignee)

Comment 22

5 years ago
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.
Attachment #609600 - Flags: review?(mbanner) → review+
(Assignee)

Comment 23

5 years ago
http://hg.mozilla.org/comm-central/rev/223c36fe5d85
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Updated

5 years ago
Duplicate of this bug: 296453
Duplicate of this bug: 644333
Duplicate of this bug: 296453
You need to log in before you can comment on or make changes to this bug.