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

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.

I'm not sure how to hook in my actor in 'dbg-server.js' without being invasive. What do you think of this?

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

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

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

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

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

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

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

(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

(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

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

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

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

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.

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

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.

A log of the protocol interactions for reference, since my previous pastebin has expired.

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

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

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

(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 on attachment 650504 [details] [diff] [review] Patch v3 yeah, this looks fine to me.

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.

See bug 772119 and bug 755661

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

(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 on attachment 658871 [details] [diff] [review] Patch v4 Carrying over r+, since the only change was removing Cc and Ci definitions.

The arguments to StartProfiler were in the wrong order.

Comment on attachment 660769 [details] [diff] [review] Support remote profiling Via Remote debugging protocol. Carrying over r+

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

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

This avoids registering the actor when nsIProfiler is not available.

https://hg.mozilla.org/integration/fx-team/rev/d8921aee4a5d https://hg.mozilla.org/integration/fx-team/rev/f11a44846f26 https://hg.mozilla.org/integration/fx-team/rev/f0a944193de7

https://hg.mozilla.org/mozilla-central/rev/d8921aee4a5d https://hg.mozilla.org/mozilla-central/rev/f11a44846f26 https://hg.mozilla.org/mozilla-central/rev/f0a944193de7