1350645 - [meta] Migrate away from soon to be removed SDK modules in DevTools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The add-ons team plans to remove large portions of SDK code after 57.  I'm assuming we'll keep using the SDK loader for the moment, but let's work out which SDK modules we need to drop.

Here's a usage count of SDK modules in /devtools:

   6 "sdk/clipboard"
   1 "sdk/console/plain-text"
   2 "sdk/content/mod"
  50 "sdk/core/heritage"
   1 "sdk/dom/events"
  77 "sdk/event/core"
  15 "sdk/event/target"
   2 "sdk/indexed-db"
   2 "sdk/io/buffer"
   3 "sdk/lang/functional"
   1 "sdk/loader/sandbox"
   1 "sdk/preferences/service"
   2 "sdk/stylesheet/style"
   3 "sdk/stylesheet/utils"
   2 "sdk/system/child_process/subprocess"
   1 "sdk/system/environment"
   2 "sdk/system/events"
   1 "sdk/system/runtime"
   1 "sdk/system/unload"
   2 "sdk/tabs"
   5 "sdk/tabs/utils"
   4 "sdk/timers"
   2 "sdk/url"
  15 "sdk/util/object"
   1 "sdk/util/uuid"
   1 "sdk/view/core"
   2 "sdk/window/helpers"
   8 "sdk/window/utils"

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:kmag, I've read in various places that the add-ons team intends to remove some of SDK code after 57, but I haven't heard a precise list.  Will all of the things above be removed, or only some?

Comment 2

[Kris Maglione [:kmag]]

See bug 1350646.

Of the above, the modules that I want to remove as soon as possible are:

 "sdk/content/mod"
 "sdk/dom/events"
 "sdk/loader/sandbox"
 "sdk/system/child_process/subprocess"
 "sdk/system/events"
 "sdk/system/unload"
 "sdk/tabs"
 "sdk/tabs/utils"
 "sdk/view/core"
 "sdk/window/helpers"

And the following sooner rather than later:

 "sdk/url"
 "sdk/util/object"
 "sdk/indexed-db"
 "sdk/preferences/service"
 "sdk/io/buffer"

The sdk/core/heritage stuff is not great for perf, so it would be great if you
could migrate to ES6 classes, but I'm not in a hurry to remove it.

Comment 3

[Patrick Brosset <:pbro>]

sdk/system/child_process/subprocess is used in WebIDE, which bug 1314811 aims to remove.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Kris Maglione [:kmag] from comment #2)

> The sdk/core/heritage stuff is not great for perf, so it would be great if
> you could migrate to ES6 classes, but I'm not in a hurry to remove it.

I would love to migrate to ES6 classes, however it won't be great for perf too, we need bug 1167472 fixed for that.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)

>   77 "sdk/event/core"
>   15 "sdk/event/target"

I'd like to keep those, maybe we could update them and move in devtools codebase; I found them better to use in some scenario than the `EventEmitter` we have in devtools – we could also think about refactoring `EventEmitter` too.

