Closed Bug 943116 Opened 6 years ago Closed 6 years ago

folderDisplay.selectedMessages is too slow

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 32.0

People

(Reporter: squib, Assigned: squib)

References

Details

(Keywords: perf)

Attachments

(2 files, 5 obsolete files)

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).
Actually, I'll unassign myself for now, but I'll probably come back to this if no one else wants to take it on.
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW
Keywords: perf
OS: Linux → All
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.
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.
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.
Attachment #8338203 - Flags: review?(mbanner)
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.
Hardware: x86_64 → All
(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 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.
Attachment #8338203 - Flags: review?(mbanner) → review+
Landed (finally): https://hg.mozilla.org/comm-central/rev/6fbb12597c38
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Wait, I forgot. There are more parts to this bug!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch Make selectedMessages faster (obsolete) — Splinter Review
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.
Attachment #8380303 - Flags: review?(mbanner)
Assignee: nobody → squibblyflabbetydoo
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).
Attachment #8380303 - Flags: review?(standard8) → review+
(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.
Attached patch Now with xpcshell tests! (obsolete) — Splinter Review
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
Attachment #8380303 - Attachment is obsolete: true
Attachment #8381566 - Flags: review?(standard8)
Attached patch Update suite as well (obsolete) — Splinter Review
And, just to be nice, here's a change to update suite code as well.
Attachment #8381566 - Attachment is obsolete: true
Attachment #8381566 - Flags: review?(standard8)
Attachment #8381626 - Flags: review?(standard8)
Comment on attachment 8381626 [details] [diff] [review]
Update suite as well

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

Looks great, thanks.
Attachment #8381626 - Flags: review?(standard8) → review+
Comment on attachment 8381626 [details] [diff] [review]
Update suite as well

Asking for a suite review too
Attachment #8381626 - Flags: review?(mnyromyr)
Attached patch Simplify suite bit (obsolete) — Splinter Review
Attachment #8381626 - Attachment is obsolete: true
Attachment #8381626 - Flags: review?(mnyromyr)
Attachment #8384016 - Flags: review+
Attachment #8384016 - Flags: review?(mnyromyr)
Comment on attachment 8384016 [details] [diff] [review]
Simplify suite bit

Stealing review, looks good, r=me
Attachment #8384016 - Flags: review?(mnyromyr) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #8384016 - Attachment is obsolete: true
Attachment #8391625 - Flags: review+
Attachment #8338203 - Attachment description: Use selectedCount and selectedMessage where possible → Use selectedCount and selectedMessage where possible [checked in]
Setting checkin-needed since Seamonkey is still closed I think.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/393fbf5c28bf
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fix testsSplinter Review
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.
Attachment #8391625 - Attachment is obsolete: true
Attachment #8391811 - Flags: review+
(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.)
I think I forgot to reland this. Here's a try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3946de95a357
Relanded: http://hg.mozilla.org/comm-central/rev/5104c2afa11c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 29.0 → Thunderbird 30.0
Target Milestone: Thunderbird 30.0 → Thunderbird 32.0
You need to log in before you can comment on or make changes to this bug.