1378856 - Stop using sdk/core/heritage in DevTools performance server

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

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

Used in:

devtools/server/performance/framerate.js
devtools/server/performance/memory.js
devtools/server/performance/profiler.js
devtools/server/performance/timeline.js

More details to follow as we triage. Perhaps we need to split into four more bugs.

Comment 1

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

Used (lazy) also in:

devtools/server/performance/recorder.js

Comment 2

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

Some of the files also use `extends: EventTarget`, so this bug should wait until the EventEmitter situation has been sorted out in bug 952653 or bug Bug 1381542 ... possibly the second one.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

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

Comment 4

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

Comment on attachment 8896977 [details]
Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

https://reviewboard.mozilla.org/r/168288/#review173528

thanks for the patch! I think it needs a bit more work. Once the sdk is gone this code will not work anymore as you're still including `sdk/*` stuff. Also you'll need to use an instance of EventEmitter-I left a comment with a link, you'll see.

::: devtools/server/performance/memory.js:12
(Diff revision 1)
>  const { reportException } = require("devtools/shared/DevToolsUtils");
> -const { Class } = require("sdk/core/heritage");
>  const { expectState } = require("devtools/server/actors/common");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");

Would you not need to stop including this one?

::: devtools/server/performance/memory.js:13
(Diff revision 1)
> -const { Class } = require("sdk/core/heritage");
>  const { expectState } = require("devtools/server/actors/common");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");
>  loader.lazyRequireGetter(this, "EventTarget", "sdk/event/target", true);

And not include this one either...

::: devtools/server/performance/memory.js:50
(Diff revision 1)
>  
> -    this._onGarbageCollection = this._onGarbageCollection.bind(this);
> +  this._onGarbageCollection = this._onGarbageCollection.bind(this);
> -    this._emitAllocations = this._emitAllocations.bind(this);
> +  this._emitAllocations = this._emitAllocations.bind(this);
> -    this._onWindowReady = this._onWindowReady.bind(this);
> +  this._onWindowReady = this._onWindowReady.bind(this);
>  
> -    events.on(this.parent, "window-ready", this._onWindowReady);
> +  events.on(this.parent, "window-ready", this._onWindowReady);

I think you might need to create an instance of EventEmitter and use that instance to send/get events instead of the events object, as described here: https://github.com/devtools-html/snippets-for-removing-the-sdk#differences-with-the-old-eventemitter

