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

VERIFIED FIXED in Firefox 40

Status

P2
normal
VERIFIED FIXED
4 years ago
4 months ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

37 Branch
Firefox 40
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox40 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Updated

4 years ago
Blocks: 1075567
Priority: -- → P2
(Assignee)

Updated

4 years ago
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)
Created attachment 8600658 [details] [diff] [review]
1159052-perf-stop.patch
Attachment #8600658 - Flags: review?(vporof)
(Assignee)

Updated

4 years ago
Blocks: 1146239
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
Created attachment 8601585 [details] [diff] [review]
1159052-perf-stop.patch

Addressed comments, rebased
Attachment #8600658 - Attachment is obsolete: true
Attachment #8601585 - Flags: review+
(Assignee)

Updated

4 years ago
See Also: → bug 1161781
https://hg.mozilla.org/mozilla-central/rev/3c91829d896e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Depends on: 1162104

Updated

3 years ago
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
status-firefox40: fixed → verified

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.