1290729 - Adding new recipients is very slow

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[:aceman]

Using the Contacts sidebar I selected about 300 recipients and clicked "Add as To".

Result:
That was very slow, took 160seconds.

Analysis:
Adding a new recipient is a complicated operation due to having to create a new element (row) in the addressing widget and even adding the recipient to spellchecker ignore list. But those were not the most expensive parts (~1-2%).

The function awSetInputAndPopupValue() (called twice in the awAddRecipient) takes 49% of the time.
The time linearly increases and at the 300th recipient the function takes ~0.49s thus adding one recipient takes about 1s in total. Adding all the 300 recipients took me 161s and then there is also some time when the JS code is done but the core is doing something (maybe GC is running) for some seconds.

Out of this function updateSendCommands() took almost all the time.

Removing that one call, takes the total time down to 4s (and also no GC).
While we need that call, once after the whole batch would be enough.

Adding 4000 recipients still took ~170s.

This is a regression from bug 431217 where I added a very slow UpdateSendLock() to updateSendCommands().

Comment 1

[:aceman]

Attachment: patch

This patch adds a "batch" flag to the awSetInputAndPopupValue() so that it can skip the updateSendCommands() until the last item.

Also adds awAddRecipientsArray(), an array-taking version of awAddRecipient so that some operations can be batched and awSetInputAndPopupValue() is only run once per recipient (not twice).

These changes got adding the 300 recipients down to 3s.

4000 recipients got down to ~70s.

Without the patch, adding 4000 recipients took several hours and TB took 7GB of RAM.

Comment 2

[:aceman]

The removal of UpdateSendLock() from the hot path changes hours -> 170s. The other batch changes make the 170s -> 70s (for 400 recipients). So the other changes are also useful.

All timing on a debug build on 3Ghz Phenom II CPU.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776377 [details] [diff] [review]
patch

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

This looks very good in general. The picky German has found a few nits. I haven't tried it, I assume it works.
Perhaps we can clarify whether you need the awAppendNewRow() and the test, see comment below.

I can try it tonight and r+ if you want to land it tonight ;-)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3228,5 @@
>  // walk through the recipients list and add them to the inline spell checker ignore list
>  function addRecipientsToIgnoreList(aAddressesToAdd)
>  {
> +  if (!gSpellChecker.enabled)
> +    return;

Please fix the indentation on the following lines. We had this discussion before and I think the consensus was not to land anything with bad indentation.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +340,5 @@
> + * @param aAddressArray   An array of recipient addresses (strings) to add.
> + */
> +function awAddRecipientsArray(aRecipientType, aAddressArray)
> +{
> +  // Let there be no empty rows so we do not need to look for them.

Nit: Can that be less pompous?
Remove any empty rows first.
What do you mean by: "Look for them"? Take them into account?

@@ +341,5 @@
> + */
> +function awAddRecipientsArray(aRecipientType, aAddressArray)
> +{
> +  // Let there be no empty rows so we do not need to look for them.
> +  awCleanupRows();

Do you need this? - the original code didn't have it.

@@ +346,5 @@
> +
> +  for (let address of aAddressArray)
> +  {
> +    if (awGetInputElement(top.MAX_RECIPIENTS).value != "")
> +      awAppendNewRow(false);

Do you need this if-clause? You removed empty rows, so therefore the top row won't be empty.
Perhaps I'm misunderstanding something here.
In my humble and uneducated opinion you either remove rows and don't test or don't remove but test.

@@ +352,5 @@
> +                            awGetPopupElement(top.MAX_RECIPIENTS), aRecipientType,
> +                            top.MAX_RECIPIENTS, true);
> +  }
> +
> +  /* be sure we still have an empty row left at the end */

Oops, Mr. Clean-up. You copied this bad comment. Needs to the upper case with a full stop at the end.

@@ +363,5 @@
> +  addRecipientsToIgnoreList(aAddressArray);
> +}
> +
> +/**
> + * This was broken out of awAddRecipients so it can be re-used...

What do the ... tell me? Please remove while you're cleaning up here.

@@ +369,5 @@
> + *
> + * @param aRecipientType  Type of recipient, e.g. addr_to.
> + * @param aAddress        A string with recipient address.
> + */
> +function awAddRecipient(aRecipientType, aAddress)

This and the following is all "boy-scout rules" clean-up with now change, right?

@@ +386,2 @@
>  
>    /* be sure we still have an empty row left at the end */

You can clean up this comment, too. Uppercase and full stop at the end ;-)

