1864510 - Fix severe startup performance regression caused by bug 1846550 for profiles with many folders

Status: RESOLVED DUPLICATE of bug 1864582

(Thunderbird :: Folder and Message Lists, defect)

Description

[betterbird.project+12]

Attachment: startup-with-sort-folders.png

+++ This bug was initially created as a clone of Bug #1846550 +++

As you can see in the profiling screenshot, TB is unresponsive during startup on a profile with 1500+ folders. Sorry, we cut off the time scale, but it takes about 20sec to start, up from about 4sec. Most of the time is spent in sorting. That's on a profile where no folder was moved so far.

In general, new development like this should be tested on large profiles, also see bug 1839063 for another example.

Comment 1

[betterbird.project+12]

Can someone with editbugs privilege please change the type to "defect".

Comment 2

[betterbird.project+12]

Without looking at the code in detail, do you retrieve the sort order for all folders at startup from the the individual folder DBs? If so, do you close the DB after retrieval? If you don't, you'll be running into the 500 open file limit on Windows soon. Maybe the sort order would best be stored in the folder cache, or maybe it is already.

Comment 3

[betterbird.project+12]

There are a few things wrong here.

The values aren't put into the folder cache, some code like this would be required:
https://searchfox.org/comm-central/rev/f9b5e9d86828d56563cf6dff61ca001230931769/mailnews/base/src/nsMsgDBFolder.cpp#2095
Note this code:
https://searchfox.org/comm-central/rev/f9b5e9d86828d56563cf6dff61ca001230931769/mailnews/base/src/nsMsgDBFolder.cpp#2107-2117
If the attribute is not in the cache, it should not be retrieved from the database but just be returned as NO_SORT_VALUE.

Using true here seems wrong:
https://hg.mozilla.org/comm-central/rev/1e6c184038ca#l9.65
This will cause opening the DB here:
https://searchfox.org/comm-central/rev/f9b5e9d86828d56563cf6dff61ca001230931769/mailnews/base/src/nsMsgDBFolder.cpp#646
But even if false were passed, the database would be opened since the sort order is not part of the basic set of cached attributes which can be seen here:
https://searchfox.org/comm-central/rev/f9b5e9d86828d56563cf6dff61ca001230931769/mailnews/base/src/nsMsgDBFolder.cpp#1319

In summary: Bug 1846550 should be backed out before it hits beta in a week. The patch needs to be reworked to cache the sort order.

Comment 4

[itagagaki]

Thank you for your careful testing and analysis. I would like to have a test case to make improvements. Where can I find one?

Comment 5

[betterbird.project+12]

Maybe a bit of background. We're from a downstream project and our project leader was a Thunderbird peer, now emeritus. We ported your change together with some CSS adjustments to our product which is based on TB 115 ESR. Starting it on one of our production profiles which has 1900 folders, we saw the startup time go up from 4 seconds to 20 seconds. So you just need to get a "real life" profile from a heavy TB user. Unfortunately we didn't expand all the nodes in the profiling result, likely one would have seen all the database operations.

If you want a pre-canned setup of 810 folders, we'll sent you a ZIP file in a PM.

As for the required changes: We've summarised them: Make your sort order an attribute cached with in the folder cache, you can simply not open all the folders on startup. If not found in the cache, assume NO_SORT_VALUE. That needs extra code in nsMsgDBFolder::GetUserSortOrder() similar to nsMsgDBFolder::GetStringProperty(). It's current implementation is not viable. You should also ask a MailNews peer like BenC for review. He's very familiar with the folder cache since he re-implemented it.

Comment 6

[gene smith]

I saw this issue too earlier today before seeing this analysis. I don't have anything near 1900 folders in my profile now and am also seeing at least a 20 second delay (status bar usually just says "checking capabilities") while starting up. I also verified that the patch for bug 1846550 is causing the startup to be delayed.

Comment 7

[betterbird.project+12]

This implements our suggestion and fixes the performance regression, the fix was straight forward. Feel free to use with the appropriate attribution.
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1864510-performance-bug-1846550.patch

Comment 8

[itagagaki]

(In reply to betterbird.project+12 from comment #7)

This implements our suggestion and fixes the performance regression, the fix was straight forward. Feel free to use with the appropriate attribution.
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1864510-performance-bug-1846550.patch

Are you unwilling to submit a patch to Thunberbird?

Comment 9

[betterbird.project+12]

Not in this case. We're not managing TB, we're managing our own product. Besides, bug 1846550 was backed out, so the patch doesn't apply any more. In general, see here for upstream use of our patches: https://hg.mozilla.org/comm-central/log?rev=betterbird.

In our opinion, this bug here should closed as well as bug 1864582. Both patches should be merged into a new patch in bug 1846550 with the due attribution.

Comment 10

[itagagaki]

I'll talk to Ben C. as you betterbird.project+12 suggested.

Comment 11

[itagagaki]

FYI: The performance patch provided by betterbird.project+12 in comment #7 has been resolved the issue on their product, Jörg said.
I would ask Ben Campbell to consider the best implementation (improvement).

Comment 12

[Magnus Melin [:mkmelin]]

Let's close this, and then you merge the fixes into the main patch in bug 1864582.