tools/tests/run.sh: download test bench data from Google Storage rather than SVN

tools/tests/run.sh: download test bench data from Google Storage rather than SVN
Committed: https://code.google.com/p/skia/source/detail?r=7749

[epoger, 2013-02-14 20:56:10]

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode79
tools/tests/run.sh:79: # Download a subset of the raw bench data for platform $1
at revision $2.
Toplevel comments:

I did this because, as noted in https://codereview.appspot.com/7336043#msg4 , I
want to add some non-tiled bench data to the self-test.

Originally, I was planning to modify and rerun tarballs/README.sh , adding more
data to the compressed tarballs, but I ran into these two problems:

1. Modifying the tarballs means using up a lot more space in SVN, because the
tarballs are committed as binary files.

2. The way I wrote tarballs/README.sh, even regenerating tarballs with the SAME
DATA AS BEFORE would require an upload of new tarballs, because tar and gzip
have embedded timestamps, usernames, etc. by default.

All in all, rather than chewing up more space in SVN, I figured I would just
make the test script download the raw data from Google Storage.

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode97
tools/tests/run.sh:97: if [ ! -f $DESTFILE ];
As noted at line 80, we don't bother downloading any files we already have. 
(This assumes that the version we downloaded previously was correct.)

I have tested this and it works as expected, at least on my machine.

[benchen, 2013-02-14 22:54:24]

Looks fine to me. Please wait for Eric's confirmation. Thanks.

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode99
tools/tests/run.sh:99:
URL=http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/${PLATFORM}/data/${FILE}
This should be fine for now since we only target Linux. Android data are still
at old location, but maybe soon they'll unify...

[epoger, 2013-02-15 01:44:16]

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode99
tools/tests/run.sh:99:
URL=http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/${PLATFORM}/data/${FILE}
On 2013/02/14 22:54:25, benchen wrote:
> This should be fine for now since we only target Linux. Android data are still
> at old location, but maybe soon they'll unify...

I don't understand this comment about platforms....

- bench_graph_svg.py runs on all platforms, processing bench data captured on
all platforms
- tools/tests/run.sh only runs on Linux (for now; that may change); since it's a
bash script, it should work on either Linux or Mac
- currently, tools/tests/run.sh only uses bench data from
Skia_Shuttle_Ubuntu12_ATI5770_Float_Bench_32 , but my assumption is that bench
data takes the same form on all platforms

At any rate, I think this self-test should be handy for as long as we are still
processing the raw bench data files.

[benchen, 2013-02-15 02:27:10]

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode99
tools/tests/run.sh:99:
URL=http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/${PLATFORM}/data/${FILE}
Got it. What I meant was that Android benches are not under playback/perfdata
subdir yet. But if PLATFORM can only be one of Linux/Mac, this should be fine.
And as you said, the bench data form should be platform-independent.
On 2013/02/15 01:44:17, epoger wrote:
> On 2013/02/14 22:54:25, benchen wrote:
> > This should be fine for now since we only target Linux. Android data are
still
> > at old location, but maybe soon they'll unify...
> 
> I don't understand this comment about platforms....
> 
> - bench_graph_svg.py runs on all platforms, processing bench data captured on
> all platforms
> - tools/tests/run.sh only runs on Linux (for now; that may change); since it's
a
> bash script, it should work on either Linux or Mac
> - currently, tools/tests/run.sh only uses bench data from
> Skia_Shuttle_Ubuntu12_ATI5770_Float_Bench_32 , but my assumption is that bench
> data takes the same form on all platforms
> 
> At any rate, I think this self-test should be handy for as long as we are
still
> processing the raw bench data files.

[epoger, 2013-02-15 07:47:17]

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode99
tools/tests/run.sh:99:
URL=http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/${PLATFORM}/data/${FILE}
On 2013/02/15 02:27:10, benchen wrote:
> Got it. What I meant was that Android benches are not under playback/perfdata
> subdir yet. But if PLATFORM can only be one of Linux/Mac, this should be fine.
> And as you said, the bench data form should be platform-independent.

Cool, sounds like we are on the same page. :-)

Now I just await Eric's thoughts on the "direct download" approach.

[EricB, 2013-02-15 12:24:20]

The approach LGTM.  I have just a couple of questions.

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode81
tools/tests/run.sh:81: # correct and up to date, and we don't download them
again.
This kind of logic always worries me.  I guess we'd never expect this data to
change.  I'm wondering if it's worth verifying that it didn't change, but maybe
that's overkill.

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode94
tools/tests/run.sh:94:
FILES="bench_r${REV}_data_skp_device_bitmap_multi_2_mode_tile_256_256_timeIndividualTiles
bench_r${REV}_data_skp_device_bitmap_multi_3_mode_tile_256_256_timeIndividualTiles
bench_r${REV}_data_skp_device_bitmap_multi_4_mode_tile_256_256_timeIndividualTiles"
Should this be defined at the top?  Also, you mentioned adding non-tiled data,
but I don't see any here.

[epoger, 2013-02-15 15:36:57]

Thanks, Eric.  Please take a look at the followup changes in patchset 2.

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode81
tools/tests/run.sh:81: # correct and up to date, and we don't download them
again.
On 2013/02/15 12:24:20, EricB wrote:
> This kind of logic always worries me.  I guess we'd never expect this data to
> change.  I'm wondering if it's worth verifying that it didn't change, but
maybe
> that's overkill.

I think your fear is correctly placed.  In this case, I think it's reasonable to
leave it as-is until we see a problem.  (Otherwise, it will need to connect to a
server every time to validate that the contents are up-to-date.)

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode94
tools/tests/run.sh:94:
FILES="bench_r${REV}_data_skp_device_bitmap_multi_2_mode_tile_256_256_timeIndividualTiles
bench_r${REV}_data_skp_device_bitmap_multi_3_mode_tile_256_256_timeIndividualTiles
bench_r${REV}_data_skp_device_bitmap_multi_4_mode_tile_256_256_timeIndividualTiles"
On 2013/02/15 12:24:20, EricB wrote:
> Should this be defined at the top?  

At your suggestion, I found a way to move the list definition to the top of the
file.  To do so, I had to separate out the ${REV} references.  Please let me
know whether you like the old way or new way better.

> Also, you mentioned adding non-tiled data,
> but I don't see any here.

Not yet. :-) I wanted to commit this first, demonstrating that the script change
does not affect the results at all.

[EricB, 2013-02-15 15:39:41]

LGTM

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.appspot.com/7343045/diff/1/tools/tests/run.sh#newcode94
tools/tests/run.sh:94:
FILES="bench_r${REV}_data_skp_device_bitmap_multi_2_mode_tile_256_256_timeIndividualTiles
bench_r${REV}_data_skp_device_bitmap_multi_3_mode_tile_256_256_timeIndividualTiles
bench_r${REV}_data_skp_device_bitmap_multi_4_mode_tile_256_256_timeIndividualTiles"
On 2013/02/15 15:36:57, epoger wrote:
> On 2013/02/15 12:24:20, EricB wrote:
> > Should this be defined at the top?  
> 
> At your suggestion, I found a way to move the list definition to the top of
the
> file.  To do so, I had to separate out the ${REV} references.  Please let me
> know whether you like the old way or new way better.
> 

I like the New Way!

> > Also, you mentioned adding non-tiled data,
> > but I don't see any here.
> 
> Not yet. :-) I wanted to commit this first, demonstrating that the script
change
> does not affect the results at all.

Good plan.