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

RESOLVED FIXED

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: jryans, Unassigned)

Tracking

({meta})

Trunk
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [qf:meta])

(Reporter)

Description

2 years ago
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"
(Reporter)

Comment 1

2 years ago
: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?
Flags: needinfo?(kmaglione+bmo)
Blocks: 1350646
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.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1353005
Depends on: 1353006
Depends on: 1353010
sdk/system/child_process/subprocess is used in WebIDE, which bug 1314811 aims to remove.
Depends on: rm-webide
Depends on: 1353012
Depends on: 1353015
Depends on: 1353042
Depends on: 1353043
Depends on: 1353045
Depends on: 1353656
No longer depends on: rm-webide
(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.
Whiteboard: [qf]
(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.
(Reporter)

Updated

2 years ago
Depends on: 952653
(Reporter)

Comment 6

2 years ago
> (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.
Flags: needinfo?(ajones)
Whiteboard: [qf] → [qf:meta]
Down to 159 occurrences, progress but not quite moving fast enough.
Depends on: 1361331
Depends on: 1361332
Depends on: 1361333
Depends on: 1361335
Depends on: 1361340
Depends on: 1361311
Depends on: 1368939
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"
}
Flags: needinfo?(ajones)
No longer depends on: 1353656
No longer depends on: 1378900
No longer depends on: 1378898
No longer depends on: 1378897
No longer depends on: 1378894
No longer depends on: 1378893
No longer depends on: 952653
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
(Reporter)

Comment 11

2 years ago
(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! :)
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?
Depends on: 1386357
Just opened bug 1386357 to track sdk/timers removal.
Depends on: 1388954
Depends on: 1392602
Depends on: 1395741
$ 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
(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.
No longer depends on: 1392635
No longer depends on: 1395903
It is with great pleasure that I resolve this bug 
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.