1459720 - Reduce devtools/actor overhead when remote profiling

Status: RESOLVED FIXED

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

Description

[:Harald Kirschner :digitarald]

Remote profiling should not cause any meaningful overhead.

Collected for a page load on a OnePlus phone via wifi debugging. Only the performance panel was ever opened:

https://perfht.ml/2wrhNd8

- makeInfallible from resource://devtools/shared/ThreadSafeDevToolsUtils.js
- onInputStreamReady from resource://devtools/shared/webconsole/network-monitor.js
- receiveMessage from resource://devtools/shared/transport/transport.js
- onNewScript from resource://devtools/server/actors/thread.js

Comment 1

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

Attachment: RDP protocol log

I had forgotten about all of these details. The problem stems from the fact that opening a toolbox causes the console actor to attach so that it is able to display log messages and network requests if the user switches to that panel. That also causes the debugger to be briefly attached just so that sources and globals are monitored. 

One potential solution is to make the new performance panel disable every other panel and only then attach to its actor. Subsequently, when switching to a different panel reattach the toolbox to the server as usual.

Comment 2

[:Harald Kirschner :digitarald]

Profile in bug 1466729 is impacted by this.

Comment 3

[:Harald Kirschner :digitarald]

Reducing the impact of panels is probably not enough here. The preferred case would be to pause any non-performance-panel work.

:ochameau, how feasible would it be to pause traffic for all panels (right now Console and Network seem to show up most) while the performance panel is recording and resume afterward? What would be possible side effects?

Comment 4

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

I think we are talking about the same thing. If the toolbox were to detach from the server and then properly reattach, there would be no unrelated RDP activity in the remote instance. Side effects presumably include the client panels not being aware that the target/toolbox can be destroyed underneath them, but this should be solvable.

Comment 5

[Julien Wajsberg [:julienw]]

Here is the profile from bug 1466729 filtered by "devtools": https://perfht.ml/2IL1acX

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to :Harald Kirschner :digitarald from comment #3)
> Reducing the impact of panels is probably not enough here. The preferred
> case would be to pause any non-performance-panel work.
> 
> :ochameau, how feasible would it be to pause traffic for all panels (right
> now Console and Network seem to show up most) while the performance panel is
> recording and resume afterward? What would be possible side effects?

It isn't only about RDP traffic. You really want to instruct the actors to pause.
If you only pause the the remote protocol, you will still have some overhead
related to actors listeners. But not only listeners. For example, the debugger tweaks
the execution flags of javascript and it becomes slower to execute.

