It should be possible to do this change on our whole tree using a script to rewrite most of the current uses of Task.jsm. See the thread at https://groups.google.com/forum/#!topic/mozilla.dev.platform/-8cCov0yuIo for some discussion of this change. I'm making this block photon-performance because I recently saw a profile where Task.jsm contributed about 700ms of CPU use in the first few seconds after startup: https://perf-html.io/public/ebdfb772af3c32bd7fb592ce8d0020a6b90de887/calltree/?invertCallstack&jsOnly&thread=0

The impact on parent process responsiveness makes it easy to justify this as a [qf:p1] also. :-)

In bug 1355056 my script couldn't remove .bind(this) calls used on generators, because they don't support the arrow syntax... but async functions do :-).

Cleanup the script's output and fix tests.

This is the script I used to generate attachment 8865250 [details] [diff] [review] and attachment 8865251 [details] [diff] [review]. It converts Task.async and Task.spawn calls, and generator functions used for tasks to the new async function and await syntax. Here is how I use this script: - For a first pass, the script converts only generators where it is fully confident that it's a task. Something that is used with Task.async, Task.spawn, add_task, or ContentTask.spawn is definitely a task. Then, something that yields on the result of a BrowserTestUtils, ContentTask, new Promise, Task, PlacesUtils, PlacesTestUtils, OS.File method is almost definitely a task. Same for something yielding on a variable or method with a name starting with 'promise' or 'waitFor'. At this point, we get attachment 8865250 [details] [diff] [review], which converts about 90% of the generators. - The remaining generators can't be automatically classified as actual generators or tasks, so they needed human review. The --show-generators option of the script will output for each generator that hasn't been converted yet: - the location of the generator (path, filename and line number). - the list of the yield statements the generator contains. Looking at the yield statements usually makes it obvious for the human eye if the generator is a task or a real generator. Sometimes I had to look a bit at the code to decide. This is the time consuming part of the process, and I've only completed it for the toolkit/ and browser/ folders. Using the information gathered at this step, I included in the script a whitelist of actual generators. Then, running the script with the --replace-generators option will convert all generators that aren't whitelisted. The script will output errors if there are unused whitelist entries; this is to make it obvious if the script has bitrotted already. This gives attachment 8865251 [details] [diff] [review]. Once tasks are converted from generators to async functions, the arrow function syntax becomes usable on them, so we can remove lots of .bind(this) calls I had to ignore in bug 1355056. I used almost the same script as in that bug (I'll attach the new version here). This gives attachment 8865252 [details] [diff] [review]. After all of this, I had a surprisingly usable browser, but tests weren't green, so I had to cleanup and debug. This is attachment 8865253 [details] [diff] [review]. In some cases it was the script output that was poor. Eg. 'Task.spawn(task)' gets converted to '(task)()'. This would have been fixable, but doesn't happen often enough to be worth it, so I just cleaned it up by hand. More often, it's uncovering actual bugs in our tests or code. And sometimes I just gave up and reverted some of the changes because I decided it wasn't worth the time it would take to fully debug the test (eg. test_DirectoryLinksProvider.js is really painful to work with...). Finally, I don't want more Task.jsm usages to be introduced after this removal. I can't expect to remove Task.jsm from the tree soon for add-on compat reason, so for now the best I can do is an eslint rule. This is attachment 8865254 [details] [diff] [review], enabled only for the browser/ and toolkit/ folders for now. Here is a greenish try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=817653bc8d8044733114458dcd746bd876f6561e (the eslint error has been fixed before attaching the patches here) After this lands, I expect to do the same thing in follow-ups for at least the services/ and addon-sdk/ folders, as they both contribute Task.jsm related stacks to my profiles. I'll likely also handle a few other folders of the tree. I don't expect to have time to work on devtools/ or mobile/, but I'm happy to give a hand if someone from these teams needs help with my script (also applies to Thunderbird... where I expect all the gloda related pseudo-generators to be a pain).

This is the version of my bind removal script that was used to produce attachment 8865252 [details] [diff] [review].

Comment on attachment 8865249 [details] [diff] [review] pre-script hand-written cleanup patch Review of attachment 8865249 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesTransactions.jsm @@ +450,4 @@ > * and all promises passed to alsoWaitFor are no longer pending. > * > * @param aFunc > * @see Task.spawn. Should probably update this comment to talk about this being a function that returns a promise rather than referencing Task.jsm.

