Add picture pile raster times histogram

Add picture pile raster times histogram


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

[vmpstr, 2013-02-04 18:32:10]

+danakj

[danakj, 2013-02-04 18:33:19]

https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc#newcode769
cc/tile_manager.cc:769: rasterize_begin_time = base::TimeTicks::HighResNow();
This changed from Now to HighResNow? The ImageDecode doesn't use this. Was this
intended?

https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc#newcode775
cc/tile_manager.cc:775: base::TimeDelta rasterize_duration = rasterize_end_time
- rasterize_begin_time;
80 col. if you drop the "rasterize_" prefix on these you won't have that
problemm and I think it will be just as clear in this context.

[vmpstr, 2013-02-04 18:40:22]

On 2013/02/04 18:33:19, danakj wrote:
> https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
> https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc#newcode769
> cc/tile_manager.cc:769: rasterize_begin_time = base::TimeTicks::HighResNow();
> This changed from Now to HighResNow? The ImageDecode doesn't use this. Was
this
> intended?

This was intentional (see crbug.com/173498), I'll be changing the rest as well.
> 
> https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc#newcode775
> cc/tile_manager.cc:775: base::TimeDelta rasterize_duration =
rasterize_end_time
> - rasterize_begin_time;
> 80 col. if you drop the "rasterize_" prefix on these you won't have that
> problemm and I think it will be just as clear in this context.

Fixed (dropped the prefix)

[vmpstr, 2013-02-04 18:40:36]

ptal

[jamesr, 2013-02-04 18:41:52]

On 2013/02/04 18:40:22, vmpstr1 wrote:
> On 2013/02/04 18:33:19, danakj wrote:
> > https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc
> > File cc/tile_manager.cc (right):
> > 
> >
https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc#newcode769
> > cc/tile_manager.cc:769: rasterize_begin_time =
base::TimeTicks::HighResNow();
> > This changed from Now to HighResNow? The ImageDecode doesn't use this. Was
> this
> > intended?
> 
> This was intentional (see crbug.com/173498), I'll be changing the rest as
well.

That bug is about base::Time::Now(), not base::TimeTicks::Now().  What's the
reason for this change?

[vmpstr, 2013-02-04 18:49:05]

On 2013/02/04 18:41:52, jamesr wrote:
> On 2013/02/04 18:40:22, vmpstr1 wrote:
> > On 2013/02/04 18:33:19, danakj wrote:
> > > https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc
> > > File cc/tile_manager.cc (right):
> > > 
> > >
> https://codereview.chromium.org/12191006/diff/1/cc/tile_manager.cc#newcode769
> > > cc/tile_manager.cc:769: rasterize_begin_time =
> base::TimeTicks::HighResNow();
> > > This changed from Now to HighResNow? The ImageDecode doesn't use this. Was
> > this
> > > intended?
> > 
> > This was intentional (see crbug.com/173498), I'll be changing the rest as
> well.
> 
> That bug is about base::Time::Now(), not base::TimeTicks::Now().  What's the
> reason for this change?

All the timings in that area use TimeTicks, and there is no
base::Time::HighResNow, so I interpreted the bug as "TimeTicks", but I'll
confirm...

[vmpstr, 2013-02-04 18:54:03]

I changed it back to Now (if the bug requires this to be changed, then I'll just
change it as a separate cl)

ptal

[danakj, 2013-02-04 18:56:24]

LGTM

https://codereview.chromium.org/12191006/diff/9002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12191006/diff/9002/cc/tile_manager.cc#newcode775
cc/tile_manager.cc:775: base::TimeDelta duration = end_time - begin_time;
nit: i think i'd drop the "duration" variable, I dont think it's adding much
here, and just put end_time-begin_time on the line below.