Closed Bug 795268 Opened 12 years ago Closed 12 years ago

Integrate SPS Profiler (JS Only)


(DevTools :: General, defect, P2)



(Not tracked)

Firefox 20


(Reporter: rcampbell, Assigned: anton)



(Keywords: dev-doc-needed, Whiteboard: [fixed-in-fx-team])


(1 file, 7 obsolete files)

Let's ship a profiler!

Front end:

First crack should be JS only, high sample rate.
Attached patch patch 1 WIP (obsolete) — Splinter Review
Assignee: nobody → rcampbell
Attachment #665850 - Attachment description: patch 1 → patch 1 WIP
Attachment #665850 - Attachment is patch: true
Attached patch patch 2 WIP (obsolete) — Splinter Review
Attached patch patch 2.1 WIP (obsolete) — Splinter Review
updating local copy.
Attachment #665852 - Attachment is obsolete: true
(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. ( will have a fork of the addon with remote debugging protocol on monday)
Yes. We are basically rewriting the parts of the addon we need. We will make this remoteable.
Also note, we're using the "cleo-light" branch of cleopatra ( for this. We may want to pull periodically from this and push changes there.
I plan on merging that to the master branch once I removed duplicate files.
Attached patch patch 2.2 WIP (obsolete) — Splinter Review
added profiler.dtd and reference.
Attachment #667597 - Attachment is obsolete: true
new try run, hopefully fixed the build:
FYI: the progress on this bug is currently here here:
Assignee: rcampbell → anton
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
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.
Profiler pull request ( is ready to be code reviewed.
Since I pushed my patch to try for QA I thought I'd put it here as well.

Attachment #665850 - Attachment is obsolete: true
Attachment #670470 - Attachment is obsolete: true
Depends on: 788977
I tried the build on mac. It's work great and the UI is much nicer.
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?
(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...
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?
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.
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.
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.


@@ +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>

Blocks: 818749
Blocks: 818751
Rebased fx-team, fixed a few compatibility issues and addressed Rob's comments.
Attachment #685402 - Attachment is obsolete: true
Attachment #689084 - Flags: review?(rcampbell)
I have also created followup bugs for TODOs and FIXMEs.
Blocks: 820951
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=""

need to know where this came from.

::: browser/devtools/profiler/cleopatra/images/throbber.svg
@@ +1,1 @@
> +<svg  xmlns=""

need to know where this came from.

::: browser/devtools/profiler/cleopatra/images/treetwisty.svg
@@ +1,1 @@
> +<svg  xmlns=""

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 -->

yay! \o/

::: browser/devtools/profiler/test/head.js
@@ +65,5 @@
> +
> +    finish();
> +    Services.prefs.setBoolPref(PROFILER_ENABLED, false);
> +  });
> +}

looks nice.
Attachment #689084 - Flags: review?(rcampbell)
Fixed memory leaks (I was not closing remote connection properly). New try push:

Didn't put it up for review since we're waiting for BenWa to figure out licensing.
Attachment #689084 - Attachment is obsolete: true
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.
(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.
Yes, that's the plan. I'll land it soon.
Rebased from fx-team, fixed test failures caused by our switch to promises and added MPL blocks to all Cleopatra files (including SVG).

Attachment #691603 - Attachment is obsolete: true
Attachment #692096 - Flags: review?(rcampbell)
(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",
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.


just to see if there was anything funny about the previous push.
Attachment #692096 - Flags: review?(rcampbell) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Is it expected that three image files are empty?
filter.png, noise.png and showall.png in browser/devtools/profiler/cleopatra/images
Depends on: 822097
(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.
Depends on: 822287
Depends on: 822041
This should have been placed in /skin/:
/browser/devtools/profiler/cleopatra/css/ *

Created bug 823026 for this.
Blocks: 822110, 823026
Try run for 832f4a94fb7c is complete.
Detailed breakdown of the results available here:
Results (out of 319 total builds):
    exception: 1
    success: 289
    warnings: 26
    failure: 3
Builds (or logs if builds failed) available at:
Depends on: 824243
Depends on: 839525
Depends on: 858501
Depends on: 858507
Depends on: CVE-2013-1688
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.