1085674 - Improve performance of address autocomplete single-character searches and many contacts in AB / Was: TB hangs for 30 seconds after entering first character in recipient autocomplete input (caused by MoreFunctionsForAddressBook addon)

Status: ASSIGNED

(Thunderbird :: Message Compose Window, defect)

Description

[F16JetJock]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

On Write page during keyboard task to input recipient address in the To/Cc/Bcc recipient address field, TB hangs for about 30 seconds after entering first character of recipient address.

This anomaly does NOT occur when first selecting multiple addresses from Address Book(s) in preparation for a To, Cc or Bcc broadcast. 


Actual results:

Rotating doughnut appears during hang time. Then after 30 seconds, doughnut disappears and Inbox window appears; however, Write window remains minimized.
When maximizing Write window, the first few characters that were input appear in recipient address field in addition to drop-down menu of similar address from Address Books.


Expected results:

There should be NO hanging; character input should be immediate. Before TB 31.x.x (TB 26.1.x), recipient address field permitted the immediate input of a recipient's address characters.

Comment 1

[:aceman]

You have already filed a bug like this and then closed it. A similar bug you also commented on was fixed (as a dupe of another one).
What is the problem this time? What is your exact TB version?

Comment 2

[F16JetJock]

TB version 31.2.0
I do apologize. I closed the previous one prematurely; however, perhaps not since the bug has nothing to do with auto-completion as the other bug inferred.
This bug is simply an issue with the Write-page recipient-space. Of seven out of ten attempts to input any address, I'm limited to the first character and then I have to wait thirty seconds before I can type the other characters, write the message and then Send.

The current Thunderbird installation was about 10/12/2014. It was part of a process of transferring a W7 image-backup to a new hard drive. Several different virus and malware programs were used to clean the new installation. Currently, W7 and all programs are operating flawlessly. Since all TB versions prior to 26.4.1 did not have this bug, I have to assume the bug was created in the first subsequent TB version which was TB 31.0.0?

The bug is so annoying that if not remedied soon, I'll have to consider using another email client.
Many thanks for your help.

Comment 3

[:aceman]

OK, so how many contacts do you have in your Personal addressbook AND the Collected addresses addressbook? What is the processor in your machine?
You may also want to watch bug 118624 where we try to enable the autocomplete search to be changed by the user (even back to TB24). So yes, we are aware that people are unhappy with the autocomplete changes.
But we are not aware of hangs as long as you describe. So we need to find out what is specific with your setup.

Comment 4

[F16JetJock]

Personal address book:  4,824 

Collected addresses: 2,816

What is the processor in your machine: AMD-FX-4100 3.6 Ghz, RAM 16Gb, W7 64-bit

Comment 5

[:aceman]

That is kinda strange.
I have 4000 cards in Personal and 16000 cards in Collected addresses.
I have a Phenom II X4 (at 3.6Ghz) and when I press 'a' in the recipient, it takes just 5 seconds to show the drop-down list of matches.
Granted there are probably many duplicates in the test addressbooks so those are removed from the final result set.

Comment 6

[F16JetJock]

To iterate, Aceman, this issue started with the advent of TB 31.0.0
From TB 1.0 to 24.6.1, this issue was non-existent.

Comment 7

[F16JetJock]

Moreover, there were no hardware or software changes or upgrades before upgrading to TB 31.0.0 from 
TB 24.6.1.

Comment 8

[:aceman]

So to reopen the idea to not do any autocomplete when a single char is typed, what if we DIDN'T autocomplete when there is more than say 1000 cards in ABs?

Is there a quick way to determine that?
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbDirectory.idl
There is a .childCards property on nsIAbDirectory, that seems to return all the cards so then we can count them. But iterating them in JS feels like it could be slow. Can we add a .cardsCount method that just returns an integer with the number of cards? That could be fast enough. What do you think?

Or if we wanted to be very sophisticated, we count measure the time it takes to autocomplete a single char on the user's machine and adjust the number accordingly?

Comment 9

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #8)
> So to reopen the idea to not do any autocomplete when a single char is
> typed, what if we DIDN'T autocomplete when there is more than say 1000 cards
> in ABs?

Interesting...
If at all, needs to be customizable by pref, including switch off.
To clarify, aceman suggests to NOT autocomplete at all when the full search term is a single character like "a" AND all ABs have more than say 1000 cards.
- What if search term is "foo a" (trailing single character) or "foo a bar" (embedded single character)?
- I assume in those cases, since results are less, we would search everything, right?

But I'm sceptical:

1) We have not yet established the reason for reporter's very high lag of 30secs, while aceman with same-size AB has only 5secs. Iow, we're trying to fix a problem whose cause we don't understand and which doesn't reliably reproduce (even reporter says 7 out of 10, which might involve typing speed).
a) Aceman, on which exact version did we land the speed-fix (and reference bug no pls)?
b) Can reporter really reproduce this right now on TB 31.2.0?
c) Does it depend on speed of typing for any letter (occurs for slow typing of "a" or "b", i.e. pause after "a" or "b"; does not occur when typing "abc" or "bcd" rapidly)?
d) Or does it depend on which single letter you type? "a" slow, "b" faster?
e) Does it always reliably reproduce for a specific search term like "a"?
f) Aceman, could this delay be caused when a very long results list (perhaps 5000 results) is passed into the xul dropdown list? Should we cut off the actual dropdown results list at some point, say 300 (because manually selecting from 1000 entries defies the purpose of quick autocomplete)?
g) Aceman, is there a way for reporter to find out how many results have been found in autocomplete for the searches which are slow? How many results do you have for your 5sec-search?

