Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Integrate SPS Profiler (JS Only)

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: rc, Assigned: anton)

Tracking

({dev-doc-needed})

unspecified
Firefox 20
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
Created attachment 665850 [details] [diff] [review]
patch 1 WIP
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Attachment #665850 - Attachment description: patch 1 → patch 1 WIP
Attachment #665850 - Attachment is patch: true
(Reporter)

Comment 2

5 years ago
Created attachment 665852 [details] [diff] [review]
patch 2 WIP
(Reporter)

Comment 3

5 years ago
Created attachment 667597 [details] [diff] [review]
patch 2.1 WIP

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. (https://github.com/glandium/Gecko-Profiler-Addon will have a fork of the addon with remote debugging protocol on monday)
(Reporter)

Comment 5

5 years ago
Yes. We are basically rewriting the parts of the addon we need. We will make this remoteable.
(Reporter)

Comment 6

5 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.
I plan on merging that to the master branch once I removed duplicate files.
(Reporter)

Comment 8

5 years ago
Created attachment 670470 [details] [diff] [review]
patch 2.2 WIP

added profiler.dtd and reference.
Attachment #667597 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
new try run, hopefully fixed the build:

https://tbpl.mozilla.org/?tree=Try&rev=12667944ee0b
(Assignee)

Comment 10

5 years ago
FYI: the progress on this bug is currently here here: https://github.com/antonkovalyov/devtools-window
(Assignee)

Updated

5 years ago
Assignee: rcampbell → anton
(Assignee)

Comment 11

5 years ago
Code so far: https://github.com/joewalker/devtools-window/pull/88
(Reporter)

Comment 12

5 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

5 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

5 years ago
Profiler pull request (https://github.com/joewalker/devtools-window/pull/121) is ready to be code reviewed.
(Assignee)

Comment 15

5 years ago
Created attachment 685402 [details] [diff] [review]
JavaScript Profiler (needs devtools-window patch)

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
(Assignee)

Updated

5 years ago
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...
(Reporter)

Comment 19

5 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?
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.
(Reporter)

Comment 22

5 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.
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 818749
(Assignee)

Updated

5 years ago
Blocks: 818751
(Assignee)

Comment 23

5 years ago
Created attachment 689084 [details] [diff] [review]
JavaScript Profiler, rebased and with comments

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

5 years ago
I have also created followup bugs for TODOs and FIXMEs.
(Assignee)

Comment 25

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=2d3c44d1ce48
(Reporter)

Updated

5 years ago
Blocks: 820951
(Reporter)

Comment 26

5 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

5 years ago
Created attachment 691603 [details] [diff] [review]
JavaScript Profiler, fixed memory leaks

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
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.
(Assignee)

Comment 31

5 years ago
Created attachment 692096 [details] [diff] [review]
JavaScript Profiler, rebased and with MPL blocks for Cleopatra

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

5 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

5 years ago
Attachment #692096 - Flags: review?(rcampbell) → review+
(Reporter)

Comment 34

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/548d2c909b81
Whiteboard: [fixed-in-fx-team]
(Reporter)

Comment 35

5 years ago
https://hg.mozilla.org/mozilla-central/rev/548d2c909b81
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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
(Reporter)

Comment 37

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

Updated

5 years ago
Depends on: 822287

Updated

5 years ago
Depends on: 822041

Comment 38

5 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.
Blocks: 822110, 823026

Comment 39

5 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

5 years ago
Depends on: 824243
Depends on: 839525
Depends on: 858501
Depends on: 858507
Depends on: 873966
You need to log in before you can comment on or make changes to this bug.