1290733 - UpdateSendLock() in compose window is very slow

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[:aceman]

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

The UpdateSendLock analyses all recipients even when one would be enough. See if it can be sped up.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Jorg K (CEST = GMT+2)]

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

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

Yes, I can see where this is going, but I really dislike the crude check (which was there before at least for some of the processing).

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2946,5 @@
> + */
> +function isValidAddress(aAddress) {
> +  return (aAddress.length > 0 &&
> +          ((aAddress.includes("@", 1) && !aAddress.endsWith("@"))
> +           || aAddress.toLowerCase() == "postmaster"));

This is pretty terrible. Can you send mail to "postmaster"? I don't think you can.
How about stealing some code from Recipients2CompFields?
headerParser.makeFromDisplayAddress(fieldValue, {})
.map(fullValue =>
headerParser.makeMimeAddress(
fullValue.name,
fullValue.email))
So if the parser returns some useful email, then we're in business.

@@ -2948,5 @@
>    if (!gMsgCompose)
>      return;
>  
> -  let msgCompFields = gMsgCompose.compFields;
> -  Recipients2CompFields(msgCompFields);

You know that Recipients2CompFields is expensive processing in the mime parser. So you're taking that out.

@@ +2999,5 @@
> +    if (recipients != "" && !isValidAddress(recipients))
> +    {
> +      invalidStr = recipients;
> +      break;
> +    }

Is this really more elegant than the if/else if/else if you're removing? One loop construct instead of a simple if. What's the gain? I'm sure this is anti-performance ;-)

Comment 3

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #2)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +2946,5 @@
> > + */
> > +function isValidAddress(aAddress) {
> > +  return (aAddress.length > 0 &&
> > +          ((aAddress.includes("@", 1) && !aAddress.endsWith("@"))
> > +           || aAddress.toLowerCase() == "postmaster"));
> 
> This is pretty terrible. Can you send mail to "postmaster"? I don't think
> you can.

I don't know, I just move that.

> How about stealing some code from Recipients2CompFields?
> headerParser.makeFromDisplayAddress(fieldValue, {})
> .map(fullValue =>
> headerParser.makeMimeAddress(
> fullValue.name,
> fullValue.email))
> So if the parser returns some useful email, then we're in business.

Yes, but it will be slower. Will it also work on a string of concatenated recipients? So that we could also use it in the CheckValidEmail function.

> 
> @@ -2948,5 @@
> >    if (!gMsgCompose)
> >      return;
> >  
> > -  let msgCompFields = gMsgCompose.compFields;
> > -  Recipients2CompFields(msgCompFields);
> 
> You know that Recipients2CompFields is expensive processing in the mime
> parser. So you're taking that out.

Yes. It first processes all the recipients (the 4000) and then we asked it if there was at least one.
That took several seconds. The point of this patch is to just find any first one which should be quick.

> @@ +2999,5 @@
> > +    if (recipients != "" && !isValidAddress(recipients))
> > +    {
> > +      invalidStr = recipients;
> > +      break;
> > +    }
> 
> Is this really more elegant than the if/else if/else if you're removing? One
> loop construct instead of a simple if. What's the gain? I'm sure this is
> anti-performance ;-)

I think it is more elegant, I do not need to copy aMsgCompFields.xx 3 times for each xx (which also descends into the C++ code to fetch the value). So I cache the value in 'recipients'.
I see no perf difference in a row of 'if..elseif' expressions and a loop with 'break'. Do you see a case when my version would execute any more evaluations?

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #3)
> I don't know, I just move that.
I know you moved it, but moving it didn't make it better ;-)

> > How about stealing some code from Recipients2CompFields?
> > headerParser.makeFromDisplayAddress(fieldValue, {})
> > .map(fullValue =>
> > headerParser.makeMimeAddress(
> > fullValue.name,
> > fullValue.email))
> > So if the parser returns some useful email, then we're in business.
> Yes, but it will be slower. Will it also work on a string of concatenated
> recipients? So that we could also use it in the CheckValidEmail function.
Concatenated? You mean, comma separated? Apparently so. See:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/test/unit/test_nsIMsgHeaderParser4.js#31
(you've just approved my changes to that).
What does your function do with: |a@b, klops|? That's valid, isn't it? Has an @ not at the end.

> Do you see a case when my version would execute any more
> evaluations?
Well, the loop control is overhead. I think the "if" is easier to read, six lines only.

Comment 5

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #4)
> (In reply to :aceman from comment #3)
> > I don't know, I just move that.
> I know you moved it, but moving it didn't make it better ;-)

So you want to remove "postmaster"?

> > > How about stealing some code from Recipients2CompFields?
> > > headerParser.makeFromDisplayAddress(fieldValue, {})
> > > .map(fullValue =>
> > > headerParser.makeMimeAddress(
> > > fullValue.name,
> > > fullValue.email))
> > > So if the parser returns some useful email, then we're in business.
> > Yes, but it will be slower. Will it also work on a string of concatenated
> > recipients? So that we could also use it in the CheckValidEmail function.
> Concatenated? You mean, comma separated? Apparently so. See:
> https://dxr.mozilla.org/comm-central/source/mailnews/mime/test/unit/
> test_nsIMsgHeaderParser4.js#31
> (you've just approved my changes to that).
> What does your function do with: |a@b, klops|? That's valid, isn't it? Has
> an @ not at the end.

Yes. I think we were OK with the crudeness. The previous version of UpdateSendLock used .hasrecipients on compfields and that returns true with even a single character typed. So I improved the precision :)

> > Do you see a case when my version would execute any more
> > evaluations?
> Well, the loop control is overhead. I think the "if" is easier to read, six
> lines only.

You worry about some theoretical loop overhead (inside of JS engine) but propose 9 calls into the C++ properties of msgCompFields (which AFAIK is very expensive) instead of 3 OR calling that makeFromDisplayAddress monstrosity? I can't follow that.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)
> So you want to remove "postmaster"?
Yes, as far as I see, it's invalid.

> Yes. I think we were OK with the crudeness. The previous version of
> UpdateSendLock used .hasrecipients on compfields and that returns true with
> even a single character typed. So I improved the precision :)
Seriously? .hasrecipients returns true if you only enter some text, even a single character?
 
> > Well, the loop control is overhead. I think the "if" is easier to read, six
> > lines only.
> You worry about some theoretical loop overhead (inside of JS engine) but
> propose 9 calls into the C++ properties of msgCompFields (which AFAIK is
> very expensive) instead of 3 OR calling that makeFromDisplayAddress
> monstrosity? I can't follow that.
I think the 'if' is easier to read.

Where exactly do I propose 9 calls instead of 3? Which 9? Perhaps you mean 4 at most:
if (isInvalidAddress(aMsgCompFields.to)) - 1
  invalidStr = aMsgCompFields.to;	
else if (isInvalidAddress(aMsgCompFields.cc)) - 2
  invalidStr = aMsgCompFields.cc;
else if (isInvalidAddress(aMsgCompFields.bcc)) - 3	
  invalidStr = aMsgCompFields.bcc; - 4 (unless cached in JS).
Sorry, I don't see 9, but yes, your version calls one less.

I suggested makeFromDisplayAddress (which goes off into JSMime and odes all sorts of things) to increase the precision. Given that we called Recipients2CompFields before which ran makeFromDisplayAddress on the whole lot, there is still a gain to be had if you only call it on the first one or until you find a good one.

It would also be good to rework CheckValidEmail at the same time. The name suggests that it does some validity checking.

Some other thought: You know that in HTML5 there is and input field of type "email" that does some basic checking. That code must be somewhere in M-C. Could that be used or at least looked at to steal some ideas from?

Comment 7

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #6)
> (In reply to :aceman from comment #5)
> > So you want to remove "postmaster"?
> Yes, as far as I see, it's invalid.

OK.

> > Yes. I think we were OK with the crudeness. The previous version of
> > UpdateSendLock used .hasrecipients on compfields and that returns true with
> > even a single character typed. So I improved the precision :)
> Seriously? .hasrecipients returns true if you only enter some text, even a
> single character?

Yes, try it even in TB45 and see when Send button gets enabled.

> Where exactly do I propose 9 calls instead of 3? Which 9? Perhaps you mean 4
> at most:
> if (isInvalidAddress(aMsgCompFields.to)) - 1
>   invalidStr = aMsgCompFields.to;	

These are 2 retrievals of aMsgCompFields.to. In my patch I also add a check of !="", so I count 3.

> else if (isInvalidAddress(aMsgCompFields.cc)) - 2
>   invalidStr = aMsgCompFields.cc;
> else if (isInvalidAddress(aMsgCompFields.bcc)) - 3	
>   invalidStr = aMsgCompFields.bcc; - 4 (unless cached in JS).
> Sorry, I don't see 9, but yes, your version calls one less.

So times 3 is 9 to me. In my loop there are 3 (1 for each of "to", "cc", "bcc") as I cache it in a variable. Surely that can be done with ifs, but stops being so nice as you wish.

> I suggested makeFromDisplayAddress (which goes off into JSMime and odes all
> sorts of things) to increase the precision. Given that we called
> Recipients2CompFields before which ran makeFromDisplayAddress on the whole
> lot, there is still a gain to be had if you only call it on the first one or
> until you find a good one.
> 
> It would also be good to rework CheckValidEmail at the same time. The name
> suggests that it does some validity checking.

Yes I agree with this, if we can find a fast checking method that satisfies both cases. UpdateSendLock is run on each keypress. CheckValidEmail only once per sending.

> Some other thought: You know that in HTML5 there is and input field of type
> "email" that does some basic checking. That code must be somewhere in M-C.
> Could that be used or at least looked at to steal some ideas from?

https://www.w3.org/TR/html-markup/input.email.html ?
But I think in XUL we only have https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/textbox.type at our disposal.

Also, the recipients can have different format depending on the type of recipient. E.g. newsgroups do not need to look as emails. So we would have to change the type on the input depending on these criteria. And hope the user does not want to first type a newsgroup and then toggle To to Newsgroup :)

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
> > Where exactly do I propose 9 calls instead of 3? Which 9? Perhaps you mean 4
> > at most:
> > if (isInvalidAddress(aMsgCompFields.to)) - 1
> >   invalidStr = aMsgCompFields.to;	
> 
> These are 2 retrievals of aMsgCompFields.to. In my patch I also add a check
> of !="", so I count 3.
Looks like I can't count.
|if (isInvalidAddress(aMsgCompFields.to))| is one retrieval, right?
And if isInvalidAddress() returns true on that value,
invalidStr = aMsgCompFields.to *may* retrieve it again.

So as I see it, the |if/else if/else if| are three retrievals at worst, like in your loop, and I have the overhead of retrieving it again for the condition that returned true. What am I missing?

Comment 9

[:aceman]

Rewriting my loop back into the ifs is this:

if (aMsgCompFields.to != "" && !isValidAddress(aMsgCompFields.to))
  invalidStr = recipients.to;
else if (aMsgCompFields.cc != "" && !isValidAddress(aMsgCompFields.cc))
  invalidStr = recipients.cc;
else if (aMsgCompFields.bcc != "" && !isValidAddress(aMsgCompFields.bcc))
  invalidStr = recipients.bcc;

Unless you find a clever way to not check the length (!="") here. isInvalidAddress() checks (aAddress.length > 0) but in my change that does not suit me here.

So that's 2-6 retrievals in the common case that there are all valid recipients (this is run just before send).
In the case there are no recipients there are 3 retrievals.
In case there are invalid recipients in some of the 3 types, you have 3-7 retrievals.

OK, so I miscounted the 9 :) There are 9 retrieval calls, but under no condition can all 9 be executed.