Comment on attachment 8865250 [details] [diff] [review] massive script-generated patch Review of attachment 8865250 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/translation/TranslationDocument.jsm @@ +207,5 @@ > * @param target A string that is either "translation" > * or "original". > */ > _swapDocumentContent(target) { > + (async function() { So the ideal here would be to make the outer function async when all it is doing is calling Task.spawn. How difficult would that be?

(In reply to Dave Townsend [:mossop] from comment #11) > ::: browser/components/translation/TranslationDocument.jsm > @@ +207,5 @@ > > * @param target A string that is either "translation" > > * or "original". > > */ > > _swapDocumentContent(target) { > > + (async function() { > > So the ideal here would be to make the outer function async when all it is > doing is calling Task.spawn. How difficult would that be? It's probably possible, but I don't think it's worth the effort. If you care strongly, I can try to identify these cases with a script, and if there are very few we can clean them up by hand. I would also be a bit concerned that it would introduce behavior changes: - calling an async function returns a promise, whereas the original code doesn't return anything. - async functions can't be transferred with structured clones (this is the reason for the pwmgr_common.js change in attachment 8865253 [details] [diff] [review], where I ended up needing to do the exact opposite transformation).

Pushed by florian@queze.net: https://hg.mozilla.org/mozilla-central/rev/eac6de13a9e7 pre-script hand-written cleanup patch, r=Mossop. https://hg.mozilla.org/mozilla-central/rev/7970ea085861 massive script-generated patch converting Task.async and Task.spawn calls, and generators clearly identifiable as tasks, rs=Mossop. https://hg.mozilla.org/mozilla-central/rev/b31650bb06c1 smaller script-generated patch converting remaining generators that are likely tasks (actual generators were identified by hand and whitelisted), r=Mossop. https://hg.mozilla.org/mozilla-central/rev/586c752c204a script-generated patch to remove .bind(this) calls we no longer need now that generator functions have been replaced with async functions, r=Mossop. https://hg.mozilla.org/mozilla-central/rev/0929827f535f Cleanup the script output and fix tests, r=Mossop. https://hg.mozilla.org/mozilla-central/rev/b2d4e9f99355 Add an eslint rule deprecating usage of Task.jsm in browser/ and toolkit/, r=Mossop.

https://hg.mozilla.org/mozilla-central/rev/eac6de13a9e7 https://hg.mozilla.org/mozilla-central/rev/7970ea085861 https://hg.mozilla.org/mozilla-central/rev/b31650bb06c1 https://hg.mozilla.org/mozilla-central/rev/586c752c204a https://hg.mozilla.org/mozilla-central/rev/0929827f535f https://hg.mozilla.org/mozilla-central/rev/b2d4e9f99355

(In reply to Pulsebot from bug 1353542 comment #13) > https://hg.mozilla.org/mozilla-central/rev/0929827f535f > Cleanup the script output and fix tests, r=Mossop. florian, you said in bug 1343150 comment 6 that this particular commit shouldn't have much impact on triggering the significantly increased timeouts of the thumbnail test, but there was a change there that doesn't seem particularly related to Tasks: https://hg.mozilla.org/mozilla-central/rev/0929827f535f#l30.11 - Services.obs.addObserver(observe, "page-thumbnail:create"); - Services.obs.addObserver(observe, "page-thumbnail:error"); + // Use weak references to avoid leaking in tests where the promise we + // return is GC'ed before it has resolved. + Services.obs.addObserver(observer, "page-thumbnail:create", true); + Services.obs.addObserver(observer, "page-thumbnail:error", true); Was this change accidental or part of the "and fix tests?" But even then, making the promise never resolve (-> test timeout) when the observer gets GCed seems like a pretty significant change.

(In reply to Ed Lee :Mardak from comment #15) > Was this change accidental or part of the "and fix tests?" But even then, > making the promise never resolve (-> test timeout) when the observer gets > GCed seems like a pretty significant change. I think it fixed some test failures, but it's long enough ago that I can't remember confidently.

Comment on attachment 8953158 [details] Bug XXX - xpcshell script used to convert files, coming from bug 1353542 Sorry, I misused mozreview push... and attached patches to the wrong bug!