Change the method for timing individual tiles in bench_pictures.

Change the method for timing individual tiles in bench_pictures.

When timing individual tiles in bench_pictures, keep a timer running
across all repeats, and then take the average. The former method of
timing each iteration separately runs into precision errors on some
platforms.

Running on my Mac Pro with OSX 10.8, the cmsecs for the new method
and the old method are roughly the same when checking the CPU time.
When checking the wall time, the old method often gives me 0ms,
while the new method gives me a larger value. I don't think this
can be entirely attributed to rounding though, since on occasion I
see the old method showing a short time period (.05 - .15ms) while
the new method shows .15ms higher (which is in range for the
difference I'm seeing for other tiles where the old method reports
0ms).

Some other changes:
PictureRenderer::resetState now takes a boolean parameter. If called
with false, it will only do a flush, while if called with true, it
will also call finish.

resetState is now called with true everywhere except in between
iterations of drawing the same tile (when timing individual tiles).

render_pictures_main no longer calls resetState directly, since it
already calls end, which calls resetState.

BUG=http://code.google.com/p/skia/issues/detail?id=1066

Committed: https://code.google.com/p/skia/source/detail?r=7424

[Leon, 2013-01-15 20:38:32]

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:108: SkAutoTDelete<BenchTimer>
longRunningTimer(this->setupTimer());
I've included both of these timers since it was requested that we compare the
two. Do we want both of them turned on? Do we perhaps want a way to turn on one
or both at compile or run time?

Also, if anyone has a better name, I am completely open to suggestion.

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:118: tiledRenderer->resetState(false);
bsalomon@, I just wanted to make sure, does this look like the right behavior?
After each iteration of drawing one tile, this calls flush.

On line 122, after drawing the same tile for all iterations, I call
resetState(true) which calls flush and finish.

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:137: configName.append(" <averaged>");
Again, if we want both timers, let me know how we can distinguish the two (right
now I just appended " <averaged>" to the already long name "tile_WxH: tile [i,
j] out of [I, J]").

[bsalomon, 2013-01-15 20:42:50]

The glFinish aspect of the change LGTM.

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:118: tiledRenderer->resetState(false);
On 2013/01/15 20:38:32, scroggo-work wrote:
> bsalomon@, I just wanted to make sure, does this look like the right behavior?
> After each iteration of drawing one tile, this calls flush.
> 
> On line 122, after drawing the same tile for all iterations, I call
> resetState(true) which calls flush and finish.

looks right to me.

[EricB, 2013-01-15 21:34:44]

LGTM for the most part, but I think we need to really nail down how this will
interact with the bench dashboard.

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:108: SkAutoTDelete<BenchTimer>
longRunningTimer(this->setupTimer());
On 2013/01/15 20:38:32, scroggo-work wrote:
> I've included both of these timers since it was requested that we compare the
> two. Do we want both of them turned on? Do we perhaps want a way to turn on
one
> or both at compile or run time?
> 
> Also, if anyone has a better name, I am completely open to suggestion.

I think it'd be nice to have both, but we need to be sure that bensong@'s parser
can handle it.  As for the names, I think "perTileTimer" would be more
informative than "timer."

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:137: configName.append(" <averaged>");
On 2013/01/15 20:38:32, scroggo-work wrote:
> Again, if we want both timers, let me know how we can distinguish the two
(right
> now I just appended " <averaged>" to the already long name "tile_WxH: tile [i,
> j] out of [I, J]").

If I understand correctly, the averaged numbers will show up as a different
config on the dashboard?  I think that makes sense, but which timer gets to take
the existing name is worth some discussion.

[Leon, 2013-01-15 21:56:01]

I also hacked in some code to compare the total time for drawing each tile
individually with drawing all the tiles. There is some variance, but the two
numbers tend to be very close, supporting my belief that I'm timing what I
should be timing.

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:108: SkAutoTDelete<BenchTimer>
longRunningTimer(this->setupTimer());
On 2013/01/15 21:34:44, EricB wrote:
> On 2013/01/15 20:38:32, scroggo-work wrote:
> > I've included both of these timers since it was requested that we compare
the
> > two. Do we want both of them turned on? Do we perhaps want a way to turn on
> one
> > or both at compile or run time?
> > 
> > Also, if anyone has a better name, I am completely open to suggestion.
> 
> I think it'd be nice to have both, but we need to be sure that bensong@'s
parser
> can handle it.  As for the names, I think "perTileTimer" would be more
> informative than "timer."

Done.

https://codereview.appspot.com/7101060/diff/1/tools/PictureBenchmark.cpp#newc...
tools/PictureBenchmark.cpp:137: configName.append(" <averaged>");
On 2013/01/15 21:34:44, EricB wrote:
> On 2013/01/15 20:38:32, scroggo-work wrote:
> > Again, if we want both timers, let me know how we can distinguish the two
> (right
> > now I just appended " <averaged>" to the already long name "tile_WxH: tile
[i,
> > j] out of [I, J]").
> 
> If I understand correctly, the averaged numbers will show up as a different
> config on the dashboard?  I think that makes sense, but which timer gets to
take
> the existing name is worth some discussion.

That was my intention, yes, although I suppose Ben can pick and choose which
information he wants to show. I just wanted to make sure the two were reported
differently.

[Leon, 2013-01-15 22:00:11]

On 2013/01/15 21:56:01, scroggo-work wrote:
> I also hacked in some code to compare the total time for drawing each tile
> individually with drawing all the tiles. There is some variance, but the two
> numbers tend to be very close, supporting my belief that I'm timing what I
> should be timing.

This part is all done locally, but could be added if we want that as well.

On an unrelated note, tomorrow I'm on vacation until 1/28. If this isn't ready
to go in before I leave today, feel free to to 'git cl patch 7101060' and
continue working on it/check it in (you'll also have to run 'git cl issue 0' in
order to detach your branch from this code review, since you won't be able
modify this Issue).

[benchen, 2013-01-15 22:08:26]

LGTM. On the parsing side, if the non-per-tile and the per-tile ones keep the
previous formats, nothing should break.

On 2013/01/15 22:00:11, scroggo-work wrote:
> On 2013/01/15 21:56:01, scroggo-work wrote:
> > I also hacked in some code to compare the total time for drawing each tile
> > individually with drawing all the tiles. There is some variance, but the two
> > numbers tend to be very close, supporting my belief that I'm timing what I
> > should be timing.
> 
> This part is all done locally, but could be added if we want that as well.
> 
> On an unrelated note, tomorrow I'm on vacation until 1/28. If this isn't ready
> to go in before I leave today, feel free to to 'git cl patch 7101060' and
> continue working on it/check it in (you'll also have to run 'git cl issue 0'
in
> order to detach your branch from this code review, since you won't be able
> modify this Issue).

[EricB, 2013-01-15 22:09:17]

LGTM as well.

[Leon, 2013-01-15 23:00:45]

On 2013/01/15 22:09:17, EricB wrote:
> LGTM as well.

I'm afraid to check in while the build is broken and I'm leaving tomorrow for a
week and a half. Someone please check this in for me.

[Leon, 2013-01-28 20:41:13]

On 2013/01/15 23:00:45, scroggo-work wrote:
> On 2013/01/15 22:09:17, EricB wrote:
> > LGTM as well.
> 
> I'm afraid to check in while the build is broken and I'm leaving tomorrow for
a
> week and a half. Someone please check this in for me.

Now that I'm back, I have checked it in.