Last Comment Bug 751034 - Support remote profiling Via Remote debugging protocol
: Support remote profiling Via Remote debugging protocol
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on: 753401 781515 789114
Blocks: 809859 792855 793672
  Show dependency treegraph
 
Reported: 2012-05-01 21:40 PDT by Benoit Girard (:BenWa)
Modified: 2012-11-08 06:55 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (3.78 KB, patch)
2012-05-01 21:43 PDT, Benoit Girard (:BenWa)
past: feedback+
Details | Diff | Splinter Review
patch (6.98 KB, patch)
2012-05-02 13:06 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
updated patch against the new API (3.51 KB, patch)
2012-08-08 07:19 PDT, Panos Astithas [:past]
b56girard: feedback+
Details | Diff | Splinter Review
protocol log (842 bytes, text/plain)
2012-08-08 07:21 PDT, Panos Astithas [:past]
no flags Details
Patch v3 (3.62 KB, patch)
2012-08-09 04:27 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review
Patch v4 (3.82 KB, patch)
2012-09-06 07:16 PDT, Panos Astithas [:past]
mh+mozilla: review+
Details | Diff | Splinter Review
part 2 - Add profiler actor when adding the browser actors (986 bytes, patch)
2012-09-12 15:12 PDT, Mike Hommey [:glandium]
past: review+
Details | Diff | Splinter Review
part 3 - Send notifications through remote debugging protocol when the profiler starts/stops (2.95 KB, patch)
2012-09-12 15:14 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Support remote profiling Via Remote debugging protocol. (3.82 KB, patch)
2012-09-13 02:57 PDT, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
part 2 - Add profiler actor when adding the browser actors (1018 bytes, patch)
2012-09-13 05:40 PDT, Mike Hommey [:glandium]
past: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-05-01 21:40:36 PDT
The aim is to control the profiler module using the Remote debugging protocol to support profiling Fennec, B2G and possibly desktop remotely.

https://wiki.mozilla.org/Remote_Debugging_Protocol
Comment 1 Benoit Girard (:BenWa) 2012-05-01 21:42:23 PDT
WIP:
Profiling add-on: https://github.com/bgirard/Gecko-Profiler-Addon/commit/9376ed7b0663380cbc819e241d4abba17a93f7b4
Gecko changes: See attachment

I have basic communication with the root actor working. I need to expose a profiler actor that will wrap the functionality in the profiler module.
Comment 2 Benoit Girard (:BenWa) 2012-05-01 21:43:37 PDT
Created attachment 620187 [details] [diff] [review]
WIP

