466048 - Thunderbird freezes/slow when selecting threaded Saved Search/Virtual Folder

Status: VERIFIED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[hansen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081118 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081118 Shredder/3.0b1pre ID:20081118031447

Gloda is active. Running IMAP.

With the new ability to thread Saved Search, it's possible to make an All-folder like Gmail and thread all messages to make it easier to follow conversations, but Thunderbird freezes for several seconds when in threaded view.

Reproducible: Always

Steps to Reproduce:
1. Make a Saved Search of several folders called All (my case, IMAP without Search Online and summing up to 10,000 messages)
2. Make the All-folder threaded (View > Sort By > Threaded)
3. Click the Inbox and go back to the All-folder
Actual Results:  
Thunderbird freezes and spikes the CPU to 100% for 4 seconds.

Expected Results:  
Show the folder instantly, like it's doing without threading.

This isn't Bug 216513, since it doesn't crash Thunderbird, but only freezes.
And I don't think it's Bug 226730, since it's 10,000 different regular mails.

Comment 1

[David :Bienvenu]

We have to recreate the threading info, which takes time. During b2 I'll have to do some performance analysis on where the bottlenecks are.

Comment 2

[hansen]

Thanks for looking into this.

And sorry to ask in this bug, but I can't find a bug for downloading headers from non-Inbox without clicking the folder. This is needed for an updated Saved Search.

Comment 3

[Magnus Melin [:mkmelin]]

That should be bug 428266.

Comment 4

[David :Bienvenu]

moving to b3

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Settings this to a better component.  Leaving blocking.

Comment 6

[David :Bienvenu]

probably not going to get to this for b3

Comment 7

[Wayne Mery (:wsmwk)]

bienvenu, would analysis via shark get part of what you need?
If so, perhaps one of our Mac triagers would be willing to do shark.

with latest nightly, imap, P4 3.2GHz, 10k messages (which really isn't a lot):
~4 seconds to complete the view ~100% cpu
~1 sec of which the UI is non-responsive
est. ~1/3 less time if non-threaded
don't know how much of response time is imap related

only found one perf bug (excluding imap component) regarding thread pane, bug 110121, which in fact looks to be closable (?).

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to comment #7)
> bienvenu, would analysis via shark get part of what you need?

bienvenu says c++ profiling is what's needed.  
Anyone with shark please give it a shot.
ref: http://benjamin.smedbergs.us/blog/tag/shark/

Comment 9

[Jay Moran (jaym/lazvon)]

Attachment: CPU callstacks - two Smart Folders clicks w/threading, one without

These images from Shark represent the following activity:

0) I compiled Shredder with Debug and created symbol set. Ran ShedderDebug, my main mail folders are on IMAP, and have local mail archives with several "Archive" folders setup already. I created a new Smart Search Folder that contains all of my Archive folders of a bit over 14,000 messages. I turned on message threading for the Smart Search Folder and clicked back just to make sure I saw the UI pause then spinning beach ball, which I most definitely did. Turned off threading, tried again and it was near instant. Turned threading back on and one more time and observed spinning. Left threading on, clicked back to Inbox.

1) Started Shark sampling by attaching to running ShredderDebug (thunderbird-bin process).

2) ShredderDebug was on IMAP Inbox folder, I clicked the Smart Search Folder and observed the UI doing nothing for a couple of seconds, then the spinning beach ball for several seconds, then the message list.

3) Clicked back to IMAP Inbox, then repeated step two with same results.

4) Turned off threaded mode, messages un-threaded quickly. Clicked back on Inbox and clicked back to Smart Search folder with no real sluggishness.

5) Stopped Shark sampling.

I'm afraid I have no idea what specifically I should be looking for. It did not appear there was any SINGLE function churning processor time though. A few notes on these four attachments:

CPU Callstacks: the first two sets of utilization are me clicking from Inbox to the 14k message threaded smart search folder.

Bottom Up: default view of Shark data, showing the function that had the most samples.

Top Down: view of what called the aforementioned function.

Heat Map: view of the function and code was hot in it.

Hopefully these are helpful, if they're not, I'd be happy to take some direction as I have a lull at work tomorrow before I'm swamped for a couple of weeks.

Comment 10

[Jay Moran (jaym/lazvon)]

Attachment: Bottom Up

Comment 11

[Jay Moran (jaym/lazvon)]

