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

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years 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 years 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 2

2 years ago
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 years ago
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 7

2 years ago
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 years ago
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)
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [photon-performance]
(Assignee)

Comment 9

2 years 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.
Attachment #8879276 - Flags: review?(dtownsend) → review+
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?
Attachment #8879280 - Flags: review?(dtownsend) → review+
Attachment #8879281 - Flags: review?(dtownsend) → review+
Attachment #8879284 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 11

2 years ago
(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 :-).
Attachment #8879279 - Flags: review?(dtownsend) → review+

Comment 12

2 years 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
You need to log in before you can comment on or make changes to this bug.