2) It will be very confusing UX when my one-letter searches (further improvements pending with the scoring bug) suddenly stop working for no apparent reason after I add my 1001st AB card. I think that's a strong argument against this, it's not consistent UX. So at least, this idea would require having very visible and accessible (in-product?) documentation, or even a behaviour where you can click an infobar button to search anyway (similar to "continue with global search?" in message view). And options to customize or switch off. More work again for any of these.

3) I think my main concern is about actual single-letter searches "a" whereas I expect we would always search normally for "foo a" and "foo a bar". What if user actually wants to use single letter searches (and with intelligent scoring/frecency algorithms, it can work very well!)?

Modified idea - could this work?

If total count of AB cards exceeds say 1000, AND
if full search term is single character ("a")
-> increase the programmatic delay before autocompleting to say, 1 or 1.5 secs.
If/as user continues to type ("ab", "a b" etc.), use default short delay again for faster autocompletion.

So technically like this:
When length of full autocomplete search string is 0 -> higher delay of 1 or 1.5 secs.
When length of full search string is > 0 -> short default delay of 0.25 secs or whatever we have now (we could also consider increasing that slightly to avoid premature triggering given that autocomplete is now slower).

So for all users who actually do NOT want to search for single character "a", if they just type on at a normal speed, even with short delays, autocomplete won't trigger immediately on the single letter yet, so the nasty effects of single-letter search will not occur prematurely against user's will.
But for users who deliberately type a single letter "a" and then noticeably wait for results, it will eventually trigger with a 1 second delay.

> Is there a quick way to determine that?
> http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/
> nsIAbDirectory.idl
> There is a .childCards property on nsIAbDirectory, that seems to return all
> the cards so then we can count them. But iterating them in JS feels like it
> could be slow. Can we add a .cardsCount method that just returns an integer
> with the number of cards? That could be fast enough. What do you think?

.cardsCount, if not already implemented perhaps as .childCards.count, sounds like a good idea to implement.

> Or if we wanted to be very sophisticated, we count measure the time it takes
> to autocomplete a single char on the user's machine and adjust the number
> accordingly?

I think that's a bit too sophisticated in that it will end up as an irritating UX where depending on your computer's current speed (due to other programs running etc.) you'll never know if your single-character searches will work today or not. If anything, I think the hard cut-off at a predictable AB size would be better. Or the compromise I suggested.

Comment 10

[Thomas D. (:thomas8)]

So in a nutshell, the workflow philosophy of the compromise suggestion of comment 9:
- for single character full search term ("a"), AND if AB is very big, we initially ignore and don't autocomplete immediately under the assumption that user probably doesn't want this as he might just type on (and we allow all users to do that without getting delayed against their will by premature triggering).
- If however one second later, full search term is still "a", we kinda wake up and say "Oh, so you REALLY want to search for single character "a", ok fine, we'll give you what you want but don't blame us if it takes a bit longer..." :)
- For all users typing more than single character, we respond and autocomplete swiftly (but perhaps with a slightly longer delay than now).

Comment 11

[:aceman]

(In reply to Thomas D. from comment #9)
> (In reply to :aceman from comment #8)
> > So to reopen the idea to not do any autocomplete when a single char is
> > typed, what if we DIDN'T autocomplete when there is more than say 1000 cards
> > in ABs?
> 
> Interesting...
> If at all, needs to be customizable by pref, including switch off.
> To clarify, aceman suggests to NOT autocomplete at all when the full search
> term is a single character like "a" AND all ABs have more than say 1000
> cards.
We can easily have it per-addressbook. E.g. if there are 20 cards in Personal AB, search it. But Collected AB may have 2000 cards and we skip that one. That could work for users that have a clean personal AB and those are the most used contacts wanted to be matched.

> - What if search term is "foo a" (trailing single character) or "foo a bar"
> (embedded single character)?
> - I assume in those cases, since results are less, we would search
> everything, right?
Yes, that is not a single char (together) so we search.

> But I'm sceptical:
> 
> 1) We have not yet established the reason for reporter's very high lag of
> 30secs, while aceman with same-size AB has only 5secs. Iow, we're trying to
> fix a problem whose cause we don't understand and which doesn't reliably
> reproduce (even reporter says 7 out of 10, which might involve typing speed).
True, that is the unknown.

> a) Aceman, on which exact version did we land the speed-fix (and reference
> bug no pls)?
AFAIK all speed improvements that I did are in TB31.x .

> b) Can reporter really reproduce this right now on TB 31.2.0?
He claims that in comment 2.

> c) Does it depend on speed of typing for any letter (occurs for slow typing
> of "a" or "b", i.e. pause after "a" or "b"; does not occur when typing "abc"
> or "bcd" rapidly)?
In theory it should depend. We do not yet know if the reporter tested that.

