cc: add PaintTimeCounter to keep track of per frame paint time

cc: add PaintTimeCounter to keep track of per frame paint time

This change adds the new PaintTimeCounter class to the LayerTreeHostImpl. It uses a RingBuffer to keep track of per frame paint times, which will be visualized in continuous painting mode.

depends on: https://codereview.chromium.org/11817011

BUG=


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

[nduca, 2013-01-10 20:05:17]

Reveman can review this. I'll rubber stamp when he is happy.

[nduca, 2013-01-10 20:05:33]

+reveman

[egraether, 2013-01-10 20:11:04]

On 2013/01/10 20:05:33, nduca wrote:
> +reveman

@reveman
Please wait with the review until this review is approved:
https://codereview.chromium.org/11817011

Some parts will change again here. I will let you know when it's ready.

[egraether, 2013-01-11 02:35:30]

On 2013/01/10 20:11:04, egraether wrote:
> On 2013/01/10 20:05:33, nduca wrote:
> > +reveman
> 
> @reveman
> Please wait with the review until this review is approved:
> https://codereview.chromium.org/11817011
> 
> Some parts will change again here. I will let you know when it's ready.

Ready for review.

[reveman, 2013-01-11 16:10:49]

lgtm with nits

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.cc#...
cc/paint_time_counter.cc:38: void
PaintTimeCounter::GetMinAndMaxPaintTime(double& min, double& max) const {
chromium style doesn't allow using references for return values. just use
pointers here instead.

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.cc#...
cc/paint_time_counter.cc:42: for (int i = ring_buffer_.BufferSize() - 1; i > 0
&& ring_buffer_.IsFilledIndex(i); --i) {
nit: lines should not be >= 80 characters.

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.h
File cc/paint_time_counter.h (right):

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:19: size_t HistorySize() const { return
ring_buffer_.BufferSize(); }
nit: fix indent

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:23: double GetPaintTimeOfRecentFrame(int n) const;
also here

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:29: PaintTimeCounter();
and here

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:31: RingBuffer<double, 80> ring_buffer_;
here

[danakj, 2013-01-11 16:39:09]

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/11827009/diff/11001/cc/paint_time_counter.cc#...
cc/paint_time_counter.cc:42: for (int i = ring_buffer_.BufferSize() - 1; i > 0
&& ring_buffer_.IsFilledIndex(i); --i) {
On 2013/01/11 16:10:49, David Reveman wrote:
> nit: lines should not be >= 80 characters.

s/>=/>/

"Each line of text in your code should be at most 80 characters long."

[egraether, 2013-01-16 19:31:56]

The previous commit took longer to commit
(https://codereview.chromium.org/11817011), so there have been some changes to
this patch, mainly type changes.

I marked the changes below.

@reveman Can you please have a quick check if you are still fine with
everything?

https://codereview.chromium.org/11827009/diff/25001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11827009/diff/25001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:301:
hostImpl->savePaintTime(m_renderingStats.totalPaintTime);
totalPaintTime is now stored in RenderingStats as TimeDelta
(https://chromiumcodereview.appspot.com/11896002/), so I switched to using
TimeDelta.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.cc#...
cc/paint_time_counter.cc:41: for (size_t i = 0; i < ring_buffer_.BufferSize();
i++) {
Loops in increasing direction because the iterator is now of type size_t.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h
File cc/paint_time_counter.h (right):

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:20: size_t HistorySize() const { return
ring_buffer_.BufferSize(); }
We decided to use size_t for the buffer size in the RingBuffer
(https://codereview.chromium.org/11817011).

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:24: base::TimeDelta GetPaintTimeOfRecentFrame(const
size_t& n) const;
Returns TimeDelta now.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:26: void SavePaintTime(const base::TimeDelta&
total_paint_time);
New paint times are saved as TimeDelta.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:27: void GetMinAndMaxPaintTimeInMilliseconds(double*
min, double* max) const;
Changed the name to indicate the measurement unit.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:32: RingBuffer<base::TimeDelta, 80> ring_buffer_;
Changed type to TimeDelta.

[reveman, 2013-01-16 19:56:04]

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h
File cc/paint_time_counter.h (right):

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:27: void GetMinAndMaxPaintTimeInMilliseconds(double*
min, double* max) const;
On 2013/01/16 19:31:56, egraether wrote:
> Changed the name to indicate the measurement unit.

why isn't this using TimeDelta for min/max?

[egraether, 2013-01-16 22:14:12]

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h
File cc/paint_time_counter.h (right):

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:27: void GetMinAndMaxPaintTimeInMilliseconds(double*
min, double* max) const;
On 2013/01/16 19:56:04, David Reveman wrote:
> On 2013/01/16 19:31:56, egraether wrote:
> > Changed the name to indicate the measurement unit.
> 
> why isn't this using TimeDelta for min/max?

Mostly because milliseconds with type double is the only real usecase. The
method is used in this patch:
https://codereview.chromium.org/11820014/patch/19001/20001

paintTimeCounter->GetMinAndMaxPaintTimeInMilliseconds(&m_paintTimeGraph.min,
&m_paintTimeGraph.max);
(m_paintTimeGraph is a private structure holding a few double values)

Changing this call to TimeDelta would result in something like this:

base::TimeDelta min, max;
paintTimeCounter->GetMinAndMaxPaintTime(&min, &max);
m_paintTimeGraph.min = min.InMillisecondsF();
m_paintTimeGraph.max = max.InMillisecondsF();

So I thought lets keep it simple stupid and just got with double in
milliseconds.

[reveman, 2013-01-16 22:48:03]

lgtm with nits and I would still prefer to see GetMinAndMaxPaintTime use
TimeDelta type.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.cc#...
cc/paint_time_counter.cc:19: base::TimeDelta
PaintTimeCounter::GetPaintTimeOfRecentFrame(const size_t& n) const {
nit: line not <= 80 characters long.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.cc#...
cc/paint_time_counter.cc:37: void
PaintTimeCounter::GetMinAndMaxPaintTimeInMilliseconds(double* min, double* max)
const {
nit: line not <= 80 characters long.

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h
File cc/paint_time_counter.h (right):

https://codereview.chromium.org/11827009/diff/25001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:27: void GetMinAndMaxPaintTimeInMilliseconds(double*
min, double* max) const;
On 2013/01/16 22:14:13, egraether wrote:
> On 2013/01/16 19:56:04, David Reveman wrote:
> > On 2013/01/16 19:31:56, egraether wrote:
> > > Changed the name to indicate the measurement unit.
> > 
> > why isn't this using TimeDelta for min/max?
> 
> Mostly because milliseconds with type double is the only real usecase. The
> method is used in this patch:
> https://codereview.chromium.org/11820014/patch/19001/20001
> 
> paintTimeCounter->GetMinAndMaxPaintTimeInMilliseconds(&m_paintTimeGraph.min,
> &m_paintTimeGraph.max);
> (m_paintTimeGraph is a private structure holding a few double values)
> 
> Changing this call to TimeDelta would result in something like this:
> 
> base::TimeDelta min, max;
> paintTimeCounter->GetMinAndMaxPaintTime(&min, &max);
> m_paintTimeGraph.min = min.InMillisecondsF();
> m_paintTimeGraph.max = max.InMillisecondsF();
> 
> So I thought lets keep it simple stupid and just got with double in
> milliseconds.
> 

I'd prefer if you used TimeDelta consistently throughout the public
PaintTimeCounter API. You'll need something like what you mentioned above to
convert the value from GetPaintTimeOfRecentFrame() in HeadsUpDisplayLayerImpl
anyhow, right?

[nduca, 2013-01-16 22:49:24]

Its okay to leave double for now. We're inconsistent in the codebase. File a bug
for using TimeDelta throughout HUD system. Its strange to mix and match. Better
to do it once.

[reveman, 2013-01-16 22:56:55]

On 2013/01/16 22:49:24, nduca wrote:
> Its okay to leave double for now. We're inconsistent in the codebase. File a
bug
> for using TimeDelta throughout HUD system. Its strange to mix and match.
Better
> to do it once.

yea, cc is inconsistent right now but it would be nice to not introduce a new
class with inconsistencies in the class itself. if 
egraether prefers to file a bug and fix this as part fixing the whole HUD system
that's fine with me though.

[egraether, 2013-01-16 23:27:26]

Resolved the nits and used TimeDelta throughout the API.

If I need to change to TimeDelta anyhow, then I better do it now.

[reveman, 2013-01-16 23:30:53]

thanks! lgtm

[commit-bot: I haz the power, 2013-01-17 00:04:10]

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

[commit-bot: I haz the power, 2013-01-17 03:33:05]

Change committed as 177332