Closed Bug 1384527 Opened 7 years ago Closed 3 years ago

Stop using Promise.jsm from DevTools

Categories

(DevTools :: General, task, P2)

Product:
DevTools
Component:
General
Type:
task

Tracking

(firefox57 wontfix, firefox95 fixed)

Status:
RESOLVED FIXED
Milestone:
95 Branch
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox95 --- fixed

People

(Reporter: ochameau, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1384527 - remove Promise.jsm from Loader.jsm.
(deleted), text/x-review-board-request
Details
Bug 1384527 - [devtools] Stop using Promise.jsm in DevTools. r=ochameau.
(deleted), text/x-phabricator-request
Details
We still import Promise.jsm from DevTools: http://searchfox.org/mozilla-central/search?q=Promise.jsm%22&case=false&regexp=false&path=devtools%2F Bug 1378173 aims at removing it from tree. We already have bug 1233890 and bug 1233891, which are about stop using require("promise") and use Promise global instead. This involves more refactoring than just mapping require("promise") to DOM Promise. It looks like we could just do that first: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c132af40db995df0f3533baa21e984e68ff7365d&selectedJob=117999510
Oh no, I've been completely fooled as I missed actually replacing require(promise) with DOM Promise. So we do have to manually remove all the calls to require(promise). My two first patches are still actionnable, it removes all direct usages of Promise.jsm. The last one will only be landable once we removed all the require(promise).
Depends on: 1233890, 1233891
Comment on attachment 8890314 [details] Bug 1384527 - Stop importing event-emitter as a JSM. This change should also remove: devtools/client/inspector/webpack/rewrite-event-emitter.js and its reference from: devtools/client/inspector/webpack.config.js loaders: [path.join(__dirname, "./webpack/rewrite-event-emitter")],
Depends on: 1386299
Comment on attachment 8890314 [details] Bug 1384527 - Stop importing event-emitter as a JSM. Moved to bug 1386299
Depends on: 1387122
Depends on: 1387123
Depends on: 1387128
Here is my plan: 0/ bug 1387122: Remove all already useless import of promise with capital P: Promise = require("promise") 1/ bug 1387123: Replace all usage of require("promise").defer by require("devtools/shared/defer").defer Why? It can be automated! Ideally we would look into each usage and try to see if we can replace each with new Promise(), that's still something we could look into bug 1233890 and bug 1233891. Some usage of defer are going to be hard to refactor to pure Promise, so I imagine we will end up with a few usages of devtools/shared/defer anyway. 2/ bug 1387128: Drop all promise = require("promise") and replace promise by Promise Thanks to step 1, we should no longer use promise.defer. All the other usages of Promise.jsm should be supported by DOM Promise: new constructor, resolve, reject, race, all. i.e. if I'm not mistaken, defer was the only things Promise.jsm implements that DOM Promise doesn't. 3/ Go manually fix the couple of custom promise import like: const { resolve } = require("promise") const {defer} = require("promise") I imagine we can do this last step in this bug. Hopefully all that can be mostly automated and I haven't missed a critical usage preventing this plan from happening?!
Attachment #8890313 - Attachment is obsolete: true
Attachment #8890314 - Attachment is obsolete: true
Gabriel, It is unclear to me what is unclear to you? The main motivation here, to finally get rid of Promise.jsm, is bug 1378173. Promise.jsm is planed to be removed from mozilla-central, so we should stop using it on devtools side. Why is it being removed? Because of DOM Promises which replaced this. And I think it introduces extra slowness related to cross compartment wrappers. Regarding bug 1386299 and the JSM boilerplate removal, getting rid of JSM for *our* modules is another story. This isn't a goal here. It just happened that I could drop Promise.jsm usage in the boilerplace by converting event-emitter.js to a a commonjs-module only. bug 1182194 is focusing on the JSM removal. I did removed all the JSM from /devtools/server a long time ago, we should still aim to convert JSM to regular modules also in client and shared. Why? * it is much easier to support unload/reload if you get rid of JSM You can see that when using the add-on workflow or if you have to ship devtools as an add-on. * it would better work in launchpad, with tools loaded in a tab. * as for Promise/Promise.jsm, it simplifies our codebase by using only one module system. Is everything crystal clear with this additonal info?
Flags: needinfo?(gl)
Depends on: 1388054
(In reply to Alexandre Poirot [:ochameau] from comment #8) > Gabriel, It is unclear to me what is unclear to you? > > The main motivation here, to finally get rid of Promise.jsm, is bug 1378173. > Promise.jsm is planed to be removed from mozilla-central, so we should stop > using it on devtools side. > > Why is it being removed? Because of DOM Promises which replaced this. > And I think it introduces extra slowness related to cross compartment > wrappers. > > Regarding bug 1386299 and the JSM boilerplate removal, > getting rid of JSM for *our* modules is another story. > This isn't a goal here. > It just happened that I could drop Promise.jsm usage in the boilerplace > by converting event-emitter.js to a a commonjs-module only. > > bug 1182194 is focusing on the JSM removal. > I did removed all the JSM from /devtools/server a long time ago, > we should still aim to convert JSM to regular modules also in client and > shared. > Why? > * it is much easier to support unload/reload if you get rid of JSM > You can see that when using the add-on workflow or if you have to ship > devtools as an add-on. > * it would better work in launchpad, with tools loaded in a tab. > * as for Promise/Promise.jsm, it simplifies our codebase by using only one > module system. > > Is everything crystal clear with this additonal info? I think there might be some misunderstanding. I am clear of what we are trying to do in this particular bug, but I am looking to see if there is a list of what we are trying to deprecate/not-to-use-anymore so that we can keep these things in mind when it comes to reviewing new patches while this work is happening.
Flags: needinfo?(gl)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #9) > I think there might be some misunderstanding. I am clear of what we are > trying to do in this particular bug, but I am looking to see if there is a > list of what we are trying to deprecate/not-to-use-anymore so that we can > keep these things in mind when it comes to reviewing new patches while this > work is happening. Ah ok. Not that I am aware of. Sole, do we have any page with a list of deprecate/not-to-use-anymore? Regarding promises, once the removal is going to be complete, the module will disappear so you won't be able to use it anymore. And JSM, I would be really surprised if someone in the team creates a new one ;)
Flags: needinfo?(spenades)
Bonjour, mes amis! On the NoSDK side, we have a list of replacements and advice of how to replace: https://github.com/devtools-html/snippets-for-removing-the-sdk but that's about it. That doesn't stop us from creating a shared list (perhaps a spreadsheet?) where we list what we're up to very succinctly. Then it's a matter of doing an in-page search before you go and change something. Maybe. Another option would be to have a thread in our email list letting people know what you intend to remove/replace and gathering input. I'm happy with any, the last one might have more noise but inform more than having just a spreadsheet that doesn't "emit pings" :-)
Flags: needinfo?(spenades)
Depends on: 1388364
Priority: -- → P2
Depends on: 1454373
Product: Firefox → DevTools
No longer blocks: dt-fission
Blocks: 1180427
Type: enhancement → task

Devtools appears to be the last code consumer of Promise.jsm for Firefox code (there's other users for tests, but we can move it to test-only once devtools uses are removed).

Is there a way we could move this forward? I think I looked at it a little while ago and came to the conclusion it'd need someone a bit more familiar with devtools code, so I'm not sure it would be suitable as mentored bugs, unless they were well described.

I don't think there's a lot consumers left (https://searchfox.org/mozilla-central/search?q=require%28%22promise%22%29&path=&case=false&regexp=false), but last time I tried to migrate all those at once TRY wasn't happy with it (I guess the timing is slightly different with regular DOM Promises).
I'll try to file independent bugs for each of those files so we can at least handle trivial ones.

Are we blocking any work here Mark?

Flags: needinfo?(standard8)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)

I don't think there's a lot consumers left (https://searchfox.org/mozilla-central/search?q=require%28%22promise%22%29&path=&case=false&regexp=false), but last time I tried to migrate all those at once TRY wasn't happy with it (I guess the timing is slightly different with regular DOM Promises).
I'll try to file independent bugs for each of those files so we can at least handle trivial ones.

Are we blocking any work here Mark?

I think the only work would be the removal of Promise.jsm/Promise-backend.jsm (I thought there was some async shutdown handling as well, but looks like I was wrong). Given there's so few cases left, it would be nice to see if we can drive it through and complete the clean-up rather than have it just sit there for ages. Quite happy for that to be scheduled in next year or something but would be nice to have a plan :)

Flags: needinfo?(standard8)

Stealing the bug from Alex :)

Assignee: poirot.alex → nchevobbe
Status: NEW → ASSIGNED
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fd5088801f6d [devtools] Stop using Promise.jsm in DevTools. r=ochameau.

Backed out for causing mochitest failures.

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/32a3cf57dd43 [devtools] Stop using Promise.jsm in DevTools. r=ochameau.

https://hg.mozilla.org/mozilla-central/rev/32a3cf57dd43

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox95: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
status-firefox57: fix-optional → wontfix
Flags: needinfo?(nchevobbe)

Thank you for finishing this Nicolas.

== Change summary for alert #31960 (as of Tue, 19 Oct 2021 09:18:25 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
6% fandom dcf linux1804-64-shippable-qr fission warm webrender 395.62 -> 370.21
3% fandom SpeedIndex linux1804-64-shippable-qr fission warm webrender 376.85 -> 366.25

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31960

(In reply to Alexandru Ionescu (needinfo me) [:alexandrui] from comment #22)

== Change summary for alert #31960 (as of Tue, 19 Oct 2021 09:18:25 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
6% fandom dcf linux1804-64-shippable-qr fission warm webrender 395.62 -> 370.21
3% fandom SpeedIndex linux1804-64-shippable-qr fission warm webrender 376.85 -> 366.25

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31960

The modified files from this patch should not be loaded in this test, so it's unlikely that this Bug triggered this alert. Looking at the retriggers, the push for this Bug shows no improvement compared to the first changeset from the retriggers (it's even slightly slower)

Alexandru: Can you check if this is really the right bug for this alert? Given it's an improvement I don't know if we want to spent too much time finding the actual Bug responsible for this, but the current association might be wrong.

Flags: needinfo?(aionescu)

(In reply to Julian Descottes [:jdescottes] from comment #23)

Alexandru: Can you check if this is really the right bug for this alert? Given it's an improvement I don't know if we want to spent too much time finding the actual Bug responsible for this, but the current association might be wrong.

Looks like bug 1733958

Thanks!

Flags: needinfo?(aionescu)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: