cc: add RingBuffer class for timestamp storing in FrameRateCounter

This change makes the timestamp saving functionality of the FrameRateCounter reuseable by abstracting it to a new RingBuffer class.

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177083

[egraether, 2013-01-09 02:04:04]

I used the RingBuffer as base class, because it was easier to have access to the
buffer array and the current index when calculating min/max and mean values.
Also it made it easier to access the buffer in the HudLayer.

https://codereview.chromium.org/11817011/diff/1/cc/frame_rate_counter.h
File cc/frame_rate_counter.h (left):

https://codereview.chromium.org/11817011/diff/1/cc/frame_rate_counter.h#oldco...
cc/frame_rate_counter.h:42: bool isBadFrame(int frameNumber) const;
This method was not used.

https://codereview.chromium.org/11817011/diff/1/cc/frame_rate_counter.h
File cc/frame_rate_counter.h (right):

https://codereview.chromium.org/11817011/diff/1/cc/frame_rate_counter.h#newco...
cc/frame_rate_counter.h:30: void getMinAndMaxFPS(double& minFPS, double& maxFPS)
const;
This method will get used in a following patch:
https://codereview.chromium.org/11820014

https://codereview.chromium.org/11817011/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/11817011/diff/1/cc/layer_tree_host_impl.cc#ol...
cc/layer_tree_host_impl.cc:939: m_fpsCounter->markEndOfFrame();
I found this call had no real purpose and was leading to confusion.

[shawnsingh, 2013-01-09 20:55:08]

Looks great!  Below I've added a few minor comments.

The only major issue is that I think it would be better to have fpsCounter own a
RingBuffer, rather than inherit it.   You mentioned that you did the inheritance
since it made some implementation points easier and cleaner - as far as I see,
the same cleanliness can be achieved by keeping the timeStampHistorySize() and
timeStampOfRecentFrame() accessors instead of removing them.  They would become
trivial wrappers to the RingBuffer methods.  Let's discuss if needed =)

https://codereview.chromium.org/11817011/diff/1/cc/heads_up_display_layer_imp...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11817011/diff/1/cc/heads_up_display_layer_imp...
cc/heads_up_display_layer_impl.cc:211: if
(base::TimeDelta(fpsCounter->readBuffer(0) - textUpdateTime).InSecondsF() >
0.25) {
Personally, as I'm reading the code here, I would like a more intuitive,
meaningful name than "readBuffer".  I would like to see something that makes it
clear that we're accessing a recent timestamp that has been stored in the
fpsCounter.  Perhaps we could keep the timeStampOfRecentFrame() and
timeStampHistorySize() functions the same, and just make them trivial inlined
wrappers to use the ring buffer?

That may also make it possible for fpsCounter to own an instance of RingBuffer,
rather than to inherit it.  I don't feel too strongly either way, but in my
opinion, the owning concept makes more sense - i.e.  "fpsCounter uses a
RingBuffer" , not  "fpsCounter is a RingBuffer".

https://codereview.chromium.org/11817011/diff/1/cc/ring_buffer.h
File cc/ring_buffer.h (right):

https://codereview.chromium.org/11817011/diff/1/cc/ring_buffer.h#newcode1
cc/ring_buffer.h:1: // Copyright 2012 The Chromium Authors. All rights reserved.

2013 now =)

https://codereview.chromium.org/11817011/diff/1/cc/ring_buffer.h#newcode5
cc/ring_buffer.h:5: #ifndef CC_RING_BUFFER_H_
As I understand, we are trying to make all new files in cc/ to follow chromium
style, even though many files are currently still using the old way from
back-in-the-day when the code was in WebKit.

So for this entire file let's do:
 - indentation of 2 spaces, not 4
 - m_buffer --> buffer_
 - m_currentIndex --> currentIndex_
 - functions that use camelCase should have first letter also capitalized -->
BufferSize(), ReadBuffer(), etc.

I'm still coming to grips with chromium style too, so I may have missed some

