708519 - Repair, move, and pref off the frame rate meter

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

The frame rate meter in native Fennec doesn't really work anymore now that we have RENDERMODE_WHEN_DIRTY, since we don't get called unless there's actually work to do. Turns out that it's hard to try to extrapolate FPS, so maybe we should just go to a count of dropped frames and a mean time to draw each frame.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached.

Comment 2

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2.

I figure I'll add some more frame rate monitor patches here.

Patch part 2 moves the frame rate monitor to the bottom right, where it's less distracting.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Can we also make it a build option? I think it's time to turn it off for nightlies. Thoughts?

Comment 4

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3.

Was just working on that :)

Patch part 3 prefs it off. The pref is not exposed through the UI (although an add-on could conceivably expose it). The easiest way to enable it is to create a file named GeckoApp.xml with the following contents:

<?xml version='1.0' encoding='utf-8' standalone='yes' ?>
<map>
<boolean name="showFrameRate" value="true" />
</map>

And put that at /data/data/org.mozilla.fennec/shared_prefs/GeckoApp.xml. You may will either need to run-as org.mozilla.fennec or get a root shell to execute this command.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 579965 [details] [diff] [review]
Proposed patch, part 3.

This looks good to me, but as you mention, it might affect startup time. Doug can profile it :)

We did remove some usage of SharedPreferences, but I think the slowdown there was writing, not reading.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 579956 [details] [diff] [review]
Proposed patch.

So first of all, general comment: jmaher has been working on adding almost exactly this behaviour to tpan/tzoom using the PanningPerfAPI class that I added a few days ago. So I'm not sure it's needed here. You should sync up with him first to see if what he's doing is sufficient for you, or if you can reuse the code he's writing (or vice-versa).

>     public void onDrawFrame(GL10 gl) {
>-        checkFPS();
>+        long frameStartTime = System.currentTimeMillis();
>+

We should switch to using SystemClock.uptimeMillis(), that's what is recommended for perf measurements in Android and what I'm using in PanningPerfAPI.

>+    private void updateDroppedFrames(long frameStartTime) {
>+        long frameElapsedTime = System.currentTimeMillis() - frameStartTime;
>+        mFrameTimings[mCurrentFrame] = (int)frameElapsedTime;
>+        mCurrentFrame = (mCurrentFrame + 1) % mFrameTimings.length;
> 
>-        if (System.currentTimeMillis() >= mFrameCountTimestamp + 1000) {
>-            mFrameCountTimestamp = System.currentTimeMillis();
>+        int droppedFrames = 0, sum = 0;
>+        for (int i = 0; i < mFrameTimings.length; i++) {
>+            if (mFrameTimings[i] > MAX_FRAME_TIME)
>+                droppedFrames++;
>+            sum += mFrameTimings[i];
>+        }
>+        int averageTime = sum / mFrameTimings.length;

I guess in the grand scheme of things this doesn't take a lot of time but when it comes to perf measurements I prefer if the measurement code itself has minimal impact on the code being measured. This could be made ~30x more efficient by keeping a running total of the sum and dropped frames rather than looping over all of mFrameTimings every single frame. If you keep an mSum and mDroppedFrames, you can decrement them with the value in mFrameTimings that you're about to clobber and then increment them with the new value you're adding.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 579960 [details] [diff] [review]
Proposed patch, part 2.

>+        }
>+    }
> }
> 
>+
>+

nit: spurious blank lines can be removed.

Comment 8

[Patrick Walton (:pcwalton)]

(In reply to Kartikaya Gupta (:kats) from comment #6)
> So first of all, general comment: jmaher has been working on adding almost
> exactly this behaviour to tpan/tzoom using the PanningPerfAPI class that I
> added a few days ago. So I'm not sure it's needed here. You should sync up
> with him first to see if what he's doing is sufficient for you, or if you
> can reuse the code he's writing (or vice-versa).

The PanningPerfAPI doesn't seem to work because it records frame timings, which aren't the same as the time each frame took to render. It'll show a low frame rate when there was actually nothing to do, while actually timing each frame gives a more accurate picture of the compositing time.

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2.

Patch part 1 version 2 addresses review comments otherwise.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13dca5fd0b5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/45bb8d3c8fe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa93820dbda1

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13dca5fd0b5a