@@ +394,3 @@
>    }
>  
>    // add the recipient to our spell check ignore list

And this one.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776377 [details] [diff] [review]
patch

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +341,5 @@
> + */
> +function awAddRecipientsArray(aRecipientType, aAddressArray)
> +{
> +  // Let there be no empty rows so we do not need to look for them.
> +  awCleanupRows();

I've tried it now. This changed the existing behaviour and removes empty rows. I'd prefer not to do that since you have the test below anyway.

Comment 5

[Jorg K (CEST = GMT+2)]

Hmm, I removed the awCleanupRows(); and ended up with empty lines before the added addresses.

Would it still be possible to leave the existing behaviour in place, that is leave empty rows and fill them in?

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> Hmm, I removed the awCleanupRows(); and ended up with empty lines before the
> added addresses.
> 
> Would it still be possible to leave the existing behaviour in place, that is
> leave empty rows and fill them in?

That is part of the speedup that we do not care about empty rows to fill in. We just append to the end fast.

(In reply to Jorg K (GMT+2, PTO during summer) from comment #4)
> I've tried it now. This changed the existing behaviour and removes empty
> rows. I'd prefer not to do that since you have the test below anyway.

Yes, that is a behaviour change. But why would the previous behaviour be desirable?

If there are 100 recipients already and a hole at position 50, and user adds 10 more and only sees 9 appended (because the first one has filled a hole far out of sight), is that expected?

Comment 7

[Jorg K (CEST = GMT+2)]

OK, just fix the nits then. And what about the test?
> +    if (awGetInputElement(top.MAX_RECIPIENTS).value != "")
> +      awAppendNewRow(false);
Is this still needed after clearing the empty entries?

Comment 8

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #3)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +3228,5 @@
> >  // walk through the recipients list and add them to the inline spell checker ignore list
> >  function addRecipientsToIgnoreList(aAddressesToAdd)
> >  {
> > +  if (!gSpellChecker.enabled)
> > +    return;
> 
> Please fix the indentation on the following lines. We had this discussion
> before and I think the consensus was not to land anything with bad
> indentation.

But this is a long function that I do not want to reindent just for fun. It is not that bad and I still saw some 4 spaces indentation in some functions. I would rather just drop this change (it was just for readability) than reindent the rest.

> ::: mail/components/compose/content/addressingWidgetOverlay.js
> @@ +340,5 @@
> > + * @param aAddressArray   An array of recipient addresses (strings) to add.
> > + */
> > +function awAddRecipientsArray(aRecipientType, aAddressArray)
> > +{
> > +  // Let there be no empty rows so we do not need to look for them.
> 
> Nit: Can that be less pompous?
> Remove any empty rows first.
Ok :)

> What do you mean by: "Look for them"? Take them into account?
Yes, as found later in the comments.

> @@ +341,5 @@
> > + */
> > +function awAddRecipientsArray(aRecipientType, aAddressArray)
> > +{
> > +  // Let there be no empty rows so we do not need to look for them.
> > +  awCleanupRows();
> 
> Do you need this? - the original code didn't have it.
Yes, the original code had a loop instead, that looked for the holes (empty rows) for each new recipient.
I just run that once here.
 
> > +  for (let address of aAddressArray)
> > +  {
> > +    if (awGetInputElement(top.MAX_RECIPIENTS).value != "")
> > +      awAppendNewRow(false);
> 
> Do you need this if-clause? You removed empty rows, so therefore the top row
> won't be empty.
> Perhaps I'm misunderstanding something here.
> In my humble and uneducated opinion you either remove rows and don't test or
> don't remove but test.

Yes, I'd be happy to remove this if as the point of the batch change is that we run in a "controlled environment", without holes and just append new rows. But the awCleanupRows() still leaves one empty row if it is the only one in the widget (row 1). I have not yet found a way to handle that case without the if in the loop. Would be happy to have the if somehow before the loop. I'll try harder :)

> @@ +352,5 @@
> > +                            awGetPopupElement(top.MAX_RECIPIENTS), aRecipientType,
> > +                            top.MAX_RECIPIENTS, true);
> > +  }
> > +
> > +  /* be sure we still have an empty row left at the end */
> 
> Oops, Mr. Clean-up. You copied this bad comment. Needs to the upper case
> with a full stop at the end.

OK :)

> @@ +369,5 @@
> > + *
> > + * @param aRecipientType  Type of recipient, e.g. addr_to.
> > + * @param aAddress        A string with recipient address.
> > + */
> > +function awAddRecipient(aRecipientType, aAddress)
> 
> This and the following is all "boy-scout rules" clean-up with now change,
> right?

Yes. But nothing uses this function anymore. Expcept addons. I think we can just make it a wrapper that calls the new function and passes a single [address].

> @@ +394,3 @@
> >    }
> >  
> >    // add the recipient to our spell check ignore list
> 
> And this one.