https://codereview.chromium.org/11817011/diff/1/cc/ring_buffer.h#newcode39
cc/ring_buffer.h:39: return (n % size + size) % size;
Wouldn't this be equivalent to (n+size) % size,  without the need for the extra
mod operation?    If I'm not mistaken, that would be more efficient, I believe
mod operators are often pretty expensive in terms of # CPU cycles.  Or am I
missing something subtle here? =)

[nduca, 2013-01-09 21:28:37]

I'm pretty slamed, so have shawn do the final review if you can. I'm happy with
anything tbh

[egraether, 2013-01-10 02:32:09]

I switched to FrameRateCounter to having a RingBuffer and rewrote the parts that
needed direct access to the buffer.

Please have a quick peek at the next review:
https://codereview.chromium.org/11827009

Because now it would be possible to implement the paint time counting as part of
the FrameRateCounter using another RingBuffer. This would allow keeping
everything in one file, without adding new ones. I think it's better to have a
separate PaintTimeCounter object, but I'm not sure what's best.

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (left):

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc#o...
cc/frame_rate_counter.cc:48: m_timeStampHistory[i] = base::TimeTicks();
Removed this initialization, because it was unnecessary. All values in the
history should get constructed by the default base::TimeTicks constructor by
default. Am I right?

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc#n...
cc/frame_rate_counter.cc:26: return m_ringBuffer.ReadBuffer(n) -
m_ringBuffer.ReadBuffer(n - 1);
Rewrote this method to make use of ReadBuffer(), because there is no direct
access to the buffer.

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.h
File cc/frame_rate_counter.h (right):

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.h#ne...
cc/frame_rate_counter.h:54: RingBuffer<base::TimeTicks, 130> m_ringBuffer;
FrameRateCounter now has a RingBuffer. I solved the loops that iterated the
buffer in a different way.

https://codereview.chromium.org/11817011/diff/6002/cc/heads_up_display_layer_...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11817011/diff/6002/cc/heads_up_display_layer_...
cc/heads_up_display_layer_impl.cc:211: base::TimeTicks now =
base::TimeTicks::Now();
This should just use the current timestamp, which is easier to understand.

[shawnsingh, 2013-01-10 21:13:01]

LGTM !!!

There's just a few edge cases that we should get right before landing.  You may
have already been doing this, but don't forget to check a debug build, to make
sure that DCHECKS don't get triggered.

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (left):

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc#o...
cc/frame_rate_counter.cc:48: m_timeStampHistory[i] = base::TimeTicks();
Default constructor seems to initialize to 0...    I think the reason for
initializing this way (to the current time) may have been so that
recentFrameInterval(n) for any n will be 0 rather than some huge number.  Maybe
we want to keep that behavior, so that the FPS counter will visualize properly
even when the entire buffer has not yet been filled.

On the other hand, I see that we don't make the entire buffer available, so
either we do an awkward "SaveToBuffer"  n times for a buffer size of n, or we
make sure that we're not using the un-visited zero values by some other
mechanism.  I feel that a fully initialized RingBuffer may be more robust to
misuse/abuse down the road.

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11817011/diff/6002/cc/frame_rate_counter.cc#n...
cc/frame_rate_counter.cc:69: double fps = 1.0 / delta.InSecondsF();
Let's add a DCHECK(delta.InSecondsF() > 0), to avoid negative deltas or
divide-by-zero.

https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h
File cc/ring_buffer.h (right):

