1077442 - Create pseudo FrontActor as PerformanceActor

Status: RESOLVED FIXED

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

Description

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

Wrapper for the front for syncing recording between TimelineActor and ProfilerActor, similar to current ./browser/devtools/profiler/utils/shared.js

Comment 1

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

Attachment: 1077442-pseudo-perffront.patch

Giant pseudo perf front. Some console methods are implemented, with notes to finish them in the console hooks bug (bug 1077464), and some profiler tests have been ported over as well

Comment 2

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

Comment on attachment 8508154 [details] [diff] [review]
1077442-pseudo-perffront.patch

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

Nice!

I have just a couple of comments and suggestions for removing code should land in different bugs. I'll take a closer look at the tests after the next patch iteration, but please remove the ones that deal with consoles, multiple recordings etc.

::: browser/devtools/performance/modules/front.js
@@ +22,5 @@
> +
> +/**
> + * A cache of all PerformanceConnection instances. The keys are Toolbox objects.
> + */
> +let SharedPerformanceConnection = new WeakMap();

SharedPerformanceConnection sounds a bit weird to me. My first instinct is to think it's about a connection that's performing well :) It wasn't the case with ShaderProfilerConnection, because "profiler" made it obvious.

Something like SharedPerformanceToolConnection sounds a bit more accurate, but it's a mouthful. In any case, this needs a better name.

@@ +29,5 @@
> + * Instantiates a shared PerformanceConnection for the specified toolbox.
> + * Consumers must yield on `open` to make sure the connection is established.
> + *
> + * @param Toolbox toolbox
> + *        The toolbox owning this connection.

Instead of taking a toolbox (like before), it'd be cleaner for this to take a target. The toolbox isn't actually used anywhere.

