1364645 - Port bug 1353542 |Switch to async/await from Task.jsm/yield| to C-C's chat

Status: RESOLVED FIXED

(MailNews Core :: Backend, task, P1)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1353542 +++

There are a few uses in mailnews:
https://dxr.mozilla.org/comm-central/search?q=task.spawn&redirect=false
https://dxr.mozilla.org/comm-central/search?q=task.async&redirect=false

Florian, would you like to assist here?

Comment 1

[Wayne Mery (:wsmwk)]

I'm curious, how long has task.jsm been in use in Thunderbird?

And since this is being billed as a perf issue, I wonder what we should expect to see in Thunderbird.

Comment 2

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+2) from comment #0)
> +++ This bug was initially created as a clone of Bug #1353542 +++
> 
> There are a few uses in mailnews:
> https://dxr.mozilla.org/comm-central/search?q=task.spawn&redirect=false
> https://dxr.mozilla.org/comm-central/search?q=task.async&redirect=false
> 
> Florian, would you like to assist here?

I'm happy to help with running my auto-conversion scripts, but the process I described at bug 1353542 comment 8 has a manual part (reviewing the remaining generators and whitelisting the actual generators) which I'm unlikely to have time to do for mailnews (nor devtools or mobile/ fwiw).

Comment 3

[Florian Quèze [:florian]]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #1)
> I'm curious, how long has task.jsm been in use in Thunderbird?

It looks like there's very little use of it in Thunderbird.

> And since this is being billed as a perf issue, I wonder what we should
> expect to see in Thunderbird.

Don't expect anything impressive in terms of performance, on Firefox it was around a 1% win (ie. not very significantly above the noise margin on Talos). A great side benefit though is that it makes the stacks we see in profiles much more readable.

Comment 4

[Jorg K (CEST = GMT+2)]

Maybe we can get Aceman interested in this little clean-up project ;-)

Comment 5

[:aceman]

No, all the async stuff makes code less readable and causes new serialization problems.

Comment 6

[Jorg K (CEST = GMT+2)]

https://dxr.mozilla.org/comm-central/search?q=task.spawn+path%3Acomm%2F&redirect=false
https://dxr.mozilla.org/comm-central/search?q=task.async+path%3Acomm%2F&redirect=false

13 call sites, all in im/ or chat/.

Comment 7

[Florian Quèze [:florian]]

And some leftover Task.jsm imports in mailnews: https://searchfox.org/comm-central/search?q=%2FTask.jsm&case=false&regexp=false&path=mailnews

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1364645-remove-task-jsm-import.patch

Thanks, Florian, I'm removing those here.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9026186 [details] [diff] [review]
1364645-remove-task-jsm-import.patch

Geoff, something to land in six hours.

Comment 10

[:aceman]

Comment on attachment 9026186 [details] [diff] [review]
1364645-remove-task-jsm-import.patch

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

Why is it OK to just remove the import? Is it unused?

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9026186 [details] [diff] [review]
1364645-remove-task-jsm-import.patch

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

Indeed, it's unused. https://searchfox.org/comm-central/search?q=Task.&case=true&regexp=false&path=mailnews

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/614e7c6b9206
Removed superfluous import of Task.jsm; r=darktrojan

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1364645-async-functions.patch - WIP (v1)

OK, this a pretty mechanical replacement.

The 'yield's in the Symbol.iterator are not replaced, so we're left with a few of these in logger.js:
* [Symbol.iterator]() { while (this.hasMoreElements()) yield this.getNext(); }

In index_im.jsm there are these 'yield's left outside the async function, but I guess they need to be converted, too?
	Line 124:     yield Gloda.kWorkDone;
	Line 632:     yield Gloda.kWorkAsync;
	Line 640:     yield Gloda.kWorkDone;
	Line 647:       yield GlodaIndexer.kWorkDone;
	Line 686:     yield GlodaIndexer.kWorkDone;
	Line 706:       yield Gloda.kWorkAsync;
	Line 708:     yield Gloda.kWorkDone;

Finally, mechanical replacement left me with:
    async function () {
    }.bind(this)).catch(Cu.reportError);

Which doesn't look right.

Actually, I didn't assign the bug to myself since I was just going for the low-hanging fruit here. But now that it's assigned, we might as well finish it.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 9026321 [details] [diff] [review]
1364645-async-functions.patch - WIP (v1)

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

Thanks for working on this!