(In reply to Panos Astithas [:past] (please ni?) from comment #4)
> I think we are talking about the same thing. If the toolbox were to detach
> from the server and then properly reattach, there would be no unrelated RDP
> activity in the remote instance. Side effects presumably include the client
> panels not being aware that the target/toolbox can be destroyed underneath
> them, but this should be solvable.

This is a good idea!
It makes a lot of sense to use attach/detach requests to stop and restart the various actors.
It reuses a concept we never really used beyond tests, so that's great.
I see a couple of challenges:
* There isn't just "attach", some actors are using their own requests.
  Like netmonitor, it is based on WebConsole actor "startListeners" request and it is done here:
    https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#460-466
* attach is a mess, done in various places, with often one request specific for each actor.
  You may want to first clean this up.
  The main attach request, sent to the BrowsingContextActor is done right here:
    https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#457
  But then, Target class also send an attach request to the console actor (this also maps to its startListeners request)
    https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#477
  And for some reason, we don't attach the thread actor (the actor used for the debugger) from Target, but from the Toolbox, right after the call to target.makeRemote, which does the two first attach:
    https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#469
  Note that this may conflict/benefit from bug 1465635, related to fission.
  Overall I think this project should coordinate with Fission initiative as you may modify the same codepaths.
* Many "recent" actors don't have any detach request they are just destroyed
  All target scoped actors are destroyed on detach:
    https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#918-921
  It isn't clear yet if that is an issue in order to re-attach?
  Is there any state in them that we will want to preserve?
  Note that perf actor is a global one and so, won't be destroyed.    
* actors may not clean everything correctly on detach
  I imagine they do clean things as tests would be very flaky otherwise,
  but I can easily see them leak a couple of listeners/states...
* you will want to think about UX of the other panels while profiling.
  Can we still switch to another panel when recording? If yes, we should show a pause overlay or something.
  Do we reload other panels on re-attach? If not, their state will most likely not be correct (they won't reflect any change that happened while profiling).

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Here is the profile from bug 1466729 filtered by "devtools":
> https://perfht.ml/2IL1acX

Note that looking at "devtools" stack is not enough.
For example the debugger will make the javascript be slower to execute and it is something hard to see on the profiler stack frames.


Otherwise we should be careful to distinguish two related but different goals:
* reducing overall impact of actors.
  Bug 1444132 is a typical example of this against the netmonitor.
  It should drastically reduce the impact of netmonitor when the toolbox is opened.
  Bug 1434305 is another one against the debugger.
  This goal is not new and we have been working on this before as part of Devtools performance initiative.
* stop everything but the performance actor when profiling.

While the first would clearly benefit to your usecase with the current setup of things, you really want something more specific.



Finally, you are struggling with the other panels and overall overhead of having a working toolbox next to the profiler.
But you don't have to.
As-is, I think that the new perf panel doesn't have to be in the toolbox. It was introduced in the toolbox because you were trying to rebuild a perf tool for web developers. This doesn't seem to be a goal these days.
The profiler (the C++ profiler) is global. It is not specific to a given document. It is running in the parent process and recording everything.
This is the reason why the new perf actor is a global actor (i.e. in DevTools actors term).
The result of this is that it isn't specific to any tab and can be queried individually without attaching to any tab.
These actors are typically used directly from WebIDE (and should later be from about:debugging) instead of the toolboxes.
See the current integration of screenshot (it uses the device actor) and preferences (preference actor).
If you integrate the same way, only the root actor will be ran. Suddently you won't have to do anything to prevent any overhead.
If your main goal is to help gecko developers, I would highly suggest thinking about this.

Comment 7

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

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> * you will want to think about UX of the other panels while profiling.
>   Can we still switch to another panel when recording? If yes, we should
> show a pause overlay or something.

Can we reuse the notification we show in the inspector when the debugger pauses JS execution on the page?

>   Do we reload other panels on re-attach? If not, their state will most
> likely not be correct (they won't reflect any change that happened while
> profiling).

That's my assumption as well, that we need to reload the panels without visual flicker (e.g. reload right after pressing "stop recording", while still displaying the perf panel).

> Finally, you are struggling with the other panels and overall overhead of
> having a working toolbox next to the profiler.
> But you don't have to.

This is a very good point and I've thought about this, but my gut feeling was that the UI work to host the new perf panel outside the toolbox would be more effort than the plan above. Maybe this is wrong though?

What are our options here? I can think of:
1. New Developer menu item "Remote profile" (or similar) that opens the new perf panel in a tab. Would the tab look too sparse?
2. Just like above, but the menu item opens a new window (like the Browser Console) to make things look more packed.

Harald, do either of these directions seem plausible? Is there another option that might align better with the about:debugging plans?

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] (please ni?) from comment #7)
> (In reply to Alexandre Poirot [:ochameau] from comment #6)
> > * you will want to think about UX of the other panels while profiling.
> >   Can we still switch to another panel when recording? If yes, we should
> > show a pause overlay or something.
> 
> Can we reuse the notification we show in the inspector when the debugger
> pauses JS execution on the page?

I think you will want something more intrusive to avoid interaction with the tool as I'm not expecting them to be functional.

> >   Do we reload other panels on re-attach? If not, their state will most
> > likely not be correct (they won't reflect any change that happened while
> > profiling).
> 
> That's my assumption as well, that we need to reload the panels without
> visual flicker (e.g. reload right after pressing "stop recording", while
> still displaying the perf panel).

I imagine that if you put an overlay on top of tool, it will be easier to reload them,
but I'm also expecting to loose the state of tools. I don't know how much that is an issue.

> > Finally, you are struggling with the other panels and overall overhead of
> > having a working toolbox next to the profiler.
> > But you don't have to.
> 
> This is a very good point and I've thought about this, but my gut feeling
> was that the UI work to host the new perf panel outside the toolbox would be
> more effort than the plan above. Maybe this is wrong though?

For now, the new perf panel isn't a big one. It isn't just a button anymore, but still. It sounds easier to move some UI code rather than addressing all the challenges of pausing all tools and actors.
The main drawback I see here is that the UX may be less intuitive to debug web pages...
If we really aim to exposing new perf tools to debug web page performance,
we may have to stay in the toolbox.

> What are our options here? I can think of:
> 1. New Developer menu item "Remote profile" (or similar) that opens the new
> perf panel in a tab. Would the tab look too sparse?
> 2. Just like above, but the menu item opens a new window (like the Browser
> Console) to make things look more packed.

It shouldn't be something else on its own. It should either be in WebIDE or about:debugging.
Now, that would be unfortunate to contribute to WebIDE just before it is removed...
So hopefully we can contribute it to about:debugging directly?
If that's in about:debugging we can both target local firefox and remotes.
That was the original intent of about:debugging, help debugging tab-agnostic targets for local firefox (like service workers and addons that make no sense in tab toolboxes), and it was also planned from the very first day to support remotes and tab debugging by only using actors and RDP for everything so that it would automagically work with remotes as soon as we added device selection UX.

Comment 9

[:Harald Kirschner :digitarald]

> > Finally, you are struggling with the other panels and overall overhead of
> > having a working toolbox next to the profiler.

Not adding anything to the core of the issue, but highlighting an opportunity: This struggle is not just the new perf panel but also affects the current performance panel. It does some smart filtering for the impact of actors and the live graph capacities, which is less than beneficial to get accurate timings. In the long term, when the new profiler replaces the performance panel, it needs to be smarter about this.

> What are our options here? I can think of:

Given that remote debugging will open a remote toolbox, neither of the 2 offers a good solution for how that flow would work.

> It shouldn't be something else on its own. It should either be in WebIDE or about:debugging.

Work started on a new about:debugging/WebIDE-replacement. Maybe it can eventually have a shortcut for profiling, but this seems to move the burden of technical constraints to a potentially unintuitive interface.

Another option (similar to what Panos probably had in mind initially), which I like to hear more trade-offs about:

- Assumption that switching panels when recording profiles is a not a typical use case that needs to be supported.
- Disconnect background tab actors when profiling starts
- Re-connect background tab actors when profiling stops
- Switching panels while recording stops profiling (could potentially warn)

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As I am not directly involved with perf work, I am probably missing some surrounding context...

With remote profiling, are we still focused on internal Gecko developers?  If we are, then placing profiling controls in about:debugging (outside of the toolbox) seems like a better fit because:

* It avoids the toolbox overhead due to many listeners that "helpfully" start when the toolbox opens
* It is a better fit for the mental model of the profiler since it records the entire browser (not just a single tab)

It is not clear to me why Harald says about:debugging would be unintuitive considering that will be using that to initiate the remote connection.

Over the years, the toolbox has gradually enabled more and more at startup in an attempt to be more helpful without requiring reloads to record additional data.  As a summary (though some bits are mentioned elsewhere in this bug):

* Console listening starts
* Network listening starts
* Profile actor connection started to observe calls to console.profile
* JS debugger API examines scripts on the page (probably slowing them down)
* Target watches for tab navigation

(In reply to :Harald Kirschner :digitarald from comment #9)
> - Assumption that switching panels when recording profiles is a not a
> typical use case that needs to be supported.
> - Disconnect background tab actors when profiling starts
> - Re-connect background tab actors when profiling stops
> - Switching panels while recording stops profiling (could potentially warn)

This would challenge a lot of existing assumptions in the tools, so it feels like a large amount of work to allow this workflow safely across all the tools.  From my perspective, it feels more complex than the work to display the profiler controls somewhere else outside of the toolbox.

Could we clarify why a toolbox with other panels is desired here, since they target a single tab while profiling is for the entire browser?

Comment 11

[Julien Wajsberg [:julienw]]

I think part of the confusion is that we actually chase 2 goals:
1. Target gecko developers for geckoview (focus and fenix work)
2. Target web developers.

I believe 1. is what we want now, while 2. is a longer term goals. But I also believe Harald would like to kill 2 birds with 1 stone and that's why he pushes towards disconnecting tab actors.

Now having read everything and especially Alexandre's comment 6, I think we should integrate in WebIDE/about:debugging now because that looks a lot easier and would unblock other teams.

Longer term we'll want to do all this background work to "silent" all tools while profiling.

Comment 12

[:Harald Kirschner :digitarald]

> Over the years, the toolbox has gradually enabled more and more at startup in an attempt to be more helpful without requiring reloads to record additional data.  As a summary (though some bits are mentioned elsewhere in this bug):

Thank you Ryan. Listing all the helpers and with Alex's insights clarifies that this is a massive amount of work helped me understand the impact better! Would it be sensible to clean up the fragmented architecture of "helpful" panel logic during Fission?

> Now having read everything and especially Alexandre's comment 6, I think we should integrate in WebIDE/about:debugging now because that looks a lot easier and would unblock other teams.

about:debugging is being re-written from scratch for 64 (bug 1462211), which is why I was hoping to find a solution that would not be blocked by landing the rewrite and then adding profiling capabilities. But seems like that is the best way forward.

Comment 13

[Julien Wajsberg [:julienw]]

> about:debugging is being re-written from scratch for 64 (bug 1462211), which is why I was hoping to find a solution that would not be blocked by landing the rewrite and then adding profiling capabilities. But seems like that is the best way forward.

What about WebIDE ?

Comment 14

[:Harald Kirschner :digitarald]

> What about WebIDE ?

It's being removed in the course of the rewrite, so any investment would need exceptionally good arguments.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

So: profiles with overheads from devtools actors are effectively suspect or close to meaningless.  We must get rid of any non-trivial overhead during recording of a profile.  (You all likely know this, but just to make it clear.)

Collecting a profile where you have to switch tabs to start it or stop it is at minimum problematic; switching tabs has major impacts, though since this would always be at the start or end you can ignore them (but it will be somewhat annoying).  Also, if we get more aggressive at releasing resources on backgrounding of a tab, that could make certain types of profiles harder to gather.

Chrome (as most of you know) has a performance panel, and on record puts up a modal "stop recording" and blocks all interaction with their devtools; I suspect they also disable or at least neuter their collection code so that it has minimal impact during the recording.

For remote profiling, I personally just want to be able to record a clean profile.  I'm not debugging, I'm not looking at anything else really.  I have no interest in any actors or other data, especially if the target is GV/Klar/Fennec.  Of course, that's me...

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Harald Kirschner :digitarald from comment #12)
> > Over the years, the toolbox has gradually enabled more and more at startup in an attempt to be more helpful without requiring reloads to record additional data.  As a summary (though some bits are mentioned elsewhere in this bug):
> 
> Thank you Ryan. Listing all the helpers and with Alex's insights clarifies
> that this is a massive amount of work helped me understand the impact
> better! Would it be sensible to clean up the fragmented architecture of
> "helpful" panel logic during Fission?

It seems independent and unrelated to Fission work.  The current toolbox startup helpers are all driven by past product choices, so if we want to make changes to them, we should work out what's desired and schedule some work to change it.  Simply removing them is easy, but that may not be the experience we actually want...

Comment 17

[Julien Wajsberg [:julienw]]

As a quick answer to this comment from Randell:

> Collecting a profile where you have to switch tabs to start it or stop it is at minimum problematic; switching tabs has major impacts, 

Remember we're remote profiling, so switching tabs on the recording side shouldn't impact the profile on the remote side.

Comment 18

[:Harald Kirschner :digitarald]

> Remember we're remote profiling, so switching tabs on the recording side shouldn't impact the profile on the remote side.

I assume :jesup imagined that the same flow would be used to profile a local Firefox; where a user would open about:debugging NG (aka new), select "This Firefox", start profiling and switch back to the tab that the user would like to investigate.

There is an open discussion on the future of the profiler-addon vs devtools-panel vs in-browser-profiler-feature vs user-reported-profiles. All this input here is helpful information!

Comment 19

[Julien Wajsberg [:julienw]]

Not sure how about:debugging will work, but I believe we'd still be in a remote profiling setup, possibly with a separate window, so it would be possible to switch back to the tab before starting profiling.

Comment 20

[:Harald Kirschner :digitarald]

> I believe we'd still be in a remote profiling setup, possibly with a separate window, so it would be possible to switch back to the tab before starting profiling.

Default will be a new tab (which could potentially move out). In general, we want to limit the number of clicks this needs for power users; so even if profiling lands on about:debugging (to support the remote debugging case) – we still need a fast path to start and capture profiles (which is right now the addon).

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: Bug 1459720 - Move the performance panel out of the toolbox to the WebIDE's top level window

This effectively removes the devtools overhead while profiling... as
long as the toolbox isn't opened as well of course.

This also removes the performance panel from the Browser Toolbox and the
Browser Content Toolbox where it shouldn't have been in the first place.

Comment 22

[Julien Wajsberg [:julienw]]

Attachment: Bug 1459720 - Add a test that checks that we correctly launch the perf panel when pushing the button in WebIDE

Comment 23

[Julien Wajsberg [:julienw]]

Try is https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4a265c5dee5de78218be7058ea5c79a3804fc2

Comment 24

[Julien Wajsberg [:julienw]]

New try with the eslint issues fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87cd051170ac3fcc20c5e58ce1af2a0678242f0d

Comment 25

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Created attachment 9012164 [details]
> Bug 1459720 - Move the performance panel out of the toolbox to the WebIDE's
> top level window
> 
> This effectively removes the devtools overhead while profiling... as
> long as the toolbox isn't opened as well of course.
> 
> This also removes the performance panel from the Browser Toolbox and the
> Browser Content Toolbox where it shouldn't have been in the first place.

When we connect to a remote Device in WebIDE, the toolbox opens automatically. Even though we have a menu entry to open the performance panel, the toolbox still starts and the actors will be created as usual. Are you using a different flow in WebIDE that doesn't open a toolbox?

Comment 26

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Comment on attachment 9012164 [details]
Bug 1459720 - Move the performance panel out of the toolbox to the WebIDE's top level window

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 27

[Julien Wajsberg [:julienw]]

We discussed on Slack that this behavior happens in some situations. To remove this behavior, one needs to go the preferences in WebIDE and uncheck the 2 options "remember last project" and "reconnect to previous runtime".
I'd be very tempted to remove them if too many people stumble upon this, but I'd like to check first with the KaiOS folks if they use this.

Comment 28

[Julien Wajsberg [:julienw]]

(leave-open because I'm still waiting for a review for the test)

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ba2376aa7fc
Move the performance panel out of the toolbox to the WebIDE's top level window r=jdescottes

Comment 30

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/1ba2376aa7fc

Comment 31

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Comment on attachment 9012252 [details]
Bug 1459720 - Add a test that checks that we correctly launch the perf panel when pushing the button in WebIDE

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f59e128ed67
Add a test that checks that we correctly launch the perf panel when pushing the button in WebIDE r=jdescottes

Comment 33

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6f59e128ed67