1353542 - Switch to async/await from Task.jsm/yield

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, enhancement, P1)

Description

[Florian Quèze [:florian]]

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

Comment 1

[(no longer active)]

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

Comment 2

[Florian Quèze [:florian]]

Attachment: pre-script hand-written cleanup patch

Comment 3

[Florian Quèze [:florian]]

Attachment: massive script-generated patch

Comment 4

[Florian Quèze [:florian]]

Attachment: smaller script-generated patch converting remaining tasks

Comment 5

[Florian Quèze [:florian]]

Attachment: script-generated patch to remove .bind(this) calls

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

Comment 6

[Florian Quèze [:florian]]

Attachment: Make the tests pass

Cleanup the script's output and fix tests.

Comment 7

[Florian Quèze [:florian]]

Attachment: Add an eslint rule

Comment 8

[Florian Quèze [:florian]]

Attachment: task.jsm xpcshell script

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

Comment 9

[Florian Quèze [:florian]]

Attachment: bind.js xpcshell script

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

Comment 10

[Dave Townsend [:mossop]]

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 11

[Dave Townsend [:mossop]]

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?

Comment 12

[Florian Quèze [:florian]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Ed Lee :Mardak]

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

Comment 16

[Florian Quèze [:florian]]

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

[Alexandre Poirot [:ochameau]]

Attachment: Bug XXX - xpcshell script used to convert files, coming from bug 1353542

Review commit: https://reviewboard.mozilla.org/r/222424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222424/

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Bug XXX - Removed unused methods from async-utils module.

Review commit: https://reviewboard.mozilla.org/r/222426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222426/

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug xxx - Convert Task.jsm to async/await in devtools/shared.

$ ./obj-firefox-artifact/dist/bin/run-mozilla.sh ./obj-firefox-artifact/dist/bin/xpcshell xpc devtools/shared
(Manually edit devtools/shared/tests/mochitest/test_eventemitter_basic.html)

Review commit: https://reviewboard.mozilla.org/r/222428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222428/

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Bug xxx - Put spaces before parentheses in "async function()" pattern.

$ find devtools/shared/ -type f -exec sed -i 's/async function(/async function (/g' {} \;

Review commit: https://reviewboard.mozilla.org/r/222430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222430/

Comment 21

[Alexandre Poirot [:ochameau]]

Attachment: Bug xxx - Get rid of the now useless require("devtools/shared/task").

$ find devtools/shared/ -type f -exec sed -i '/devtools\/shared\/task/d' {} \;
$ git checkout devtools/shared/gcli
$ git checkout devtools/shared/fronts/css-properties.js

Review commit: https://reviewboard.mozilla.org/r/222432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222432/

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Bug xxx - manual (eslint) fixes

Review commit: https://reviewboard.mozilla.org/r/222434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222434/

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Bug xxx - Convert generators to async functions in devtools/shared

$ ./obj-firefox-artifact/dist/bin/run-mozilla.sh ./obj-firefox-artifact/dist/bin/xpcshell xpc devtools/shared --replace-generators
(xpc script has a whitelist of files that uses generator outside of Task.jsm scope)

Review commit: https://reviewboard.mozilla.org/r/222436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222436/

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: Bug xxx - manual fixes.

Review commit: https://reviewboard.mozilla.org/r/222438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222438/

Comment 25

[Alexandre Poirot [:ochameau]]

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!