Closed Bug 1107949 Opened 5 years ago Closed 5 years ago

The framerate actor stops recording after a tab navigation in e10s

Categories

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

x86
macOS
defect
Not set

Tracking

(e10s+, firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
e10s + ---
firefox38 --- fixed

People

(Reporter: vporof, Assigned: jsantell)

References

(Blocks 1 open bug)

Details

(Whiteboard: [e10s-m6])

Attachments

(1 file, 1 obsolete file)

Only in e10s
Blocks: perf-tool-v2
Blocks: dte10s
tracking-e10s: --- → +
I believe the cause of the inability to get more data on the front end is `FramerateActor._onRefreshDriverTick` not called after refresh
Duplicate of this bug: 1101191
Whiteboard: [e10s-m6]
Looks like this is no longer an issue in e10s. I can run the perf++ tool, and click on links/navigate using URL bar and the perf tool keeps recording correctly. Please confirm, Victor.
Assignee: nobody → jsantell
Flags: needinfo?(vporof)
Nope, still doesn't work. Refreshing while recording stops the framerate graph.
Flags: needinfo?(vporof)
So, this is still a problem according to Victor. Jordan, is this still on your radar? It's one of our high priority bugs for e10s.
Flags: needinfo?(jsantell)
I was wrong -- Victor's right, still an issue. I think this is a platform issue, will work on a STR for chrome window and request animation frame and file a bug if this is the case
Flags: needinfo?(jsantell)
For non-e10s, we take the content window and get the corresponding top-level chrome browser window. In e10s there is no wrapper around the content window, so this kinda makes sense. What framerates do we intend while it's reloading? There's no content to exist until the window is recreated. Can attach anything to the `this._chromeWin` as a test and it gets cleared upon refreshing the page, for example.

With e10s, I'm not sure what should be the intended result. We can restart the rAF on reload, and in the mean time, spit out 60 FPS? Thoughts?
Flags: needinfo?(vporof)
Flags: needinfo?(ejpbruel)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> 
> With e10s, I'm not sure what should be the intended result. We can restart
> the rAF on reload, and in the mean time, spit out 60 FPS? Thoughts?

Presumably the time in between the current page navigation end (when the content window stops being available) and the other page navigation start (when a new content window is available) is infinitesimally small. All we need is a content window, and since that is almost immediately available, so I wouldn't worry about doing anything magical in between. Just stop recording ticks for a brief time while we retrigger rAF.
Flags: needinfo?(vporof)
Attached patch 1107949-framerate-e10s.patch (obsolete) — Splinter Review
Woopwoop
Flags: needinfo?(ejpbruel)
Attachment #8561789 - Flags: review?(vporof)
Comment on attachment 8561789 [details] [diff] [review]
1107949-framerate-e10s.patch

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

I'd like us to avoid the call watcher, and possibly remove the redundant _delta property. Test looks good!

::: toolkit/devtools/server/actors/framerate.js
@@ +25,5 @@
>    initialize: function(conn, tabActor) {
>      protocol.Actor.prototype.initialize.call(this, conn);
>      this.tabActor = tabActor;
> +    this._observer = new ContentObserver(this.tabActor);
> +    this._win = tabActor.window;

s/_win/_contentWin/ to make this more obvious.

@@ +30,3 @@
>      this._onRefreshDriverTick = this._onRefreshDriverTick.bind(this);
> +    this._onGlobalCreated = this._onGlobalCreated.bind(this);
> +    on(this._observer, "global-created", this._onGlobalCreated);

You can now use `on(this.tabActor, "window-ready", this._onGlobalCreated);` instead of the call watcher. We should deprecate it.

@@ +118,5 @@
>  
>      // Store the amount of time passed since the recording started.
> +    let currentTime = this._win.performance.now() + this._delta;
> +    // Store _lastTickTime so we can use this as a delta on a page refresh
> +    // to normalize times

Nit: . after sentences.

@@ +130,5 @@
> +   * When the content window for the tab actor is created.
> +   */
> +  _onGlobalCreated: function (win) {
> +    if (this._recording) {
> +      this._delta = this._lastTickTime;

Instead of having a whole new _delta, why not simply change the _startTime? Same math happens in _onRefreshDriverTick.
Attachment #8561789 - Flags: review?(vporof)
Removed content observer, modify start time via elapsed time on page reload
Attachment #8561789 - Attachment is obsolete: true
Attachment #8562933 - Flags: review?(vporof)
Attachment #8562933 - Flags: review?(vporof) → review+
Will this fix allow us to mark the bug as resolved?

Thanks for your work on this Jordan. Great job!
Yup, should be all done once landed!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/bba3baa6b36e
Keywords: checkin-needed
Whiteboard: [e10s-m6] → [e10s-m6][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bba3baa6b36e
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [e10s-m6][fixed-in-fx-team] → [e10s-m6]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.