> d) Or does it depend on which single letter you type? "a" slow, "b" faster?
> e) Does it always reliably reproduce for a specific search term like "a"?
> f) Aceman, could this delay be caused when a very long results list (perhaps
> 5000 results) is passed into the xul dropdown list? Should we cut off the
> actual dropdown results list at some point, say 300 (because manually
> selecting from 1000 entries defies the purpose of quick autocomplete)?
I can test this theory. In bug 984875 I cut much time just by rewriting the internal sorting algorithm that processes the list of results. I have not timed how long the populating of the dropdown widget (seem to be outside of nsAbAutoCompleteSearch.js). But from my comments there it seems the total time got near the time needed just to retrieve the cards from database (run the AB search query). So maybe there is nothing to gain anymore (short of optimizing the C++ AB backend).

> g) Aceman, is there a way for reporter to find out how many results have
> been found in autocomplete for the searches which are slow? How many results
> do you have for your 5sec-search?
Yes, that could be found out by a one line code change in the nsAbAutoCompleteSearch.js . Not sure if he is able to do that.

> 2) It will be very confusing UX when my one-letter searches (further
> improvements pending with the scoring bug) suddenly stop working for no
> apparent reason after I add my 1001st AB card. I think that's a strong
> argument against this, it's not consistent UX. So at least, this idea would
> require having very visible and accessible (in-product?) documentation, or
> even a behaviour where you can click an infobar button to search anyway
> (similar to "continue with global search?" in message view). And options to
> customize or switch off. More work again for any of these.
> 
> 3) I think my main concern is about actual single-letter searches "a"
> whereas I expect we would always search normally for "foo a" and "foo a
> bar". What if user actually wants to use single letter searches (and with
> intelligent scoring/frecency algorithms, it can work very well!)?
These seaches should still work, they are not single letter as a whole.

> Modified idea - could this work?
> 
> If total count of AB cards exceeds say 1000, AND
> if full search term is single character ("a")
> -> increase the programmatic delay before autocompleting to say, 1 or 1.5
> secs.
> If/as user continues to type ("ab", "a b" etc.), use default short delay
> again for faster autocompletion.
> 
> So technically like this:
> When length of full autocomplete search string is 0 -> higher delay of 1 or
> 1.5 secs.
> When length of full search string is > 0 -> short default delay of 0.25 secs
> or whatever we have now (we could also consider increasing that slightly to
> avoid premature triggering given that autocomplete is now slower).
> 
> So for all users who actually do NOT want to search for single character
> "a", if they just type on at a normal speed, even with short delays,
> autocomplete won't trigger immediately on the single letter yet, so the
> nasty effects of single-letter search will not occur prematurely against
> user's will.
> But for users who deliberately type a single letter "a" and then noticeably
> wait for results, it will eventually trigger with a 1 second delay.
Interesting idea, I look at that.

> > Is there a quick way to determine that?
> > http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/
> > nsIAbDirectory.idl
> > There is a .childCards property on nsIAbDirectory, that seems to return all
> > the cards so then we can count them. But iterating them in JS feels like it
> > could be slow. Can we add a .cardsCount method that just returns an integer
> > with the number of cards? That could be fast enough. What do you think?
> 
> .cardsCount, if not already implemented perhaps as .childCards.count, sounds
> like a good idea to implement.
.childCards is a nsISimpleEnumerator which does not have a .count property. We could wrap it in fixIterator or something, but that is probably equivalent to walking all cards in JS that I didn't want to do.

Comment 12

[F16JetJock]

In my effort to help identify a pattern to the single-character 30-second pause, I discovered that in the Tools menu of the Write page, there is an option/drop-down menu of Addresses Autocomplete. 

I thus followed with an experiment where I first deselected Personal Address Book (4,824 records). The pause time was reduced to ten seconds max (for only 30% of the single-character entries). I then deselected Collected Address (2,816 records), and discovered that 100% of pauses ceased (able to immediately input additional characters).

Note: I have a total of 38 address books excluding Personal and Collected address books. Average records in each of the 38 books is about 30 (with 172 records in one book). 

Consequently, it appears that the autocomplete function begins at the first character, and while the CPU is consumed** with the autocomplete function that has to search all address books including Personal and Collected, the two latter books appear to be the primary cause of the hangs.

** I say CPU is consumed because the keyboard and mouse become inoperative until the hang ceases. Task Manager/CPU Usage up to 57% during the hang period.

Comment 13

[:aceman]

(In reply to F16JetJock from comment #12)
> In my effort to help identify a pattern to the single-character 30-second
> pause, I discovered that in the Tools menu of the Write page, there is an
> option/drop-down menu of Addresses Autocomplete. 
I do not see such an option there. Can you attach a screenshot of that menu? Maybe you have an addon/extension that adds this?

> I thus followed with an experiment where I first deselected Personal Address
> Book (4,824 records). The pause time was reduced to ten seconds max (for
> only 30% of the single-character entries). I then deselected Collected
> Address (2,816 records), and discovered that 100% of pauses ceased (able to
> immediately input additional characters).
That is to be expected when you excluded most of the contacts from the search.

> Note: I have a total of 38 address books excluding Personal and Collected
> address books. Average records in each of the 38 books is about 30 (with 172
> records in one book). 
Now this is a new point. I am sure not many people have 38 addressbooks :)
But the previous paragraph seems to imply the slowness is caused by the number of contacts in the 2 big ABs (Personal and Collected), not by the number of other ABs.

