+++ 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 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.

(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).

(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.

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

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

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/.

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

Thanks, Florian, I'm removing those here.

Comment on attachment 9026186 [details] [diff] [review] 1364645-remove-task-jsm-import.patch Geoff, something to land in six hours.

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 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

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

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 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.

(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.

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

This works now.

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.

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

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

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.

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

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/5eb606076270 Follow-up: Remove left-over reference to Task.jsm. r=florian