Implement event loop responsiveness instrumentation for Gonk

RESOLVED FIXED in mozilla30

Status

()

Core
Widget: Gonk
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gsvelto, Assigned: vingtetun)

Tracking

Trunk
mozilla30
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Event loop responsiveness instrumentation (bug 606574) is not available under Firefox OS because the relevant interfaces haven't been implemented in Gonk yet.

The Gonk implementation will need to take into account a few differences:

- We probably want to run the tracer thread only in the foreground application, applications in the background are running at a very low priority and we don't care about event loop lag since they're not visible.

- We might still want to gather event-loop lag in the main b2g process as it can induce lag in the running application if it's not responding fast enough.

- Currently the tracer code prints the long lags into a file provided via the MOZ_INSTRUMENT_EVENT_LOOP_OUTPUT variable or to |stdout|. This is probably not what we want as we'll have one event loop per process and we want to gather information from all of them. We could either log to multiple files (maybe appending the app's PID to the filename or something) or log to a single file but prefixing the various entries with the app's PID. I think the latter approach is better but this should be discussed. Printing to the logcat could also be an option as it's easy to access and already provides PID prefixing.
Created attachment 8362636 [details] [diff] [review]
WIP

WIP to support the instrumentation tool for the event loop on Gonk. I mostly copy-paste the Android impl and create the necessary file.
(Reporter)

Comment 2

4 years ago
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #1)
> WIP to support the instrumentation tool for the event loop on Gonk. I mostly
> copy-paste the Android impl and create the necessary file.

Excellent :) Do you want to pick it up yourself or shall I do the touch up to make it play nice with background/foreground apps and optionally dump to the logcat?
(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #1)
> > WIP to support the instrumentation tool for the event loop on Gonk. I mostly
> > copy-paste the Android impl and create the necessary file.
> 
> Excellent :) Do you want to pick it up yourself or shall I do the touch up
> to make it play nice with background/foreground apps and optionally dump to
> the logcat?

I plan to work with the devtools teams for that. The idea is to fire an observer when something is above the specified threshold.

This observer is caught by some devtools that can add the additional informations such as:
 - is it an app ?
 - manifest url if this is an app
 - is the process foreground/background

Then this information is relied to the main process, then to the system app, and from here we can filters based on what we are interested in, such as the current displayed app on the screen for example.

I'm more or less thinking of cleaning up a little bit this patch, and then ask for review.
Then open a followup to add the observer and make some devtools to listen for it and relay the information to the system app.

The idea behind all that is that if the system app received everything (only for eng builds obviously) we can fix the heuristic in the system app about what to displayed and when.

Let me know what you think.
Created attachment 8362888 [details] [diff] [review]
event.loop.patch
Attachment #8362636 - Attachment is obsolete: true
Attachment #8362888 - Flags: review?(mwu)
(Reporter)

Comment 5

4 years ago
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> I plan to work with the devtools teams for that. The idea is to fire an
> observer when something is above the specified threshold.
> 
> This observer is caught by some devtools that can add the additional
> informations such as:
>  - is it an app ?
>  - manifest url if this is an app
>  - is the process foreground/background
> 
> Then this information is relied to the main process, then to the system app,
> and from here we can filters based on what we are interested in, such as the
> current displayed app on the screen for example.
>
> I'm more or less thinking of cleaning up a little bit this patch, and then
> ask for review.
> Then open a followup to add the observer and make some devtools to listen
> for it and relay the information to the system app.

Sounds good to me.

> The idea behind all that is that if the system app received everything (only
> for eng builds obviously) we can fix the heuristic in the system app about
> what to displayed and when.

Distinguishing between foreground / background apps and also apps with high priority (like holding the CPU wakelock) should be done when issuing the tracing message we can then forward it up to the system app for analysis as you suggest. This will probably require some design changes compared to the current schema which only prints out measured delays though it seems that the original author had already thought about it :)

http://hg.mozilla.org/mozilla-central/file/cdc0ab2c0cba/toolkit/xre/EventTracer.cpp#l136
Blocks: 962044

Comment 6

4 years ago
Comment on attachment 8362888 [details] [diff] [review]
event.loop.patch

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

::: widget/gonk/WidgetTraceEvent.cpp
@@ +15,5 @@
> +namespace mozilla {
> +  class TracerRunnable : public nsRunnable {
> +    public:
> +      TracerRunnable() {
> +        mTracerLock = new Mutex("TracerRunnable");

Why are you dynamically allocating these? Just use member variables.
Attachment #8362888 - Flags: review?(mwu) → review+
try: https://tbpl.mozilla.org/?tree=Try&rev=ba77613a0c5a
https://hg.mozilla.org/integration/b2g-inbound/rev/cd6558ae6711
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla30
https://hg.mozilla.org/mozilla-central/rev/cd6558ae6711
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Assignee: nobody → 21
You need to log in before you can comment on or make changes to this bug.