I'm not sure how to hook in my actor in 'dbg-server.js' without being invasive. What do you think of this?
Comment 3 Panos Astithas [:past] 2012-05-02 04:53:14 PDT
(In reply to Benoit Girard (:BenWa) from comment #2)
> I'm not sure how to hook in my actor in 'dbg-server.js' without being
> invasive. What do you think of this?

I think the best way to hook your actors is following the example extension in:

https://github.com/campd/sample-debug-actors

The most relevant bits are here:

https://github.com/campd/sample-debug-actors/blob/master/chrome/content/sample-actors.js

which basically boils down to call DebuggerServer.addTabRequest("actorName", actorHandler). That way you don't need to mess with anything else, besides your own actor. The comments in that file explain various other bits of how an actor is structured for cleanup, etc.

Also, see overlay.js in that repo for how the startup is done, which pretty much matches what you already do.
Comment 4 Panos Astithas [:past] 2012-05-02 09:26:58 PDT
Comment on attachment 620187 [details] [diff] [review]
WIP

Review of attachment 620187 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed in IRC, I agree that this is a fine way to do it, if you want to have the profiler always on, and not as an extension. One minor thing to note, is that you don't have to put the actors in a module, as we generally load them inside the debugger server module.
Comment 5 Benoit Girard (:BenWa) 2012-05-02 10:37:37 PDT
If I change it to profiler-actors.js I'm not sure where to what url to use if it's not a module and how to include it in the Makefile.
Comment 6 Benoit Girard (:BenWa) 2012-05-02 13:06:15 PDT
Created attachment 620442 [details] [diff] [review]
patch

I know loading the JSM is wrong. Not sure how to get a js file to be picked up in tools/profiler and what URL to use there.

Other then that this patch should be all we need. The rest of the changes should be done in the client add-on.
Comment 7 Jim Blandy :jimb 2012-05-03 23:01:14 PDT
Comment on attachment 620442 [details] [diff] [review]
patch

Review of attachment 620442 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +134,5 @@
> +    try {
> +      // If we disable the profiler on non Tier-1 platforms we wont
> +      // find this script.
> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
> +    } catch(e) {}

Isn't this going to mask all kinds of problems loading the file: syntax errors, etc.? If so... well, let me just say that eating errors is one thing that will get me really upset. :)
Comment 8 Benoit Girard (:BenWa) 2012-05-04 07:43:07 PDT
Well I figured worse case it would disable the profiler actor if there was a syntax error and not impact the dbg-server.

Maybe I can look for a better way to check at runtime if that script is present.
Comment 9 Dave Camp (:dcamp) 2012-05-04 08:54:24 PDT
Comment on attachment 620442 [details] [diff] [review]
patch

I'm going to pass the review over to Panos, but if Jim wants to take it that would be fine too.
Comment 10 Jan Honza Odvarko [:Honza] 2012-05-07 03:03:41 PDT
What about having a 'DebuggerServer.addRequest' API that would allow to dynamically extend the 'BrowserRootActor.prototype.requestTypes' array (also from within an extension). This way we don't need to change dbg-browser-actors.js every time a new actor (a child of the root) appears.

DebuggerServer.addRequest = function DS_addRequest(aName, aFunction) {
  DebuggerServer.BrowserRootActor.prototype.requestTypes[aName] = function(aRequest) {
    return aFunction(this, aRequest);
  }
};

This would be similar to what is there for tab actors: 'DebuggerServer.addTabRequest'

The function could live in dbg-browser-actors.js 

Honza
Comment 11 Panos Astithas [:past] 2012-05-07 10:47:49 PDT
Comment on attachment 620442 [details] [diff] [review]
patch

Review of attachment 620442 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Benoit Girard (:BenWa) from comment #5)
> If I change it to profiler-actors.js I'm not sure where to what url to use
> if it's not a module and how to include it in the Makefile.

I see the native part gets put in the components folder, maybe you can register the JS part as a component, too?

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +206,5 @@
>   * The request types this actor can handle.
>   */
>  BrowserRootActor.prototype.requestTypes = {
> +  "listTabs": BrowserRootActor.prototype.onListTabs,
> +  "getProfiler": BrowserRootActor.prototype.onGetProfiler

I don't see onGetProfiler defined in this patch. Perhaps you forgot to hg qref?

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +134,5 @@
> +    try {
> +      // If we disable the profiler on non Tier-1 platforms we wont
> +      // find this script.
> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
> +    } catch(e) {}

I share Jim's concern. One way to check for the existence of the actors is like this:

FileUtils.getFile("CurProcD", "profiler-actors.jsm", true).exists()

@@ +245,5 @@
>  
>      // Create a root actor for the connection and send the hello packet.
>      conn.rootActor = this.createRootActor(conn);
>      conn.addActor(conn.rootActor);
> +    if (this.createProfilerActor()) {

You don't want to invoke the function here, just check for its presence.

@@ +246,5 @@
>      // Create a root actor for the connection and send the hello packet.
>      conn.rootActor = this.createRootActor(conn);
>      conn.addActor(conn.rootActor);
> +    if (this.createProfilerActor()) {
> +      conn.addActor(this.createProfilerActor());

If you are going to need an extension to use the profiler anyway, then I think Honza's proposal in comment 10 is better than hard-wiring the profile actor here. Jim, what do you think?

Unless I got this backwards and you don't need the extension for recording the profile. In that case there would have to be some way to start the server and register the actors. What would the UI be in that case?

::: tools/profiler/profiler-actors.jsm
@@ +53,5 @@
> + */
> +function ProfilerActor()
> +{
> +  this.conn = null; // set by ActorPool.actor
> +  this.profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);

We don't hard-wire these things in other actors, just pass the connection as a parameter to the constructor. And if you need to avoid creating multiple nsIProfiler instances, you can store a reference to the profiler actor in the browser root actor (under _profilerActor or something) and reuse it.

@@ +58,5 @@
> +}
> +
> +ProfilerActor.prototype = {
> +  actorPrefix: "profiler",
> +  actorID: "profiler",

With the above you don't need to hard-wire the actorID, just let the pool assign one and the client can get it from the getProfiler protocol response.

@@ +79,5 @@
> +    return { "profile": profile }    
> +  },
> +  onIsActive: function(aRequest) {
> +    var isActive = this.profiler.IsActive();
> +    return { "isActive": isActice }    

isActice -> isActive

@@ +106,5 @@
> +  "getProfile": ProfilerActor.prototype.onGetProfile,
> +  "isActive": ProfilerActor.prototype.onIsActive,
> +  "getResponsivenessTimes": ProfilerActor.prototype.onGetResponsivenessTimes,
> +  "getFeatures": ProfilerActor.prototype.onGetFeatures,
> +  "getSharedLibraryInformation": ProfilerActor.prototype.onGetSharedLibraryInformation

I can't provide feedback on the profiler protocol bits, as I haven't tested the extension yet. I'll get back to you on this after I've had a chance to play with it first.
Comment 12 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-08 01:00:42 PDT
>::: toolkit/devtools/debugger/server/dbg-server.js
>@@ +134,5 @@
>> +    try {
>> +      // If we disable the profiler on non Tier-1 platforms we wont
>> +      // find this script.
>> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
>> +    } catch(e) {}
>
>I share Jim's concern. One way to check for the existence of the actors is like this:
>
>FileUtils.getFile("CurProcD", "profiler-actors.jsm", true).exists()

On the other hand, the exists() check is extra IO on the main thread, and we have been specifically stamping out exists/open pairs in the rest of the codebase.
Comment 13 Panos Astithas [:past] 2012-05-08 02:50:38 PDT
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #12)
> >::: toolkit/devtools/debugger/server/dbg-server.js
> >@@ +134,5 @@
> >> +    try {
> >> +      // If we disable the profiler on non Tier-1 platforms we wont
> >> +      // find this script.
> >> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
> >> +    } catch(e) {}
> >
> >I share Jim's concern. One way to check for the existence of the actors is like this:
> >
> >FileUtils.getFile("CurProcD", "profiler-actors.jsm", true).exists()
> 
> On the other hand, the exists() check is extra IO on the main thread, and we
> have been specifically stamping out exists/open pairs in the rest of the
> codebase.

That is a good point. In that case the extension API described in comment 10 becomes even more attractive. Alternatively, we can provide a stub actor implementation for non Tier-1 platforms, or a generic base implementation with hooks in there for platform-specific bits.
Comment 14 Panos Astithas [:past] 2012-05-08 04:31:11 PDT
(In reply to Panos Astithas [:past] from comment #11)
> @@ +106,5 @@
> > +  "getProfile": ProfilerActor.prototype.onGetProfile,
> > +  "isActive": ProfilerActor.prototype.onIsActive,
> > +  "getResponsivenessTimes": ProfilerActor.prototype.onGetResponsivenessTimes,
> > +  "getFeatures": ProfilerActor.prototype.onGetFeatures,
> > +  "getSharedLibraryInformation": ProfilerActor.prototype.onGetSharedLibraryInformation
> 
> I can't provide feedback on the profiler protocol bits, as I haven't tested
> the extension yet. I'll get back to you on this after I've had a chance to
> play with it first.

I played with the extension a bit and I have some feedback on the protocol. Great visualization, btw!

I put the packet exchange that I observed in a pastebin for reference:

http://past.pastebin.mozilla.org/1625237

My first observation is that only one method seems to be working at the moment, 'getProfileStr'. I don't know if I had to do something different to trigger 'getResponsivenessTimes', 'getFeatures', etc.

The initial getProfiler actor lookup request is missing, as discussed previously. With that, the client can get the actor ID to sent its followup requests to, without having to hard-code the name 'profiler'.

The data exchanged via getProfileStr is a big opaque string. If you take a look at the remote protocol specification, the common practice is to break down the data in objects and properties. I think that the form of the data you are exchanging maps nicely to that, although I haven't dived deeply into the specifics. What kind of data is being exchanged through 'getProfile'?

'startProfiler'/'stopProfiler' don't seem to be triggered by the respective buttons. The same is true for 'getSharedLibraryInformation' although the client appears to take advantage of the fact that it's running in the same process as the server to find them?

That's all I have so far, I'll gladly take another pass if you can tell me how to trigger the other request handlers.
Comment 15 Jan Honza Odvarko [:Honza] 2012-05-08 04:45:07 PDT
(In reply to Panos Astithas [:past] from comment #13)
> That is a good point. In that case the extension API described in comment 10
> becomes even more attractive. Alternatively, we can provide a stub actor
> implementation for non Tier-1 platforms, or a generic base implementation
> with hooks in there for platform-specific bits.
Should we have another bug reported for DebuggerServer.addRequest or we discuss & commit as part of this bug...?

Honza
Comment 16 Benoit Girard (:BenWa) 2012-05-08 07:13:51 PDT
(In reply to Panos Astithas [:past] from comment #14)
> I played with the extension a bit and I have some feedback on the protocol.
> Great visualization, btw!

Cool, you might of missed this but it's possible to focus on sub ranges while the browser is non responsive which is the primary feature of this profiler, catching short jank.

> 
> I put the packet exchange that I observed in a pastebin for reference:
> 
> http://past.pastebin.mozilla.org/1625237
> 
> My first observation is that only one method seems to be working at the
> moment, 'getProfileStr'. I don't know if I had to do something different to
> trigger 'getResponsivenessTimes', 'getFeatures', etc.

This is current just a test. Down the road all the calls would be used to select profiling features, set sampling rates and get data to graph responsiveness.

> 
> The initial getProfiler actor lookup request is missing, as discussed
> previously. With that, the client can get the actor ID to sent its followup
> requests to, without having to hard-code the name 'profiler'.
> 
> The data exchanged via getProfileStr is a big opaque string. If you take a
> look at the remote protocol specification, the common practice is to break
> down the data in objects and properties. I think that the form of the data
> you are exchanging maps nicely to that, although I haven't dived deeply into
> the specifics. What kind of data is being exchanged through 'getProfile'?

This is exactly the 'getProfile' format. getProfileStr is our old format that I'm keeping around as I switch all the components to use the JSON format.

> 
> 'startProfiler'/'stopProfiler' don't seem to be triggered by the respective
> buttons. The same is true for 'getSharedLibraryInformation' although the
> client appears to take advantage of the fact that it's running in the same
> process as the server to find them?

The addon was only sending a few test messages over the protocol. Once this land I will add a 'remote' mode where you will give it connection information to a device such as Fennec/B2G and data will be exchanged over the protocol.

(In reply to Jan Honza Odvarko from comment #15)
> Should we have another bug reported for DebuggerServer.addRequest or we
> discuss & commit as part of this bug...?

Filed bug 752896
Comment 17 Jim Blandy :jimb 2012-05-09 11:25:53 PDT
(In reply to Benoit Girard (:BenWa) from comment #8)
> Maybe I can look for a better way to check at runtime if that script is
> present.

The catch clause should at least call Cu.reportError.
Comment 18 Jim Blandy :jimb 2012-05-09 11:28:07 PDT
(In reply to Panos Astithas [:past] from comment #11)
> If you are going to need an extension to use the profiler anyway, then I
> think Honza's proposal in comment 10 is better than hard-wiring the profile
> actor here. Jim, what do you think?

Yeah, I agree. Making people patch the root actor every time they want to add new functionality seems like it would be discouraging.
Comment 19 Jim Blandy :jimb 2012-05-09 11:28:28 PDT
(In reply to Jim Blandy :jimb from comment #18)
> Yeah, I agree. Making people patch the root actor every time they want to
> add new functionality seems like it would be discouraging.

... and it doesn't work for add-ons.
Comment 20 Benoit Girard (:BenWa) 2012-07-25 13:30:04 PDT
This bug is still depending on some architectural changes in the debug protocol but I can't find the bug, does someone know where it is.

Our current signal scheme is being stretched to its limits and is caused very query users interactions and wont scale to multi-process on b2g so this is blocking several profiling improvements.
Comment 21 Panos Astithas [:past] 2012-07-31 06:21:11 PDT
(In reply to Benoit Girard (:BenWa) from comment #20)
> This bug is still depending on some architectural changes in the debug
> protocol but I can't find the bug, does someone know where it is.
> 
> Our current signal scheme is being stretched to its limits and is caused
> very query users interactions and wont scale to multi-process on b2g so this
> is blocking several profiling improvements.

I'll get back to bug 753401 in the next few days.
Comment 22 Panos Astithas [:past] 2012-08-08 07:19:00 PDT
Created attachment 650112 [details] [diff] [review]
updated patch against the new API

So after the changes in bug 753401, the only required bit is the profiler actor definition. I've moved it next to the other remote debugging protocol actor definitions, since this is where the web console actors are going to land for now. If at some point we decide to reconsider, we can move them somewhere else.

The changes in the profiler actor are minimal, a "disconnect" method, no actorID hard-coding, and minor cleanups. This patch combined with some changes in the add-on client (for which a pull request will be forthcoming), result in the same functionality as in the previous patch. Benoit, let me know what you think, and then we can ask another devtools peer to review it, so we can land this.
Comment 23 Panos Astithas [:past] 2012-08-08 07:21:35 PDT
Created attachment 650114 [details]
protocol log

A log of the protocol interactions for reference, since my previous pastebin has expired.
Comment 24 Panos Astithas [:past] 2012-08-08 07:35:14 PDT
(In reply to Panos Astithas [:past] from comment #22)
> This patch combined with some
> changes in the add-on client (for which a pull request will be forthcoming),
> result in the same functionality as in the previous patch.

https://github.com/bgirard/Gecko-Profiler-Addon/pull/19
Comment 25 Benoit Girard (:BenWa) 2012-08-08 07:43:22 PDT
Comment on attachment 650112 [details] [diff] [review]
updated patch against the new API

Review of attachment 650112 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +19,5 @@
> +  actorPrefix: "profiler",
> +
> +  disconnect: function() {
> +    if (this.profiler) {
> +      this.profiler.StopProfiler();

I'm not sure if stopping the profiler on disconnect is the right thing to do. Will this get called if you have multiple connections open and one is closed? Also other things can be controlling the profiler (console.profile, c++) and since we only have a global profiling instance I think stopping the profiler on a disconnect may be very confusing if you're controlling profiling via another method and not expecting it to stop if doing an unrelated action (using the debugger).
Comment 26 Panos Astithas [:past] 2012-08-08 07:59:28 PDT
(In reply to Benoit Girard (:BenWa) from comment #25)
> I'm not sure if stopping the profiler on disconnect is the right thing to
> do. Will this get called if you have multiple connections open and one is
> closed? Also other things can be controlling the profiler (console.profile,
> c++) and since we only have a global profiling instance I think stopping the
> profiler on a disconnect may be very confusing if you're controlling
> profiling via another method and not expecting it to stop if doing an
> unrelated action (using the debugger).

I wasn't sure either. disconnect is called when the actor is removed from the pool, so it's the place for any cleanup activities necessary. I'm not clear on the profiler lifecycle, so if there is nothing that needs to be done here, even better.

I assume that you want to start the sampling/data-collection process as soon as the browser starts and maybe stop it when the browser ends. The debugger server however seems to start too soon as it is now in the add-on, and I think it would be best to start it when the user initiates some sort of interaction that should result in results being shown. The lifecycle of the protocol actors doesn't necessarily have to be the same as the lifecycle of the sampling components. I know the client is just an early prototype, but I just wanted to throw that out there.
Comment 27 Benoit Girard (:BenWa) 2012-08-08 08:07:30 PDT
(In reply to Panos Astithas [:past] from comment #26)
> (In reply to Benoit Girard (:BenWa) from comment #25)
> > I'm not sure if stopping the profiler on disconnect is the right thing to
> > do. Will this get called if you have multiple connections open and one is
> > closed? Also other things can be controlling the profiler (console.profile,
> > c++) and since we only have a global profiling instance I think stopping the
> > profiler on a disconnect may be very confusing if you're controlling
> > profiling via another method and not expecting it to stop if doing an
> > unrelated action (using the debugger).
> 
> I wasn't sure either. disconnect is called when the actor is removed from
> the pool, so it's the place for any cleanup activities necessary. I'm not
> clear on the profiler lifecycle, so if there is nothing that needs to be
> done here, even better.

It might make sense to track if the profiler was started using the debug protocol (startProfiler) using a bool and if so then we stop it on disconnect. It's a bit harry if something else is reading but we don't really support that use case anyways.

> 
> I assume that you want to start the sampling/data-collection process as soon
> as the browser starts and maybe stop it when the browser ends. The debugger
> server however seems to start too soon as it is now in the add-on, and I
> think it would be best to start it when the user initiates some sort of
> interaction that should result in results being shown. The lifecycle of the
> protocol actors doesn't necessarily have to be the same as the lifecycle of
> the sampling components. I know the client is just an early prototype, but I
> just wanted to throw that out there.

We have an environment variable MOZ_PROFILER_STARTUP that will start the profiler as early as possible (XRE_main). If users are interested in sampling startup they can start the browser with this variable (or force it using a patch) and then attach with the protocol to dump the profile once the browser is started.
Comment 28 Panos Astithas [:past] 2012-08-09 04:27:12 PDT
Created attachment 650504 [details] [diff] [review]
Patch v3

(In reply to Benoit Girard (:BenWa) from comment #27)
> (In reply to Panos Astithas [:past] from comment #26)
> > (In reply to Benoit Girard (:BenWa) from comment #25)
> > > I'm not sure if stopping the profiler on disconnect is the right thing to
> > > do. Will this get called if you have multiple connections open and one is
> > > closed? Also other things can be controlling the profiler (console.profile,
> > > c++) and since we only have a global profiling instance I think stopping the
> > > profiler on a disconnect may be very confusing if you're controlling
> > > profiling via another method and not expecting it to stop if doing an
> > > unrelated action (using the debugger).
> > 
> > I wasn't sure either. disconnect is called when the actor is removed from
> > the pool, so it's the place for any cleanup activities necessary. I'm not
> > clear on the profiler lifecycle, so if there is nothing that needs to be
> > done here, even better.
> 
> It might make sense to track if the profiler was started using the debug
> protocol (startProfiler) using a bool and if so then we stop it on
> disconnect. It's a bit harry if something else is reading but we don't
> really support that use case anyways.

Did that, and also made the profiler reference private. Requesting review might be a bit premature, since bug 753401 hasn't landed yet, but it shouldn't hurt.
Comment 29 Rob Campbell [:rc] (:robcee) 2012-08-10 08:55:56 PDT
Comment on attachment 650504 [details] [diff] [review]
Patch v3

yeah, this looks fine to me.
Comment 30 Benoit Girard (:BenWa) 2012-08-14 08:53:17 PDT
I'm working on adding a JS Source view to the profiler. I think for this to work properly we will need to fetch the document from the target since some script may be generated conditionally based on the session and UA. We should add something to the profiling protocol to fetch a document and return the source.
Comment 31 Dave Camp (:dcamp) 2012-08-14 11:39:25 PDT
See bug 772119 and bug 755661
Comment 32 Mike Hommey [:glandium] 2012-09-06 06:10:31 PDT
The patch needs to be updated to remove the Ci and Cc declarations:
TypeError: redeclaration of var Ci - @chrome://global/content/devtools/dbg-profiler-actors.js:6
Comment 33 Panos Astithas [:past] 2012-09-06 07:16:28 PDT
Created attachment 658871 [details] [diff] [review]
Patch v4

(In reply to Mike Hommey [:glandium] from comment #32)
> The patch needs to be updated to remove the Ci and Cc declarations:
> TypeError: redeclaration of var Ci -
> @chrome://global/content/devtools/dbg-profiler-actors.js:6

Done.
Comment 34 Mike Hommey [:glandium] 2012-09-12 15:08:29 PDT
Comment on attachment 658871 [details] [diff] [review]
Patch v4

Carrying over r+, since the only change was removing Cc and Ci definitions.
Comment 35 Mike Hommey [:glandium] 2012-09-12 15:12:13 PDT
Created attachment 660588 [details] [diff] [review]
part 2 - Add profiler actor when adding the browser actors
Comment 36 Mike Hommey [:glandium] 2012-09-12 15:14:31 PDT
Created attachment 660590 [details] [diff] [review]
part 3 - Send notifications through remote debugging protocol when the profiler starts/stops
Comment 37 Mike Hommey [:glandium] 2012-09-13 02:57:16 PDT
Created attachment 660769 [details] [diff] [review]
Support remote profiling Via Remote debugging protocol.

The arguments to StartProfiler were in the wrong order.
Comment 38 Mike Hommey [:glandium] 2012-09-13 02:58:37 PDT
Comment on attachment 660769 [details] [diff] [review]
Support remote profiling Via Remote debugging protocol.

Carrying over r+
Comment 39 Panos Astithas [:past] 2012-09-13 03:34:53 PDT
Comment on attachment 660588 [details] [diff] [review]
part 2 - Add profiler actor when adding the browser actors

Review of attachment 660588 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine as long as you are sure that all products (incl. Fennec and B2G) contain nsIProfiler and won't throw here.

There are also people looking into getting the remote protocol to work for Thunderbird. Is nsIProfiler present there, too?
Comment 40 Panos Astithas [:past] 2012-09-13 04:43:55 PDT
Comment on attachment 660590 [details] [diff] [review]
part 3 - Send notifications through remote debugging protocol when the profiler starts/stops

Review of attachment 660590 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +166,5 @@
>  const UnsolicitedNotifications = {
>    "newScript": "newScript",
>    "tabDetached": "tabDetached",
> +  "tabNavigated": "tabNavigated",
> +  "profilerStateChanged": "profilerStateChanged"

Nit: we keep this list sorted.

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +12,2 @@
>  {
> +  this._conn = aConnection;

If you don't use the connection in the constructor you don't need this, because one is patched to it when it is added to the actor pool. This is why this.conn.send in the observe method works, even though you store a _conn here.

@@ +14,4 @@
>    this._profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
>    this._started = false;
> +
> +  Cu.import("resource://gre/modules/Services.jsm");

This is redundant, since Services is already imported by dbg-server.js.

@@ +15,5 @@
>    this._started = false;
> +
> +  Cu.import("resource://gre/modules/Services.jsm");
> +  Services.obs.addObserver(this, "profiler-started", false);
> +  Services.obs.addObserver(this, "profiler-stopped", false);

Since the protocol provides operations to start and stop the profiler, what is the intended use for these noticications? Isn't the client controlling the profiler lifecycle?
Comment 41 Mike Hommey [:glandium] 2012-09-13 05:05:14 PDT
(In reply to Panos Astithas [:past] from comment #40)
> Since the protocol provides operations to start and stop the profiler, what
> is the intended use for these noticications? Isn't the client controlling
> the profiler lifecycle?

It does, but it's also possible it's not alone doing so.
Comment 42 Mike Hommey [:glandium] 2012-09-13 05:40:47 PDT
Created attachment 660812 [details] [diff] [review]
part 2 - Add profiler actor when adding the browser actors

This avoids registering the actor when nsIProfiler is not available.

Note You need to log in before you can comment on or make changes to this bug.