> Consequently, it appears that the autocomplete function begins at the first
> character, and while the CPU is consumed** with the autocomplete function
> that has to search all address books including Personal and Collected, the
> two latter books appear to be the primary cause of the hangs.
Yes, while the autocomplete is running through all your many contacts you can't type anything.
That is why we propose to postpone the autocomplete so that you manage to type more characters before it starts. The time it takes to finish the autocomplete depends on the number of matching contacts. The longer the string you type the more specific it is and less matches are found.

> ** I say CPU is consumed because the keyboard and mouse become inoperative
> until the hang ceases. Task Manager/CPU Usage up to 57% during the hang
> period.
It is strange that even the mouse would be non-working. The OS should handle that regardless what TB is doing.

Comment 14

[:aceman]

The autocomplete code is only triggered when the string changes (user types any character). So we can't just decide to let it be if we have 1 char and then after 1 second decide to run the search. But we can change the timeout after each keypress so e.g. we can manage it so that the timeout is 1500 ms while there is 0-1 chars input and reduce it to 300ms (today's value) if there are more chars.
So I think I can do this:
-at start of compose, determine number of contacts in all ABs.
-if it is above 1000 set initial timeout to 1500, otherwise keep 300
-if more than 1 chars are input, set timeout to 300 regardless of number of contacts.

Is that basically what you proposed?

Comment 15

[:aceman]

Attachment: WIP patch

So this is a WIP patch with the management of the timeout as described in my previous comment. It also adds the limiting of number of results.

Comment 16

[F16JetJock]

Attachment: "Addresses Autocomplete" address-selection-index window that appears on the selection of the Addresses Autocomplete option under Options in the tool bar at top of Write page.

For Aceman

Comment 17

[F16JetJock]

