330265 - nsIAbView is very slow when clearing the view of cards/lists in an address book

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

With a ldap address book of 1000 entries then selecting the clear button or switching to another address book hangs the application for about 7 seconds on my computer.

The problem is that we are deleting a card and removing it from the tree view at the same time, so we get 1000 individual removals (and hence updates) of the tree view even though it doesn't need to be seen.

Reworking the function to do it all in one batch means its so fast I can't time it by hand :-)

Patch coming up...

Comment 1

[Mark Banner (:standard8)]

Attachment: Patch v1

This patch improves the speed by doing a block update on the tree item. I also clear the selection at the beginning so that we're not having to update it each time.

I've also included a fix for [Exception... "'[JavaScript Error: "cards[i] has no properties" {file: "chrome://messenger/content/addressbook/abCommon.js" line: 315}] as this kept was showing up when selecting a blank view after clearing everything in the view and I originally thought it was directly related - it wasn't but it needs fixing anyway.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 214846 [details] [diff] [review]
Patch v1

For clearing I don't need the batch update - I can just do the row count in one go.

Comment 3

[Mark Banner (:standard8)]

Attachment: Patch v2

Same as before, but with improved code - removed the unnecessary bits.

Comment 4

[[e-mail]]

Who calls nsAbView::Close?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Who calls nsAbView::Close?

The nsAbView destructor, or function CloseAbView(), the function is called in various places, including SetAbView - selecting a new view - and unloading/closing dialogs which contain the results pane window (sidepanel, contact selection etc).

Comment 6

[[e-mail]]

(In reply to comment #0)
>The problem is that we are deleting a card and removing it from the tree view
>at the same time, so we get 1000 individual removals (and hence updates) of the
>tree view even though it doesn't need to be seen.
I looked at the whole source for nsAbView::Close and I see that it in fact sets mTree to null before removing the cards, which should mean that it doesn't update the tree. Can you give me steps to reproduce? Note that for some reason my test LDAP server only returns me 100 hits even though I've checked in the debugger and the front end code is correctly reading my maxHist preference.

Comment 7

[Mark Banner (:standard8)]

> I looked at the whole source for nsAbView::Close and I see that it in fact sets
> mTree to null before removing the cards, which should mean that it doesn't
> update the tree. Can you give me steps to reproduce? Note that for some reason
> my test LDAP server only returns me 100 hits even though I've checked in the
> debugger and the front end code is correctly reading my maxHist preference.

Firstly, I assume you did restart SeaMonkey after changing max hits? I don't think it will always pick up the change if you do it whilst its running.

Ok, I may not have tested the close case, but certainly the following should work:

1) Open ldap address book
2) Perform quick search to get lots of items
3) Wait for it to complete
4) Select "Clear" next to the quick search box.

Comment 8

[[e-mail]]

Comment on attachment 214848 [details] [diff] [review]
Patch v2

OK, so that's only the case of calling Init on an existing view. We can optimize this separately.

Comment 9

[[e-mail]]

Attachment: Optimize view reuse

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 214894 [details] [diff] [review]
Optimize view reuse

Although this patch works, it isn't as fast as mine (your version takes about 2-3 seconds, mine is complete almost straight away).

I think what is happening here is that the UI updates for the OnCountChanged are still happening and slowing the whole thing down.

Comment 11

[[e-mail]]

Attachment: View reuse v2

Is this any faster?

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 214949 [details] [diff] [review]
View reuse v2

Yep, that's much better.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 214949 [details] [diff] [review]
View reuse v2

Should rv be checked and handled for the calls to ClearSelection and SetView?   If not, please explicitly cast those method calls to void to make it clear that those results are being intentionally ignored.  r=dmose with any appropriate changes.

Comment 14

[[e-mail]]

Fix checked in to the trunk.