Ok.

Comment 9

[:aceman]

(In reply to :aceman from comment #8)
> > @@ +369,5 @@
> > > + *
> > > + * @param aRecipientType  Type of recipient, e.g. addr_to.
> > > + * @param aAddress        A string with recipient address.
> > > + */
> > > +function awAddRecipient(aRecipientType, aAddress)
> > 
> > This and the following is all "boy-scout rules" clean-up with now change,
> > right?
> 
> Yes. But nothing uses this function anymore. Expcept addons. I think we can
> just make it a wrapper that calls the new function and passes a single
> [address].

Unless you insist on the hole-filling behaviour at least for the single recipient case.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #8)
> I'll try harder :)
Maybe before the loop set a bool to say that you can reuse the first empty row?

// After the clear, we have no empty rows unless the widget is completely empty.
useFirstEmptyRow = top.MAX_RECIPIENTS == 0 && awGetInputElement(top.MAX_RECIPIENTS).value == "";

loop {
  if (useFirstEmptyRow) {
    useFirstEmptyRow = false;
  } else {
    // add row
  }

And no, let's be consistent. If n>1 addresses don't fill in the holes, neither should one address.

Comment 11

[:aceman]

Attachment: patch v2

Ok, try this.

Yes, there is still an if() in the loop, but checking a boolean is faster than getting the value of an XUL element.

Comment 12

[Jorg K (CEST = GMT+2)]

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

I've looked at the patch and tested it. It's all good. But now I have to do something terrible :-( - change my mind after playing with it a little more:

It changes the system behaviour quite drastically, refer to the discussion in comment #6, and I don't know whether we will get yet another outcry from our conservative user base.

Old behaviour:
If you have a bunch of recipients and some holes in between, dragging/adding one more addresses will fill the holes and add the rest to the end.

New behaviour:
Compact the holes, add to the end.

Now in comment #6 you gave a reason for the new behaviour: Someone adds 10 new recipients and one fills a hole out of sight, 9 get added to the end. Ugly.

Here my use case:
User added 25 recipients (BTW, which ISP accepts a large number of recipients anyway?). User clears position 13 and 14 and drags two new recipients onto the hole. Wouldn't they expect those to go where they were dragged?

I don't want to be responsible for this change. While a speed-up is interesting, adding 300 recipients or even 4000 (!!!) is really unrealistic. People doing such things use mail merge (available as add-on) or a mailing list they can in fact administer.

I think the rule should be not to unexpectedly change anything the user didn't touch.

Look, the awCleanupRows() goes through the whole thing to find and eliminate holes. You can do something tricky to analyse the existing rows once, store that in an array, and when if comes to filling in addresses, you use that added magic. (In fact, just just need to create an array of hole indices. Then you fill the holes and when the array is exhausted, you add to the end.) I know you can do it ;-) 15 lines more gives the same speed-up or even more, since you avoid the reshuffling in awCleanupRows(), and leaves the old behaviour.

Or if you really want the new behaviour, get Magnus to f+ it.

Comment 13

[:aceman]

I am not sure dragging goes through this same function. But OK, I can try to fill the holes :)

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> I am not sure dragging goes through this same function. But OK, I can try to
> fill the holes :)
It's really not hard, and another speed-up :-) since nothing gets reshuffled.

