Repair, move, and pref off the frame rate meter

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 579956 [details] [diff] [review]
Proposed patch.

Proposed patch attached.
Attachment #579956 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 2

6 years ago
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.
Attachment #579960 - Flags: review?(bugmail.mozilla)
Can we also make it a build option? I think it's time to turn it off for nightlies. Thoughts?
(Assignee)

Comment 4

6 years ago
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.
Attachment #579965 - Flags: review?(bugmail.mozilla)
Attachment #579965 - Flags: feedback?(mark.finkle)
(Assignee)

Updated

6 years ago
Summary: Repair the frame rate meter → Repair, move, and pref off the frame rate meter
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.
Attachment #579965 - Flags: feedback?(mark.finkle) → feedback+
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.
Attachment #579956 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 579960 [details] [diff] [review]
Proposed patch, part 2.

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

nit: spurious blank lines can be removed.
Attachment #579960 - Flags: review?(bugmail.mozilla) → review+
Attachment #579965 - Flags: review?(bugmail.mozilla) → review+

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 580252 [details] [diff] [review]
Proposed patch, part 1, version 2.

Patch part 1 version 2 addresses review comments otherwise.
Attachment #579956 - Attachment is obsolete: true
Attachment #580252 - Flags: review?(bugmail.mozilla)
Attachment #580252 - Flags: review?(bugmail.mozilla) → review+
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
https://hg.mozilla.org/mozilla-central/rev/13dca5fd0b5a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.