Closed
Bug 795268
Opened 13 years ago
Closed 12 years ago
Integrate SPS Profiler (JS Only)
Categories
(DevTools :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 20
People
(Reporter: rcampbell, Assigned: anton)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 7 obsolete files)
206.93 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
Let's ship a profiler!
Front end: https://github.com/bgirard/cleopatra
Addon: https://github.com/bgirard/Gecko-Profiler-Addon
First crack should be JS only, high sample rate.
Reporter | ||
Comment 1•13 years ago
|
||
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Reporter | ||
Updated•13 years ago
|
Attachment #665850 -
Attachment description: patch 1 → patch 1 WIP
Attachment #665850 -
Attachment is patch: true
Reporter | ||
Comment 2•13 years ago
|
||
Reporter | ||
Comment 3•13 years ago
|
||
updating local copy.
Attachment #665852 -
Attachment is obsolete: true
Comment 4•13 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #1)
> Created attachment 665850 [details] [diff] [review]
> patch 1 WIP
Please make it use the remote debugging protocol instead of nsIProfiler directly. (https://github.com/glandium/Gecko-Profiler-Addon will have a fork of the addon with remote debugging protocol on monday)
Reporter | ||
Comment 5•13 years ago
|
||
Yes. We are basically rewriting the parts of the addon we need. We will make this remoteable.
Reporter | ||
Comment 6•13 years ago
|
||
Also note, we're using the "cleo-light" branch of cleopatra (https://github.com/bgirard/cleopatra/tree/cleo-light) for this. We may want to pull periodically from this and push changes there.
Comment 7•13 years ago
|
||
I plan on merging that to the master branch once I removed duplicate files.
Reporter | ||
Comment 8•13 years ago
|
||
added profiler.dtd and reference.
Attachment #667597 -
Attachment is obsolete: true
Reporter | ||
Comment 9•13 years ago
|
||
new try run, hopefully fixed the build:
https://tbpl.mozilla.org/?tree=Try&rev=12667944ee0b
Assignee | ||
Comment 10•13 years ago
|
||
FYI: the progress on this bug is currently here here: https://github.com/antonkovalyov/devtools-window
Assignee | ||
Updated•13 years ago
|
Assignee: rcampbell → anton
Assignee | ||
Comment 11•13 years ago
|
||
Code so far: https://github.com/joewalker/devtools-window/pull/88
Reporter | ||
Comment 12•12 years ago
|
||
lookin' good.
A couple of thoughts about UX:
First-opening, you're presented with a page to load profile data or parse from a text field. Not sure how useful this is going to be for users without the Addon. There's no way to export profile data from this version of the profiler so it's largely useless.
If we're just looking for something to fill the space, some instructional text explaining how to use the Profiler might be a good start. "Click the Start button to begin sampling JavaScript".
Also not keen on the toolbar with the single button. It takes up a bunch of space for a single use.
I see a couple of options to get rid of the toolbar:
1) Move the Start button into the Developer Toolbar (weird, will cause things to move and will disappear when the user changes tabs in the toolbox).
2) Move the Start button into the tab strip along side the other little candy-like buttons.
3) Create commands to start and stop the profiler and have that installed on the Tab strip area.
I'll take a look at the implementation tomorrow.
Priority: -- → P2
Assignee | ||
Comment 13•12 years ago
|
||
Check out another take on UI (same pull request). This one allows for multiple reports (not implemented yet) and different types of profiles (also not implemented).
Basic start/stop functionality still works.
Assignee | ||
Comment 14•12 years ago
|
||
Profiler pull request (https://github.com/joewalker/devtools-window/pull/121) is ready to be code reviewed.
Assignee | ||
Comment 15•12 years ago
|
||
Since I pushed my patch to try for QA I thought I'd put it here as well.
Build: https://tbpl.mozilla.org/?tree=Try&rev=7a2f82e7a006
Attachment #665850 -
Attachment is obsolete: true
Attachment #670470 -
Attachment is obsolete: true
Comment 16•12 years ago
|
||
I tried the build on mac. It's work great and the UI is much nicer.
Comment 17•12 years ago
|
||
Hmm, I also tried the Mac build, but all I see are the pseudo-stack entries and also some native frames. No JS frames anywhere in sight. Am I doing something wrong?
Comment 18•12 years ago
|
||
(In reply to comment #17)
> Hmm, I also tried the Mac build, but all I see are the pseudo-stack entries and
> also some native frames. No JS frames anywhere in sight. Am I doing something
> wrong?
Actually there are some, I was confused by the other frames visible in the output...
Reporter | ||
Comment 19•12 years ago
|
||
Benwa, Ehsan: I was talking to Anton about the native frames showing up as addresses and that we don't see those in the Addon version. Are we missing a setting or some cleopatra magic to either filter those our or annotate them differently?
Comment 20•12 years ago
|
||
The Addon has code to resolve the addresses to symbol. For devtools we should just filter those out. That code hasn't been written yet. Any label starting with '0x' is assumed to a hex encoded address and can be filtered out by cleopatra-light when parsing the profile.
Comment 21•12 years ago
|
||
Ehsan and I discussed today that we should avoid showing labels that are not meaningful via in the profiler and instead replace them with something like '(Gecko)' or '(Engine)'. And for some labels we should provide a link to a wikipage explaining why you're seeing this frame in the profile, what is being computed and how to reduce/avoid it. The aim there would be to teach users how the engine is working as they encounter problem related to it. I don't think any of this should gate landing this patch so I would rather discuss this at length in a follow-up bug but we should block shipping this on making the profiler friendlier.
Reporter | ||
Comment 22•12 years ago
|
||
Comment on attachment 685402 [details] [diff] [review]
JavaScript Profiler (needs devtools-window patch)
Review of attachment 685402 [details] [diff] [review]:
-----------------------------------------------------------------
This just needs some additional commenting in ProfilerController.jsm. Looks pretty good for starters! We'll either want to file bugs for TODOs/FIXMEs or remove them for landing.
Oh, and you need a StringBundle to take care of your l10n issue.
Clearing review request.
::: browser/devtools/framework/ToolDefinitions.jsm
@@ +11,5 @@
> +Cu.import("resource:///modules/WebConsolePanel.jsm");
> +Cu.import("resource:///modules/devtools/DebuggerPanel.jsm");
> +Cu.import("resource:///modules/devtools/StyleEditorDefinition.jsm");
> +Cu.import("resource:///modules/devtools/InspectorDefinition.jsm");
> +Cu.import("resource:///modules/devtools/ProfilerPanel.jsm");
do we want to import this if we're not using it? Could move it into the if statement below.
::: browser/devtools/profiler/ProfilerController.jsm
@@ +16,5 @@
> +XPCOMUtils.defineLazyGetter(this, "DebuggerServer", function () {
> + Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
> + return DebuggerServer;
> +});
> +
Should have comments for all constructors and methods in javadoc format. See, ../debugger/debugger-controller.js for example.
@@ +69,5 @@
> + }
> +};
> +
> +function ProfilerController() {
> + // TODO: Check if profile has already been started (by an addon for example).
do we need a bug for this as a followup? Typically TODOs come with follow-up bugs wherever possible.
::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +24,5 @@
> + id: "jsprofiler",
> + killswitch: "devtools.profiler.enabled",
> + icon: "chrome://browser/skin/devtools/tools-icons-small.png",
> + url: "chrome://browser/content/profiler.xul",
> + label: "Profiler", // FIXME: l10n
You should fix this prior to landing. You'll need a StringBundle and an entry.
@@ +29,5 @@
> +
> + isTargetSupported: function (target) {
> + if (target.isRemote || target.isChrome) {
> + return false;
> + }
presumably we can do remote and chrome targets in followups.
@@ +315,5 @@
> + */
> + startProfiling: function PP_startProfiling(onStart) {
> + this.controller.start(function (err) {
> + if (err) {
> + // FIXME: Error handling.
anything to do here or just making noises?
@@ +336,5 @@
> + */
> + stopProfiling: function PP_stopProfiling(onStop) {
> + this.controller.isActive(function (err, isActive) {
> + if (err) {
> + // FIXME: Error handling.
likewise. Do we need a bug or are we just happy with bailing out?
@@ +347,5 @@
> + }
> +
> + this.controller.stop(function (err, data) {
> + if (err) {
> + // FIXME: Error handling.
and...
@@ +366,5 @@
> + /**
> + * Cleanup.
> + */
> + destroy: function PP_destroy() {
> + // FIXME: Need an actualt destroy function that closes
what? :)
::: browser/devtools/profiler/profiler.xul
@@ +43,5 @@
> + -->
> + </vbox>
> + </box>
> + </box>
> +</window>
groovy.
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 23•12 years ago
|
||
Rebased fx-team, fixed a few compatibility issues and addressed Rob's comments.
Attachment #685402 -
Attachment is obsolete: true
Attachment #689084 -
Flags: review?(rcampbell)
Assignee | ||
Comment 24•12 years ago
|
||
I have also created followup bugs for TODOs and FIXMEs.
Assignee | ||
Comment 25•12 years ago
|
||
Reporter | ||
Comment 26•12 years ago
|
||
Comment on attachment 689084 [details] [diff] [review]
JavaScript Profiler, rebased and with comments
Review of attachment 689084 [details] [diff] [review]:
-----------------------------------------------------------------
almost done. Just need to figure out provenance on the .svg files in the cleopatra directory and get a license file or headers in place.
::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +141,5 @@
> + destroy: function PUI_destroy() {
> + this.isReady = null
> + this.panel = null;
> + this.uid = null;
> + this.iframe = null;
for the sake of completeness, might want to remove win and doc from here as well.
::: browser/devtools/profiler/cleopatra/cleopatra.html
@@ +1,1 @@
> +<!DOCTYPE html>
None of these files have any license blocks on them. I spoke to BenWa and they didn't really think much about it, but he's going to verify that the .svg files are kosher and add a license.txt file to the cleo-light branch of cleopatra. This should all be MPL.
::: browser/devtools/profiler/cleopatra/images/circlearrow.svg
@@ +1,1 @@
> +<svg xmlns="http://www.w3.org/2000/svg"
need to know where this came from.
::: browser/devtools/profiler/cleopatra/images/throbber.svg
@@ +1,1 @@
> +<svg xmlns="http://www.w3.org/2000/svg"
need to know where this came from.
::: browser/devtools/profiler/cleopatra/images/treetwisty.svg
@@ +1,1 @@
> +<svg xmlns="http://www.w3.org/2000/svg"
do we have any equivalent treetwisty graphics already in tree we could use instead? A quick mxr didn't find anything for all platforms.
IN any case, we're going to find out where these came from and see if we can use them.
::: browser/devtools/profiler/cleopatra/js/devtools.js
@@ +1,1 @@
> +var gInstanceUID;
is this a new file?
Should have an MPL license block on it.
::: browser/devtools/profiler/cleopatra/js/parser.js
@@ +1,1 @@
> +Array.prototype.clone = function() { return this.slice(0); }
wonderfully undocumented.
/cc BenWa ;)
::: browser/devtools/profiler/profiler.xul
@@ +1,5 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> + - License, v. 2.0. If a copy of the MPL was not distributed with this
> + - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
yay! \o/
::: browser/devtools/profiler/test/head.js
@@ +65,5 @@
> +
> + finish();
> + Services.prefs.setBoolPref(PROFILER_ENABLED, false);
> + });
> +}
looks nice.
Attachment #689084 -
Flags: review?(rcampbell)
Assignee | ||
Comment 27•12 years ago
|
||
Fixed memory leaks (I was not closing remote connection properly). New try push: https://tbpl.mozilla.org/?tree=Try&rev=832f4a94fb7c
Didn't put it up for review since we're waiting for BenWa to figure out licensing.
Attachment #689084 -
Attachment is obsolete: true
Comment 28•12 years ago
|
||
All the files in Cleopatra-light are going to get licensed MPL2. I've also fixed bug 820951 which should be merged soon and make the addresses go away.
Comment 29•12 years ago
|
||
(In reply to comment #28)
> All the files in Cleopatra-light are going to get licensed MPL2. I've also
> fixed bug 820951 which should be merged soon and make the addresses go away.
Benoit, we should probably add a LICENSE file to the cleopatra repo to help avoid this confusion in the future.
Comment 30•12 years ago
|
||
Yes, that's the plan. I'll land it soon.
Assignee | ||
Comment 31•12 years ago
|
||
Rebased from fx-team, fixed test failures caused by our switch to promises and added MPL blocks to all Cleopatra files (including SVG).
Try: https://tbpl.mozilla.org/?tree=Try&rev=e59164aecf31
Attachment #691603 -
Attachment is obsolete: true
Attachment #692096 -
Flags: review?(rcampbell)
Comment 32•12 years ago
|
||
(In reply to Anton Kovalyov (:anton) from comment #31)
+XPCOMUtils.defineLazyGetter(this, "DebuggerServer", function () {
+ Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
+ return DebuggerServer;
+});
Nit: you can use a LazyModuleGetter here:
XPCOMUtils.defineLazyModuleGetter(this, "DebuggerServer",
"resource://gre/modules/devtools/dbg-server.jsm");
Reporter | ||
Comment 33•12 years ago
|
||
yeah, that's true. Pretty much equivalent.
I just ran the entire set of tests from devtools on my machine and didn't get any failures. I know the try servers are special beasts, but not sure why they had these failures.
pushed https://tbpl.mozilla.org/?tree=Try&rev=7a317f298423
just to see if there was anything funny about the previous push.
Reporter | ||
Updated•12 years ago
|
Attachment #692096 -
Flags: review?(rcampbell) → review+
Reporter | ||
Comment 34•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Reporter | ||
Comment 35•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Comment 36•12 years ago
|
||
Is it expected that three image files are empty?
filter.png, noise.png and showall.png in browser/devtools/profiler/cleopatra/images
Reporter | ||
Comment 37•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #36)
> Is it expected that three image files are empty?
> filter.png, noise.png and showall.png in
> browser/devtools/profiler/cleopatra/images
probably not.
Filed bug 822110.
Comment 38•12 years ago
|
||
This should have been placed in /skin/:
chrome://browser/content/profiler.css
/browser/devtools/profiler/cleopatra/css/ *
etc...
Created bug 823026 for this.
Comment 39•12 years ago
|
||
Try run for 832f4a94fb7c is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=832f4a94fb7c
Results (out of 319 total builds):
exception: 1
success: 289
warnings: 26
failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/akovalyov@mozilla.com-832f4a94fb7c
Updated•12 years ago
|
Depends on: CVE-2013-1688
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•