The equivalent to my loop is this:
let recipients = aMsgCompFields.to;
if (recipients != "" && !isValidAddress(recipients))
  invalidStr = recipients;
else {
  recipients = aMsgCompFields.cc;
  if (recipients != "" && !isValidAddress(recipients))
    invalidStr = recipients;
}
else {
  recipients = aMsgCompFields.bcc;
  if (recipients != "" && !isValidAddress(recipients))
    invalidStr = recipients;
}

This has 1-3 retrievals in any case. But stops being elegant.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #9)
> This has 1-3 retrievals in any case. But stops being elegant.
Do elegant and give me a nice validation ;-)

Comment 11

[:aceman]

So what does that mean? :)

Comment 12

[Jorg K (CEST = GMT+2)]

We keep the loop and we see whether we can improve the validation (removing "postmaster") and rework CheckValidEmail a little (apparently the aMsgCompFields.hasRecipients isn't so useful). Wasn't that the idea? BTW, which address does CheckValidEmailAddress() check with
  if (isInvalidAddress(aMsgCompFields.to))
Only the first one? I can't see a loop. What am I missing?

Comment 13

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #12)
> We keep the loop and we see whether we can improve the validation (removing
> "postmaster")

Ah, OK :)

> and rework CheckValidEmail a little (apparently the
> aMsgCompFields.hasRecipients isn't so useful).

Didn't I do that in the patch? The .hasRecipients is dropped from updateSendLock().

> Wasn't that the idea? BTW,
> which address does CheckValidEmailAddress() check with
>   if (isInvalidAddress(aMsgCompFields.to))
> Only the first one? I can't see a loop. What am I missing?

aMsgCompFields.to contains all of the addresses, concatenated. Therefore CheckValidEmailAddress() was happy with at least one @ in that contatenation.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> > and rework CheckValidEmail a little (apparently the
> > aMsgCompFields.hasRecipients isn't so useful).
> Didn't I do that in the patch? The .hasRecipients is dropped from
> updateSendLock()
Sorry, my mistake: CheckValidEmailAddress(). .hasRecipients doesn't make much sense there either.

> aMsgCompFields.to contains all of the addresses, concatenated. Therefore
> CheckValidEmailAddress() was happy with at least one @ in that concatenation.
That's what I meant by "better validation". So "a, @, b" is valid?

The lock should go away when the first one is valid and the overall check, which is run only once, should check all addresses, even if it's the same crude check.

Comment 15

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #14)
> > aMsgCompFields.to contains all of the addresses, concatenated. Therefore
> > CheckValidEmailAddress() was happy with at least one @ in that concatenation.
> That's what I meant by "better validation". So "a, @, b" is valid?
> 
> The lock should go away when the first one is valid and the overall check,
> which is run only once, should check all addresses, even if it's the same
> crude check.

Now I understand. I can try that, if it isn't too slow.

Comment 16

[:aceman]

Attachment: patch v2

Did you mean this?

Comment 17

[Jorg K (CEST = GMT+2)]

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

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

Looks good, but I'd clean a little harder ;-)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2984,1 @@
>    if (!aMsgCompFields.hasRecipients) {

Weren't we going to replace this? You said that this is already true when you type a single character (in fact, I checked myself). Surely your loop below notices if there are no recipients at all.

If we remove this, we can put the steel brush to it and remove the attribute hasRecipients together with Get/Set and mime_sanity_check_fields*.

Comment 18

[Jorg K (CEST = GMT+2)]

Or maybe the steel brush is a bad idea since add-ons may be using it. However, we can remove the use of .hasRecipients.

Comment 19

[:aceman]

Yes, I would not like to remove it, it may be useful elsewhere.

But it also considers newsgroups, that my loop does not count. Should I just check them msgComFields.newsgroups != "" ?

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #19)
> But it also considers newsgroups, that my loop does not count. Should I just
> check them msgComFields.newsgroups != "" ?
I've looked at
https://dxr.mozilla.org/comm-central/rev/8f57173fe6a885b1de01b1e9510a3ad8e3f45753/mailnews/compose/src/nsMsgCompFields.cpp#248
(getting into the habit of using permalinks)
and it seems like a waste to GetTo, GetCc, GetBcc, GetNewsgroups when you get them anyway in your loop.
Yes, for the newgroups you could just go: msgComFields.newsgroups.trim() != "".
I noticed that mime_sanity_check_fields_recipients() is busily skipping spaces.

Yes, something like:
if (totalToCcBcc == 0 && msgComFields.newsgroups.trim() == "")
  complain about no recipients.

I'll leave the elegance to you ;-)

Comment 21

[:aceman]

Attachment: patch v2.1

Ok :)

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8779876 [details] [diff] [review]
patch v2.1

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

Nice. I shouldn't r+ this since there is a logic error. But I trust you ;-)
r=jorgk.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2993,5 @@
> +        invalidStr = recipient;
> +        break;
> +      }
> +      if (invalidStr)
> +        break;

This is at the wrong indentation level. It needs to move after the } that follows.

