1159389 - Refactor profiler actor to use the same infrastructure as other, newer actors

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1159389-profiler-refactor.patch

This was brutal.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7704170f991

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Smoke screen tests work on Firefox OS 3.0 and Gecko Profiler

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

jryans, r?ing you as well for some of the actorish stuff -- Victor for the rest

Comment 5

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

Comment on attachment 8631628 [details] [diff] [review]
1159389-profiler-refactor.patch

Review of attachment 8631628 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like you managed the conversion without having to check "is server p.js?" anywhere, is that right?

If so, hooray!  If I missed it, where does that happen?

Looks good overall.

::: browser/devtools/performance/modules/logic/actors.js
@@ +149,5 @@
>    /**
>     * Wrapper around `profiler.isActive()` to take profiler status data and emit.
>     */
>    getStatus: Task.async(function *() {
> +    let data = (yield CompatUtils.callFrontMethod("isActive").call(this));

What are the wrapping parens for?  Doesn't seem needed, but there must be a reason...

::: toolkit/devtools/server/actors/profiler.js
@@ +91,5 @@
> +    response: RetVal("json"),
> +  }),
> +
> +  stopProfiler: actorBridge("stop", {
> +    request: {},

p.js does not require this key to exist, so you can skip it for the {} case.

@@ +191,3 @@
>     */
> +  _onProfilerEvent: function (eventName, data) {
> +    console.log(eventName, data);

Remove this?

@@ +202,5 @@
> +    // style event as well.
> +    if (eventName === "eventNotification") {
> +      events.emit(this, data.topic, data);
> +    }
> +    // Otherwise if a modern protocol.js event, emit it also as `eventNotification`

Do you plan to convert the front end / client code to only listen for the p.js event style?  Don't see why you'd want to worry about both.

Add a bug number for doing this work, assuming I understand correctly.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8631628 [details] [diff] [review]
1159389-profiler-refactor.patch

Review of attachment 8631628 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/actors.js
@@ +149,5 @@
>    /**
>     * Wrapper around `profiler.isActive()` to take profiler status data and emit.
>     */
>    getStatus: Task.async(function *() {
> +    let data = (yield CompatUtils.callFrontMethod("isActive").call(this));

This is necessary when in an if statement, like `if ((yield promise)) {`, so I think this was vestigial, can be removed now!

::: toolkit/devtools/server/actors/profiler.js
@@ +91,5 @@
> +    response: RetVal("json"),
> +  }),
> +
> +  stopProfiler: actorBridge("stop", {
> +    request: {},

nice!

@@ +202,5 @@
> +    // style event as well.
> +    if (eventName === "eventNotification") {
> +      events.emit(this, data.topic, data);
> +    }
> +    // Otherwise if a modern protocol.js event, emit it also as `eventNotification`

So -- the reason for handling both events on the server should make sense -- handle older servers. Now why a p.js server that emits "modern events" would also emit the old style "eventNotification" event is for other consumers so they don't have to immediately update their code (Gecko Profiler) -- hence the deprecation message. Does that sound like a solid plan?

Comment 7

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)
> So -- the reason for handling both events on the server should make sense --
> handle older servers. Now why a p.js server that emits "modern events" would
> also emit the old style "eventNotification" event is for other consumers so
> they don't have to immediately update their code (Gecko Profiler) -- hence
> the deprecation message. Does that sound like a solid plan?

Ah, I see, I wasn't aware of the other consumers.  It sounds okay, but please add a comment that this is for consumers outside of DevTools.

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 8631628 [details] [diff] [review]
1159389-profiler-refactor.patch

Review of attachment 8631628 [details] [diff] [review]:
-----------------------------------------------------------------

very clean

::: browser/devtools/performance/modules/logic/actors.js
@@ +79,5 @@
>    destroy: Task.async(function *() {
>      if (this._poller) {
>        yield this._poller.destroy();
>      }
> +

No front.destroy() here?

::: browser/devtools/performance/test/browser_perf-data-massaging-01.js
@@ -37,5 @@
>    let secondRecordingProfile = secondRecording.getProfile();
>    let secondRecordingSamples = secondRecordingProfile.threads[0].samples.data;
>  
> -  isnot(secondRecording._profilerStartTime, 0,
> -    "The profiling start time should not be 0 on the second recording.");

Why was this removed?

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8631628 [details] [diff] [review]
1159389-profiler-refactor.patch

Review of attachment 8631628 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/actors.js
@@ +79,5 @@
>    destroy: Task.async(function *() {
>      if (this._poller) {
>        yield this._poller.destroy();
>      }
> +

Good point

@@ -82,3 @@
>      yield this.unregisterEventNotifications({ events: this.EVENTS });
> -    // TODO bug 1159389, listen directly to actor if supporting new front
> -    this._target.client.removeListener("eventNotification", this._onProfilerEvent);

+1

::: browser/devtools/performance/test/browser_perf-data-massaging-01.js
@@ -37,5 @@
>    let secondRecordingProfile = secondRecording.getProfile();
>    let secondRecordingSamples = secondRecordingProfile.threads[0].samples.data;
>  
> -  isnot(secondRecording._profilerStartTime, 0,
> -    "The profiling start time should not be 0 on the second recording.");

This no longer is testing the same assumption/implementation detail -- what is profilerStartTime with a backend performanceactor? And this is just teseting profiler mechanics which are tested elsewhere

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1159389-profiler-refactor.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3802a424cef6

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1159389-profiler-refactor.patch

One more try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd5d400e4b9f

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1159389-profiler-refactor.patch

again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51083a64250

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d964bc1d6bc2

Comment 14

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

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