Switch to async/await from Task.jsm/yield

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Async Tooling
P1
normal
RESOLVED FIXED
2 months ago
3 days ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance] [qf:p1])

Attachments

(8 attachments)

(Assignee)

Description

2 months ago
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.  :-)
Whiteboard: [photon] → [photon][qf:p1]
(Assignee)

Updated

a month ago
Depends on: 1242505, 1343158

Updated

a month ago
Whiteboard: [photon][qf:p1] → [qf:p1]

Updated

a month ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [qf:p1] → [photon-performance] [qf:p1]

Updated

a month ago
Flags: qe-verify? → qe-verify-

Updated

23 days ago
Iteration: 55.4 - May 1 → 55.5 - May 15
(Assignee)

Updated

17 days ago
See Also: → bug 1362882
(Assignee)

Comment 2

17 days ago
Created attachment 8865249 [details] [diff] [review]
pre-script hand-written cleanup patch
Attachment #8865249 - Flags: review?(dtownsend)
(Assignee)

Comment 3

17 days ago
Created attachment 8865250 [details] [diff] [review]
massive script-generated patch
Attachment #8865250 - Flags: review?(dtownsend)
(Assignee)

Comment 4

17 days ago
Created attachment 8865251 [details] [diff] [review]
smaller script-generated patch converting remaining tasks
Attachment #8865251 - Flags: review?(dtownsend)
(Assignee)

Comment 5

17 days ago
Created attachment 8865252 [details] [diff] [review]
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 :-).
Attachment #8865252 - Flags: review?(dtownsend)
(Assignee)

Comment 6

17 days ago
Created attachment 8865253 [details] [diff] [review]
Make the tests pass

Cleanup the script's output and fix tests.
Attachment #8865253 - Flags: review?(dtownsend)
(Assignee)

Comment 7

17 days ago
Created attachment 8865254 [details] [diff] [review]
Add an eslint rule
Attachment #8865254 - Flags: review?(dtownsend)
(Assignee)

Comment 8

17 days ago
Created attachment 8865258 [details]
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).
(Assignee)

Comment 9

17 days ago
Created attachment 8865259 [details]
bind.js xpcshell script

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.
Attachment #8865249 - Flags: review?(dtownsend) → review+
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?
Attachment #8865253 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 12

15 days ago
(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).
Attachment #8865254 - Flags: review?(dtownsend) → review+
Attachment #8865251 - Flags: review?(dtownsend) → review+
Attachment #8865250 - Flags: review?(dtownsend) → review+
Attachment #8865252 - Flags: review?(dtownsend) → review+
(Assignee)

Updated

14 days ago
Blocks: 1363771
(Assignee)

Updated

14 days ago
No longer blocks: 1348289

Comment 13

13 days ago
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.

Comment 14

13 days ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

12 days ago
Blocks: 1364645
Blocks: 1365005

Updated

3 days ago
Blocks: 1364677
You need to log in before you can comment on or make changes to this bug.