::: devtools/server/performance/memory.js:57
(Diff revision 1)
> +
> +Memory.prototype = {
> +  /**
> +   * Requires a root actor and a StackFrameCache.
> +   */
> +  initialize: function (parent, frameCache = new StackFrameCache()) {

do we still need this method? if you're doing the things in the new constructor, and this function is empty, perhaps it needs to be removed?

::: devtools/server/performance/profiler.js:10
(Diff revision 1)
>  
>  const { Cc, Ci, Cu } = require("chrome");
>  const Services = require("Services");
> -const { Class } = require("sdk/core/heritage");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");

remove this?

::: devtools/server/performance/profiler.js:11
(Diff revision 1)
>  const { Cc, Ci, Cu } = require("chrome");
>  const Services = require("Services");
> -const { Class } = require("sdk/core/heritage");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");
>  loader.lazyRequireGetter(this, "EventTarget", "sdk/event/target", true);

and remove this?

::: devtools/server/performance/recorder.js:12
(Diff revision 1)
>  loader.lazyRequireGetter(this, "promise");
> -loader.lazyRequireGetter(this, "Class",
> -  "sdk/core/heritage", true);
> -loader.lazyRequireGetter(this, "EventTarget",
> -  "sdk/event/target", true);
>  loader.lazyRequireGetter(this, "events",

Remove this?

::: devtools/server/performance/timeline.js:28
(Diff revision 1)
>  const { Ci, Cu } = require("chrome");
> -const { Class } = require("sdk/core/heritage");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  // Be aggressive about lazy loading, as this will run on every
>  // toolbox startup
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");

Remove?

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8896977 [details]
Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168288/diff/1-2/

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the review Sole!

I am attaching new version, all review comments resolved.

My question is whether using static on, off, emit methods is ok.
It looks like it does what we want, but I am not sure.

Like as follows:

const EventEmitter = require("devtools/shared/event-emitter");
const events = EventEmitter;
events.on(...)

Using static version of the methods allows specifying the `target`, while
using:

const events = new EventEmitter();

... always uses the 'EventEmitter' instance as the target.


There are still test failures, so the patch is still WIP.

Honza

Comment 7

[Julian Descottes [:jdescottes]]

I would advise to wait until Bug 1137935 is resolved before moving forward here. 

In Bug 1137935 I am trying to remove all usage of sdk/event/* in devtools at once. See https://bugzilla.mozilla.org/show_bug.cgi?id=1137935#c3 for an explanation why I think it's preferable to do it in one shot rather than in a file by file manner.

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Thanks for the review Sole!
> 
> I am attaching new version, all review comments resolved.
> 
> My question is whether using static on, off, emit methods is ok.
> It looks like it does what we want, but I am not sure.
> 
> Like as follows:
> 
> const EventEmitter = require("devtools/shared/event-emitter");
> const events = EventEmitter;
> events.on(...)
> 

FWIW, I think using the static version works fine and is the easiest way to migrate existing code that relied on the sdk event-emitter.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8896977 [details]
Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168288/diff/2-3/

Comment 9

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

Comment on attachment 8896977 [details]
Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

https://reviewboard.mozilla.org/r/168288/#review177410

Looks good to me, if all tests are passing let's try to land this. However, I'd like to understand why in some cases we have ES6 classes and in others we don't. It might be worth to add comments if there are specific reason for that, and/or file a follow up bug to have more consistency (as we are doing for other similar bugs)

::: devtools/server/performance/memory.js:34
(Diff revision 3)
>   *
>   * To be consumed by actor's, like MemoryActor using this module to
>   * send information over RDP, and TimelineActor for using more light-weight
>   * utilities like GC events and measuring memory consumption.
>   */
> -exports.Memory = Class({
> +function Memory(parent, frameCache = new StackFrameCache()) {

Just out of curiosity, is there a reason why this is not a class?

::: devtools/server/performance/recorder.js:50
(Diff revision 3)
>   * shared by all tools in a target.
>   *
>   * @param Target target
>   *        The target owning this connection.
>   */
> -exports.PerformanceRecorder = Class({
> +function PerformanceRecorder(conn, tabActor) {

Same curiosity as above (maybe there are some decorators that are not visible in the review?).

::: devtools/server/performance/timeline.js:41
(Diff revision 3)
>  const DEFAULT_TIMELINE_DATA_PULL_TIMEOUT = 200;
>  
>  /**
>   * The timeline actor pops and forwards timeline markers registered in docshells.
>   */
> -exports.Timeline = Class({
> +function Timeline(tabActor) {

Ditto.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the review Matteo!

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #9)
> ::: devtools/server/performance/memory.js:34
> (Diff revision 3)
> >   *
> >   * To be consumed by actor's, like MemoryActor using this module to
> >   * send information over RDP, and TimelineActor for using more light-weight
> >   * utilities like GC events and measuring memory consumption.
> >   */
> > -exports.Memory = Class({
> > +function Memory(parent, frameCache = new StackFrameCache()) {
> 
> Just out of curiosity, is there a reason why this is not a class?
The Memory object users method decorators (`expectState`).
So, I am following:
https://github.com/devtools-html/snippets-for-removing-the-sdk#how-to-deal-with-decorators

> ::: devtools/server/performance/recorder.js:50
> (Diff revision 3)
> >   * shared by all tools in a target.
> >   *
> >   * @param Target target
> >   *        The target owning this connection.
> >   */
> > -exports.PerformanceRecorder = Class({
> > +function PerformanceRecorder(conn, tabActor) {
> 
> Same curiosity as above (maybe there are some decorators that are not
> visible in the review?).
Yep, precisely. Same as above.

> ::: devtools/server/performance/timeline.js:41
> (Diff revision 3)
> >  const DEFAULT_TIMELINE_DATA_PULL_TIMEOUT = 200;
> >  
> >  /**
> >   * The timeline actor pops and forwards timeline markers registered in docshells.
> >   */
> > -exports.Timeline = Class({
> > +function Timeline(tabActor) {
> 
> Ditto.
Again, decorators.

I am seeing some Try failures. I am working on fixing them.

Honza


Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=536dc6d348a94210ae60ab8776aaf44e8a99006d

Honza

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf2a7774f7b9
Stop using sdk/core/heritage in DevTools performance server; r=zer0

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf2a7774f7b9

Comment 14

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

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #1)
> Used (lazy) also in:
> 
> devtools/server/performance/recorder.js

This part was missed in the current patch, Sole filed a follow up bug to fix this: Bug 1378856

Comment 15

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

(Ah, clipboard didn't work: the bug was of course bug 1395903)