I could work on them since I was part of the original Add-on SDK team, if it's needed.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #4)
> (In reply to Kris Maglione [:kmag] from comment #2)
> > The sdk/core/heritage stuff is not great for perf, so it would be great if
> > you could migrate to ES6 classes, but I'm not in a hurry to remove it.
>
> I would love to migrate to ES6 classes, however it won't be great for perf
> too, we need bug 1167472 fixed for that.

I'd be willing to bet that whatever the perf cost, it's less than the cost of
core/heritage. As I understand it, classes aren't JITtable under certain
circumstances, mainly in the scope where they're declared, but not when
they're used.

If you're planning on making the switch in the future, anyway, it might be
worth migrating some classes now and testing the perf impact, and just waiting
to land if it causes trouble.

> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> >   77 "sdk/event/core"
> >   15 "sdk/event/target"
>
> I'd like to keep those, maybe we could update them and move in devtools
> codebase; I found them better to use in some scenario than the
> `EventEmitter` we have in devtools – we could also think about refactoring
> `EventEmitter` too.
>
> I could work on them since I was part of the original Add-on SDK team, if
> it's needed.

Well, I don't have any say over what you do in the devtools module, so you're
free to move those modules there if you want. I'd advise against it, though.
From the profiling I've done, the event modules add a lot of overhead in a lot
of places.

It may not be as much of an issue for the devtools uses as it is for some of
the extreme ways the SDK uses it, and it's gotten better after some recent
changes, but I still think you'd be better off with the simpler EventTarget
class used elsewhere, that doesn't rely so heavily on WeakMaps and the
`method` gunk used by the SDK.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> 
> >   77 "sdk/event/core"
> >   15 "sdk/event/target"
> 
> I'd like to keep those, maybe we could update them and move in devtools
> codebase; I found them better to use in some scenario than the
> `EventEmitter` we have in devtools – we could also think about refactoring
> `EventEmitter` too.

We do have bug 952653 to hopefully someday use only one event system in DevTools.  `event-emitter` seems more appealing to me these days, since it seems to have better perf than sdk/event/core, and it's also used in various Firefox modules as well.

Maybe we can explore using bug 952653 for the sdk/event portion here.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

In total I'm currently seeing 178 occurrences of requiring the sdk.

http://searchfox.org/mozilla-central/search?q=%5C(%5B%22%27%5Dsdk%2F&case=false&regexp=true&path=devtools%2F

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Down to 159 occurrences, progress but not quite moving fast enough.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Don't forget not to miss the modules with simplified mappings e.g.:

`require("timer")` can be used instead of `require("sdk/timers")`.

Here is the full list of simplified mappings from addon-sdk/source/mapping.json:

{
  "api-utils": "sdk/deprecated/api-utils",
  "base64": "sdk/base64",
  "content": "sdk/content/content",
  "deprecate": "sdk/util/deprecate",
  "event/core": "sdk/event/core",
  "events": "sdk/deprecated/events",
  "functional": "sdk/core/functional",
  "l10n/core": "sdk/l10n/json/core",
  "l10n/html": "sdk/l10n/html",
  "l10n/loader": "sdk/l10n/loader",
  "l10n/locale": "sdk/l10n/locale",
  "l10n/prefs": "sdk/l10n/prefs",
  "list": "sdk/util/list",
  "loader": "sdk/loader/loader",
  "namespace": "sdk/core/namespace",
  "preferences-service": "sdk/preferences/service",
  "promise": "sdk/core/promise",
  "system": "sdk/system",
  "system/events": "sdk/system/events-shimmed",
  "tabs/tab": "sdk/tabs/tab",
  "tabs/utils": "sdk/tabs/utils",
  "timer": "sdk/timers",
  "traits": "sdk/deprecated/traits",
  "unload": "sdk/system/unload",
  "window-utils": "sdk/deprecated/window-utils",
  "window/utils": "sdk/window/utils",
  "windows/dom": "sdk/windows/dom",
  "windows/loader": "sdk/windows/loader",
  "xul-app": "sdk/system/xul-app",
  "url": "sdk/url",
  "traceback": "sdk/console/traceback",
  "xhr": "sdk/net/xhr",
  "match-pattern": "sdk/util/match-pattern",
  "file": "sdk/io/file",
  "runtime": "sdk/system/runtime",
  "xpcom": "sdk/platform/xpcom",
  "querystring": "sdk/querystring",
  "text-streams": "sdk/io/text-streams",
  "app-strings": "sdk/deprecated/app-strings",
  "environment": "sdk/system/environment",
  "keyboard/utils": "sdk/keyboard/utils",
  "dom/events": "sdk/dom/events-shimmed",
  "utils/data": "sdk/io/data",
  "test/assert": "sdk/test/assert",
  "hidden-frame": "sdk/frame/hidden-frame",
  "collection": "sdk/util/collection",
  "array": "sdk/util/array",
  "clipboard": "sdk/clipboard",
  "context-menu": "sdk/context-menu",
  "hotkeys": "sdk/hotkeys",
  "indexed-db": "sdk/indexed-db",
  "l10n": "sdk/l10n",
  "notifications": "sdk/notifications",
  "page-mod": "sdk/page-mod",
  "page-worker": "sdk/page-worker",
  "panel": "sdk/panel",
  "passwords": "sdk/passwords",
  "private-browsing": "sdk/private-browsing",
  "request": "sdk/request",
  "selection": "sdk/selection",
  "self": "sdk/self",
  "simple-prefs": "sdk/simple-prefs",
  "simple-storage": "sdk/simple-storage",
  "tabs": "sdk/tabs",
  "timers": "sdk/timers",
  "windows": "sdk/windows",
  "harness": "sdk/test/harness",
  "run-tests": "sdk/test/runner",
  "test": "sdk/test"
}

Comment 10

[Alexandre Poirot [:ochameau]]

Quick update on current usage:

$ egrep -Eoh "\"sdk/.*\"" -r devtools/ | sort | uniq -c
      5 "sdk/clipboard"
     15 "sdk/core/heritage"
     61 "sdk/event/core"
      6 "sdk/event/target"
      2 "sdk/indexed-db"
      1 "sdk/lang/functional/concurrent.js"
      2 "sdk/system/child_process/subprocess" [should be removed soon in bug 1353656]
      1 "sdk/system/environment" [should be removed shortly in bug 1378833]
      1 "sdk/system/events" [only one test [1]]
      1 "sdk/system/runtime" [should be removed shortly in bug 1378835]
      4 "sdk/timers"
     10 "sdk/util/object"
      2 "sdk/util/uuid"

:jryans, is this a command similar to the one you used in comment 0?

From kris's comment 2, we still have some uses of the following:
 "sdk/system/child_process/subprocess" [should be removed soon in bug 1353656]
 "sdk/system/environment" [should be removed shortly in bug 1378833]
 "sdk/system/events" [only used in a test [1]]
 "sdk/system/runtime" [should be removed shortly in bug 1378835]
 "sdk/util/object" [Mostly used in performance panel and in protocol.js]
 "sdk/indexed-db" [I'm afraid this one would have to be copied]

[1]: http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/addon-source/browser_dbg_addon3/lib/main.js#2

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> :jryans, is this a command similar to the one you used in comment 0?

Yes, looks similar! :)

Comment 12

[Alexandre Poirot [:ochameau]]

Weekly update:
$ egrep -Eoh "\"sdk/.*\"" -r devtools/ | sort | uniq -c
      4 "sdk/clipboard"      (bug 1378820, landed but not on the git mirror yet?)
     12 "sdk/core/heritage"  (also blocked on bug 1381542)
     61 "sdk/event/core"     (bug 1381542)
      6 "sdk/event/target"   (bug 1381542)
      2 "sdk/indexed-db"     (bug 1361333, r+)
      1 "sdk/lang/functional/concurrent.js" [1]
      1 "sdk/system/events"  [2]
      4 "sdk/timers"         [3]
     10 "sdk/util/object"    (bug 1361332, in autoland)

So most of everything is soon to be blocked on bug 1381542,
I'm introducing some delay by suggesting waiting for tomorrow to land the event-emitter refactoring on FF57.
I also do an extensive review of the new API, which introduces some delay.

[1] This is a false report from a comment:
http://searchfox.org/mozilla-central/source/devtools/shared/debounce.js#13

[2] We may miss a bug for the last usage of sdk/system/events:
http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/addon-source/browser_dbg_addon3/lib/main.js#2

[3] I haven't found a bug to get rid of sdk/timers?

Comment 13

[Alexandre Poirot [:ochameau]]

Just opened bug 1386357 to track sdk/timers removal.

Comment 14

[Alexandre Poirot [:ochameau]]

$ egrep -Eoh "\"sdk/.*\"" -r devtools/ | sort | uniq -c
      2 "sdk/core/heritage"
One being in comment, here, that we can safely ignore:
http://searchfox.org/mozilla-central/source/devtools/shared/extend.js#11

But what about this one? Is that just a leftover of `Class` is still used somewhere?
http://searchfox.org/mozilla-central/source/devtools/server/performance/recorder.js#11-12

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> But what about this one? Is that just a leftover of `Class` is still used
> somewhere?
> http://searchfox.org/mozilla-central/source/devtools/server/performance/
> recorder.js#11-12
I couldn't find any usage of this import. Seems safe to just remove it.

Comment 16

[Soledad Penades [:sole] [:spenades]]

It is with great pleasure that I resolve this bug