Last Comment Bug 708519 - Repair, move, and pref off the frame rate meter
: Repair, move, and pref off the frame rate meter
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Patrick Walton (:pcwalton)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-07 20:32 PST by Patrick Walton (:pcwalton)
Modified: 2012-01-09 14:54 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Proposed patch. (6.37 KB, patch)
2011-12-07 20:39 PST, Patrick Walton (:pcwalton)
bugmail: review-
Details | Diff | Splinter Review
Proposed patch, part 2. (3.51 KB, patch)
2011-12-07 21:15 PST, Patrick Walton (:pcwalton)
bugmail: review+
Details | Diff | Splinter Review
Proposed patch, part 3. (4.81 KB, patch)
2011-12-07 22:09 PST, Patrick Walton (:pcwalton)
bugmail: review+
mark.finkle: feedback+
Details | Diff | Splinter Review
Proposed patch, part 1, version 2. (7.07 KB, patch)
2011-12-08 17:00 PST, Patrick Walton (:pcwalton)
bugmail: review+
Details | Diff | Splinter Review

Description Patrick Walton (:pcwalton) 2011-12-07 20:32:09 PST
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) 2011-12-07 20:39:32 PST
Created attachment 579956 [details] [diff] [review]
Proposed patch.

Proposed patch attached.
Comment 2 Patrick Walton (:pcwalton) 2011-12-07 21:15:59 PST
Created attachment 579960 [details] [diff] [review]
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?) 2011-12-07 21:44:25 PST
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) 2011-12-07 22:09:26 PST
Created attachment 579965 [details] [diff] [review]
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?) 2011-12-07 22:19:09 PST
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:kats@mozilla.com) 2011-12-08 06:58:03 PST
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:kats@mozilla.com) 2011-12-08 06:59:40 PST
Comment on attachment 579960 [details] [diff] [review]
Proposed patch, part 2.

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

nit: spurious blank lines can be removed.
Comment 8 Patrick Walton (:pcwalton) 2011-12-08 16:57:36 PST
(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) 2011-12-08 17:00:46 PST
Created attachment 580252 [details] [diff] [review]
Proposed patch, part 1, version 2.

Patch part 1 version 2 addresses review comments otherwise.
Comment 11 Ed Morley [:emorley] 2011-12-10 20:33:32 PST
https://hg.mozilla.org/mozilla-central/rev/13dca5fd0b5a

Note You need to log in before you can comment on or make changes to this bug.