Attachment: Top Down

Comment 12

[Jay Moran (jaym/lazvon)]

Attachment: Heat Map

Comment 13

[David :Bienvenu]

Jay, thanks very much for the maps.

Here's what happens when you open a smart folder:

1. We open all the db's underlying the smart folder.
2. We add the cached hits for the smart folder to the view
(the first two block the UI completely - by step 3, the UI should be unblocked, though perhaps not terribly responsive)
3. Then we kick off the asynchronous searches to update the smart folder.

If one or more of the db's already open, step 1 will be faster. Inbox db's tend to be open and stay that way.

If you're threaded, I expect step 2 to be significantly slower, since the threading code hasn't been optimized.

Looking at the top down map, we're spending some time getting the search results table when we're checking if we already knew about a search hit. This can be cached somewhere, though I'd need to make sure we don't hold onto it too long. I'll try to whip up a patch so we can take advantage of your lull :-) Are you doing your own release build, or are you profiling a nightly build? In other words, will a patch be helpful?

Comment 14

[Jay Moran (jaym/lazvon)]

Yes, this is my own build with a fresh update. I can apply and test a patch and run it through shark again.

Comment 15

[David :Bienvenu]

Attachment: fix one hotspot

Jay, here's a patch to try.

Comment 16

[Jay Moran (jaym/lazvon)]

Attachment: Shark Top Down report - text versions

Comment 17

[Jay Moran (jaym/lazvon)]

Attachment: Sharp Top Down Text Version - PATCHED results

Previous text file was Top Down text of the UN-patched code. This file is the Top Down text of the patched code.

I didn't notice a huge improvement in the time myself (at least with Debug version). I can try an optimized build if that might make a difference.

Comment 18

[Jay Moran (jaym/lazvon)]

Attachment: Shark Top Down Text Version - UNpatched results

Attached incorrect file the first time.

Comment 19

[David :Bienvenu]

Attachment: correct patch for caching search results table.

I think this does help some, though I wish we didn't have to do a strcmp to see that we're using the cached table.

Comment 20

[David :Bienvenu]

Comment on attachment 375928 [details]
Sharp Top Down Text Version - PATCHED results

this implies that we're spending a ton of time restoring the selection because we had to move a thread when a new header was added. Was there really a selection to restore? Had you opened the smart folder in the same session before, and selected a message, so that we tried to select the message again? Or are we needlessly playing with the selection even though nothing is selected? I'll poke around the code.

Comment 21

[David :Bienvenu]

Jay, never mind, I see that we're restoring a non-existent selection, which involves creating a PLEvent, and firing it, which apparently isn't cheap. And the larger the folder, the more often we would end up doing this. I'll try to whip up a patch that fixes that.

Comment 22

[Jay Moran (jaym/lazvon)]

In all cases I had clicked on the smart search folder previously to starting the shark profile. In the first set of images I uploaded, I actually captured the samples for TWO clicks into the smart folder.

I've applied the new patch... well... I'm attempting to. First time I've had to deal with Mercurial and patching (and patching over another patch).

Comment 23

[David :Bienvenu]

Attachment: remove unneeded selection save and restore

this patch makes it so we don't save and restore the selection if there is no selection.

Comment 24

[David :Bienvenu]

Comment on attachment 376020 [details] [diff] [review]
remove unneeded selection save and restore

Neil might have a better way of telling if there is a selection. And iirc, I might be owed an "I told you so" since I think Neil wanted me to do something like this in the first place.

Comment 25

[Jay Moran (jaym/lazvon)]

Attachment: New Shark Top Down after all patches applied

Comment 26

[David :Bienvenu]

Comment on attachment 376020 [details] [diff] [review]
remove unneeded selection save and restore

I need to do something similar for nsMsgThreadedDBView::MoveThreadAt - ideally, SaveAndClearSelection could be smarter, along with RestoreSelection, but I hate messing with selection...

Comment 27

[Jay Moran (jaym/lazvon)]

Very nice speed ups on an optimized build. Still a slight noticable UI lockup, but NO spinning beach ball, and total time to click into 14k message smart folder is less than 1 second on a Mac Book Pro notebook (Dual Core 2.6Ghz, 4GB of RAM, 7200RPM drive). Happy to test more if there are more patches... do you want a Shark profile of the optimized build?

Comment 28

