Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: anton, Assigned: anton)

Tracking

Trunk
Firefox 21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(relnote-firefox 21+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
We need to profile B2G and other remote instances using our JavaScript Profiler.
After bug 820524 is done, we will be able to reuse the same code in the profiler.
Depends on: 820524
(Assignee)

Comment 2

5 years ago
Created attachment 709284 [details] [diff] [review]
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.
Attachment #709284 - Flags: feedback?(rcampbell)
Attachment #709284 - Flags: feedback?(past)
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.
Attachment #709284 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 710269 [details] [diff] [review]
Remote profiling (with leaking tests)

Patch above works, tests pass but leak tons of objects.
Attachment #709284 - Attachment is obsolete: true
Attachment #709284 - Flags: feedback?(rcampbell)
Attachment #710269 - Flags: feedback?(rcampbell)
(Assignee)

Comment 5

5 years ago
Created attachment 710381 [details] [diff] [review]
Remote profiling

Nothing is leaking and remote connections work.
Attachment #710269 - Attachment is obsolete: true
Attachment #710269 - Flags: feedback?(rcampbell)
Attachment #710381 - Flags: review?(rcampbell)
Attachment #710381 - Flags: review?(past)
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?
Attachment #710381 - Flags: review?(past) → review+
(Assignee)

Comment 7

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

Comment 8

5 years ago
Created attachment 710844 [details] [diff] [review]
Remote profiling

Updated my patch based on Panos' comments. Carrying over r+, waiting for robcee to okay the first question before landing it.
Attachment #710381 - Attachment is obsolete: true
Attachment #710381 - Flags: review?(rcampbell)
Attachment #710844 - Flags: review+
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!
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bb1ce31bada5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
relnote-firefox: --- → ?
We'll note this for Android/B2G developers. Thanks!
relnote-firefox: ? → 21+
You need to log in before you can comment on or make changes to this bug.