1107949 - The framerate actor stops recording after a tab navigation in e10s

Status: RESOLVED FIXED

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

Description

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

Only in e10s

Comment 1

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

I believe the cause of the inability to get more data on the front end is `FramerateActor._onRefreshDriverTick` not called after refresh

Comment 3

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

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.

Comment 4

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

Nope, still doesn't work. Refreshing while recording stops the framerate graph.

Comment 5

[Eddy Bruel [:ejpbruel]]

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.

Comment 6

[Jordan Santell [:jsantell] [@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

Comment 7

[Jordan Santell [:jsantell] [@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?

Comment 8

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

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

Comment 9

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

Attachment: 1107949-framerate-e10s.patch

Woopwoop

Comment 10

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

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

Comment 11

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

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.

Comment 12

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

Attachment: 1107949-framerate-e10s.patch

Removed content observer, modify start time via elapsed time on page reload

Comment 13

[Eddy Bruel [:ejpbruel]]

Will this fix allow us to mark the bug as resolved?

Thanks for your work on this Jordan. Great job!

Comment 14

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

Yup, should be all done once landed!

Comment 15

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

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

Comment 16

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

Merge: https://treeherder.mozilla.org/#/jobs?repo=gum&revision=08329c29ef68

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bba3baa6b36e