Closed Bug 450134 Opened 12 years ago Closed 11 years ago

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

Categories

(MailNews Core :: Address Book, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: perf, regression, Whiteboard: [no l10n impact])

Attachments

(1 file, 2 obsolete files)

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?
Flags: blocking-thunderbird3?
Duplicate of this bug: 472576
(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?
That's well beyond my skill level, sorry.
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
(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.
Keywords: perf
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.
Assignee: Pidgeot18 → bugzilla
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Keywords: regression
Target Milestone: --- → Thunderbird 3.0b3
Status: NEW → ASSIGNED
Blocks: 112939
Whiteboard: [have patch in progress][no l10n impact]
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0rc1
Attached patch Perf Improvement v1 (obsolete) — Splinter Review
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.
Attachment #364504 - Flags: superreview?(bienvenu)
Attachment #364504 - Flags: review?(neil)
Lets aim for b3 as we have the patch.
Whiteboard: [have patch in progress][no l10n impact] → [no l10n impact][has patch][needs review bienvenu,neil]
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
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.
Attachment #364504 - Flags: review?(neil) → review-
Comment on attachment 364504 [details] [diff] [review]
Perf Improvement v1

clearing review request based on Neil's comments.
Attachment #364504 - Flags: superreview?(bienvenu)
Attached patch Perf Improvement v2 (obsolete) — Splinter Review
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.
Attachment #364504 - Attachment is obsolete: true
Attachment #366812 - Flags: superreview?(bienvenu)
Attachment #366812 - Flags: review?(neil)
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"); ]
Updated patch, fixing Neil's comment.
Attachment #366812 - Attachment is obsolete: true
Attachment #366925 - Flags: superreview?(bienvenu)
Attachment #366925 - Flags: review?(neil)
Attachment #366812 - Flags: superreview?(bienvenu)
Attachment #366812 - Flags: review?(neil)
Attachment #366925 - Flags: review?(neil) → review+
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.
Attachment #366925 - Flags: superreview?(bienvenu) → superreview+
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 ,
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [no l10n impact][has patch][needs review bienvenu,neil] → [no l10n impact]
You need to log in before you can comment on or make changes to this bug.