Switch to async/await from Task.jsm/yield - part 2

RESOLVED FIXED in Firefox 56

Status

()

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

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
mozilla56
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(8 attachments)

(Assignee)

Description

2 months ago
Bug 1353542 converted browser/ and toolkit/, but there are plenty of uses of Task.jsm in the rest of the tree.

This bug will covert the rest of the tree except mobile/ and devtools/.
(Assignee)

Comment 1

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2faf6953a8733c15e755b0e86dc5a21a96eabf19
(Assignee)

Comment 2

2 months ago
Created attachment 8879275 [details]
task.jsm.js xpcshell script

This is the script I used to generate the main patch I'll attach here.
I changed the heuristic a bit to make better automated decisions about whether a generator is a task or not. It now takes into account the presence of ".next(" and "Task." in the file to help decide, so I can no longer produce separate patches for the replacements where the script is 100% confident and for where it's a bit speculative. However, given that there are few non-test files involved here, I don't think it'll be a problem for reviews.
(Assignee)

Comment 3

2 months ago
Created attachment 8879276 [details] [diff] [review]
script generated patch to remove Task.jsm calls

Here are the non-test files affected by this patch:
 addon-sdk/source/lib/sdk/system/child_process/subprocess.js                                            |   15 +-
 dom/browser-element/BrowserElementChildPreload.js                                                      |    7 +-
 dom/ipc/manifestMessages.js                                                                            |   13 +-
 dom/manifest/ManifestFinder.jsm                                                                        |    7 +-
 dom/manifest/ManifestObtainer.jsm                                                                      |   28 +-
 dom/push/PushRecord.jsm                                                                                |   12 +-
 uriloader/exthandler/nsHandlerService-json.js                                                          |    7 +-
Attachment #8879276 - Flags: review?(dtownsend)
(Assignee)

Comment 4

2 months ago
Created attachment 8879278 [details] [diff] [review]
hand cleanup for the script generated patch to remove Task.jsm calls
Attachment #8879278 - Flags: review?(dtownsend)
(Assignee)

Comment 5

2 months ago
Created attachment 8879279 [details] [diff] [review]
script generated patch to remove Promise.defer calls
Attachment #8879279 - Flags: review?(dtownsend)
(Assignee)

Comment 6

2 months ago
Created attachment 8879280 [details] [diff] [review]
script generated patch to remove useless bind calls
Attachment #8879280 - Flags: review?(dtownsend)
(Assignee)

Comment 7

2 months ago
Created attachment 8879281 [details] [diff] [review]
script generated patch to remove generators from add_task in browser/ and toolkit/

The script in bug 1353542 missed the generators in add_task that didn't contain any yield. Let's clean this up!
Attachment #8879281 - Flags: review?(dtownsend)
(Assignee)

Comment 8

2 months ago
Created attachment 8879284 [details] [diff] [review]
ban Task.jsm during startup

The few pieces of toolkit code that still need to support Task.jsm can be adapted to load it lazily only when really needed, and then we can ban the Task.jsm module during startup using the browser_startup.js test.
Attachment #8879284 - Flags: review?(dtownsend)

Updated

2 months ago
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [photon-performance]
(Assignee)

Comment 9

2 months ago
Comment on attachment 8879284 [details] [diff] [review]
ban Task.jsm during startup

Review of attachment 8879284 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +592,5 @@
>              // Keep Task.spawn here to preserve API compat; unfortunately
>              // func was a generator rather than a task here.
> +            let isGenerator = func =>
> +              Object.prototype.toString.call(func) == "[object Generator]";
> +            if (isGenerator(func)) {

paolo pointed out that I don't need this first test that should always be false. Next time I regenerate the patch I'll simplify this block, and I'll definitely do a full try run again before landing.

Updated

2 months ago
Attachment #8879276 - Flags: review?(dtownsend) → review+

Updated

2 months ago
Attachment #8879278 - Flags: review?(dtownsend) → review+
Comment on attachment 8879279 [details] [diff] [review]
script generated patch to remove Promise.defer calls

Review of attachment 8879279 [details] [diff] [review]:
-----------------------------------------------------------------

Can I see a diff -w of this?

Updated

2 months ago
Attachment #8879280 - Flags: review?(dtownsend) → review+

Updated

2 months ago
Attachment #8879281 - Flags: review?(dtownsend) → review+

Updated

2 months ago
Attachment #8879284 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 11

2 months ago
Created attachment 8880194 [details] [diff] [review]
Diff -w of attachment 8879279 [details] [diff] [review]

(In reply to Dave Townsend [:mossop] from comment #10)
> Comment on attachment 8879279 [details] [diff] [review]
> script generated patch to remove Promise.defer calls
> 
> Review of attachment 8879279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can I see a diff -w of this?

There you go! Thanks for the other reviews :-).

Updated

2 months ago
Attachment #8879279 - Flags: review?(dtownsend) → review+

Comment 12

2 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/mozilla-central/rev/b0dd91a6a1fd
script generated patch to remove Task.jsm calls, r=Mossop.
https://hg.mozilla.org/mozilla-central/rev/9c7644b6de3c
hand cleanup for the script generated patch to remove Task.jsm calls, r=Mossop.
https://hg.mozilla.org/mozilla-central/rev/52e7befb507a
script generated patch to remove Promise.defer calls, r=Mossop.
https://hg.mozilla.org/mozilla-central/rev/a65201e69ebf
script generated patch to remove useless bind calls, r=Mossop.
https://hg.mozilla.org/mozilla-central/rev/05d686d3c53d
script generated patch to remove generators from add_task in browser/ and toolkit/, r=Mossop.
https://hg.mozilla.org/mozilla-central/rev/7a9536f89bc7
ban Task.jsm during startup, r=Mossop, a=sheriffduty

Comment 13

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0dd91a6a1fd
https://hg.mozilla.org/mozilla-central/rev/9c7644b6de3c
https://hg.mozilla.org/mozilla-central/rev/52e7befb507a
https://hg.mozilla.org/mozilla-central/rev/a65201e69ebf
https://hg.mozilla.org/mozilla-central/rev/05d686d3c53d
https://hg.mozilla.org/mozilla-central/rev/7a9536f89bc7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.