Closed
Bug 1162583
Opened 9 years ago
Closed 9 years ago
Do not render graphs realtime if e10s disabled
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(e10s+, firefox40 verified, firefox41 fixed)
VERIFIED
FIXED
Firefox 41
People
(Reporter: paul, Assigned: jsantell)
References
Details
Attachments
(1 file)
23.15 KB,
patch
|
vporof
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•9 years ago
|
||
Latest nightly on Windows, non E10S, I can't stop the recording.
Updated•9 years ago
|
Blocks: perf-tool-v2
Updated•9 years ago
|
tracking-e10s:
--- → +
Reporter | ||
Comment 2•9 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.
Assignee | ||
Comment 3•9 years ago
|
||
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•9 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•9 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
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
Assignee | ||
Comment 7•9 years ago
|
||
On IRC, VP said the above patch was good to land
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c4477d31401c
Whiteboard: [fixed-in-fx-team]
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4477d31401c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Blocks: perf-40-uplifts
Updated•9 years ago
|
Flags: qe-verify+
Comment 10•9 years ago
|
||
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?
Comment 11•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f933abba006
status-firefox40:
--- → fixed
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
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.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•