962511 - Expose event loop lag notifications via a devtools actor

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Comment 1

[Paul Rouget [:paul]]

Attachment: WIP

We should extend the DOMUtils API, not docshell. And adding a obs listener crashes the process.

Comment 2

[Paul Rouget [:paul]]

Attachment: v2

Comment 3

[Paul Rouget [:paul]]

Attachment: expose mozilla::InitEventTracing to JS via appShellService

Comment 4

[Paul Rouget [:paul]]

Attachment: implement an eventlooplag actor

Comment 5

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=435185a8830d

Comment 6

[Paul Rouget [:paul]]

Rob, Ehsan:

Bug 962044 introduces a new observerService notification: "event-loop-lag". The notification happens when an operation in the event loop takes more that 20ms (bug 968237 will offer a way to change this threshold).

To track slow operations, we need to use `mozilla::InitEventTracing()`. I expose this to the appShellService.

Then I expose this feature in an actor.

Comment 7

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=e668fc000d4f

Comment 8

[Paul Rouget [:paul]]

Comment on attachment 8371439 [details] [diff] [review]
implement an eventlooplag actor

Cancelling review. Wrong patch.

Comment 9

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=e6deb8646412

Comment 10

[Paul Rouget [:paul]]

Attachment: implement an eventlooplag actor

Comment 11

[(no longer active)]

Comment on attachment 8371438 [details] [diff] [review]
expose mozilla::InitEventTracing to JS via appShellService

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

::: xpfe/appshell/public/nsIAppShellService.idl
@@ +135,5 @@
> +
> +  /**
> +   * Start/stop tracking lags in the event loop.
> +   * If the event look gets unresponsive for a certain duration,
> +   * a "event-loop-lag" notification is sent.

That is only dispatched on gonk: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/EventTracer.cpp#103

So this will only work on native b2g and nowhere else.

@@ +137,5 @@
> +   * Start/stop tracking lags in the event loop.
> +   * If the event look gets unresponsive for a certain duration,
> +   * a "event-loop-lag" notification is sent.
> +   * @param aThreshold, minimum duration (in ms) of a lag
> +   * @return true if tracking didn't fail.

Nit: succeeded.

@@ +138,5 @@
> +   * If the event look gets unresponsive for a certain duration,
> +   * a "event-loop-lag" notification is sent.
> +   * @param aThreshold, minimum duration (in ms) of a lag
> +   * @return true if tracking didn't fail.
> +   */

Please document that calling this multiple times has no effect.

@@ +140,5 @@
> +   * @param aThreshold, minimum duration (in ms) of a lag
> +   * @return true if tracking didn't fail.
> +   */
> +  bool startEventLoopLagTracking(in uint32_t aThreshold);
> +  void stopEventLoopLagTracking();

So this can be triggered through other means as well (see <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4085> for example.)

It might make sense to expose whether this tracing is already in progress using a readonly bool argument, so that the client of the API can write code which correctly handles that case.

