Closed Bug 1159052 Opened 9 years ago Closed 9 years ago

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

Categories

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

37 Branch
defect

Tracking

(firefox40 verified)

VERIFIED FIXED
Firefox 40
Tracking Status
firefox40 --- verified

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: perf-tool-v2
Priority: -- → P2
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
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)
Attached patch 1159052-perf-stop.patch (obsolete) — Splinter Review
Attachment #8600658 - Flags: review?(vporof)
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
Attachment #8600658 - Flags: review?(vporof) → review+
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
Addressed comments, rebased
Attachment #8600658 - Attachment is obsolete: true
Attachment #8601585 - Flags: review+
See Also: → 1161781
https://hg.mozilla.org/mozilla-central/rev/3c91829d896e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Depends on: 1162104
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.