(In reply to :aceman from comment #14)
> The autocomplete code is only triggered when the string changes (user types
> any character). So we can't just decide to let it be if we have 1 char and
> then after 1 second decide to run the search. But we can change the timeout
> after each keypress so e.g. we can manage it so that the timeout is 1500 ms
> while there is 0-1 chars input and reduce it to 300ms (today's value) if
> there are more chars.
> So I think I can do this:
> -at start of compose, determine number of contacts in all ABs.
> -if it is above 1000 set initial timeout to 1500, otherwise keep 300
> -if more than 1 chars are input, set timeout to 300 regardless of number of
> contacts.
> 
> Is that basically what you proposed?

(In reply to :aceman from comment #13)
> (In reply to F16JetJock from comment #12)
> > In my effort to help identify a pattern to the single-character 30-second
> > pause, I discovered that in the Tools menu of the Write page, there is an
> > option/drop-down menu of Addresses Autocomplete. 
> I do not see such an option there. Can you attach a screenshot of that menu?
> Maybe you have an addon/extension that adds this?
> 
> > I thus followed with an experiment where I first deselected Personal Address
> > Book (4,824 records). The pause time was reduced to ten seconds max (for
> > only 30% of the single-character entries). I then deselected Collected
> > Address (2,816 records), and discovered that 100% of pauses ceased (able to
> > immediately input additional characters).
> That is to be expected when you excluded most of the contacts from the
> search.
> 
> > Note: I have a total of 38 address books excluding Personal and Collected
> > address books. Average records in each of the 38 books is about 30 (with 172
> > records in one book). 
> Now this is a new point. I am sure not many people have 38 addressbooks :)
> But the previous paragraph seems to imply the slowness is caused by the
> number of contacts in the 2 big ABs (Personal and Collected), not by the
> number of other ABs.
> 
> > Consequently, it appears that the autocomplete function begins at the first
> > character, and while the CPU is consumed** with the autocomplete function
> > that has to search all address books including Personal and Collected, the
> > two latter books appear to be the primary cause of the hangs.
> Yes, while the autocomplete is running through all your many contacts you
> can't type anything.
> That is why we propose to postpone the autocomplete so that you manage to
> type more characters before it starts. The time it takes to finish the
> autocomplete depends on the number of matching contacts. The longer the
> string you type the more specific it is and less matches are found.
> 
> > ** I say CPU is consumed because the keyboard and mouse become inoperative
> > until the hang ceases. Task Manager/CPU Usage up to 57% during the hang
> > period.
> It is strange that even the mouse would be non-working. The OS should handle
> that regardless what TB is doing.

RE: Aceman: It is strange that even the mouse would be non-working. The OS should handle that regardless what TB is doing.

Performed a retest. I was wrong; only the keyboard ceases to operate; mouse remains functional.

Comment 18

[:aceman]

Thanks for the attachment. I do not see any such dialog in base Thunderbird. Maybe you use an addon (Tools->Addon manager->Extensions)? Can you please try Help->Restart with addons disabled? And then try the typing test. After you finish testing you can close TB and start it normally with all your addons back enabled.

Comment 19

[F16JetJock]

Attachment: Add-ons.11.07.14.docx

List of add-ons whilst one is causing the "30-second hang" problem.

Comment 20

[F16JetJock]

Indeed, the hang problem ceased after deactivating all TB add-ons.

Comment 21

[F16JetJock]

It appears MoreFunctionsForAddressBook 0.7 was the culprit add-on.
I reactivated all other add-ons except the latter, retested, and the 30-second hangs ceased.
I will continue in this configuration until 11-10-14 before "solving" this bug.

Many thanks to all for directing me to resolution on the matter.

Comment 22

[:aceman]

Thanks for the test and finding the problem addon. You could email the author about the problem so he can fix it and you can use the addon again.

Anyway, we can proceed with the uploaded patch as it should be an improvement for all users anyway.

Comment 23

[Thomas D. (:thomas8)]

Comment on attachment 8518442 [details] [diff] [review]
WIP patch

Review of attachment 8518442 [details] [diff] [review]:
-----------------------------------------------------------------

Aceman, thanks a lot for offering this patch for what we thought out together.
This is not a technical review, I'm just reflecting the intended behaviour.

I think this looks very promising, and probably we can just try it as-is.
If we are not very sure about the real-life consequences of this, and as a matter of principle, I'd suggest using prefs to make this customizable for users with different needs & scenarios, and be on the safe side.
I have some questions and thoughts here and there but I'm not very sure about them.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +105,5 @@
>  var gAttachmentsSize;
>  var gNumUploadingAttachments;
>  
>  const kComposeAttachDirPrefName = "mail.compose.attach.dir";
> +const numberOfCardsCausingSlowAutocomplete = 1000;

Should this be a pref? We only read it at each start of composition, so no performance issues.

@@ +4118,5 @@
>    var autoCompleteWidget = document.getElementById("addressCol2#1");
> +  // If the autocomplete is expected to be slow, increase the timeout before it kicks in.
> +  // That way, user has more time to type more chars, hopefully producing
> +  // less results = faster autocomplete.
> +  autoCompleteWidget.timeout = (gSlowAutocomplete && (autoCompleteWidget.value.trim().length < 2)) ? 1500 : 300;

Nice :)

Is 1,5 secs delay for single-char-search long enough (still pretty fast?), or should it be 2 secs? This is really only for the single character search + huge AB case... so say somebody without 10-finger-touchtyping can easily take more than 1 sec before hitting the second character. For anyone else who types faster, we don't wait for the initial longer timeout to complete, but we'll immediately restart on the short default timeout, right?

I'm also wondering about the default timeout of 300 - which is so short that we'll always trigger autocomplete for every character even when user is typing at pretty high speed... Which might be intended to rapidly reduce results on the fly, but it also slows us down... not sure about this one.

All of these three numbers might be candidates for prefs...
mail.recipientAutoComplete.antiSlow.numCharsThreshold=1 (in the code, add 1 before using this)
mail.recipientAutoComplete.antiSlow.delayedTimeout=2000
mail.recipientAutoComplete.antiSlow.defaultTimeout=300

In support of prefs, Autocomplete is one of the most important UI parts used by everybody each and every time they compose, and different users have different needs (perhaps that company computer is fast enough so it doesn't choke on 10000 addresses...)

Otherwise we can try land this without prefs and see if it works well enough as a forced default...
In that case, would it make things easier for addons if we hardcoded those numbers as (global) variables instead of in-place?

@@ +4126,5 @@
>    // honor it as well
>    //
>    try
>    {
> +    // TODO: do we really need to do this at each keypress?

We should file a bug for this! Anything which slows down autocomplete should be dealt with as a matter of priority...

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +375,5 @@
>        }
>        result._searchResults.sort(order_by_popularity_and_email);
> +      // Keep only a limited number of most relevant results
> +      // as too many results are not useful in autocomplete.
> +      result._searchResults.splice(maxResults);

maxResults=100 per this patch

Do we have data how much this improves performance?

Mhhh. Cutting off the results list looks like a great idea at first, but I'm wondering how this interacts with the initial feeding stages of user's personal "frecency" algorithm which we'll implement in Bug 382415. Example:

User types "a". 300 matching contacts having "a". We only show first 100 (based on some sort of intelligent default ranking per Bug 970456). If the name I'm looking for happens to be item 101 or 250 in results list, it's not shown. So I think it's much harder (but perhaps still possible) for user to create a new frecency link between just "a" and the target contact (because at this initial stage he cannot pick that contact from the list after typing only one character). He'll probably have to type more characters to narrow down, and then over time frecency algorithm will allow him to type less again when the contact is now more popular wrt frecency. Fair enough. Otoh, even if we always show all contacts, it'll still be hard to select the target contact between 300 others after typing just "a", so yeah, it's hard either way. So well, it might just work as you proposed. Let's try :)

Depending on AB data size and structure, the cutoff might be quite random...
Do we need some indication that there are more results which have been omitted?

Comment 24

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #14)
> The autocomplete code is only triggered when the string changes (user types
> any character). So we can't just decide to let it be if we have 1 char and
> then after 1 second decide to run the search. But we can change the timeout
> after each keypress so e.g. we can manage it so that the timeout is 1500 ms
> while there is 0-1 chars input and reduce it to 300ms (today's value) if
> there are more chars.
> So I think I can do this:
> -at start of compose, determine number of contacts in all ABs.
> -if it is above 1000 set initial timeout to 1500, otherwise keep 300
> -if more than 1 chars are input, set timeout to 300 regardless of number of
> contacts.
> 
> Is that basically what you proposed?

Yes :)

Comment 25

[:aceman]

(In reply to Thomas D. from comment #23)
> I'm also wondering about the default timeout of 300 - which is so short that
> we'll always trigger autocomplete for every character even when user is
> typing at pretty high speed... Which might be intended to rapidly reduce
> results on the fly, but it also slows us down... not sure about this one.
I'm fine with increasing the times.

> All of these three numbers might be candidates for prefs...
> mail.recipientAutoComplete.antiSlow.numCharsThreshold=1 (in the code, add 1
> before using this)
> mail.recipientAutoComplete.antiSlow.delayedTimeout=2000
> mail.recipientAutoComplete.antiSlow.defaultTimeout=300
> 
> In support of prefs, Autocomplete is one of the most important UI parts used
> by everybody each and every time they compose, and different users have
> different needs (perhaps that company computer is fast enough so it doesn't
> choke on 10000 addresses...)
I have no problem making these as prefs if the reviewer accepts that.

> Otherwise we can try land this without prefs and see if it works well enough
> as a forced default...
> In that case, would it make things easier for addons if we hardcoded those
> numbers as (global) variables instead of in-place?
> 
> @@ +4126,5 @@
> >    // honor it as well
> >    //
> >    try
> >    {
> > +    // TODO: do we really need to do this at each keypress?
> 
> We should file a bug for this! Anything which slows down autocomplete should
> be dealt with as a matter of priority...
Yeah, bug 1107209.

> ::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js> @@ +375,5 @@
> >        }
> >        result._searchResults.sort(order_by_popularity_and_email);
> > +      // Keep only a limited number of most relevant results
> > +      // as too many results are not useful in autocomplete.
> > +      result._searchResults.splice(maxResults);
> 
> maxResults=100 per this patch
> 
> Do we have data how much this improves performance?
See below.

> Mhhh. Cutting off the results list looks like a great idea at first, but I'm
> wondering how this interacts with the initial feeding stages of user's
> personal "frecency" algorithm which we'll implement in Bug 382415. Example:
> 
> User types "a". 300 matching contacts having "a". We only show first 100
> (based on some sort of intelligent default ranking per Bug 970456). If the
> name I'm looking for happens to be item 101 or 250 in results list, it's not
> shown. So I think it's much harder (but perhaps still possible) for user to
> create a new frecency link between just "a" and the target contact (because
> at this initial stage he cannot pick that contact from the list after typing
> only one character). He'll probably have to type more characters to narrow
> down, and then over time frecency algorithm will allow him to type less
> again when the contact is now more popular wrt frecency. Fair enough. Otoh,
> even if we always show all contacts, it'll still be hard to select the
> target contact between 300 others after typing just "a", so yeah, it's hard
> either way. So well, it might just work as you proposed. Let's try :)
Do we expect the user to look over more than 100 results and find the proper item? I am not sure he would attempt that in that small dropdown list.

> Depending on AB data size and structure, the cutoff might be quite random...
> Do we need some indication that there are more results which have been
> omitted?
That would be nice but I don't know how to do that.
Also, the cutting of the results inside the result building code is wrong, the cache would not work.
I'd need to put it in the dropdown list construction code.

Anyway, in my benchmarks limiting the number of results does not yield an decisive speedup.
I downclocked my CPU to see any effects. On the 20000 contacts addressbook, pressing 'a' took about 25secs to autocomplete without any limiting. With limiting to 10 items, it took about 23secs. The time was varying by some seconds for unknown reasons (maybe CPU/JS JIT cache).

So I am not sure the limiting is worth implementing.

Comment 26

[Wayne Mery (:wsmwk)]

(Untriaged is a bad place for confirmed bug reports)

Comment 27

[Thomas D. (:thomas8)]

Caveat: We must ensure implementing this bug (deliberately postponing autocompletion for single-character search strings to avoid unwanted delays from too many unspecific results in big ABs) will be compatible with Bug 1012397, where users expect immediate autocomplete resolution when pressing tab or enter.

Comment 28

[:aceman]

Comment on attachment 8518442 [details] [diff] [review]
WIP patch

Mkmelin, should we continue in this direction? Should I make some prefs for the timeouts/card counts?

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8518442 [details] [diff] [review]
WIP patch

Review of attachment 8518442 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose something like this could work. I wouldn't add too many prefs

Comment 30

[Bryan]

I run version 38.6.0 and the autocomplete feature (email write) makes Thunderbird nearly unusable. I don't see how it would harm anyone if Thunderbird waited until you had given it 2 characters before it hit a big data base and tried to return thousands of results to you screen. Somehow this is much worse with the last couple of updates, not better. Sorry, this is the only active outlet I could find for this bug.

Comment 31

[:aceman]

Attachment: patch v2

This should work now. I set the autocomplete timeout to 1.5s (instead of 0.5s) if there are more than 1000 cards or there are at most 2 characters typed. Also if there is any LDAP AB it sets the slow timeout.

For local ABs, I measured the speed and getting the number of cards in 2 ABs of 20000 cards in total takes about 0.5s (in debug non-optimized build). I also made it shortcut to not count any more ABs as soon as we hit the limit of 1000 cards.

Comment 32

[:aceman]

Open questions to decide:
1. what to do with single char searches? Do not search anything? Have an even longer timeout? This is to protect the user if he types single char, then stops for some reason (like thinking whom to address) and then gets stuck for seconds due to search of that single char, resulting in a top of useless hits.
2. If there are more than 1000 cards (the slow path), and more than 2 chars are input, lower the timeout to 0.5s (as the fast path is) or keep at 1.5s anyway?

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8794446 [details] [diff] [review]
patch v2

Review of attachment 8794446 [details] [diff] [review]:
-----------------------------------------------------------------

Can we have a preference to switch the whole thing off, or better even, configure the thresholds, like the number of addresses that trigger slow treatment and the two timeouts? I'm happy with what we have now, and I don't want to burden neither myself nor other users with a 1.5 (!!) delay penalty for having more than 1000 contacts.

I have 2300 contacts, and after typing "h" the delay before the menu pops up is noticeable, but I'd estimate about less than 500 ms. And my machine is three years old with a comparatively slow AMD APU. People with a current i5 or i7 won't have any noticeable delay.

Single character searches *must* still search. Isn't there a prioritisation of "frequent" recipients? I frequently type "h" and then <enter> to confirm the first guy that pops up since this is the one I want.

Otherwise it looks good. I haven't tried it. I think the 1500/500 ms should be configurable; is there a timeout currently? There is also bug 1151520 where people hit enter too fast and get no autocompletion. Will that happen now if they hit enter in the first 1.5 seconds (assuming the "slow case")?

You have my feedback ;-) We must absolutely make sure that this doesn't affect "normal" users which currently have no problems.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4489,5 @@
> +  // If the autocomplete is expected to be slow, increase the timeout before it kicks in.
> +  // That way, user has more time to type more chars, hopefully producing
> +  // less results = faster autocomplete.
> +  autoCompleteWidget.timeout =
> +    (gSlowAutocomplete || (autoCompleteWidget.value.trim().length < 3)) ? 1500 : 500;

My mind can understand <= 2 easier ;-)

Comment 34

[:aceman]

Yes, I'm all for prefs :) Magnus said in comment 29 to not make too many prefs.

So which prefs exactly do you suggest?
1. No. of cards
2. slow timeout
3. fast timeout
Is that too many? :)

Currently there is a timeout of 300ms.

Bug 1151520 must make sure that after Enter any timeout is disregarded, autocompletion is run immediately and first match is used.

I understand the appeal of choosing a recipient by hitting a single character, but I find it too fragile. There MAY be an algorithm for frequent usage (unless it is broken). But still we change the search query from time to time so that the results (and their order) may change. So I would not expect one character searches to be a representative use-case, even less so for >1000 cards in AB. At least I always type about 5 chars to be sure to get the right hit.

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #34)
> So which prefs exactly do you suggest?
> 1. No. of cards
> 2. slow timeout
> 3. fast timeout

Thinking about it loud, you only need two:
mail.autocomplete.initial_delay, default: 300
mail.autocomplete.delay, default: 300

Whoever wants more delay for whatever reason can just tweak those numbers. You can even lose the card counting. It really makes little sense since you don't know how many contacts people have and how whether they have a problem at all. And it takes time to count despite your optimisation.

I'm totally fine with my 2300 contacts, and a threshold of 1000 would just be annoying if it caused noticeable delay. You could add a preference to decide after how many characters the "normal" delay kicks in, currently after 3 or more.

> I understand the appeal of choosing a recipient by hitting a single
> character, but I find it too fragile. There MAY be an algorithm for frequent
> usage (unless it is broken).
For the so-called "popularity index" see here: Bug 1134986.

I don't want to discuss the single character search any further. It works for me, my contact Henk comes up then I type a single "h" and of course I check before I confirm him.

Comment 36

[:aceman]

And when would those 2 delays be used?

Comment 37

[Jorg K (CEST = GMT+2)]

Like you have it: 1 or 2 characters typed, or more. As I said, ...
You could add a preference to decide after how many characters the "normal" delay kicks in, currently after 3 or more.

Comment 38

[:aceman]

Attachment: patch v3

OK, alternative approach per Jorg's suggestions.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 8794550 [details] [diff] [review]
patch v3

Review of attachment 8794550 [details] [diff] [review]:
-----------------------------------------------------------------

Can you explain why 1000 ms initial delay is good. Also, I asked a question:

There is also bug 1151520 where people hit enter too fast and get no autocompletion. Will that happen now if they hit enter in the first 1 second (assuming the "slow case")?

::: mailnews/mailnews.js
@@ +536,5 @@
>  // the first externally defined trait will have index 1001
>  pref("mailnews.traits.lastIndex", 1000);
>  
>  pref("mail.autoComplete.highlightNonMatches", true);
> +pref("mail.autoComplete.initial_delay", 1000);

Hmm, trying to trick me ;-)
"alternative approach per Jorg's suggestions."
No, I suggested 300 ms for both so existing behaviour isn't changed.

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 8794550 [details] [diff] [review]
patch v3

