Provide an option to bench drawing individual tiles in bench_pictures.

Provide an option to bench drawing individual tiles in bench_pictures.

Provides output like the following:
running bench [1236 12045] androidpolice.skp
  tile_256x256: tile [0,0] out of [5,48]: msecs =   1.00
  tile_256x256: tile [1,0] out of [5,48]: msecs =   1.50
  tile_256x256: tile [2,0] out of [5,48]: msecs =   1.00
  tile_256x256: tile [3,0] out of [5,48]: msecs =   1.50
  tile_256x256: tile [4,0] out of [5,48]: msecs =   2.50
  tile_256x256: tile [0,1] out of [5,48]: msecs =   2.00
  tile_256x256: tile [1,1] out of [5,48]: msecs =   3.50
  tile_256x256: tile [2,1] out of [5,48]: msecs =   3.50
  tile_256x256: tile [3,1] out of [5,48]: msecs =   6.00
  tile_256x256: tile [4,1] out of [5,48]: msecs =   2.50
  tile_256x256: tile [0,2] out of [5,48]: msecs =   2.00

BUG=https://code.google.com/p/skia/issues/detail?id=1016

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

[Leon, 2012-12-13 18:38:12]

Please let me know if this satisfies the goal, and if the output should be
modified.

[benchen, 2012-12-13 19:35:28]

Thanks Leon. In terms of output format, I see that there won't be repetitions,
so any of avg, min, med, 25th should just return the max tile time.
And I guess non-tiled benches will stay the same. So, will need to change
parse() and listAlgorithm() at
https://code.google.com/p/skia/source/browse/trunk/bench/bench_util.py to handle
both cases, in order not to break anything. The RE's will probably be trickier.
Can we assume there's only walltime in tiled output? Or there can be CPU time?
I'm on vacation this afternoon, but plan to work on the parsing tomorrow (while
you guys are watching the movie).

On 2012/12/13 18:38:12, scroggo-work wrote:
> Please let me know if this satisfies the goal, and if the output should be
> modified.

[EricB, 2012-12-13 19:39:28]

Kudos for the quick turnaround.  LGTM but want to make sure this format works
for Ben.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:320: * Tells drawOneTile to draw the next tile from the
one it is currently drawing. Must be called
Maybe this should read something like, "Move to the next tile and return its
indices"?  The current wording is a bit confusing.

[DerekS, 2012-12-13 19:55:25]