::: xpfe/appshell/src/nsAppShellService.cpp
@@ +871,5 @@
> +
> +NS_IMETHODIMP
> +nsAppShellService::StartEventLoopLagTracking(uint32_t aThreshold, bool* aResult)
> +{
> +    // aThreshold is not used yet. See bug 968237

This is misleading.  Let's not expose it at all for now.

Comment 12

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

Comment on attachment 8371609 [details] [diff] [review]
implement an eventlooplag actor

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

looks good to me, but I'd like Panos to give it a quick look over too.

Comment 13

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

Comment on attachment 8371609 [details] [diff] [review]
implement an eventlooplag actor

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

This looks rather straightforward, but I have a couple of questions about actor scope and availability.

::: toolkit/devtools/server/actors/eventlooplag.js
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +exports.register = function(handle) {
> +  handle.addGlobalActor(EventLoopLagActor, "eventLoopLagActor");
> +  handle.addTabActor(EventLoopLagActor, "eventLoopLagActor");

What is the point of registering this actor as tab-scoped, too? Doesn't this setting affect the event loop of the main thread? It looks to me like this should only be a global actor.

@@ +19,5 @@
> +  handle.removeGlobalActor(EventLoopLagActor);
> +  handle.removeTabActor(EventLoopLagActor);
> +};
> +
> +let EventLoopLagActor = protocol.ActorClass({

This file is comment-clean :-/ Please add at least a comment explaining the purpose of this actor.

::: toolkit/devtools/server/main.js
@@ +387,2 @@
>      if ("nsIProfiler" in Ci)
>        this.addActors("resource://gre/modules/devtools/server/actors/profiler.js");

I thought you mentioned the other day that the required engine infrastructure is not present on every platform or every channel. Doesn't this need some conditional registration like the one the profiler actor uses?

::: toolkit/devtools/server/tests/unit/test_eventlooplag_actor.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +function run_test()

Add a brief comment describing the behavior this file tests please.

@@ +41,5 @@
> +  function gotLagEvent(time) {
> +    do_print("lag: " + time);
> +    do_check_true(time >= threshold);
> +    front.stop().then(() => {
> +      do_test_finished();

Use finishClient(client) instead of calling do_test_finished() directly, so that the debugger client gets a chance to shut down properly.

::: toolkit/devtools/server/tests/unit/xpcshell.ini
@@ +113,5 @@
>  [test_breakpoint-16.js]
>  [test_breakpoint-17.js]
>  [test_breakpoint-18.js]
> +[test_eventlooplag_actor.js]
> +run-if = toolkit == "gonk"

Why not run the test on every platform if the actor is going to be present anyway?

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Panos Astithas [:past] from comment #13)
> ::: toolkit/devtools/server/tests/unit/xpcshell.ini
> @@ +113,5 @@
> >  [test_breakpoint-16.js]
> >  [test_breakpoint-17.js]
> >  [test_breakpoint-18.js]
> > +[test_eventlooplag_actor.js]
> > +run-if = toolkit == "gonk"
> 
> Why not run the test on every platform if the actor is going to be present
> anyway?

The observer notification is only present for Gonk at the moment. There is no reasons that it can't be exposed to others platforms as well but I believe this can be changed later.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #11).
> @@ +140,5 @@
> > +   * @param aThreshold, minimum duration (in ms) of a lag
> > +   * @return true if tracking didn't fail.
> > +   */
> > +  bool startEventLoopLagTracking(in uint32_t aThreshold);
> > +  void stopEventLoopLagTracking();
> 
> So this can be triggered through other means as well (see
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.
> cpp#4085> for example.)
> 
> It might make sense to expose whether this tracing is already in progress
> using a readonly bool argument, so that the client of the API can write code
> which correctly handles that case.
> 

Do you mind if this is done in a followup as it requires to touch the profiler code which is a bit out of the scope of this bug ? (should be a small patch though).

> ::: xpfe/appshell/src/nsAppShellService.cpp
> @@ +871,5 @@
> > +
> > +NS_IMETHODIMP
> > +nsAppShellService::StartEventLoopLagTracking(uint32_t aThreshold, bool* aResult)
> > +{
> > +    // aThreshold is not used yet. See bug 968237
> 
> This is misleading.  Let's not expose it at all for now.

I plan to make this value configurable via a pref in bug 968237. It's a bit less flexible but I don't think it matters really. What it means is that this method won't need this parameter.

Comment 16

[(no longer active)]

(In reply to comment #15)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #11).
> > @@ +140,5 @@
> > > +   * @param aThreshold, minimum duration (in ms) of a lag
> > > +   * @return true if tracking didn't fail.
> > > +   */
> > > +  bool startEventLoopLagTracking(in uint32_t aThreshold);
> > > +  void stopEventLoopLagTracking();
> > 
> > So this can be triggered through other means as well (see
> > <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.
> > cpp#4085> for example.)
> > 
> > It might make sense to expose whether this tracing is already in progress
> > using a readonly bool argument, so that the client of the API can write code
> > which correctly handles that case.
> > 
> 
> Do you mind if this is done in a followup as it requires to touch the profiler
> code which is a bit out of the scope of this bug ? (should be a small patch
> though).

Sure, no problem.

> > ::: xpfe/appshell/src/nsAppShellService.cpp
> > @@ +871,5 @@
> > > +
> > > +NS_IMETHODIMP
> > > +nsAppShellService::StartEventLoopLagTracking(uint32_t aThreshold, bool* aResult)
> > > +{
> > > +    // aThreshold is not used yet. See bug 968237
> > 
> > This is misleading.  Let's not expose it at all for now.
> 
> I plan to make this value configurable via a pref in bug 968237. It's a bit
> less flexible but I don't think it matters really. What it means is that this
> method won't need this parameter.

Yeah.  I actually think that the pref might be better because then it doesn't matter who gets to call this function first.  But it's a little bit less flexible as you mention.

Comment 17

[Paul Rouget [:paul]]

Attachment: expose mozilla::InitEventTracing to JS via appShellService

Comment 18

[Paul Rouget [:paul]]

Attachment: implement an eventlooplag actor

Comment 19

[Paul Rouget [:paul]]

Attachment: implement an eventlooplag actor

Comment 20

[Paul Rouget [:paul]]

Comment on attachment 8373226 [details] [diff] [review]
expose mozilla::InitEventTracing to JS via appShellService

Addressed Ehsan's comment.

We will add a readonly property to know if tracking is already enabled.
The threshold will be set via a preference.

Comment 21

[Paul Rouget [:paul]]

Comment on attachment 8373323 [details] [diff] [review]
implement an eventlooplag actor

Address Panos' comments.

(In reply to Panos Astithas [:past] from comment #13)
> ::: toolkit/devtools/server/actors/eventlooplag.js
> @@ +11,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +exports.register = function(handle) {
> > +  handle.addGlobalActor(EventLoopLagActor, "eventLoopLagActor");
> > +  handle.addTabActor(EventLoopLagActor, "eventLoopLagActor");
> 
> What is the point of registering this actor as tab-scoped, too? Doesn't this
> setting affect the event loop of the main thread? It looks to me like this
> should only be a global actor.

This actor needs to be available from apps and oop tabs, but also from the root actor. So we need both.

> ::: toolkit/devtools/server/main.js
> @@ +387,2 @@
> >      if ("nsIProfiler" in Ci)
> >        this.addActors("resource://gre/modules/devtools/server/actors/profiler.js");
> 
> I thought you mentioned the other day that the required engine
> infrastructure is not present on every platform or every channel. Doesn't
> this need some conditional registration like the one the profiler actor uses?

We're working on enabling this on other platforms as well (gonk only at the moment). In the meantime, it won't fail, but the event won't be dispatched.

Comment 22

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

Comment on attachment 8373323 [details] [diff] [review]
implement an eventlooplag actor

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

(In reply to Paul Rouget [:paul] from comment #21)
> > What is the point of registering this actor as tab-scoped, too? Doesn't this
> > setting affect the event loop of the main thread? It looks to me like this
> > should only be a global actor.
> 
> This actor needs to be available from apps and oop tabs, but also from the
> root actor. So we need both.

Global-scoped actors should already do that, as we load them in every process. A quick testing however shows that this doesn't work as expected under the App Manager though, due to the way the webapps actor works. Notice how the profiler doesn't work at all under the app manager. We should definitely fix this, but not in this bug.

Comment 23

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=069f91fddc3d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/278919f3e25d
https://hg.mozilla.org/integration/fx-team/rev/42053cfdc5ff

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/278919f3e25d
https://hg.mozilla.org/mozilla-central/rev/42053cfdc5ff

Comment 26

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

(In reply to Panos Astithas [:past] from comment #22)
> Global-scoped actors should already do that, as we load them in every
> process. A quick testing however shows that this doesn't work as expected
> under the App Manager though, due to the way the webapps actor works. Notice
> how the profiler doesn't work at all under the app manager. We should
> definitely fix this, but not in this bug.

The profiler problem was a red herring, I didn't have the right prefs set on the device when I tested. The profiler works fine in the App Manager toolbox, even though it's only registered as a global actor, so I think this actor would work just global-scoped, too.

Comment 27

[Oleg Romashin (:romaxa)]

nsAppShellService.cpp:49:25: fatal error: EventTracer.h: No such file or directory

Comment 28

[Oleg Romashin (:romaxa)]

Attachment: missing ifdef MOZ_INSTRUMENT_EVENT_LOOP

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f73ed457607

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f73ed457607