@@ +51,5 @@
> + *
> + * @param Toolbox toolbox
> + *        The toolbox owning this connection.
> + */
> +function PerformanceConnection(toolbox) {

Ditto for PerformanceConnection naming, and taking a target instead of a toolbox.

@@ +62,5 @@
> +
> +  this._pendingTimelineConsumers = 0;
> +  this._pendingConsoleRecordings = [];
> +  this._finishedConsoleRecordings = [];
> +  this._onEventNotification = this._onEventNotification.bind(this);

None of these are necessary yet, remove them until the patches to control multiple simultaneous connections and console recordings land.

@@ +64,5 @@
> +  this._pendingConsoleRecordings = [];
> +  this._finishedConsoleRecordings = [];
> +  this._onEventNotification = this._onEventNotification.bind(this);
> +
> +  Services.obs.notifyObservers(null, "profiler-connection-created", null);

Outdated observer notification name.

@@ +77,5 @@
> +
> +  // Underlying ProfilerActor
> +  get profiler () {
> +     return this._profiler;
> +  },

Are these getters used anywhere yet? If not, remove them.
As a general rule, I'd like us to avoid landing unused code.

@@ +102,5 @@
> +    yield this._connectTimelineActor();
> +
> +    this._connected = true;
> +
> +    Services.obs.notifyObservers(null, "profiler-connection-opened", null);

Another outdated observer notification name.

@@ +145,5 @@
> +  /**
> +   * Initializes a connection to a timeline actor.
> +   */
> +  _connectTimelineActor: function() {
> +    // Only initialize the framerate front if the respective actor is available.

Only initialize the *framerate* front? I think you mean *timeline*.

@@ +149,5 @@
> +    // Only initialize the framerate front if the respective actor is available.
> +    // Older Gecko versions don't have an existing implementation, in which case
> +    // all the methods we need can be easily mocked.
> +    //
> +    // If the timeline actor exists, all underlying actors (memory, framerate) exist.

I'd add that "all underlying actors (memory, framerate) exist and have the expected methods and behavior".

@@ +153,5 @@
> +    // If the timeline actor exists, all underlying actors (memory, framerate) exist.
> +    // If using the Performance tool, and timeline actor does not exist (FxOS devices < Gecko 35),
> +    // then just use the mocked actor and do not display timeline data.
> +    //
> +    // TODO use framework level feature detection from bug 1069673

Is there any reason to land this before bug 1069673? How about fixing this now, instead of having to deal with it in a followup.

@@ +211,5 @@
> +          this._pendingTimelineConsumers++;
> +          break;
> +        case "stop":
> +          if (--this._pendingTimelineConsumers > 0) return;
> +          break;

None of this is needed yet. Remove.

@@ +213,5 @@
> +        case "stop":
> +          if (--this._pendingTimelineConsumers > 0) return;
> +          break;
> +      }
> +      checkPendingTimelineConsumers(this);

Ditto.

@@ +224,5 @@
> +   *
> +   * @return object
> +   *         A promise that is resolved once the notifications are registered.
> +   */
> +  _registerEventNotifications: Task.async(function*() {

We have a different bug for this.

@@ +236,5 @@
> +   *
> +   * @param object response
> +   *        The data received from the backend.
> +   */
> +  _onEventNotification: function(event, response) {

We have a different bug for this too.

@@ +266,5 @@
> +   * @param number currentTime
> +   *        The time in milliseconds when the call was made, relative to
> +   *        when the nsIProfiler module was started.
> +   */
> +  _onConsoleProfileStart: Task.async(function *({ profileLabel, currentTime }) {

Ditto.

@@ +281,5 @@
> +   *
> +   * @param object profilerData
> +   *        The profiler data received from the backend.
> +   */
> +  _onConsoleProfileEnd: Task.async(function *(profilerData) {

Ditto.

@@ +314,5 @@
> +  EventEmitter.decorate(this);
> +
> +  this._request = connection._request;
> +  this.pendingConsoleRecordings = connection._pendingConsoleRecordings;
> +  this.finishedConsoleRecordings = connection._finishedConsoleRecordings;

Remove.

@@ +319,5 @@
> +
> +  // Pipe events from `connection` to the front
> +  connection.on("profile", (e, args) => this.emit(e, args));
> +  connection.on("profileEnd", (e, args) => this.emit(e, args));
> +  connection.on("profiler-unexpectedly-stopped", (e, args) => this.emit(e, args));

Remove.

@@ +325,5 @@
> +  // Pipe events from TimelineActor to the PerformanceFront
> +  // TODO should these be cleaned up/unbound?
> +  connection.timeline.on("markers", markers => this.emit("markers", markers));
> +  connection.timeline.on("memory", (delta, measurement) => this.emit("memory", delta, measurement));
> +  connection.timeline.on("ticks", (delta, timestamps) => this.emit("ticks", delta, timestamps));

Don't think these are needed yet. IIRC they were only used in tests.

@@ +356,5 @@
> +    }
> +
> +    // The timeline actor is target-dependent, so just make sure
> +    // it's recording.
> +    yield this._request("timeline", "start", { withTicks: true, withMemory: true });

Memory recording won't always be turned on. For now, just use some prefs for this, the UI will be added later.

@@ +381,5 @@
> +
> +    // TODO do we keep this? As the new Timeline actor is event based, with
> +    // no methods of getting retroactive data
> +    //let ticksData = yield this._request("timeline", "getPendingTicks", beginAt, endAt);
> +    yield this._request("timeline", "stop");

This logic should probably be somewhere else, since the framerate is displayed live. Remove it for now.

@@ +475,5 @@
> +
> +/**
> + * Asserts the value sanity of `pendingTimelineConsumers`.
> + */
> +function checkPendingTimelineConsumers(connection) {

Not needed yet. Remove.

::: browser/devtools/performance/panel.js
@@ +36,5 @@
>      this.panelWin.gTarget = this.target;
>  
> +    this._connection = getPerformanceConnection(this.panelWin.gToolbox);
> +    yield this._connection.open();
> +    this.panelWin.gFront = new PerformanceFront(this._connection);

Uber nit: add a newline before setting gFront.

Comment 3

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

Do we really need to get rid of the minimal console hooks at this point and tests for the shared profiler connection and how they interact? When building this out, since the pieces were there, it was easier just keeping them rather than doing it fully piece by piece

Comment 4

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

I would prefer having the patches as minimal as possible. Since the actual console support won't be implemented for quite a while (it's very low on our roadmap), you might expect things to break in the meantime, or some (unexpected) architectural changes to pop up, in which case you have more code to refactor.

Comment 5

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

Comment on attachment 8508154 [details] [diff] [review]
1077442-pseudo-perffront.patch

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

::: browser/devtools/performance/modules/front.js
@@ +22,5 @@
> +
> +/**
> + * A cache of all PerformanceConnection instances. The keys are Toolbox objects.
> + */
> +let SharedPerformanceConnection = new WeakMap();

SharedProfilerConnection sounds like it'd still work in this case, as only the underlying profiler connection is shared. Will revert back to that name

@@ +64,5 @@
> +  this._pendingConsoleRecordings = [];
> +  this._finishedConsoleRecordings = [];
> +  this._onEventNotification = this._onEventNotification.bind(this);
> +
> +  Services.obs.notifyObservers(null, "profiler-connection-created", null);

Gonna leave this as `profiler-connection-*` to continue with the `ProfilerConnection` naming scheme, as this is still a shared profiler connection

@@ +153,5 @@
> +    // If the timeline actor exists, all underlying actors (memory, framerate) exist.
> +    // If using the Performance tool, and timeline actor does not exist (FxOS devices < Gecko 35),
> +    // then just use the mocked actor and do not display timeline data.
> +    //
> +    // TODO use framework level feature detection from bug 1069673

If it lands before this is ready, sure thing, otherwise, waiting just holds up perf development, and the support method used is just a helper (doesn't do anything fancy)

@@ +325,5 @@
> +  // Pipe events from TimelineActor to the PerformanceFront
> +  // TODO should these be cleaned up/unbound?
> +  connection.timeline.on("markers", markers => this.emit("markers", markers));
> +  connection.timeline.on("memory", (delta, measurement) => this.emit("memory", delta, measurement));
> +  connection.timeline.on("ticks", (delta, timestamps) => this.emit("ticks", delta, timestamps));

How else can we know if any of this works?

Comment 6

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)
> Comment on attachment 8508154 [details] [diff] [review]
> 1077442-pseudo-perffront.patch
> 
> Review of attachment 8508154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/performance/modules/front.js
> @@ +22,5 @@
> > +
> > +/**
> > + * A cache of all PerformanceConnection instances. The keys are Toolbox objects.
> > + */
> > +let SharedPerformanceConnection = new WeakMap();
> 
> SharedProfilerConnection sounds like it'd still work in this case, as only
> the underlying profiler connection is shared. Will revert back to that name
> 

I don't want us to hinder development too much because of naming debates, but I think `SharedProfilerConnection` wouldn't be too accurate either. The "shared" part was referring to the fact that this bridge is going to be used by all tools in a toolbox, not just because of the fact that the profiler module is shared. A single instance of a connection (so to all actors inside it) should be shared between all tools using a target. This is especially true now if we add the timeline and console.timeline becoming synonymous to console.profile.

Anyway, I'll defer to your best judgement here. I'd prefer SharedPerformanceActors for the shared connection, and I guess PerformanceFront is probably fine for the front. What do you think?

> @@ +64,5 @@
> > +  this._pendingConsoleRecordings = [];
> > +  this._finishedConsoleRecordings = [];
> > +  this._onEventNotification = this._onEventNotification.bind(this);
> > +
> > +  Services.obs.notifyObservers(null, "profiler-connection-created", null);
> 
> Gonna leave this as `profiler-connection-*` to continue with the
> `ProfilerConnection` naming scheme, as this is still a shared profiler
> connection
> 

See above. Maybe `performance-actors-*` is a good name?

> @@ +153,5 @@
> > +    // If the timeline actor exists, all underlying actors (memory, framerate) exist.
> > +    // If using the Performance tool, and timeline actor does not exist (FxOS devices < Gecko 35),
> > +    // then just use the mocked actor and do not display timeline data.
> > +    //
> > +    // TODO use framework level feature detection from bug 1069673
> 
> If it lands before this is ready, sure thing, otherwise, waiting just holds
> up perf development, and the support method used is just a helper (doesn't
> do anything fancy)
> 

Ok.

> @@ +325,5 @@
> > +  // Pipe events from TimelineActor to the PerformanceFront
> > +  // TODO should these be cleaned up/unbound?
> > +  connection.timeline.on("markers", markers => this.emit("markers", markers));
> > +  connection.timeline.on("memory", (delta, measurement) => this.emit("memory", delta, measurement));
> > +  connection.timeline.on("ticks", (delta, timestamps) => this.emit("ticks", delta, timestamps));
> 
> How else can we know if any of this works?

You're right, these are used in tests right?

Comment 7

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

Attachment: 1077442-pseudo-perffront.patch

Comment 8

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

Comment on attachment 8509766 [details] [diff] [review]
1077442-pseudo-perffront.patch

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

::: browser/devtools/performance/modules/front.js
@@ +256,5 @@
> +
> +    // Fetch the recorded refresh driver ticks, during the same time window
> +    // as the filtered profiler data.
> +    let beginAt = findEarliestSampleTime(profilerData);
> +    let endAt = findOldestSampleTime(profilerData);

These two aren't used anywhere anymore.

@@ +321,5 @@
> + *        The profiler data received from the backend.
> + * @return number
> + *         The earliest sample time (in milliseconds).
> + */
> +function findEarliestSampleTime(profilerData) {

Unused function.

@@ +339,5 @@
> + *        The profiler data received from the backend.
> + * @return number
> + *         The oldest sample time (in milliseconds).
> + */
> +function findOldestSampleTime(profilerData) {

Ditto.

::: browser/devtools/performance/panel.js
@@ +34,5 @@
>    open: Task.async(function*() {
>      this.panelWin.gToolbox = this._toolbox;
>      this.panelWin.gTarget = this.target;
>  
> +    this._connection = getPerformanceActorsConnection(this.panelWin.gTarget);

s/this.panelWin.gTarget/this.target/

::: browser/devtools/performance/test/browser.ini
@@ +5,5 @@
> +  doc_simple-test.html
> +  head.js
> +
> +# Commented out tests are profiler tests
> +# that need to be moved over to performance tool

Are we going to do this in the respective bugs (land console support, etc.)? If so, it might be a good idea to add those bug numbers here.

Comment 9

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

Attachment: 1077442-pseudo-perffront.patch

Removed unused functions, added bug numbers to missing tests

Comment 10

[Carsten Book [:Tomcat]]

Hi Jordan, could you provide a try run for this bug? Thanks!

Comment 11

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

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a89ec9d7af37

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4760de1a0120