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

RESOLVED FIXED in Thunderbird 65.0

Status

P1
normal
RESOLVED FIXED
2 years ago
12 days ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

({perf})

unspecified
Thunderbird 65.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
+++ 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?
Flags: needinfo?(florian)
See Also: → bug 1364677
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.
Keywords: perf
(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).
Flags: needinfo?(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.
(Assignee)

Comment 4

2 years ago
Maybe we can get Aceman interested in this little clean-up project ;-)
Flags: needinfo?(acelists)

Comment 5

2 months ago
No, all the async stuff makes code less readable and causes new serialization problems.
Flags: needinfo?(acelists)
(Assignee)

Comment 8

2 months ago
Created attachment 9026186 [details] [diff] [review]
1364645-remove-task-jsm-import.patch

Thanks, Florian, I'm removing those here.
Attachment #9026186 - Flags: review?(acelists)
(Assignee)

Updated

2 months ago
Keywords: leave-open
(Assignee)

Comment 9

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

Geoff, something to land in six hours.
Attachment #9026186 - Flags: review?(geoff)

Comment 10

2 months ago
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?

Updated

2 months ago
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
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
Attachment #9026186 - Flags: review?(geoff)
Attachment #9026186 - Flags: review?(acelists)
Attachment #9026186 - Flags: review+

Comment 12

2 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/614e7c6b9206
Removed superfluous import of Task.jsm; r=darktrojan
(Assignee)

Comment 13

2 months ago
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(); }

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.
Attachment #9026321 - Flags: feedback?(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.
Attachment #9026321 - Flags: feedback?(florian) → review-
(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);
(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.
(Assignee)

Comment 17

2 months ago
Created attachment 9026378 [details] [diff] [review]
1364645-async-functions.patch (v2)

I fixed all the offences, now testing. Somehow chat doesn't appear to work.
Attachment #9026321 - Attachment is obsolete: true
(Assignee)

Comment 18

2 months ago
Created attachment 9026385 [details] [diff] [review]
1364645-async-functions.patch (v2b)

This works now.
Attachment #9026378 - Attachment is obsolete: true
Attachment #9026385 - Flags: review?(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.
Attachment #9026385 - Flags: review?(florian) → review+
(Assignee)

Comment 20

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

Updated

2 months ago
Keywords: leave-open
(Assignee)

Updated

2 months ago
Summary: Port bug 1353542 |Switch to async/await from Task.jsm/yield| to C-C (mailnews, char, calendar) → Port bug 1353542 |Switch to async/await from Task.jsm/yield| to C-C's chat

Comment 21

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8429a12e23af
Replace Task.async()/Task.spawn() with async function and yield with await. r=florian
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
(Assignee)

Updated

2 months ago
Target Milestone: --- → Thunderbird 65.0
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.
Flags: needinfo?(jorgk)
(Assignee)

Comment 23

13 days ago
Thanks for the heads-up, I'll remove it tonight. I'll assume r=florian on the removal of that line.
Flags: needinfo?(jorgk)
(Assignee)

Comment 24

13 days ago
Created attachment 9034214 [details] [diff] [review]
1364645-follow-up.patch
Attachment #9034214 - Flags: review?(florian)
Attachment #9034214 - Flags: review?(florian) → review+

Comment 25

12 days ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5eb606076270
Follow-up: Remove left-over reference to Task.jsm. r=florian
You need to log in before you can comment on or make changes to this bug.