Closed Bug 1016526 Opened 7 years ago Closed 7 years ago

The framerate actor returns incorrect data

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(2 files, 1 obsolete file)

We're mapping framerate across buckets of a specified duration, not frame counts. This is incorrect, because for a bucket of 100ms and a steady framerate of 9fps, the returned values are [0,9,0,9].

Since the new profiler frontend needs to plot framerate and not frame counts, the actor needs to be changed accordingly.
Attached patch v1 (obsolete) — Splinter Review
This patch simply makes the actor return the raw recorded refresh driver ticks, so that the frontend may process it however it likes. However, a simple "plotFPS" method is provided that could be used to "massage" the ticks into framerate (that is, frames per second, not frame count) by using a configurable time-based moving average (as opposed to sample size based).

We need to show framerate in the frontend, like in the mockup: http://cl.ly/image/0O1T251O1U16 The raw data returned by the actor can be used to evidentiate blips or sudden drops in rendering.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8429480 - Flags: review?(bgirard)
Comment on attachment 8429480 [details] [diff] [review]
v1

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

::: toolkit/devtools/server/actors/framerate.js
@@ +114,5 @@
> +    }
> +
> +    let frameCount = 0;
> +    let prevTime = 0;
> +    let currTime = 0;

you're declaring currTime twice.

@@ +124,5 @@
> +      if (elapsedTime < interval) {
> +        continue;
> +      }
> +
> +      let framerate = 1000 / (elapsedTime / frameCount);

If you have a frame right when you start recording (at t=0) then this will return a frame rate of 1000 FPS which is bogus.

There's no way for this to report a FPS of zero. Which indicates that this doesn't match up with what we discussed.

@@ +126,5 @@
> +      }
> +
> +      let framerate = 1000 / (elapsedTime / frameCount);
> +      while ((prevTime = prevTime + interval) < currTime) {
> +        timeline[prevTime] = framerate;

And this just copies the frame rate between prevTime and currTime.
Attachment #8429480 - Flags: review?(bgirard) → review-
(In reply to Benoit Girard (:BenWa) from comment #2)
> Comment on attachment 8429480 [details] [diff] [review]
> @@ +124,5 @@
> > +      if (elapsedTime < interval) {
> > +        continue;
> > +      }
> > +
> > +      let framerate = 1000 / (elapsedTime / frameCount);
> 
> If you have a frame right when you start recording (at t=0) then this will
> return a frame rate of 1000 FPS which is bogus.
> 

I don't think that's the case, because it's short-circuited by the `interval`.

> There's no way for this to report a FPS of zero. Which indicates that this
> doesn't match up with what we discussed.

We discussed putting frame counts into buckets. We need to show framerate. Calculating framerate ("frames per something") involves averaging frame counts over time, by definition. Filling out gaps with zeroes is not something we want to do.

> @@ +126,5 @@
> > +      }
> > +
> > +      let framerate = 1000 / (elapsedTime / frameCount);
> > +      while ((prevTime = prevTime + interval) < currTime) {
> > +        timeline[prevTime] = framerate;
> 
> And this just copies the frame rate between prevTime and currTime.

This is intentional. It "projects" the curve to better reflect reality. When dealing with pauses after a smooth execution, this effectively turns a descending line into a U-shaped line in the graph.

Again, we're dealing with framerate here, which involves averaging things out by definition. This is (one of the graphs) the frontend will need to show. Sudden blips and drops in framerate can be represented differently, but framerate needs to be smoothed out implicitly.
Attached patch v2Splinter Review
Attachment #8429480 - Attachment is obsolete: true
Attachment #8431751 - Flags: review?(bgirard)
Comment on attachment 8431751 [details] [diff] [review]
v2

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

Nice and simple.

I know this isn't exactly what you wanted. I'm open to revisit this discussion later if we're not happy with the results.
Attachment #8431751 - Flags: review?(bgirard) → review+
Attached patch v2.1Splinter Review
Simply renamed "time" with "delta".
Attachment #8431850 - Flags: review+
Blocks: 1018418
https://hg.mozilla.org/mozilla-central/rev/816287ef49ad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.