Review of attachment 8794550 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mailnews.js
@@ +537,5 @@
>  pref("mailnews.traits.lastIndex", 1000);
>  
>  pref("mail.autoComplete.highlightNonMatches", true);
> +pref("mail.autoComplete.initial_delay", 1000);
> +pref("mail.autoComplete.delay", 300);

Just occurred to me. This shouldn't be in mailnews. This should to to all-thunderbird.js, right?

Comment 41

[Jorg K (CEST = GMT+2)]

I tried it now, I don't like the 1000 ms initial delay. It should be as before with 300 ms.

<offtopic>
These preferences are the *ideal* tool to test bug 1151520:
Set the initial delay to, say, 5000. Type one character, then <enter>.
Result, no autocomplete, you get the one character. Now repeat with tab:
Type one character, then <tab>. You also get no autocomplete at first, but when the timeout is up, it magically autocompletes to something. Anyway, just a surprising observation. I'm sure Aceman will move on to the autocomplete bug after that ;-)
</offtopic>

Comment 42

[:aceman]

Ian, would you like any of this for Seamonkey?

Comment 43

[Ian Neal]

Potentially yes, depending on the solution.
For patch 3, if the initial delay is the same as the main delay is there any point in having two delay preferences?

Comment 44

[Jorg K (CEST = GMT+2)]