@@ +2997,5 @@
> +        break;
> +    }
> +  }
> +
> +  if (recipientCount == 0 && aMsgCompFields.newsgroups == "") {

Two questions:
1) How can I test this? The Send button is disabled when there are no recipients, so how can I get here?
2) aMsgCompFields.newsgroups is already trimmed?
   .hasRecipients used to trim spaces in mime_sanity_check_fields_recipients().

Comment 23

[:aceman]

Attachment: patch v2.2

(In reply to Jorg K (GMT+2, PTO during summer) from comment #22)
> Nice. I shouldn't r+ this since there is a logic error. But I trust you ;-)
> r=jorgk.
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +2993,5 @@
> > +        invalidStr = recipient;
> > +        break;
> > +      }
> > +      if (invalidStr)
> > +        break;
> 
> This is at the wrong indentation level. It needs to move after the } that
> follows.

Right, good catch.

> > +  if (recipientCount == 0 && aMsgCompFields.newsgroups == "") {
> 
> Two questions:
> 1) How can I test this? The Send button is disabled when there are no
> recipients, so how can I get here?

I think I implemented this check before the disabling of the send button :)
So normally you can't but it is a safety measure in case there is a way to attempt send bypassing the send button/command.

You can try it by setting gSendLocked = false and exiting updateSendLock() immediately.

> 2) aMsgCompFields.newsgroups is already trimmed?
>    .hasRecipients used to trim spaces in

I put the trim() there but it got lost somehow. Maybe forgot hg qref.

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #23)
> Right, good catch.
That's why we do reviews ;-)

> You can try it by setting gSendLocked = false and exiting updateSendLock()
> immediately.
Done that. Worked.

> I put the trim() there but it got lost somehow. Maybe forgot hg qref.
Always "hg diff" before attaching a patch ;-)

Conclusion: All good. Now all you need to do is to fix the current bustage and land this ;-)

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/cd8d6e07925e