1384527 - Stop using Promise.jsm from DevTools

Status: RESOLVED FIXED

(DevTools :: General, task, P2)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1384527 - Remove Promise.jsm usages from DevTools.

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

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1384527 - Stop importing event-emitter as a JSM.

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

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1384527 - remove Promise.jsm from Loader.jsm.

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

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

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")],

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8890314 [details]
Bug 1384527 - Stop importing event-emitter as a JSM.

Moved to bug 1386299

Comment 7

[Alexandre Poirot [:ochameau]]

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?!

Comment 8

[Alexandre Poirot [:ochameau]]

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?

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 10

[Alexandre Poirot [:ochameau]]

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

Comment 11

[Soledad Penades [:sole] [: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" :-)

Comment 12

[Mark Banner (:standard8)]

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.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

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?

Comment 14

[Mark Banner (: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 :)

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1384527 - [devtools] Stop using Promise.jsm in DevTools. r=ochameau.

Depends on D127929

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Stealing the bug from Alex :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd5088801f6d
[devtools] Stop using Promise.jsm in DevTools. r=ochameau.

Comment 18

[Cristian Tuns]

Backed out for causing mochitest failures.

• Backout link
• Push with failures
• Failure Log

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32a3cf57dd43
[devtools] Stop using Promise.jsm in DevTools. r=ochameau.

Comment 20

[Marian-Vasile Laza]

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

Comment 21

[Mark Banner (:standard8)]

Thank you for finishing this Nicolas.

Comment 22

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== 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

Comment 23

[Julian Descottes [:jdescottes]]

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

Comment 24

[Mark Banner (:standard8)]

(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

Comment 25

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Thanks!