Issue 6937047: Provide an option to bench drawing individual tiles in bench_pictures.

Issue 6937047: Provide an option to bench drawing individual tiles in bench_pictures. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 9 months ago by Leon

Modified:
11 years, 9 months ago

Reviewers:
EricB, DerekS, benchen

CC:
skia-review_googlegroups.com

Base URL:
https://skia.googlecode.com/svn/trunk

Visibility:
Public.

More Reviews

Description

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

Patch Set 1 #

Patch Set 2 : Exit early if timeIndividualTiles is set without tile mode. #

Total comments: 12

Patch Set 3 : Improve a comment. #

Patch Set 4 : Address comments. #

Created: 11 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+193 lines, -43 lines)			Patch
M	tools/PictureBenchmark.h	View	1	2 chunks	+14 lines, -0 lines	0 comments	Download
M	tools/PictureBenchmark.cpp	View	1 2 3	3 chunks	+61 lines, -25 lines	0 comments	Download
M	tools/PictureRenderer.h	View	1 2 3	4 chunks	+52 lines, -5 lines	0 comments	Download
M	tools/PictureRenderer.cpp	View	1 2 3	7 chunks	+45 lines, -7 lines	0 comments	Download
M	tools/bench_pictures_main.cpp	View	1	5 chunks	+21 lines, -6 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

Leon

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

11 years, 9 months ago (2012-12-13 18:38:12 UTC) #1

benchen

Thanks Leon. In terms of output format, I see that there won't be repetitions, so ...

11 years, 9 months ago (2012-12-13 19:35:28 UTC) #2

EricB

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

11 years, 9 months ago (2012-12-13 19:39:28 UTC) #3

DerekS

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#newcode109 tools/PictureBenchmark.cpp:109: result.append("\n"); the first time through this loop it is ...

11 years, 9 months ago (2012-12-13 19:55:25 UTC) #4

Leon

> Thanks Leon. In terms of output format, I see that there won't be repetitions, ...

11 years, 9 months ago (2012-12-13 20:29:06 UTC) #5

> 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

Leon, please provide me with sample of all possible outputs. The one you gave in ...

11 years, 9 months ago (2012-12-13 20:58:01 UTC) #6

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.

lgtm

Expand All Messages | Collapse All Messages