Comment 15

[:aceman]

Attachment: patch v3

Yes, awCleanupRows checked all rows anyway, so if we do it instead in our function, I'm fine. As long as it does not go quadratic again, as before the patch.

Comment 16

[Jorg K (CEST = GMT+2)]

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

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

Great, I told you it was easy. But not quite right still ;-)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +380,5 @@
>    {
>      awAppendNewRow(true);
> +    awSetInputAndPopupValue(awGetInputElement(top.MAX_RECIPIENTS), "",
> +                            awGetPopupElement(top.MAX_RECIPIENTS), "addr_to",
> +                            top.MAX_RECIPIENTS, false);

Do I see a logic error here?
You must call this with false at least once, right, but you may not.
Take:
1) empty
2) full
3) empty.

You fill in 1). 3) is empty and this call is never made.

Besides, what about the focus? The appends above are called with "false" (no focus), but the one that will focus may never run.

Somehow you need to know in the loop already, whether you need to make a call with append/true and awSetInputAndPopupValue/false.

Comment 17

[:aceman]

Attachment: patch v4

Yes, thanks. Let's handle the focus and end of batch explicitly at the end.
The focus is put on the next hole (non-empty row), or the one we create at the end if there is none.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776424 [details] [diff] [review]
patch v4

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

Excellent solution. Even better with the following nits fixed ;-)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +250,5 @@
>    inputElem.id = "addressCol2#" + rowNumber;
>    inputElem.setAttribute("aria-labelledby", popupElem.id);
>  }
>  
> +function awSetInputAndPopupValue(inputElem, inputValue, popupElem, popupValue, rowNumber, aBatch = false)

Can you change this to:
aNotifyRecipientsChanged = true.
If the parameters were commented, the comment could read:
aNotifyRecipientsChanged - Notify that the recipients have changed. Generally we notify unless recipients are added in batch when the caller takes care of the notification.

@@ +373,4 @@
>  
> +    awSetInputAndPopupValue(awGetInputElement(row), address,
> +                            awGetPopupElement(row), aRecipientType,
> +                            row, true);

With the change above, this would need to change to false.

@@ +378,2 @@
>  
> +  // Be sure we still have an empty row left at the end.

// Be sure we still have an empty row left.

@@ +381,2 @@
>    {
>      awAppendNewRow(true);

// Insert empty row at the top and focus.

@@ +381,5 @@
>    {
>      awAppendNewRow(true);
> +    awSetInputAndPopupValue(awGetInputElement(top.MAX_RECIPIENTS), "",
> +                            awGetPopupElement(top.MAX_RECIPIENTS), "addr_to",
> +                            top.MAX_RECIPIENTS, true);

false here, too.

@@ +382,5 @@
>      awAppendNewRow(true);
> +    awSetInputAndPopupValue(awGetInputElement(top.MAX_RECIPIENTS), "",
> +                            awGetPopupElement(top.MAX_RECIPIENTS), "addr_to",
> +                            top.MAX_RECIPIENTS, true);
> +  } else {

Comment please:
// Focus the next empty row, if any, or the pre-existing empty top row.

Comment 19

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #18)
> @@ +381,2 @@
> >    {
> >      awAppendNewRow(true);
> 
> // Insert empty row at the top and focus.

At the top?

Comment 20

[Jorg K (CEST = GMT+2)]

OK, you're right:
// Insert empty row at the end and focus.
// Focus the next empty row, if any, or the pre-existing empty last row.

Comment 21

[:aceman]

Attachment: patch v4.1

OK, thanks, added the comments.

Pushed to try now:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=32a339dd4d7ee4092d2849482c06f442b3669773

Comment 22

[Jorg K (CEST = GMT+2)]

Perfect, thanks for following my picky suggestions.
(Sorry, I didn't get to the other review, too much other stuff to do. And bug 1290839 didn't make my day.)

Comment 23

[:aceman]

I think the tests passed.

https://hg.mozilla.org/comm-central/rev/01f5f751c3aad6ef989081f49e9a0c377886796b