Closed Bug 962511 Opened 6 years ago Closed 6 years ago

Expose event loop lag notifications via a devtools actor

Categories

(DevTools :: WebIDE, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: paul, Assigned: paul)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 6 obsolete files)

No description provided.
Attached patch WIP (obsolete) — Splinter Review
We should extend the DOMUtils API, not docshell. And adding a obs listener crashes the process.
Summary: Expose JS janks notifications via an devtools actor → Expose JS janks notifications via a devtools actor
Attached patch v2 (obsolete) — Splinter Review
Attachment #8363577 - Attachment is obsolete: true
No longer blocks: devtools-layers
Blocks: 968237
Attachment #8363748 - Attachment is obsolete: true
Attachment #8371438 - Flags: review?(ehsan)
Attached patch implement an eventlooplag actor (obsolete) — Splinter Review
Attachment #8371439 - Flags: review?(rcampbell)
Summary: Expose JS janks notifications via a devtools actor → Expose event loop lag notifications via a devtools actor
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 on attachment 8371439 [details] [diff] [review]
implement an eventlooplag actor

Cancelling review. Wrong patch.
Attachment #8371439 - Flags: review?(rcampbell)
Attached patch implement an eventlooplag actor (obsolete) — Splinter Review
Attachment #8371439 - Attachment is obsolete: true
Attachment #8371609 - Flags: review?(rcampbell)
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.
Attachment #8371438 - Flags: review?(ehsan) → review-
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.
Attachment #8371609 - Flags: review?(rcampbell)
Attachment #8371609 - Flags: review?(past)
Attachment #8371609 - Flags: review+
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?
Attachment #8371609 - Flags: review?(past)
(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.
(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.
(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.
Attached patch implement an eventlooplag actor (obsolete) — Splinter Review
Attachment #8371609 - Attachment is obsolete: true
Attachment #8373227 - Attachment is obsolete: true
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.
Attachment #8373226 - Flags: review?(ehsan)
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.
Attachment #8373323 - Flags: review?(past)
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.
Attachment #8373323 - Flags: review?(past) → review+
Attachment #8373226 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/278919f3e25d
https://hg.mozilla.org/mozilla-central/rev/42053cfdc5ff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
(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.
nsAppShellService.cpp:49:25: fatal error: EventTracer.h: No such file or directory
Attachment #8374658 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.