Closed
Bug 1123815
(enable-perf-tool)
Opened 10 years ago
Closed 10 years ago
[meta] Enable Performance++ Tool
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jsantell, Assigned: vporof)
References
Details
(Keywords: meta)
Attachments
(1 file, 4 obsolete files)
19.67 KB,
patch
|
Details | Diff | Splinter Review |
This will enable the performance tool when we're ready to ship it for (expected) Fx38. Things we need to do for this:
* Disable ability to run either profiler or timeline
* Remove build flags to enable performance tool
* Remove `devtools.performance_dev.enabled` and change to `devtools.performance.enabled`
* Change toolbox options 'show-platform-data' from `devtools.profiler.ui.show-platform-data` to `devtools.performance.ui.show-platform-data`
Reporter | ||
Updated•10 years ago
|
Blocks: perf-tool-v2
Assignee | ||
Comment 1•10 years ago
|
||
* Adapt all existing profiler and timeline tests to the new perf tool
* Move all shared files into the devtools/performance folder
* Remove all dead code
Updated•10 years ago
|
Updated•10 years ago
|
Comment 2•10 years ago
|
||
Just added dependency on bugs that I judged necessary to fix before enabling the new tool and before removing the current ones.
Comment 3•10 years ago
|
||
Feel free to remove/add some if you disagree though.
Assignee | ||
Updated•10 years ago
|
Summary: Enable Performance++ Tool → enable-perf-tool
Assignee | ||
Updated•10 years ago
|
Alias: enable-perf-tool
Summary: enable-perf-tool → Enable Performance++ Tool
Assignee | ||
Comment 4•10 years ago
|
||
Enabled tests on gum: https://treeherder.mozilla.org/#/jobs?repo=gum
There's two types of failures afaict:
1. Shared profiler connections conflicting, and making browser_toolbox_* tests fail, because they try to open both the timeline and the new perf tool.
2. Crashes, like bug 1130669
I suggest we land a patch on gum that disables the old timeline and profiler (not just the tests, but the tools too, because we want to avoid the toolbox opening both of them and the new perf tool at the same time), and see what still fails.
Flags: needinfo?(jsantell)
Reporter | ||
Comment 5•10 years ago
|
||
gDevToolsBrowser._connectToProfiler will have to be changed over to use new profiler front.
Have a patch for this switch, disabling timeline/profiler and their tests. Thoughts on moving them to browser/devtools/deprecated/{timeline|profiler}? Or just removing them? Would like easy access to any remaining tests we want to copy over, etc.
Waiting to wrap up bug 1101235 for this as well.
Flags: needinfo?(jsantell)
Reporter | ||
Comment 6•10 years ago
|
||
This is a patch that does the following:
* enables perf++ by default, no longer needing build flag
* No longer builds profiler/timeline tools, and tests will not run
* Convert over remaining prefs to performance (ui-show-platform)
* framework uses new front from perf rather than profiler
NOT TO BE LANDED IN FX-TEAM YET
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8563038 [details] [diff] [review]
1123815-turn-on-perf.patch do not land on fx-team yet
Review of attachment 8563038 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/app/profile/firefox.js
@@ -1407,5 @@
> pref("devtools.debugger.ui.variables-sorting-enabled", true);
> pref("devtools.debugger.ui.variables-only-enum-visible", false);
> pref("devtools.debugger.ui.variables-searchbox-visible", false);
>
> -// Enable the Profiler
Keep the "Enable the ..." comment.
::: browser/devtools/framework/gDevTools.jsm
@@ +1170,5 @@
> * Connects to the SPS profiler when the developer tools are open. This is
> * necessary because of the WebConsole's `profile` and `profileEnd` methods.
> */
> _connectToProfiler: function DT_connectToProfiler(event, toolbox) {
> + let ProfilerFront = devtools.require("devtools/performance/front");
You're not really importing the "ProfilerFront". Maybe rename to SharedPerformanceUtils?
::: browser/devtools/main.js
@@ -45,5 @@
> const styleEditorProps = "chrome://browser/locale/devtools/styleeditor.properties";
> const shaderEditorProps = "chrome://browser/locale/devtools/shadereditor.properties";
> const canvasDebuggerProps = "chrome://browser/locale/devtools/canvasdebugger.properties";
> const webAudioEditorProps = "chrome://browser/locale/devtools/webaudioeditor.properties";
> const profilerProps = "chrome://browser/locale/devtools/profiler.properties";
File a bug to rename profiler.properties/dtd to performance.properties/dtd and all the variables here.
@@ +248,3 @@
> Tools.performance = {
> id: "performance",
> ordinal: 19,
Don't forget to change the ordinals. Keep the performance ordinal where the old profiler used to be.
@@ +374,3 @@
> Tools.netMonitor,
> Tools.storage,
> + Tools.performance,
Performance before netMonitor.
Attachment #8563038 -
Flags: review?(vporof) → review+
Reporter | ||
Comment 8•10 years ago
|
||
Fixed nits. We have an audit of localized strings in bug 1082695, but renaming this would cause all previous strings to have to be re-localized, I believe, so that's something we should investigate if its worth renaming or not
Attachment #8563038 -
Attachment is obsolete: true
Attachment #8563052 -
Flags: review+
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
For the record, these are the changesets that are currently on gum *BUT NOT* on fx-team:
1. Build Performance++ tool on gum only:
https://hg.mozilla.org/projects/gum/rev/70f69456c140
2. Enable performance tools by default, disable profiler and timeline tools/tests:
https://hg.mozilla.org/projects/gum/rev/a03794f349b5
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8563402 -
Flags: review?(jsantell)
Assignee | ||
Updated•10 years ago
|
Attachment #8563052 -
Attachment description: 1123815-turn-on-perf.patch do not land on fx-team yet → 1123815-turn-on-perf.patch [do not land on fx-team yet]
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8563405 -
Flags: review?(jsantell)
Reporter | ||
Updated•10 years ago
|
Attachment #8563405 -
Flags: review?(jsantell) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8563402 -
Flags: review?(jsantell) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Landed on gum. Current patches on gum but not fx-team:
1. Build Performance++ tool on gum only:
https://hg.mozilla.org/projects/gum/rev/70f69456c140
2. Enable performance tools by default, disable profiler and timeline tools/tests:
https://hg.mozilla.org/projects/gum/rev/a03794f349b5
3. Use `devtools.performance.enabled` instead of `devtools.profiler.enabled`
https://hg.mozilla.org/projects/gum/rev/9332647ea6ac
4. Use `performance` instead of `jsprofiler` as the tool id in some tests
https://hg.mozilla.org/projects/gum/rev/c7e286c5c6a4
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8563052 [details] [diff] [review]
1123815-turn-on-perf.patch [do not land on fx-team yet]
Marking as obsolete because bugzilla-todos is bugging me.
Attachment #8563052 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8563402 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8563405 -
Attachment is obsolete: true
Reporter | ||
Comment 15•10 years ago
|
||
Should note that those 3 patches should be ultimately landed on fx-team when we're ready (the initial patch with build flag is not needed as the subsequent patch removes that)
Attachment 8563052 [details] [diff]
Attachment 8563402 [details] [diff]
Attachment 8563405 [details] [diff]
Assignee | ||
Updated•10 years ago
|
Depends on: perf-tool-crashes
Assignee | ||
Updated•10 years ago
|
Summary: Enable Performance++ Tool → [meta] Enable Performance++ Tool
Assignee | ||
Updated•10 years ago
|
Depends on: perf-tool-tests
Assignee | ||
Updated•10 years ago
|
No longer depends on: perf-tool-crashes
Assignee | ||
Comment 16•10 years ago
|
||
For the record, here is the diff between fx-team and gum.
Comment 17•10 years ago
|
||
Hi! I want to try out this new performance tool. I have the latest nightly and I set "devtools.performance_dev.enabled;true" but the Performance tab shows "Firefox can't find the file at chrome://browser/content/devtools/performance.xul." What am I doing wrong? From what I can understand from this discussion, the new performance tool ca be used by meer mortals (like me) on a Nightly build :P
Assignee | ||
Comment 18•10 years ago
|
||
The new perf tool is currently behind a build flag, so you'll either need to build firefox yourself using `ac_add_options --enable-devtools-perf`, or grab a gum build from https://treeherder.mozilla.org/#/jobs?repo=gum. Alternatively, wait for another week or so and hopefully this will be in Nightly prefed on by default.
Comment 19•10 years ago
|
||
Thank you! I'll wait for a week :D
Assignee | ||
Comment 20•10 years ago
|
||
Gum is green. Let's land this.
Assignee | ||
Comment 21•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 23•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Comment 24•10 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: New performance tools
[Suggested wording]: Developer Tools : New performance tools
[Links (documentation, blog post, etc)]: Possibly hacks.mo.org in the future.
Updated•10 years ago
|
Flags: qe-verify+
Comment 25•10 years ago
|
||
(In reply to Tim Nguyen [:ntim] from comment #24)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: New performance tools
> [Suggested wording]: Developer Tools : New performance tools
> [Links (documentation, blog post, etc)]: Possibly hacks.mo.org in the future.
I think we want to wait until 40 to announce this so there is more time to bugfix, add features, etc.
relnote-firefox:
? → ---
Assignee | ||
Comment 26•10 years ago
|
||
If anything, the relnote should be fore bug 1075567
Assignee | ||
Comment 27•10 years ago
|
||
s/fore/for/
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•