https://codereview.appspot.com/6937047/diff/2001/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureBenchmark.cpp#n...
tools/PictureBenchmark.cpp:109: result.append("\n");
the first time through this loop it is printed on the line with any previous
output (according to your CL description) can we ensure that the first line is
on its own line like all the others?

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:362: bool TiledPictureRenderer::tileDimensions(int &x,
int &y) {
if the tile dimensions are reset shouldn't you reset fNextSingleTile to -1?

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:329: bool nextTile(int& i, int& j);
perhaps you could call this setCurrentTile and rename drawOneTile to
drawCurrentTile.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:334: * It is an error to call this without calling
nextTile, or if nextTile returns false.
without *first* calling nextTile.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:353: int    fNextSingleTile;
fCurrentTileOffset?

[Leon, 2012-12-13 20:29:06]

> Thanks Leon. In terms of output format, I see that there won't be repetitions,
> so any of avg, min, med, 25th should just return the max tile time.

I don't follow. It will do the same number of repetitions as the normal mode,
only
it will render each tile fRepeats times.

> And I guess non-tiled benches will stay the same. So, will need to change

Yes, and in fact this does not change the normal tiled mode; it is only
different
if --timeIndividualTiles is passed as an argument.

> parse() and listAlgorithm() at
> https://code.google.com/p/skia/source/browse/trunk/bench/bench_util.py to
handle
> both cases, in order not to break anything. The RE's will probably be
trickier.

What are RE's? Nothing will break at least until we actually turn this on, which
will be
a separate change to the buildbot scripts.

> Can we assume there's only walltime in tiled output? Or there can be CPU time?

I don't know of any reason to assume that walltime is the only relevant time.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureBenchmark.cpp
File tools/PictureBenchmark.cpp (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureBenchmark.cpp#n...
tools/PictureBenchmark.cpp:109: result.append("\n");
On 2012/12/13 19:55:25, DerekS wrote:
> the first time through this loop it is printed on the line with any previous
> output (according to your CL description) can we ensure that the first line is
> on its own line like all the others?

Done.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:362: bool TiledPictureRenderer::tileDimensions(int &x,
int &y) {
On 2012/12/13 19:55:25, DerekS wrote:
> if the tile dimensions are reset shouldn't you reset fNextSingleTile to -1?

I'm not sure what you're suggesting. fNextSingleTile does get set to -1 in
init(), which also sets up fTilesX and fTilesY (indirectly, by calling
setupTiles or setupPowerOf2Tiles). Does that address your concern?

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:320: * Tells drawOneTile to draw the next tile from the
one it is currently drawing. Must be called
On 2012/12/13 19:39:28, EricB wrote:
> Maybe this should read something like, "Move to the next tile and return its
> indices"?  The current wording is a bit confusing.

Done. Much better!

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:329: bool nextTile(int& i, int& j);
On 2012/12/13 19:55:25, DerekS wrote:
> perhaps you could call this setCurrentTile and rename drawOneTile to
> drawCurrentTile.

I don't like the name setCurrentTile since it doesn't allow you to arbitrarily
set it. Maybe setCurrentTileToNext?

I have changed drawOneTile.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:334: * It is an error to call this without calling
nextTile, or if nextTile returns false.
On 2012/12/13 19:55:25, DerekS wrote:
> without *first* calling nextTile. 

Done.

https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:353: int    fNextSingleTile;
On 2012/12/13 19:55:25, DerekS wrote:
> fCurrentTileOffset?

Done.

[benchen, 2012-12-13 20:58:01]

Leon, please provide me with sample of all possible outputs. The one you gave in
the email only had walltime and one repetition I think. Would like to know all
possible cases in order to get prepared.

Thanks,
Ben

On 2012/12/13 20:29:06, scroggo-work wrote:
> > Thanks Leon. In terms of output format, I see that there won't be
repetitions,
> > so any of avg, min, med, 25th should just return the max tile time.
> 
> I don't follow. It will do the same number of repetitions as the normal mode,
> only
> it will render each tile fRepeats times.
> 
> > And I guess non-tiled benches will stay the same. So, will need to change
> 
> Yes, and in fact this does not change the normal tiled mode; it is only
> different
> if --timeIndividualTiles is passed as an argument.
> 
> > parse() and listAlgorithm() at
> > https://code.google.com/p/skia/source/browse/trunk/bench/bench_util.py to
> handle
> > both cases, in order not to break anything. The RE's will probably be
> trickier.
> 
> What are RE's? Nothing will break at least until we actually turn this on,
which
> will be
> a separate change to the buildbot scripts.
> 
> > Can we assume there's only walltime in tiled output? Or there can be CPU
time?
> 
> I don't know of any reason to assume that walltime is the only relevant time.
> 
> https://codereview.appspot.com/6937047/diff/2001/tools/PictureBenchmark.cpp
> File tools/PictureBenchmark.cpp (right):
> 
>
https://codereview.appspot.com/6937047/diff/2001/tools/PictureBenchmark.cpp#n...
> tools/PictureBenchmark.cpp:109: result.append("\n");
> On 2012/12/13 19:55:25, DerekS wrote:
> > the first time through this loop it is printed on the line with any previous
> > output (according to your CL description) can we ensure that the first line
is
> > on its own line like all the others?
> 
> Done.
> 
> https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.cpp
> File tools/PictureRenderer.cpp (right):
> 
>
https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.cpp#ne...
> tools/PictureRenderer.cpp:362: bool TiledPictureRenderer::tileDimensions(int
&x,
> int &y) {
> On 2012/12/13 19:55:25, DerekS wrote:
> > if the tile dimensions are reset shouldn't you reset fNextSingleTile to -1?
> 
> I'm not sure what you're suggesting. fNextSingleTile does get set to -1 in
> init(), which also sets up fTilesX and fTilesY (indirectly, by calling
> setupTiles or setupPowerOf2Tiles). Does that address your concern?
> 
> https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h
> File tools/PictureRenderer.h (right):
> 
>
https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
> tools/PictureRenderer.h:320: * Tells drawOneTile to draw the next tile from
the
> one it is currently drawing. Must be called
> On 2012/12/13 19:39:28, EricB wrote:
> > Maybe this should read something like, "Move to the next tile and return its
> > indices"?  The current wording is a bit confusing.
> 
> Done. Much better!
> 
>
https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
> tools/PictureRenderer.h:329: bool nextTile(int& i, int& j);
> On 2012/12/13 19:55:25, DerekS wrote:
> > perhaps you could call this setCurrentTile and rename drawOneTile to
> > drawCurrentTile.
> 
> I don't like the name setCurrentTile since it doesn't allow you to arbitrarily
> set it. Maybe setCurrentTileToNext?
> 
> I have changed drawOneTile.
> 
>
https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
> tools/PictureRenderer.h:334: * It is an error to call this without calling
> nextTile, or if nextTile returns false.
> On 2012/12/13 19:55:25, DerekS wrote:
> > without *first* calling nextTile. 
> 
> Done.
> 
>
https://codereview.appspot.com/6937047/diff/2001/tools/PictureRenderer.h#newc...
> tools/PictureRenderer.h:353: int    fNextSingleTile;
> On 2012/12/13 19:55:25, DerekS wrote:
> > fCurrentTileOffset?
> 
> Done.

[DerekS, 2012-12-13 21:06:54]

lgtm