https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h#newcode26
cc/ring_buffer.h:26: bool IsFilledIndex(unsigned int n) const {
I just noticed that the style guide asks us never to use unsigned int.  So we
have to use normal int for this whole class, and right here we should add a
DCHECK that it is >= 0.

https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h#newcode33
cc/ring_buffer.h:33: return buffer_[BufferIndex(n)];
Can we DCHECK here that n is always >= 0 and strictly < size ?

Perhaps we also want to DCHECK(IsFilledIndex(n)) here?

[danakj, 2013-01-10 23:31:40]

https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h
File cc/ring_buffer.h (right):

https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h#newcode26
cc/ring_buffer.h:26: bool IsFilledIndex(unsigned int n) const {
On 2013/01/10 21:13:01, shawnsingh wrote:
> I just noticed that the style guide asks us never to use unsigned int.  So we
> have to use normal int for this whole class, and right here we should add a
> DCHECK that it is >= 0.

What, are you sure it doesn't mean to use "unsigned" instead of "unsigned int"?

[egraether, 2013-01-10 23:38:29]

Resolved the nits.

https://codereview.chromium.org/11817011/diff/9003/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11817011/diff/9003/cc/frame_rate_counter.cc#n...
cc/frame_rate_counter.cc:69: DCHECK(delta.InSecondsF() > 0);
checking divide by 0

https://codereview.chromium.org/11817011/diff/9003/cc/frame_rate_counter.cc#n...
cc/frame_rate_counter.cc:108: DCHECK(frameTimesTotal > 0);
checking divide by 0

https://codereview.chromium.org/11817011/diff/9003/cc/frame_rate_counter.cc#n...
cc/frame_rate_counter.cc:123: return base::TimeTicks();
Returns base::TimeTicks() for an invalid index. No more need to fill the buffer
with base::TimeTicks() in the constructor.

https://codereview.chromium.org/11817011/diff/9003/cc/ring_buffer.h
File cc/ring_buffer.h (right):

https://codereview.chromium.org/11817011/diff/9003/cc/ring_buffer.h#newcode29
cc/ring_buffer.h:29: DCHECK(n >= 0);
Prevents using negative indices.

https://codereview.chromium.org/11817011/diff/9003/cc/ring_buffer.h#newcode36
cc/ring_buffer.h:36: DCHECK(IsFilledIndex(n));
Forbids access to uninitialized indices.

[shawnsingh, 2013-01-10 23:43:09]

On 2013/01/10 23:31:40, danakj wrote:
> https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h
> File cc/ring_buffer.h (right):
> 
> https://codereview.chromium.org/11817011/diff/6002/cc/ring_buffer.h#newcode26
> cc/ring_buffer.h:26: bool IsFilledIndex(unsigned int n) const {
> On 2013/01/10 21:13:01, shawnsingh wrote:
> > I just noticed that the style guide asks us never to use unsigned int.  So
we
> > have to use normal int for this whole class, and right here we should add a
> > DCHECK that it is >= 0.
> 
> What, are you sure it doesn't mean to use "unsigned" instead of "unsigned
int"?

[shawnsingh, 2013-01-10 23:43:37]

oops... clicked send too early by mistake.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types

[danakj, 2013-01-11 00:13:39]

On Thu, Jan 10, 2013 at 6:43 PM, <shawnsingh@chromium.org> wrote:

> oops... clicked send too early by mistake.
>
> http://google-styleguide.**googlecode.com/svn/trunk/**
>
cppguide.xml#Integer_Types<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types>


Huh! And then in chromium style:

unsigned int is basically banned by the Google style guide.  However, size_t is
acceptable in situations where it makes sense, such as when dealing with a
count of objects or bytes in memory, the size of a string, or the number of
objects in an STL container.  A good clue is that when you're calling
library functions that take or return size_t, you should be using size_t as
well rather than casting or coercing to int.

\themoreyouknowmeme.



>
>
>
https://codereview.chromium.**org/11817011/<https://codereview.chromium.org/1...
>

[shawnsingh, 2013-01-11 00:32:46]

egraether@,  based on the chromium addendum that danakj pointed out, let's use
size_t where appropriate - particularly for the template parameter and the
return value of the size() accessor.   I'm guessing those 2 changes will
propagate the need for size_t elsewhere, and once it compiles, this patch is
good to land, no need to review again.   Cheers!

[commit-bot: I haz the power, 2013-01-11 03:04:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11817011/16001

[commit-bot: I haz the power, 2013-01-11 03:53:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[egraether, 2013-01-11 18:08:22]

https://codereview.chromium.org/11817011/diff/25003/cc/ring_buffer.h
File cc/ring_buffer.h (right):

https://codereview.chromium.org/11817011/diff/25003/cc/ring_buffer.h#newcode12
cc/ring_buffer.h:12: template<typename T, int size>
I removed the size_t again, because of signed/unsigned mismatch errors on the
test servers, which are hard to fix because of a long tail of changes.

I tested using negative sizes, which gives a compile time error, so I think that
using int is fine. 

However, if someone thinks I should really switch to size_t, I can update this
in another patch.

[commit-bot: I haz the power, 2013-01-11 18:08:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11817011/25003

[jamesr1, 2013-01-11 19:20:26]

Don't use int for the side of a buffer.
On Jan 11, 2013 10:08 AM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/egraether@**
>
chromium.org/11817011/25003<https://chromium-status.appspot.com/cq/egraether@chromium.org/11817011/25003>
>
>
>
https://chromiumcodereview.**appspot.com/11817011/<https://chromiumcodereview...
>
> --
>
>
>

[egraether, 2013-01-11 19:29:10]

On 2013/01/11 19:20:26, jamesr1 wrote:
> Don't use int for the size of a buffer.

To my defense, int was used before.
Should I change it in this patch or in a following one?

[shawnsingh, 2013-01-11 19:32:37]

Not your fault =)

Personally I would prefer we get this correct up front, so we don't need an
additional follow-up.   I think it's OK if it takes a few iterations of trybots
to get size_t as desired.

[egraether, 2013-01-11 19:48:41]

On 2013/01/11 19:32:37, shawnsingh wrote:
> Not your fault =)
> 
> Personally I would prefer we get this correct up front, so we don't need an
> additional follow-up.   I think it's OK if it takes a few iterations of
trybots
> to get size_t as desired.

Ok, then lets fix it now.

How far does this need to go? I could just change the template parameter and
leave everything else the same. Internally it would use size_t, BufferSize()
would return int and I don't have to mess with typecasting in FrameRateCounter:

template<typename T, size_t size>
class RingBuffer {
...
  int BufferSize() const {
    return size;
  }
...

[danakj, 2013-01-11 19:55:48]

On Fri, Jan 11, 2013 at 2:48 PM, <egraether@chromium.org> wrote:

> On 2013/01/11 19:32:37, shawnsingh wrote:
>
>> Not your fault =)
>>
>
>  Personally I would prefer we get this correct up front, so we don't need
>> an
>> additional follow-up.   I think it's OK if it takes a few iterations of
>>
> trybots
>
>> to get size_t as desired.
>>
>
> Ok, then lets fix it now.
>
> How far does this need to go? I could just change the template parameter
> and
> leave everything else the same. Internally it would use size_t,
> BufferSize()
> would return int and I don't have to mess with typecasting in
> FrameRateCounter:
>
> template<typename T, size_t size>
> class RingBuffer {
> ...
>   int BufferSize() const {
>     return size;
>

I'd say dont turn the size_t into an int ever.


>   }
> ...
>
>
https://chromiumcodereview.**appspot.com/11817011/<https://chromiumcodereview...
>

[nduca, 2013-01-11 23:23:34]

Do the right thing. Throughout.

[egraether, 2013-01-11 23:28:06]

https://chromiumcodereview.appspot.com/11817011/diff/17008/cc/ring_buffer.h
File cc/ring_buffer.h (right):

https://chromiumcodereview.appspot.com/11817011/diff/17008/cc/ring_buffer.h#n...
cc/ring_buffer.h:12: template<typename T, size_t size>
Used size_t for buffer size, index, parameters and loops. I tried to make it
consistently use size_t. It seems to work now, the test servers fail for other
reasons (which I don't understand).

Please have a look.

[shawnsingh, 2013-01-14 18:43:22]

LGTM again; looked primarily at the size_t changes.

[commit-bot: I haz the power, 2013-01-14 20:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11817011/41002

[commit-bot: I haz the power, 2013-01-15 19:38:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11817011/47001

[commit-bot: I haz the power, 2013-01-16 04:06:19]

Change committed as 177083