828049 - Remote profiling

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

Description

[Anton Kovalyov (:anton)]

We need to profile B2G and other remote instances using our JavaScript Profiler.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

After bug 820524 is done, we will be able to reuse the same code in the profiler.

Comment 2

[Anton Kovalyov (:anton)]

Attachment: Remote profiling (WIP)

So this patch seems to be working when tested manually. Asking for feedback to see if I missed anything and to get ideas on how to test this.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 709284 [details] [diff] [review]
Remote profiling (WIP)

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

::: browser/devtools/profiler/ProfilerController.jsm
@@ +29,5 @@
>    }
>  
> +  this.isRemote = true;
> +
> +  if (!client) {

The client should always be provided, but see below.

@@ +58,5 @@
>        }.bind(this));
> +    }.bind(this);
> +
> +    if (this.isRemote) {
> +      return void listTabs();

Nice!

@@ +140,5 @@
>   */
> +function ProfilerController(target) {
> +  let client;
> +
> +  if (target && target.isRemote) {

It looks like a target is always supplied by ProfilerPanel, as I would expect, so you can only check for target.isRemote. Furthermore, the profiler (like the debugger and web console) should always be provided with a RemoteTarget, so you can rely on a client property being present. This will require that you either make a few changes in this bug and base it on top of bug 820524, or land this as it is, and I'll base that patch on yours, adding those modifications.

As it is now, the profiler is using its own remote connection in the local profiling case.

Comment 4

[Anton Kovalyov (:anton)]

Attachment: Remote profiling (with leaking tests)

Patch above works, tests pass but leak tons of objects.

Comment 5

[Anton Kovalyov (:anton)]

Attachment: Remote profiling

Nothing is leaking and remote connections work.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 710381 [details] [diff] [review]
Remote profiling

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

I have a question that I'd like you guys to think about and a couple of minor cleanups. Otherwise this looks good to me.

::: browser/devtools/framework/ToolDefinitions.jsm
@@ +153,4 @@
>    tooltip: l10n("profiler.tooltip", profilerStrings),
>  
>    isTargetSupported: function (target) {
> +    return true;

An interesting question is what should happen when the user picks a tab to debug from the 'Connect...' menu item's list of available targets. The available choices there are tabs and the process as a whole. Picking the latter leads to chrome debugging and a global console, and it seems obvious that it should also include the profiler. But what if the user picks his web app to debug? Does it make sense to include the profiler in this case, even though it won't limit its data to that tab only?

My feeling is that we should display it even then, since the tool will likely provide invaluable insights even amidst a heap of unrelated information. But in case we decide that we only want to show the profiler for the Main Process, isTargetSupported should return !!target.chrome.

::: browser/devtools/profiler/test/head.js
@@ +17,5 @@
> +registerCleanupFunction(function () {
> +  Services.prefs.clearUserPref(PROFILER_ENABLED);
> +  Services.prefs.clearUserPref(REMOTE_ENABLED);
> +  DebuggerServer.closeListener(true);
> +  DebuggerServer.destroy();

You shouldn't need to call both destroy() and closeListener(), since the former calls the latter if there are no connections still open. If by this time there are connections still open, we should find out why. Keeping only destroy() should suffice.

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +331,5 @@
>    _connectionClosed: function DS_connectionClosed(aConnection) {
>      delete this._connections[aConnection.prefix];
> +    if (this.onConnectionClosed) {
> +      this.onConnectionClosed(aConnection);
> +    }

I don't see this being used anywhere. Probably debugging leftover?

Comment 7

[Anton Kovalyov (:anton)]

Thanks!

(In reply to Panos Astithas [:past] from comment #6)
> Comment on attachment 710381 [details] [diff] [review]
> Remote profiling
> 
> Review of attachment 710381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a question that I'd like you guys to think about and a couple of
> minor cleanups. Otherwise this looks good to me.
> 
> ::: browser/devtools/framework/ToolDefinitions.jsm
> @@ +153,4 @@
> >    tooltip: l10n("profiler.tooltip", profilerStrings),
> >  
> >    isTargetSupported: function (target) {
> > +    return true;
> 
> An interesting question is what should happen when the user picks a tab to
> debug from the 'Connect...' menu item's list of available targets. The
> available choices there are tabs and the process as a whole. Picking the
> latter leads to chrome debugging and a global console, and it seems obvious
> that it should also include the profiler. But what if the user picks his web
> app to debug? Does it make sense to include the profiler in this case, even
> though it won't limit its data to that tab only?
> 
> My feeling is that we should display it even then, since the tool will
> likely provide invaluable insights even amidst a heap of unrelated
> information. But in case we decide that we only want to show the profiler
> for the Main Process, isTargetSupported should return !!target.chrome.

I think we should display it even then—since usually when you profile a website you interact with it (or website does something heavy) so even though profiler is global to the browser website's code will be more prominent in the profile. And after bug 834751 is done it will be even more explicit and useful.

Comment 8

[Anton Kovalyov (:anton)]

Attachment: Remote profiling

Updated my patch based on Panos' comments. Carrying over r+, waiting for robcee to okay the first question before landing it.

Comment 9

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

yeah, I agree that we should allow it for the remote tab case even though there may be leakage from the browser and other tabs. It's an unfortunate limitation of our architecture that we can't isolate a tab completely short of masking.

land away!

Comment 10

[Anton Kovalyov (:anton)]

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

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/bb1ce31bada5

Comment 12

[Alex Keybl [:akeybl]]

We'll note this for Android/B2G developers. Thanks!