cc: Include analysis in raster benchmark costs

cc: Include analysis in raster benchmark costs

Insert analysis costs into the raster benchmark.  This will make it
possible to combine analysis and raster into a single step and be
confident that it will not regress the total costs of rastering a tile.

ATTN perf sheriffs: this will regress benchmarks because it is doing
more work, but it is a more accurate representation of the work that
gets done per unit of raster work.  This should be treated as a "perf
rebaseline" and not as a regression.

R=vmpstr@chromium.org
BUG=none

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

[ernstm, 2013-11-19 22:49:55]

The new_rasterize_and_record benchmark is not yet running on the bots. You need
to make changes to the old benchmark as well. I think the only thing you need to
do is passing a pointer to RenderingStatsInstrumentation to RasterCommon in
PicturePileImpl::RasterForAnalysis.

[enne (OOO), 2013-11-19 22:55:00]

On 2013/11/19 22:49:55, Manfred Ernst wrote:
> The new_rasterize_and_record benchmark is not yet running on the bots. You
need
> to make changes to the old benchmark as well. I think the only thing you need
to
> do is passing a pointer to RenderingStatsInstrumentation to RasterCommon in
> PicturePileImpl::RasterForAnalysis.

Is it going to be running on the bots soon?

[vmpstr, 2013-11-19 22:58:50]

On 2013/11/19 22:55:00, enne wrote:
> On 2013/11/19 22:49:55, Manfred Ernst wrote:
> > The new_rasterize_and_record benchmark is not yet running on the bots. You
> need
> > to make changes to the old benchmark as well. I think the only thing you
need
> to
> > do is passing a pointer to RenderingStatsInstrumentation to RasterCommon in
> > PicturePileImpl::RasterForAnalysis.
> 
> Is it going to be running on the bots soon?

I have to figure out how, then I can do it. For the new rasterize and record,
this lgtm.

[enne (OOO), 2013-11-20 18:01:39]

On 2013/11/19 22:58:50, vmpstr wrote:
> On 2013/11/19 22:55:00, enne wrote:
> > On 2013/11/19 22:49:55, Manfred Ernst wrote:
> > > The new_rasterize_and_record benchmark is not yet running on the bots. You
> > need
> > > to make changes to the old benchmark as well. I think the only thing you
> need
> > to
> > > do is passing a pointer to RenderingStatsInstrumentation to RasterCommon
in
> > > PicturePileImpl::RasterForAnalysis.
> > 
> > Is it going to be running on the bots soon?
> 
> I have to figure out how, then I can do it. For the new rasterize and record,
> this lgtm.

I'll wait on this until the new benchmark lands.

[ernstm, 2013-11-20 22:03:10]

On 2013/11/20 18:01:39, enne wrote:
> On 2013/11/19 22:58:50, vmpstr wrote:
> > On 2013/11/19 22:55:00, enne wrote:
> > > On 2013/11/19 22:49:55, Manfred Ernst wrote:
> > > > The new_rasterize_and_record benchmark is not yet running on the bots.
You
> > > need
> > > > to make changes to the old benchmark as well. I think the only thing you
> > need
> > > to
> > > > do is passing a pointer to RenderingStatsInstrumentation to RasterCommon
> in
> > > > PicturePileImpl::RasterForAnalysis.
> > > 
> > > Is it going to be running on the bots soon?
> > 
> > I have to figure out how, then I can do it. For the new rasterize and
record,
> > this lgtm.
> 
> I'll wait on this until the new benchmark lands.

I would still recommend updating the old benchmark, because it may be a while
until we turn it off and your other patch would then cause a perf regression.

[enne (OOO), 2013-11-20 23:30:42]

On 2013/11/20 22:03:10, Manfred Ernst wrote:

> > I'll wait on this until the new benchmark lands.
> 
> I would still recommend updating the old benchmark, because it may be a while
> until we turn it off and your other patch would then cause a perf regression.

This is a lot more complicated to do in the old path raster and record
benchmark, because it does all the timing inside of RasterCommon, and then both
analysis and raster call that function.  Do you have any recommendations on how
I should refactor the old path to make this work?

[enne (OOO), 2013-11-21 18:51:37]

In particular, should I take the best analysis time and add it to the best
raster time?  Or do both and take the best sum since that's going to be closest
to the new path?

[enne (OOO), 2013-12-04 20:23:11]

ernstm, what do you think of this?

[enne (OOO), 2013-12-06 21:20:04]

Oh, I guess I should CC ernstm first in order to get a reply.  :)

[ernstm, 2013-12-07 00:30:25]

On 2013/12/06 21:20:04, enne wrote:
> Oh, I guess I should CC ernstm first in order to get a reply.  :)

It's not really necessary to do exactly what the new benchmark does (there are
minor differences anyway). You can add the best analysis time to the rendering
stats separately from the best (actual) raster time. If you pass in a pointer to
rendering_stats_instrumentation to RasterForAnalysis and from there to
RasterCommon, the benchmarking code in RasterCommon should do that. Taking the
min of each and adding them separately will probably have slightly lower
variance compared to taking the min of the sum.

[enne (OOO), 2013-12-10 19:18:23]

On 2013/12/07 00:30:25, Manfred Ernst wrote:
> On 2013/12/06 21:20:04, enne wrote:
> > Oh, I guess I should CC ernstm first in order to get a reply.  :)
> 
> It's not really necessary to do exactly what the new benchmark does (there are
> minor differences anyway). You can add the best analysis time to the rendering
> stats separately from the best (actual) raster time. If you pass in a pointer
to
> rendering_stats_instrumentation to RasterForAnalysis and from there to
> RasterCommon, the benchmarking code in RasterCommon should do that. Taking the
> min of each and adding them separately will probably have slightly lower
> variance compared to taking the min of the sum.

Isn't adding the best analysis time and the best raster time what I'm doing
here?

[ernstm, 2013-12-10 20:37:41]

On 2013/12/10 19:18:23, enne wrote:
> On 2013/12/07 00:30:25, Manfred Ernst wrote:
> > On 2013/12/06 21:20:04, enne wrote:
> > > Oh, I guess I should CC ernstm first in order to get a reply.  :)
> > 
> > It's not really necessary to do exactly what the new benchmark does (there
are
> > minor differences anyway). You can add the best analysis time to the
rendering
> > stats separately from the best (actual) raster time. If you pass in a
pointer
> to
> > rendering_stats_instrumentation to RasterForAnalysis and from there to
> > RasterCommon, the benchmarking code in RasterCommon should do that. Taking
the
> > min of each and adding them separately will probably have slightly lower
> > variance compared to taking the min of the sum.
> 
> Isn't adding the best analysis time and the best raster time what I'm doing
> here?

Sorry for the confusion. I thought this patch was only making the changes to the
new benchmark and you were asking for input on how to adapt the old benchmark.
Didn't realize that these changes are already part of this patch. So yes, your
patch does exactly what I've described above. LGTM.

[commit-bot: I haz the power, 2013-12-10 21:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/74713006/120001

[commit-bot: I haz the power, 2013-12-10 21:43:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-13 01:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/74713006/140001

[commit-bot: I haz the power, 2013-12-13 08:20:20]

Change committed as 240540