[David :Bienvenu]

I suspect we could get some small improvement by speeding up nsMsgSearchDBView::GetXFThreadFromMsgHdr, either with a small cache of msgHdrs and their corresponding xf threads (though we're using a hash table ultimately anyway), or we could go whole hog and trade space for time and have msg hdrs cache their owning xf thread, if any. But I need to think about it a bit, and I don't think I can get to it today.

If Shark will give a profile with symbols in an optimized build, I think that could be very helpful to see.

Comment 29

[Jay Moran (jaym/lazvon)]

Attachment: Optimized Build - Shark Bottom Up

Comment 30

[Jay Moran (jaym/lazvon)]

Attachment: Optimized Build - Shark hottest function top down

Comment 31

[[e-mail]]

Comment on attachment 376020 [details] [diff] [review]
remove unneeded selection save and restore

>+  PRBool hasSelection = mTree && mTreeSelection &&
>+                        NS_SUCCEEDED(mTreeSelection->GetCurrentIndex(&currentIndex)) &&
>+                        currentIndex >= 0 && currentIndex < GetSize();
I'm not convinced this (or the one in bug 379806) is the right check.

I could make it so that SaveAndClearSelection doesn't freeze the selection if it is empty, which avoids firing the selection changed event. (Things always work out if you restore the saved or a single-element selection.)

Would it be easier to tweak m_saveRestoreSelectionDepth in OnNewSearch?

Comment 32

[David :Bienvenu]

(In reply to comment #31)
> 
> >+  PRBool hasSelection = mTree && mTreeSelection &&
> >+                        NS_SUCCEEDED(mTreeSelection->GetCurrentIndex(&currentIndex)) &&
> >+                        currentIndex >= 0 && currentIndex < GetSize();
> I'm not convinced this (or the one in bug 379806) is the right check.

Surely there must be a right check that we could use.
> 
> I could make it so that SaveAndClearSelection doesn't freeze the selection if
> it is empty, which avoids firing the selection changed event. (Things always
> work out if you restore the saved or a single-element selection.)

That sounds OK - but I think you'd need to also change RestoreSelection, since unfreezing an already unfrozen selection will still fire a selection changed event, from my reading of the code.

> 
> Would it be easier to tweak m_saveRestoreSelectionDepth in OnNewSearch?

That wouldn't help with nsMsgThreadedDBView's similar issue, though I suppose I could do something similar there. And in the case where we get a lot of headers added from the new search results, I'd still like to avoid restoring the selection until there actually is one.

Comment 33

[[e-mail]]

(In reply to comment #32)
> (In reply to comment #31)
> > >+  PRBool hasSelection = mTree && mTreeSelection &&
> > >+                        NS_SUCCEEDED(mTreeSelection->GetCurrentIndex(&currentIndex)) &&
> > >+                        currentIndex >= 0 && currentIndex < GetSize();
> > I'm not convinced this (or the one in bug 379806) is the right check.
> Surely there must be a right check that we could use.
That depends on how "right" you want to be ;-) At a minimum, I can see now that a combination of this check and the one in bug 379806 would be acceptable in that it would then suffice to detect the case of a new view.

> > I could make it so that SaveAndClearSelection doesn't freeze the selection if
> > it is empty, which avoids firing the selection changed event. (Things always
> > work out if you restore the saved or a single-element selection.)
> That sounds OK - but I think you'd need to also change RestoreSelection, since
> unfreezing an already unfrozen selection will still fire a selection changed
> event, from my reading of the code.
You're right, I'd misread that - I was hoping it would only fire the event if the selection was previously frozen.

> > Would it be easier to tweak m_saveRestoreSelectionDepth in OnNewSearch?
> That wouldn't help with nsMsgThreadedDBView's similar issue, though I suppose I
> could do something similar there. And in the case where we get a lot of headers
> added from the new search results, I'd still like to avoid restoring the
> selection until there actually is one.
What's the code path for a lot of new headers added to an existing selection?

Comment 34

[David :Bienvenu]

(In reply to comment #33)

> That depends on how "right" you want to be ;-) At a minimum, I can see now that
> a combination of this check and the one in bug 379806 would be acceptable in
> that it would then suffice to detect the case of a new view.

What would that look like? Would we also check for mTreeSelection->GetCount() and GetRangeCount()?

> You're right, I'd misread that - I was hoping it would only fire the event if
> the selection was previously frozen.

I'm thinking that we could make SaveAndClearSelection check for an empty selection, and return PR_FALSE if there was no selection, w/o incrementing the selectionDepth. Callers would then know not to call RestoreSelection. I guess that would mean really getting it right, since it would affect all callers.


> What's the code path for a lot of new headers added to an existing selection?

In the case of a cross-folder saved search (and a normal folder, for that matter), we synchronously build up the view with headers we know about. Since it's synchronous, the user can't select anything. Then we kick off an asynchronous process to figure out which headers to add/remove from the view, and while this is going on, the user can select a message. So for saved searches, nsMsgSearchDBView::AddHdrFromFolder will be called both for the synchronous part of building up the view, and for the asynchronous search results for new headers.

Comment 35

[David :Bienvenu]

Attachment: fix per Neil's comments

use GetRangeCount() instead to tell  if there's a selection.

Comment 36

[David :Bienvenu]

Attachment: oops, misunderstood Neil's comment in irc

set hasSelection to true either if getRangeCount > 0, or currentIndex is in range.

Comment 37

[[e-mail]]

Attachment: Save/Restore selection option

(not thoroughly tested but seems to work)

Comment 38

[David :Bienvenu]

Comment on attachment 376465 [details] [diff] [review]
Save/Restore selection option

we should try this - one thing that's a little scary is that we'd have to convince ourselves that nothing interesting could happen between SaveAndClearSelection and RestoreSelection that would get them out of sync, e.g., if SaveAndClearSelection bailed out w/o suppressing selection events, and then selection happened, and then RestoreSelection got called...

Comment 39

[David :Bienvenu]

I've checked in the large performance win, so I'm going to take mark this fixed, though there's still stuff we can do to speed this up a bit. I think caching the search results table is still worthwhile, and the other changes I mentioned before. Perhaps I should file a follow-on bug for improving cross-folder threaded view startups in general.

Comment 40

[Wayne Mery (:wsmwk)]

wow. the results are seriously astounding!
tested on windows vista laptop, imap, 11k messages, threaded, comparing 20090429 to 20090509
initial selection (after precharging by selecting 2 biggest real folders): was 8-10 seconds, now 2-3 sec
reentering, after selecting through several others: was 567 sec, now 1-2 sec
now, also, almost 0 UI locked time.

I'll leave marking Verified to hansen.

Comment 41

[hansen]

I'm with Wayne - awesome improvement! Thanks David, Neil and Jay.
On my really old laptop, it's almost unnoticeable and doesn't trigger UI locking anymore. Way more than expected.

Comment 42

[Mark Banner (:standard8)]

Comment on attachment 376014 [details] [diff] [review]
correct patch for caching search results table.

With this patch applied, I'm seeing lots of:

WARNING: NS_ENSURE_SUCCESS(err, err) failed with result 0x80004005: file /Users/moztest/comm/main/src/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 5138

when selecting a saved search folder (I created it with the patch applied, then selecting the folder, restarting TB didn't fix the warnings).

Removing the patch, we get one warning then all is fine - which implies the search table creation isn't working properly.

>+nsresult nsMsgDatabase::GetSearchResultsTable(const char *searchFolderUri,
>+                                              PRBool createIfMissing,
>+                                              nsIMdbTable **table)
>+{
>+  if (!m_cachedSearchFolderUri.Equals(nsDependentCString(searchFolderUri)))
>+  {

If you look at where this function is called, they are all interface methods on nsIMsgDatabase, and all take "string" arguments for the searchFolderUri. However all the places those interface methods are called have nsCString variables on which .get() is called. So if you want to, you could avoid the .get() and nsDependentCString per call by changing searchFolderUri to an nsACString and the interfaces to use ACString.

Comment 43

[Mark Banner (:standard8)]

(In reply to comment #37)
> Created an attachment (id=376465) [details]
> Save/Restore selection option
> 
> (not thoroughly tested but seems to work)

Neil/David: what's happening with this patch (review from bienvenu requested)? do we still need it?

Comment 44

[David :Bienvenu]

we don't need it; it was an alternative to what I checked in, and seemed worth investigating at some point.

Comment 45

[David :Bienvenu]

Comment on attachment 376465 [details] [diff] [review]
Save/Restore selection option

clearing review request - what we have now seems to be working fine.