(In reply to Ian Neal from comment #43)
> For patch 3, if the initial delay is the same as the main delay is there any
> point in having two delay preferences?
The idea is that if you only typed one or two characters the delay should be longer to avoid the autocomplete to kick in before you've reduced the results by typing a third character. I think it makes sense, but I'd prefer not to change the system behaviour by making the default values different.

Autocompletion seems to be a very contentious issue as a glance at bug 1151520 shows. Making the initial delay longer than it is now will aggravate bug 1151520 even more.

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 8794550 [details] [diff] [review]
patch v3

Clearing the review for now. Looks good, but the initial delay should what we have now not to change system behaviour.

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 8794446 [details] [diff] [review]
patch v2

Review of attachment 8794446 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +106,5 @@
>  var gAttachmentsSize;
>  var gNumUploadingAttachments;
>  
>  var kComposeAttachDirPrefName = "mail.compose.attach.dir";
> +const numberOfCardsCausingSlowAutocomplete = 1000;

k for constants

@@ +2584,5 @@
> +      } catch(e) {
> +        // If the AB does not report number of cards, assume it is slow (e.g. LDAP).
> +        gSlowAutocomplete = true;
> +      }
> +    }

Not sure this logic holds up. Yes, if you have a huge local address book it will be slow. If you have normal local addressbook + ldap lookup, it will still likely be very fast.

