943116 - folderDisplay.selectedMessages is too slow

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jim Porter (:squib)]

folderDisplay.selectedMessages can take a while for large selections, mainly because it's QI'ing every single message. Worse, we use it all over the place, even where it's not strictly necessary. First up, we should remove the unnecessary uses of it. Then we should try to make it faster (but this will probably be a breaking API change).

Comment 1

[Jim Porter (:squib)]

Actually, I'll unassign myself for now, but I'll probably come back to this if no one else wants to take it on.

Comment 2

[Jim Porter (:squib)]

Rough strategy: first, add a getter called something like selectedCountWithCollapsed (feel free to argue about the name) that returns the number of selected messages *including* ones in collapsed threads, and then use it where possible. Then replace uses of selectedMessages that only cares about the first one with selectedMessage. Next, make selectedMessages faster and update uses of it where necessary. Finally, if things are still slow, approximate some of our code by only looking at the first N selected messages.

Comment 3

[Jim Porter (:squib)]

Wait, I lied. We don't need selectedCountWithCollapsed; selectedCount already does that. I got confused, since selectedIndices only returns the *visible* indices, and excludes the collapsed rows.

Comment 4

[Jim Porter (:squib)]

Attachment: Use selectedCount and selectedMessage where possible [checked in]

Ok, here's a patch for the first part. I don't see too much in the way of performance improvement, but it's a start.

Comment 5

[:aceman]

This looks great. Can you please comment the methods according to comment 3, if it is not done already?

I wonder how this will help with bug 778907, as the patch there seems to avoid using selectedMessages.

Comment 6

[Jim Porter (:squib)]

(In reply to :aceman from comment #5)
> This looks great. Can you please comment the methods according to comment 3,
> if it is not done already?

They're commented pretty clearly, but I failed at reading. :)

> I wonder how this will help with bug 778907, as the patch there seems to
> avoid using selectedMessages.

All of this stuff will help that bug in some way, although they may not be huge gains. Also, I think parts of the patch on bug 778907 need to be rethought (e.g. I think we should still use selectedIndices there, since selectedIndices *excludes* collapsed thread rows, which can be a big help to performance). I'll probably look into other optimizations in bug 942638, where I'm reworking the multi-message summaries in a pretty big way.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8338203 [details] [diff] [review]
Use selectedCount and selectedMessage where possible [checked in]

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

Looks good. These should definitely reduce xpcom - js boundary cross-overs, even if they don't result in a large perf improvement.

Comment 8

[Jim Porter (:squib)]

Landed (finally): https://hg.mozilla.org/comm-central/rev/6fbb12597c38

Comment 9

[Jim Porter (:squib)]

Wait, I forgot. There are more parts to this bug!

Comment 10

[Jim Porter (:squib)]

One thing that might help here would be to cache the result of selectedMessages and clear it out whenever the selection changes. My only worry is that this could cause really weird bustage if we didn't clear it out correctly.

Comment 11

[Jim Porter (:squib)]

Attachment: Make selectedMessages faster

This patch makes folderDisplay.selectedMessages about 5x faster. It's still only a modest improvement in overall performance when lots of messages are selected, but I think it safely eliminates the need to worry about using selectedMessages.

In my tests, it took about 3-4 seconds to open the context menu for 22k selected messages. This patch shaves off about 0.3 seconds from that.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8380303 [details] [diff] [review]
Make selectedMessages faster

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

Looks good, though it'd be good if we could have a test for it (not clear if this is already tested by mozmill though, so I'm willing to punt on it for now).

Something I spotted whilst looking around, I suspect that some of the issue here is canArchiveSelectedMessages and canDeleteSelectedMessages - they both get the selected messages, and then iterate through them, calling into c++ multiple times. It might be worth experimenting with those a bit and seeing if there's something around there that can improve/reduce the calls across the boundaries (or otherwise optimise).

Comment 13

[Jim Porter (:squib)]

(In reply to Mark Banner (:standard8) from comment #12)
> Something I spotted whilst looking around, I suspect that some of the issue
> here is canArchiveSelectedMessages and canDeleteSelectedMessages - they both
> get the selected messages, and then iterate through them, calling into c++
> multiple times. It might be worth experimenting with those a bit and seeing
> if there's something around there that can improve/reduce the calls across
> the boundaries (or otherwise optimise).

See bug 975795. :) 

With this patch, bug 975795, and some optimizations in the multi-message summary, selecting 22k messages takes under a second. It used to take around 5 seconds.

Comment 14

[Jim Porter (:squib)]

Attachment: Now with xpcshell tests!

Ok, here are some tests that I sort of cargo-culted from other xpcshell tests on the nsIMsgDBView.

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

Comment 15

[Jim Porter (:squib)]

Attachment: Update suite as well

And, just to be nice, here's a change to update suite code as well.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8381626 [details] [diff] [review]
Update suite as well

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

Looks great, thanks.

Comment 17

[Jim Porter (:squib)]

Comment on attachment 8381626 [details] [diff] [review]
Update suite as well

Asking for a suite review too

Comment 18

[Jim Porter (:squib)]

Attachment: Simplify suite bit

Comment 19

[Ian Neal]

Comment on attachment 8384016 [details] [diff] [review]
Simplify suite bit

Stealing review, looks good, r=me

Comment 20

[Jim Porter (:squib)]

Attachment: Patch for checkin

Comment 21

[Jim Porter (:squib)]

Setting checkin-needed since Seamonkey is still closed I think.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/393fbf5c28bf

Comment 23

[Joshua Cranmer [:jcranmer]]

Backed out in https://hg.mozilla.org/comm-central/rev/7927666950b9 due to test failures in mailnews/base/test/unit/test_nsMsgDBView.js on Linux x86-64 Debug and possibly other platforms.

Comment 24

[Jim Porter (:squib)]

Attachment: Fix tests

This fixes the tests. I'll kick off a try run when the tree is less orange and then I'll land this once I'm satisfied that everything is green.

Comment 25

[WADA:World Anti-bad-Duping Agency]

(In reply to Jim Porter (:squib) from comment #24)
> Created attachment 8391811 [details] [diff] [review]
> Fix tests

Following is example in FolderTreeView.
(wrapper for selection.getRangeAt of TreeSelection object).
> window.gFolderTreeView.getSelectedFolders :
>   function ftv_getSelectedFolders() {
>    let selection = this.selection;
>    if (!selection)
>      return [];
>
>    let folderArray = [];
>    let rangeCount = selection.getRangeCount();
>    for (let i = 0; i < rangeCount; i++) {
>      let startIndex = {};
>      let endIndex = {};
>      selection.getRangeAt(i, startIndex, endIndex);
>      for (let j = startIndex.value; j <= endIndex.value; j++) {
>        if (j < this._rowMap.length)
>          folderArray.push(this._rowMap[j]._folder);
>      }
>    }
>    return folderArray;
>  }

"getter as wrapper of access mothod to enumerated data" in gFolderDisplay is smarter than "object method to get enumerated data" in folderTreeView ;-)

This bug is for performance problem.
Is there no need to measure "gFolderDisplay.selectedMessages time" for 100 mails, 1000 mails, 10000 mails etc. in Test?
(one time manual measurement is pretty easy. "XUL menu in addon" + "var t1=new Date();var s=gFolderDisplay.selectedMessages;var t2=new Data();var msg=t2-t1;put msg to error console" is sufficient.)

Comment 26

[Jim Porter (:squib)]

I think I forgot to reland this. Here's a try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3946de95a357

Comment 27

[Jim Porter (:squib)]

Relanded: http://hg.mozilla.org/comm-central/rev/5104c2afa11c