1159052 - Stopping a recording should stop all overview updates and not stretch duration

Status: VERIFIED FIXED

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

Description

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

When stopping, while waiting for the profiler to finish and send data, etc., the overview graphs keeps rendering and stretches the duration.

Ex: hitting "stop" at 10s in, the overview graphs will render for another few seconds, resulting in a 14s profile for example, with the last 4 seconds containing no data for some of the panels.

Comment 1

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

Few things here:

* Front now emits all recording state changes (start/starting/stop/stopping), and no longer differentiates between console or not (can use isConsole on model if we need to discern, although all views currently do this if needed)
* RecordingModel's are no longer considered `isRecording()` after receiving the "stopping" call, and are not considered `isCompleted()` until after the "stopped" call.
* Overview graphs stop rendering on the "stopping" call.

Our time issues are messed up, and the part 2 will be pretty much be handled in bug 1152829 with time syncing, but this needs to be done anyway for older geckos. Instead of using the profiler stop time (which we can only get after the profiler sends all of its data), this uses a Date.now() (or can use performance.now()) on the "stopping" event, giving us the duration of the recording. Additional timeline data will not be able to make it to the model, but we can have profiler data beyond the limit (on a big profile, this could be a few seconds -- Shu's dedupe patch will reduce this obviously, but there'll still be a difference), so that could skew the entire results slightly.

Continuing basing the stop time on the profiler will mean we have empty spaces in the overview. To combat samples outside of the Date.now() endTime, we can filter the out of range samples like we do currently for older geckos' startTime, and for current geckos to increase performance, we can reopen bug 1155535, which lets us filter on endTime as well.

Thoughts on this approach? (patch soon)

Comment 2

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

Attachment: 1159052-perf-stop.patch

Comment 3

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

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

Comment 4

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

Comment on attachment 8600658 [details] [diff] [review]
1159052-perf-stop.patch

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

very clean, nicely done!

::: browser/devtools/performance/modules/actors.js
@@ +223,5 @@
>  
>      yield this.attach();
>  
> +    // Reconstruct our options because the server actor fails when being passed
> +    // undefined values in objects.

T___T

::: browser/devtools/performance/modules/front.js
@@ +342,5 @@
>  
> +    // Flag the recording as no longer recording, so that `model.isRecording()`
> +    // is false. Do this before we fetch all the data, and then subsequently
> +    // the recording can be considered "completed".
> +    let endTime = Date.now();

Is there a reason _onStopping recording takes in a timestamp? Can't we just get it there?

::: browser/devtools/performance/modules/recording-model.js
@@ +285,5 @@
>     */
>    addTimelineData: function (eventName, ...data) {
>      // If this model isn't currently recording,
>      // ignore the timeline data.
> +    if (!this.isRecording()) {

Useless change?

::: browser/devtools/performance/performance-controller.js
@@ +330,5 @@
>      if (latest && latest.isRecording()) {
>        yield this.stopRecording();
>      }
> +    // If last recording is not recording, but finalizing itself,
> +    // wait for that to finish

brb finalizing myself

@@ +429,5 @@
>  
>      this.emit(EVENTS.THEME_CHANGED, data.newValue);
>    },
>  
> +  _onRecordingStateChange: function (state, model) {

Nit: jsdoc at least for the param types

::: browser/devtools/performance/performance-view.js
@@ +122,5 @@
>    /**
>     * Adds the `locked` attribute on the record button. This prevents it
>     * from being clicked while recording is started or stopped.
>     */
>    _lockRecordButton: function () {

Buttons* now, not button.

@@ +125,5 @@
>     */
>    _lockRecordButton: function () {
> +    for (let button of $$(".record-button")) {
> +      button.setAttribute("locked", "true");
> +    }

YES! But in a different bug maybe? Not feeling strongly about this.

::: browser/devtools/performance/views/overview.js
@@ +200,5 @@
>     * Called when recording will start. No recording because it does not
>     * exist yet, but can just disable from here. This will only trigger for
>     * manual recordings.
>     */
> +  _onRecordingWillStart: OverviewViewOnStateChange(Task.async(function* () {

wow those method signatures

Comment 5

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

Comment on attachment 8600658 [details] [diff] [review]
1159052-perf-stop.patch

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

::: browser/devtools/performance/modules/front.js
@@ +342,5 @@
>  
> +    // Flag the recording as no longer recording, so that `model.isRecording()`
> +    // is false. Do this before we fetch all the data, and then subsequently
> +    // the recording can be considered "completed".
> +    let endTime = Date.now();

Since we're using Date.now(), it can be anywhere. There's a lot of extra time stuff in the front that hopefully will be resolved with bug 1152829

::: browser/devtools/performance/modules/recording-model.js
@@ +285,5 @@
>     */
>    addTimelineData: function (eventName, ...data) {
>      // If this model isn't currently recording,
>      // ignore the timeline data.
> +    if (!this.isRecording()) {

If recording is only based off of one value (`_recording`), yes, but that can change in the future (like how this patch introduced the difference between recording and completion (where completion also has to check imported status)

::: browser/devtools/performance/performance-view.js
@@ +122,5 @@
>    /**
>     * Adds the `locked` attribute on the record button. This prevents it
>     * from being clicked while recording is started or stopped.
>     */
>    _lockRecordButton: function () {

changed

@@ +125,5 @@
>     */
>    _lockRecordButton: function () {
> +    for (let button of $$(".record-button")) {
> +      button.setAttribute("locked", "true");
> +    }

It's all related to sync'ing the stopping state, so I think this is relevant here

Comment 6

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

Attachment: 1159052-perf-stop.patch

Addressed comments, rebased

Comment 7

[Pulsebot]

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

Comment 8

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/3c91829d896e

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/3c91829d896e

Comment 10

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 40.0a1 (2015-04-27) under Windows 7 64-bit.

Verified fixed on Firefox 40 Beta 8 (20150727174134) using Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.4.