@@ +4483,5 @@
>  }
>  
> +function setupAutocompleteTimeout()
> +{
> +  var autoCompleteWidget = document.getElementById("addressCol2#1");

this would only work for the first row. You'd need to pass in "this" autocomplete widget instead, as argument to the setupAutocompleteTimeout function

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +287,5 @@
> +// A generic implementation for ABs that do not have a faster method.
> +NS_IMETHODIMP
> +nsAbDirProperty::GetCardCount(uint32_t *count)
> +{
> +printf("ACE: ABDir");

remove this of course

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 8794550 [details] [diff] [review]
patch v3

Review of attachment 8794550 [details] [diff] [review]:
-----------------------------------------------------------------

The thing with prefs is that devs and techy people may find them and adjust to good values for themselves, and then we end up with no real feedback. The prefs are only useful for initial testing.

You need to design for an end user that isn't going to change any defaults. If you can't make it work for him, the feature is flawed and of little value in the end. There's also significant maintenance burden: somone's bound to set the wrong value, forget about it and then complain about it wasting everybody's time.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4473,5 @@
> + * @param aRecipientBox  The autocomplete input element where recipient is being typed.
> + */
> +function setupAutocompleteTimeout(aRecipientBox)
> +{
> +  // With too few characters typed, the autocomplete is expected to be slow,

That depends. My addressbook isn't that huge, but autocomplete is almost instant even with one char

Comment 48

[Jorg K (CEST = GMT+2)]

<somewhat off-topic>
Just as an observation already made in comment #44: Being able to tweak the delay is a great debugging aid for bug 1151520. But of course you can do the tweaking while debugging in a local build.
</somewhat off-topic>

Comment 49

[Richard Leger]

In light of the comleted re-design of recipent management as pills in compose window (Bug 440377), I believe this bug could be closed.

Comment 50

[Thomas D. (:thomas8)]

(In reply to Richard Leger from comment #49)

In light of the comleted re-design of recipent management as pills in compose window (Bug 440377), I believe this bug could be closed.

Has the underlying autocomplete algorithm been changed? Don't think so...

Comment 51

[Wayne Mery (:wsmwk)]

Randy, how bad is this for you when using current beta? (IIRC you are using betas)
And, is this related to your bug 1602004?