450134 - Investigate possible performance improvement for autocomplete by using the directory search facilities

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

In bug 413260 we realised that we had a performance regression, and one of the ways to speed it up was to possibly use the directory search facility that we already have (i.e. get a directory with a query on its uri) and then use those results, rather than getting all the cards, and manually searching through them.

Joshua, would you be able to take this on?

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to comment #0)
> Joshua, would you be able to take this on?

If joshua doesn't have time, perhaps Steve could take on this important prerequisite?

Comment 3

[Steve Simms]

That's well beyond my skill level, sorry.

Comment 4

[David Taber]

Hi,

I'm the guy who filed 472576 (the dupe of this) and I have some more info (because I obviously have way too much time on my hands.

There's a severe memory usage problem here:
  - within an email, every time the auto-complete kicks in, there's a temporary 20+ MB expansion of memory footprint (5500 contacts, but less than 3000 have an email in the address book)...if you have 10 auto-completes within one mail, the memory footprint can easily be 250 MB
  - fortunately, garbage collection seems to happen after a few seconds once you're done with the mail addressing

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> There's a severe memory usage problem here:

Yes we know there is one with very large address books, although we haven't said it so far in this bug - the solution is in comment 0 - we need to use the directory search facility rather than getting all cards.

Comment 6

[Mark Banner (:standard8)]

I think this should block, its a clear perf regression and we have a potentially easy solution (I've already prototyped the idea in bug 452232 with LDAP). Taking and targeting at b3.

Comment 7

[Mark Banner (:standard8)]

Attachment: Perf Improvement v1

My test profiles has 11 local address books, 10 of which have < 10 entries, the eleventh has 5489 entries with all bar 3 fields filed in.

On a debug build this patch gives me approximately 30% speed increase on a simple search for "mark" which returns 4 entries. I think this should give us back a lot of what we lost in bug 413260.

What this patch is doing:

- Uses the directory query search to search for cards, rather than getting all cards and searching through them for matches)
-- Although the directory query also does this (get all cards, and find matches), I believe that because we're in c++ the string matching functions are better optimised and faster to access also, we're not going across the js/c++ boundary every time we want to get a card property.

- In nsAbDirectoryQuery we now get the boolean expression from the arguments at the start of process, rather than every time we look at a card. This saves a QI per card.

- It changes an include in nsAbLDAPDirectory because we don't need to include nsAbDirectoryQuery, only the interface.

Ideally we'd redo the nsAbDirectoryQuery so that we could query the database directly, rather than getting all cards and filtering. However with the potential switch to a different DB backed, I don't think that is worth doing at the moment.

Comment 8

[Mark Banner (:standard8)]

Lets aim for b3 as we have the patch.

Comment 9

[[e-mail]]

Comment on attachment 364504 [details] [diff] [review]
Perf Improvement v1

>+    // Now check through the mailing lists
Unfortunately there are parts of the code that assume that you're dealing with nsIAbCard objects, such as filtering an existing list of search results.

>+      this._parser.makeFullAddress(item.displayName,
>+                                   item.primaryEmail) :
[Odd wrapping here]

>+      while (insertPosition < result._searchResults.length &&
>+             emailAddress > result._searchResults[insertPosition].value)
>+        ++insertPosition;
Do lists have a popularity? Seeing as cards sort first by popularity, you need to do that for lists, even if the popularity is always zero.

>+      let searchFullPart = "or(PrimaryEmail,bw,@V)(DisplayName,bw,@V)(FirstName,bw,@V)(LastName,bw,@V)(NickName,bw,@V)";
>+      searchFullPart = searchFullPart.replace(/@V/g, encodeURIComponent(fullString));
>+
>+      let searchQuery;
>+      if (firstWord && rest) {
>+        let searchFNLNPart = "or(and(FirstName,bw,@V1)(LastName,bw,@V2))(and(FirstName,bw,@V2)(LastName,bw,@V1))";
>+        searchFNLNPart = searchFNLNPart.replace(/@V1/g, encodeURIComponent(firstWord));
>+        searchFNLNPart = searchFNLNPart.replace(/@V2/g, encodeURIComponent(rest));
>+
>+        searchQuery = "?(or(" + searchFullPart + ")(" + searchFNLNPart + "))";
>+      }
>+      else
>+        searchQuery = "?(" + searchFullPart + ")";
I would write
var searchQuery = "(or...)";
searchQuery = searchQuery.replace(/@V/g, encodeURIComponent(fullString));
if (firstWord && rest) {
  ...

  searchQuery = "(or" + searchQuery + searchFNLNPart + ")";
}
searchQuery = "?" + searchQuery;

>-    if (*resultLimit == 0)
>-        return NS_OK;
>+  if (resultLimit == 0)
>+    return NS_OK;
These changes are wrong. resultLimit is a pointer to a stack variable which starts off as the maximum number of cards and is decremented as cards are found. If it hits zero then we want to stop looking for cards.

Comment 10

[David :Bienvenu]

Comment on attachment 364504 [details] [diff] [review]
Perf Improvement v1

clearing review request based on Neil's comments.

Comment 11

[Mark Banner (:standard8)]

Attachment: Perf Improvement v2

New version. I realised that nsAbDirectoryQuery does return mailing lists as well as cards (all on the nice nsIAbCard interface). Therefore all we really needed was to change how we do the search.

The additional bit I've changed in nsAbDirectoryQuery is to allow us to search for something being a mailing list (or not). This allows us to restrict the search on the Notes field to mailing lists - mailing lists use that field in the back-end as their "Description" field.

We still maintain approximately (or maybe slightly better) perf improvement with this patch.

Comment 12

[[e-mail]]

Comment on attachment 366812 [details] [diff] [review]
Perf Improvement v2

>diff --git a/mailnews/addrbook/src/nsAbAutoCompleteSearch.js b/mailnews/addrbook/src/nsAbAutoCompleteSearch.js
Ah, that's much simpler :-)

>+    if (name.EqualsLiteral("IsMailList"))
>+    {
>+      PRBool isMailList;
>+      rv = card->GetIsMailList(&isMailList);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      // Only equals is supported.
>+      if (conditionType != nsIAbBooleanConditionTypes::Is)
>+        return NS_ERROR_FAILURE;
>+
>+      *matchFound = isMailList == matchValue.EqualsLiteral("true");
Strictly speaking, isn't this supposed to check against "TRUE"?
[Or better still, *matchFound = isMailList ?
    matchValue.EqualsLiteral("TRUE") : matchValue.EqualsLiteral("FALSE"); ]

Comment 13

[Mark Banner (:standard8)]

Attachment: Perf Improvement v3

Updated patch, fixing Neil's comment.

Comment 14

[[e-mail]]

Comment on attachment 366925 [details] [diff] [review]
Perf Improvement v3

Nice, I got a 5× speedup (estimated UI response time) for a 4000-card address book.

Comment 15

[David :Bienvenu]

Comment on attachment 366925 [details] [diff] [review]
Perf Improvement v3

very nice - one nit:

+      // Construct the search query, using a query means we can optimise

should be a ; instead of ,

Comment 16

[Mark Banner (:standard8)]

Patch checked in: http://hg.mozilla.org/comm-central/rev/7cee254d0f80

Marking in-testsuite+ as the existing autocomplete tests already cover the changes in this patch.