968237 - We should be able to dynamically set the threshold of the event loop lag tracker

Status: RESOLVED FIXED

(Core :: Widget, defect)

Comment 1

[(not currently active) Ted Mielczarek]

What's the use case here?

Comment 2

[Paul Rouget [:paul]]

Bug 962044 introduces a notification when an operation in the event loop takes too long to end. Today, the threshold come from a environment variable:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/EventTracer.cpp#112

We want to be able to change this value from the developer tools.

Comment 3

[(not currently active) Ted Mielczarek]

Right, but my question is *why*?

Comment 4

[Paul Rouget [:paul]]

Gaia developers want to be notified when the event loop gets stuck for a long time.

The duration of what we consider a lag will vary. For some devices, it will be 200ms, on other it will be 100ms.


Also, first we want to find the longest operations (200ms for example), fix the problems, then find the operation taking more than 150ms, then 100ms.

Does this answer your question?

Comment 5

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

Attachment: jank.threshold.patch

Something as simple as a pref should be enough.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8372813 [details] [diff] [review]
jank.threshold.patch

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

::: toolkit/xre/EventTracer.cpp
@@ +132,3 @@
>  #ifdef MOZ_WIDGET_GONK
> +  nsresult rv = NS_ERROR_FAILURE;
> +  rv = Preferences::GetInt("devtools.janks.threshold", &thresholdInterval);

You're not supposed to access preferences off the main thread. This will eventually become a fatal assertion. You'll need to do this in InitEventTracer and pass it through TracerStartClosure.

@@ +132,5 @@
>  #ifdef MOZ_WIDGET_GONK
> +  nsresult rv = NS_ERROR_FAILURE;
> +  rv = Preferences::GetInt("devtools.janks.threshold", &thresholdInterval);
> +  if (!NS_SUCCEEDED(rv)) {
> +    thresholdInterval = 100;

If you're going to do this, do it on every platform, not just gonk. Also, I'm not wild about the fact that gonk has a different default.

Comment 7

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

Attachment: jank.threshold.patch

Removed the special default value for gonk and stop reading the pref outside of the main thread (sorry about that).

Comment 8

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

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

Comment 9

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

Attachment: jank.threshold.patch

Same patch but with Preferences.h out of the #ifdef gonk since Try was complaining.

Comment 10

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

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

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a23dfaabbe18

Comment 12

[Phil Ringnalda (:philor)]

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