795268 - Integrate SPS Profiler (JS Only)

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Rob Campbell [:rc] (:robcee)]

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.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Attachment: patch 1 WIP

Comment 2

[Rob Campbell [:rc] (:robcee)]

Attachment: patch 2 WIP

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: patch 2.1 WIP

updating local copy.

Comment 4

[Mike Hommey [:glandium]]

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

Comment 5

[Rob Campbell [:rc] (:robcee)]

Yes. We are basically rewriting the parts of the addon we need. We will make this remoteable.

Comment 6

[Rob Campbell [:rc] (:robcee)]

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

[Benoit Girard (:BenWa)]

I plan on merging that to the master branch once I removed duplicate files.

Comment 8

[Rob Campbell [:rc] (:robcee)]

Attachment: patch 2.2 WIP

added profiler.dtd and reference.

Comment 9

[Rob Campbell [:rc] (:robcee)]

new try run, hopefully fixed the build:

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

Comment 10

[Anton Kovalyov (:anton)]

FYI: the progress on this bug is currently here here: https://github.com/antonkovalyov/devtools-window

Comment 11

[Anton Kovalyov (:anton)]

Code so far: https://github.com/joewalker/devtools-window/pull/88

Comment 12

[Rob Campbell [:rc] (:robcee)]

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.

Comment 13

[Anton Kovalyov (:anton)]

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.

Comment 14

[Anton Kovalyov (:anton)]

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

Comment 15

[Anton Kovalyov (:anton)]

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

Comment 16

[Benoit Girard (:BenWa)]

I tried the build on mac. It's work great and the UI is much nicer.

Comment 17

[(no longer active)]

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

[(no longer active)]

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

Comment 19

[Rob Campbell [:rc] (:robcee)]

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

[Benoit Girard (:BenWa)]

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

[Benoit Girard (:BenWa)]

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 22

[Rob Campbell [:rc] (:robcee)]

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.

Comment 23

[Anton Kovalyov (:anton)]

Attachment: JavaScript Profiler, rebased and with comments

Rebased fx-team, fixed a few compatibility issues and addressed Rob's comments.

Comment 24

[Anton Kovalyov (:anton)]

I have also created followup bugs for TODOs and FIXMEs.

Comment 25

[Anton Kovalyov (:anton)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=2d3c44d1ce48

Comment 26

[Rob Campbell [:rc] (:robcee)]

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.

Comment 27

[Anton Kovalyov (:anton)]

Attachment: 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.

Comment 28

[Benoit Girard (:BenWa)]

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

[(no longer active)]

(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

[Benoit Girard (:BenWa)]

Yes, that's the plan. I'll land it soon.

Comment 31

[Anton Kovalyov (:anton)]

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

Comment 32

[Victor Porof [:vporof][:vp]]

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

Comment 33

[Rob Campbell [:rc] (:robcee)]

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.

Comment 34

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/548d2c909b81

Comment 35

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/548d2c909b81

Comment 36

[Mike Hommey [:glandium]]

Is it expected that three image files are empty?
filter.png, noise.png and showall.png in browser/devtools/profiler/cleopatra/images

Comment 37

[Rob Campbell [:rc] (:robcee)]

(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

[Alfred Kayser]

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

[Mozilla RelEng Bot]

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/[e-mail]-832f4a94fb7c