If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Do not render graphs realtime if e10s disabled

VERIFIED FIXED in Firefox 40

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: paul, Assigned: jsantell)

Tracking

unspecified
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox40 verified, firefox41 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
Latest nightly on Windows, non E10S, I can't stop the recording.
Blocks: 1075567
tracking-e10s: --- → +
(Reporter)

Comment 2

3 years ago
Let's do that:

If the pref tool is open in a non-E10S version of Firefox, we show a button that says: "Firefox is not running in multi-process mode. Click [here] to re-open this tab in a multi-process window". This will open a new E10S Firefox window with only one tab.

Afaik, it is possible to open a E10S window in any version of Firefox, but I'm not 100% sure.
Speaking with jimm in #e10s:

* Opening a new e10s window in non-e10s mode is not possible in current release/beta. This may change when Fx40 is release.
* e10s team is maybe planning on removing the "open e10s window" option on aurora upwards, so unlikely we'll have that option for when the perf tools goes up the train.
* Reasoning: There are problems with running e10s and non-e10s at the same time.
(Assignee)

Updated

2 years ago
Summary: Make sure the new perf tool works properly if E10S is disabled → Do not render graphs realtime if e10s disabled
(Assignee)

Updated

2 years ago
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Created attachment 8607271 [details] [diff] [review]
1162583-disable-rendering-none10s.patch

I hope you're ready for an obscenely terrible patch.

So this adds a few ways to get e10s support (whether or not e10s is even available on the platform) as well as if e10s is on. These are temporary measures, as we will soon (!) always have e10s, and are isolated for removal in the future.

The other component is disabling realtime rendering if e10s is not on, and displaying a message based on whether a user can enable it or not. Not sure if we can/should add a button here to just enable it for a user -- can think about that later. Also if we want to add non-realtime rendering as a pref in the future, we can use some of these components here, but didn't want to make that a pref now, as it'd be confusing on how to resolve that with e10s support.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a86e85814629
Attachment #8607271 - Flags: review?(vporof)
Comment on attachment 8607271 [details] [diff] [review]
1162583-disable-rendering-none10s.patch

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

SYSTEM.JS wow

::: browser/devtools/performance/performance-controller.js
@@ +524,5 @@
> +   * it's already enabled, respectively.
> +   *
> +   * @return {object}
> +   */
> +  getMultiprocessStatus: function () {

getMpStatus seems out of place here. Shouldn't it be in system.js or something?

::: browser/devtools/performance/system.js
@@ +12,5 @@
> +
> +// If e10s is possible on the platform.
> +#ifdef E10S_TESTING_ONLY
> +SYSTEM.MULTIPROCESS_SUPPORTED = true;
> +#endif

Why not have this a module with exports? Seems weird to me to include it in xul, since it's not a view at all.

::: browser/devtools/performance/views/overview.js
@@ +33,5 @@
>    /**
> +   * How frequently we attempt to render the graphs. Overridden
> +   * in tests.
> +   */
> +  OVERVIEW_UPDATE_INTERVAL: OVERVIEW_UPDATE_INTERVAL,

Nit: rename this property to camelCase.
Attachment #8607271 - Flags: review?(vporof) → review+
Comment on attachment 8607271 [details] [diff] [review]
1162583-disable-rendering-none10s.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +524,5 @@
> +   * it's already enabled, respectively.
> +   *
> +   * @return {object}
> +   */
> +  getMultiprocessStatus: function () {

Could move it to a module, if we can get preprocessing there. Otherwise, everything else goes through the controller, so why not this? This also handles setting the attribute on the XUL container to change the display messages when e10s is disabled/unsupported

::: browser/devtools/performance/system.js
@@ +12,5 @@
> +
> +// If e10s is possible on the platform.
> +#ifdef E10S_TESTING_ONLY
> +SYSTEM.MULTIPROCESS_SUPPORTED = true;
> +#endif

Not sure how to do preprocessing outside of things in jar.mn
On IRC, VP said the above patch was good to land
 https://hg.mozilla.org/integration/fx-team/rev/c4477d31401c
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c4477d31401c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Blocks: 1167252
Flags: qe-verify+
Comment on attachment 8607271 [details] [diff] [review]
1162583-disable-rendering-none10s.patch


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8607271 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f933abba006
status-firefox40: --- → fixed
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8607271 [details] [diff] [review]
1162583-disable-rendering-none10s.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8607271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 40.0a1 (2015-06-09) using Windows 7 (x64), Ubuntu 13.10 (x64) and Mac OS X 10.9.5.

There's no realtime graph rendering while e10s is disabled and the following message is displayed while recording: "Enable multiprocess Firefox in preferences for rendering recording data in realtime.", screenshot also available - http://i.imgur.com/0FAL161.png.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.