Closed Bug 1123815 (enable-perf-tool) Opened 5 years ago Closed 5 years ago

[meta] Enable Performance++ Tool

Categories

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

37 Branch
defect
Not set

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)

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`
* 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
Depends on: 1119023
Depends on: 1122058
Depends on: 1077464
Depends on: 1122662, 1077462
Just added dependency on bugs that I judged necessary to fix before enabling the new tool and before removing the current ones.
Feel free to remove/add some if you disagree though.
Depends on: 1121086
Depends on: 1128859
Depends on: 1129539
Depends on: 1101146
Summary: Enable Performance++ Tool → enable-perf-tool
Alias: enable-perf-tool
Summary: enable-perf-tool → Enable Performance++ Tool
Depends on: 1111020
Depends on: 1129960
Depends on: 1129187
No longer depends on: 1110550
Depends on: 1130054
Depends on: 1077476
Depends on: 1130204
Depends on: 1130202
Depends on: 1130200
Depends on: 1107949
Depends on: 1130274
Depends on: 1130589
Depends on: 1130669
Depends on: 1130671
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)
Depends on: 1131577
Depends on: 1131615
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)
Depends on: 1132206
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: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8563038 - Flags: review?(vporof)
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+
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+
No longer depends on: 1131615
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
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]
Attachment #8563405 - Flags: review?(jsantell) → review+
Attachment #8563402 - Flags: review?(jsantell) → review+
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
Depends on: 1132525
Depends on: 1132713
Depends on: 1082695
Depends on: 1132755
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
Attachment #8563402 - Attachment is obsolete: true
Attachment #8563405 - Attachment is obsolete: true
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]
Depends on: 1133054
Depends on: 1133129
Depends on: 1132370
Depends on: 1133230
Depends on: perf-tool-crashes
Summary: Enable Performance++ Tool → [meta] Enable Performance++ Tool
Depends on: 1133791
Depends on: 1133793
Depends on: 1133796
Depends on: 1133800
No longer depends on: 1129960
No longer depends on: 1130669
No longer depends on: 1132206
No longer depends on: 1132370
Depends on: 1132370
No longer depends on: 1133129
No longer depends on: 1133230
No longer depends on: 1133791
No longer depends on: 1133793
No longer depends on: 1133796
No longer depends on: 1133800
Depends on: perf-tool-tests
No longer depends on: perf-tool-crashes
No longer depends on: 1132370
No longer depends on: 1130054
No longer depends on: 1133054
Blocks: 1136105
No longer depends on: 1132525
Depends on: 1121194
No longer blocks: 1136105
Depends on: 1141313
Depends on: 1141817
No longer depends on: 1122058
No longer depends on: 1082695
No longer depends on: 1077462
For the record, here is the diff between fx-team and gum.
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
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.
Thank you! I'll wait for a week :D
Gum is green. Let's land this.
\o/
Assignee: jsantell → vporof
https://hg.mozilla.org/mozilla-central/rev/2ae4dac8f094
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
No longer depends on: 1130589
No longer depends on: 1122662
No longer depends on: 1121086
No longer depends on: 1077464
No longer depends on: 1066504
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.
relnote-firefox: --- → ?
Flags: qe-verify+
OS: Mac OS X → All
Hardware: x86 → All
Flags: qe-verify+
(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: ? → ---
If anything, the relnote should be fore bug 1075567
s/fore/for/
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.