Closed Bug 975795 Opened 6 years ago Closed 6 years ago

folderDisplay.canArchiveSelectedMessages is too slow

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 30.0

People

(Reporter: squib, Assigned: squib)

References

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

Attached patch Fix this (obsolete) — Splinter Review
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
Keywords: perf
OS: Linux → All
Hardware: x86_64 → All
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #8380315 - Attachment is obsolete: true
Attachment #8380383 - Flags: review?(mkmelin+mozilla)
Here's a try run with this and some other patches: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=ef1c9cb91761
Attached patch Fix BatchMessageMover (obsolete) — Splinter Review
Oops, I broke BatchMessageMover. This should be better.
Attachment #8380383 - Attachment is obsolete: true
Attachment #8380383 - Flags: review?(mkmelin+mozilla)
Attachment #8380386 - Flags: review?(mkmelin+mozilla)
And here's a try run with the new stuff: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d05ff8ac3db8
Attached patch Fix test failures (obsolete) — Splinter Review
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
Attachment #8380386 - Attachment is obsolete: true
Attachment #8380386 - Flags: review?(mkmelin+mozilla)
Attachment #8380399 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8380399 [details] [diff] [review]
Fix test failures

Urgh. Clearing review since this still isn't right.
Attachment #8380399 - Flags: review?(mkmelin+mozilla)
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
Attachment #8380399 - Attachment is obsolete: true
Attachment #8380427 - Flags: review?(mkmelin+mozilla)
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
Attachment #8380427 - Flags: review?(mkmelin+mozilla) → review+
(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.
Landed: https://hg.mozilla.org/comm-central/rev/06c6dffa7130
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
Blocks: 778907
You need to log in before you can comment on or make changes to this bug.