::: chat/components/src/logger.js
@@ +528,5 @@
>  Log.prototype = {
>    __proto__: ClassInfo("imILog", "Log object"),
>    _entryPaths: null,
>    format: "json",
> +  getConversation: async function () {

nit: I think this should be:
  async getConversation() {
to match the Firefox style. On Firefox code the line in your patch would trigger this eslint error:
  error  Expected method shorthand.  object-shorthand (eslint)

@@ +694,5 @@
>  function Logger() { }
>  Logger.prototype = {
>    // Returned Promise resolves to an array of entries for the
>    // log folder if it exists, otherwise null.
> +  async function _getLogArray(aAccount, aNormalizedName) {

Have you tested the patch locally? I think this will produce a parse error, this line should be:
  async _getLogArray(...
ie. remove the 'function' keyword.

@@ +752,5 @@
>    _getEnumerator: function logger__getEnumerator(aLogArray, aGroupByDay) {
>      let enumerator = aGroupByDay ? DailyLogEnumerator : LogEnumerator;
>      return aLogArray.length ? new enumerator(aLogArray) : EmptyEnumerator;
>    },
> +  async function getLogPathsForConversation(aConversation) {

Same here and in at least 4 other places in the rest of the patch.

::: chat/protocols/irc/ircCommands.jsm
@@ +281,5 @@
>              let t = 0;
>              const kMaxBlockTime = 10; // Unblock every 10ms.
>              do {
>                if (Date.now() > t) {
> +                await Promise.resolve();

I think this is trying to wait until the next tick of the event loop, but I don't trust this code. I would be more comfortable with:
  await new Promise(resolve => Services.tm.dispatchToMainThread(resolve));

@@ +290,5 @@
>                serverConv.writeMessage(serverName,
>                  name + " (" + roomInfo.participantCount + ") " + roomInfo.topic,
>                  {incoming: true, noLog: true});
>              } while (pendingChats.length);
> +          }

In this case you are replacing a Task.spawn call rather than a Task.async, so you need to call the async function. This line should end with:
  ();
I don't remember if you need to wrap the whole async function in parens (ie. (async function() { ...} )(); ) for this to work. If you do it just to be safe I won't complain ;-).

::: mail/components/im/modules/index_im.jsm
@@ +386,5 @@
>        };
>      }
>  
>      let conv = this._knownConversations[convId];
> +    async function () {

This is another Task.spawn call, so the new async function needs to be called.

Comment 15

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+1) from comment #13)
> Created attachment 9026321 [details] [diff] [review]
> 1364645-async-functions.patch - WIP (v1)
> 
> OK, this a pretty mechanical replacement.
> 
> The 'yield's in the Symbol.iterator are not replaced, so we're left with a
> few of these in logger.js:
> * [Symbol.iterator]() { while (this.hasMoreElements()) yield this.getNext();
> }

This seems fine.

> In index_im.jsm there are these 'yield's left outside the async function,
> but I guess they need to be converted, too?
> 	Line 124:     yield Gloda.kWorkDone;
> 	Line 632:     yield Gloda.kWorkAsync;
> 	Line 640:     yield Gloda.kWorkDone;
> 	Line 647:       yield GlodaIndexer.kWorkDone;
> 	Line 686:     yield GlodaIndexer.kWorkDone;
> 	Line 706:       yield Gloda.kWorkAsync;
> 	Line 708:     yield Gloda.kWorkDone;

No, please leave these lines as-is. This is part of Gloda that implemented an asynchronous system using generators and yield that is pretty similar to what Task.jsm was doing, but it's a different implementation. Ideally someday someone should do a big refactoring in Gloda to switch it to using async/await, but that's a big can of worm that's way outside the scope of this bug.


> Finally, mechanical replacement left me with:
>     async function () {
>     }.bind(this)).catch(Cu.reportError);
> 
> Which doesn't look right.

    Task.spawn(function* () {
      ...
    }.bind(this)).catch(Cu.reportError);

can become:

    (async function() {
      ...
    }).call(this).catch(Cu.reportError);

Comment 16

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #15)

> > Finally, mechanical replacement left me with:
> >     async function () {
> >     }.bind(this)).catch(Cu.reportError);
> > 
> > Which doesn't look right.
> 
>     Task.spawn(function* () {
>       ...
>     }.bind(this)).catch(Cu.reportError);
> 
> can become:
> 
>     (async function() {
>       ...
>     }).call(this).catch(Cu.reportError);

Actually, we can do cleaner:
  (async () => {
   })().catch(Cu.reportError);
should work.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1364645-async-functions.patch (v2)

I fixed all the offences, now testing. Somehow chat doesn't appear to work.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1364645-async-functions.patch (v2b)

This works now.

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 9026385 [details] [diff] [review]
1364645-async-functions.patch (v2b)

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

Looks good to me, thanks! I'm glad we are one stop closer to fully eliminating Task.jsm from mozilla-central :-).

note: there's a typo in my name in the commit message.

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks for the quick turnaround. I fixed the typo, sorry, and thanks for checking the commit message as well (many reviewers don't).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8429a12e23af
Replace Task.async()/Task.spawn() with async function and yield with await. r=florian

Comment 22

[Florian Quèze [:florian]]

FYI, I just r+'ed patches in bug 1517456 that will move Task.jsm from resource://gre/modules/Task.jsm to resource://testing-common/Task.jsm. There's one leftover reference in comm-central at https://searchfox.org/comm-central/rev/63276d503babcd01ffeb1826ec0f69a3574732cd/chat/components/src/test/test_logger.js#10 that will (I think) make the test_logger.js test fail when bug 1517456 lands. This line can just be removed.

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, I'll remove it tonight. I'll assume r=florian on the removal of that line.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1364645-follow-up.patch

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5eb606076270
Follow-up: Remove left-over reference to Task.jsm. r=florian