975795 - folderDisplay.canArchiveSelectedMessages is too slow

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Jim Porter (:squib)]

Attachment: Fix this

folderDisplay.canArchiveSelectedMessages is extremely slow for large selections. This is because it tries to fetch the identity for *every* selected message, including parsing the "delivered-to" headers (which doesn't even work for anything but the currently *displayed* message).

We can optimize this considerably by only parsing the "delivered-to" headers for the currently-displayed message, and for regular folders*, fast-path it by checking if all identities for the server have archiving enabled.

I have a patch here for this that *massively* improves the performance here. We go from taking about 3 seconds to open the context menu with 22k messages selected to virtually instantaneously. Considerable improvements are also seen when initially selecting all those messages. The main thing still slowing us down there is the multi-message summary (see bug 778907).

Not putting this up for review just yet since it's 2:30 AM here and I probably messed something up. I'll double-check it in the morning.

* Anything but cross-folder searches

Comment 1

[Jim Porter (:squib)]

Attachment: Updated patch

Ok, I've updated this to be a little smarter when you've only selected one message. I also fixed BatchMessageMover so that it will bail out if you try to archive a message that can't be archived.

I cleaned up BatchMessageMover too, so it should be easier to follow now.

Comment 2

[Jim Porter (:squib)]

Here's a try run with this and some other patches: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=ef1c9cb91761

Comment 3

[Jim Porter (:squib)]

Attachment: Fix BatchMessageMover

Oops, I broke BatchMessageMover. This should be better.

Comment 4

[Jim Porter (:squib)]

And here's a try run with the new stuff: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d05ff8ac3db8

Comment 5

[Jim Porter (:squib)]

Attachment: Fix test failures

I think this fixes the test failures now. I also changed the logic somewhat; if there are <= 100 messages, it examines them all so as to be maximally accurate. Otherwise, it tries to see if all the identities for the server have archives enabled (or disabled); if they're all in the same state, it just uses that value.

Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=61033b457729

Comment 6

[Jim Porter (:squib)]

Comment on attachment 8380399 [details] [diff] [review]
Fix test failures

Urgh. Clearing review since this still isn't right.

Comment 7

[Jim Porter (:squib)]

Attachment: Fixed for real, I think...

Ok, I think this is correct finally. There are now some Mozmill tests to ensure that this works correctly. They pass locally, but here's the try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=2d0548f0f1cc

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8380427 [details] [diff] [review]
Fixed for real, I think...

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

::: mail/base/content/folderDisplay.js
@@ +2098,5 @@
> +      if (this.displayedFolder.server) {
> +        let serverIdentities = MailServices.accounts.getIdentitiesForServer(
> +          this.displayedFolder.server
> +        );
> +        let identityIter = fixIterator(

unused

@@ +2102,5 @@
> +        let identityIter = fixIterator(
> +          serverIdentities, Components.interfaces.nsIMsgIdentity
> +        );
> +
> +        let allEnabled = undefined;

don't need to assign it

@@ +2105,5 @@
> +
> +        let allEnabled = undefined;
> +        for (let identity in fixIterator(
> +          serverIdentities, Components.interfaces.nsIMsgIdentity
> +        )) {

this alignment is hard to read, i'd prefer lining up serverIdentities and Components

::: mail/base/content/mailCommands.js
@@ +98,5 @@
> +  }
> +
> +  if (!server) {
> +    let accountKey = hdr.accountKey;
> +    if (accountKey.length > 0) {

could fix this to be if (accountKey) while your at it

Comment 9

[Jim Porter (:squib)]

(In reply to Magnus Melin from comment #8)
> @@ +2102,5 @@
> > +        let identityIter = fixIterator(
> > +          serverIdentities, Components.interfaces.nsIMsgIdentity
> > +        );
> > +
> > +        let allEnabled = undefined;
> 
> don't need to assign it

I fixed everything except for this. I wanted to be extra-explicit that "allEnabled = undefined" means we don't know whether archiving is always enabled or always disabled. If the server has no identities for some reason, we just say "I dunno" and move on to doing it the hard way.

Comment 10

[Jim Porter (:squib)]

Landed: https://hg.mozilla.org/comm-central/rev/06c6dffa7130