Make SKP bench JSON ouput better

Make SKP bench JSON ouput better

BUG=skia:
NOTREECHECKS=true

Committed: https://skia.googlesource.com/skia/+/4d1a364e391a8fd79c9e133d27957761d35068d0

[kelvinly, 2014-06-19 21:44:49]

Current JSON output looks like this:
{"buildNumber":-1,"gitHash":"001011010101","gitNumber":-1,"isTrybot":false,"key":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_tabl_ukwsj.skp_simple_viewport_1000x1000_wall","params":{"arch":"x86_64","benchName":"tabl_ukwsj.skp","builderName":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release","configuration":"Release","gpu":"GTX660","measurementType":"wall","mode":"simple","model":"ShuttleA","os":"Ubuntu12","role":"Perf","scale":1.0,"viewport":"1000x1000"},"timestamp":1410065408,"value":113.62847630}

[kelvinly, 2014-06-19 21:51:19]

On 2014/06/19 21:44:49, kelvinly wrote:
> Current JSON output looks like this:
>
{"buildNumber":-1,"gitHash":"001011010101","gitNumber":-1,"isTrybot":false,"key":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_tabl_ukwsj.skp_simple_viewport_1000x1000_wall","params":{"arch":"x86_64","benchName":"tabl_ukwsj.skp","builderName":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release","configuration":"Release","gpu":"GTX660","measurementType":"wall","mode":"simple","model":"ShuttleA","os":"Ubuntu12","role":"Perf","scale":1.0,"viewport":"1000x1000"},"timestamp":1410065408,"value":113.62847630}

Hmm, this was run with 
skia/out/Debug/bench_pictures -r
$BUILDBOT/buildbot/third_party/chromium_buildbot/slave/Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release/build/playback/skps
--config 8888 --viewport 1000 1000 --timers wg --logFile testlog --jsonLog
testjson --repeat 10 --logPerIter --builderName
Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release --gitHash 001011010101 --timestamp
10000000000 --gitNumber -1 

So I'm not sure why the timestamp isn't 100000000000....

[benchen, 2014-06-19 22:37:44]

On 2014/06/19 21:51:19, kelvinly wrote:
> On 2014/06/19 21:44:49, kelvinly wrote:
> > Current JSON output looks like this:
> >
>
{"buildNumber":-1,"gitHash":"001011010101","gitNumber":-1,"isTrybot":false,"key":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_tabl_ukwsj.skp_simple_viewport_1000x1000_wall","params":{"arch":"x86_64","benchName":"tabl_ukwsj.skp","builderName":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release","configuration":"Release","gpu":"GTX660","measurementType":"wall","mode":"simple","model":"ShuttleA","os":"Ubuntu12","role":"Perf","scale":1.0,"viewport":"1000x1000"},"timestamp":1410065408,"value":113.62847630}
> 
> Hmm, this was run with 
> skia/out/Debug/bench_pictures -r
>
$BUILDBOT/buildbot/third_party/chromium_buildbot/slave/Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release/build/playback/skps
> --config 8888 --viewport 1000 1000 --timers wg --logFile testlog --jsonLog
> testjson --repeat 10 --logPerIter --builderName
> Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release --gitHash 001011010101
--timestamp
> 10000000000 --gitNumber -1 
> 
> So I'm not sure why the timestamp isn't 100000000000....

The reason is probably that 10000000000 exceeds int32..

[kelvinly, 2014-06-19 22:41:19]

On 2014/06/19 22:37:44, benchen wrote:
> On 2014/06/19 21:51:19, kelvinly wrote:
> > On 2014/06/19 21:44:49, kelvinly wrote:
> > > Current JSON output looks like this:
> > >
> >
>
{"buildNumber":-1,"gitHash":"001011010101","gitNumber":-1,"isTrybot":false,"key":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_tabl_ukwsj.skp_simple_viewport_1000x1000_wall","params":{"arch":"x86_64","benchName":"tabl_ukwsj.skp","builderName":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release","configuration":"Release","gpu":"GTX660","measurementType":"wall","mode":"simple","model":"ShuttleA","os":"Ubuntu12","role":"Perf","scale":1.0,"viewport":"1000x1000"},"timestamp":1410065408,"value":113.62847630}
> > 
> > Hmm, this was run with 
> > skia/out/Debug/bench_pictures -r
> >
>
$BUILDBOT/buildbot/third_party/chromium_buildbot/slave/Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release/build/playback/skps
> > --config 8888 --viewport 1000 1000 --timers wg --logFile testlog --jsonLog
> > testjson --repeat 10 --logPerIter --builderName
> > Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release --gitHash 001011010101
> --timestamp
> > 10000000000 --gitNumber -1 
> > 
> > So I'm not sure why the timestamp isn't 100000000000....
> 
> The reason is probably that 10000000000 exceeds int32..

Ah, that makes sense, my bad

[jcgregorio, 2014-06-20 00:47:56]

On 2014/06/19 21:44:49, kelvinly wrote:
> Current JSON output looks like this:
>
{"buildNumber":-1,"gitHash":"001011010101","gitNumber":-1,"isTrybot":false,"key":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_tabl_ukwsj.skp_simple_viewport_1000x1000_wall","params":{"arch":"x86_64","benchName":"tabl_ukwsj.skp","builderName":"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release","configuration":"Release","gpu":"GTX660","measurementType":"wall","mode":"simple","model":"ShuttleA","os":"Ubuntu12","role":"Perf","scale":1.0,"viewport":"1000x1000"},"timestamp":1410065408,"value":113.62847630}

I thought we were dropping viewport?

[jcgregorio, 2014-06-20 00:52:14]

Before this gets committed I want to look at possibly changing the schema one
last time to use a repeated record to hold the key, value pairs in params.

I.e. make params a repeated record of param{key: string, value: string}

That way we can use the same schema for both bench and skps and won't have to
change the schema every time we add/remove params.
It might make the queries a little harder to construct, and that's what I want
to experiment with.

[kelvinly, 2014-06-20 01:16:40]

On 2014/06/20 00:52:14, jcgregorio wrote:
> Before this gets committed I want to look at possibly changing the schema one
> last time to use a repeated record to hold the key, value pairs in params.
> 
> I.e. make params a repeated record of param{key: string, value: string}
> 
> That way we can use the same schema for both bench and skps and won't have to
> change the schema every time we add/remove params.
> It might make the queries a little harder to construct, and that's what I want
> to experiment with.

Sounds good, I'll patch that in tomorrow. Good luck with the {key, value} set
up, I'm not entirely sure you can query successfully with that set up, so it
should be a good challenge!

A slightly bigger problem: this change needs to be committed on all the bots at
the same exact time as the change on the buildbots to support this (this one
will probably crash if it doesn't get the arguments, and the old bench_pictures
doesn't seem to like getting the extra arguments, and the new format will also
break the old upload step on the buildbots). If the code isn't updated on the
same run it'll definitely fail. Any thoughts on how to commit these
successfully?

[benchen, 2014-06-20 12:14:47]

On 2014/06/20 01:16:40, kelvinly wrote:
> On 2014/06/20 00:52:14, jcgregorio wrote:
> > Before this gets committed I want to look at possibly changing the schema
one
> > last time to use a repeated record to hold the key, value pairs in params.
> > 
> > I.e. make params a repeated record of param{key: string, value: string}
> > 
> > That way we can use the same schema for both bench and skps and won't have
to
> > change the schema every time we add/remove params.
> > It might make the queries a little harder to construct, and that's what I
want
> > to experiment with.
> 
> Sounds good, I'll patch that in tomorrow. Good luck with the {key, value} set
> up, I'm not entirely sure you can query successfully with that set up, so it
> should be a good challenge!
> 
> A slightly bigger problem: this change needs to be committed on all the bots
at
> the same exact time as the change on the buildbots to support this (this one
> will probably crash if it doesn't get the arguments, and the old
bench_pictures
> doesn't seem to like getting the extra arguments, and the new format will also
> break the old upload step on the buildbots). If the code isn't updated on the
> same run it'll definitely fail. Any thoughts on how to commit these
> successfully?

I guess the skia tree will basically be closed for today. How about doing this
late today or over the weekend, closing the tree (after it reopens), waiting for
things to settle, commiting the buildbot change, and reopening the tree. Maybe
check with Eric for better ideas.

[kelvinly, 2014-06-20 15:28:33]

It works!
Latest schema output (same parameters as last run)
{
    "gitNumber": -1, 
    "gitHash": "001011010101", 
    "timestamp": 1410065408, 
    "value": 10.0809463, 
    "buildNumber": -1, 
    "params": [
        {
            "value": "x86_64", 
            "key": "arch"
        }, 
        {
            "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
            "key": "builderName"
        }, 
        {
            "value": "Release", 
            "key": "configuration"
        }, 
        {
            "value": "GTX660", 
            "key": "gpu"
        }, 
        {
            "value": "ShuttleA", 
            "key": "model"
        }, 
        {
            "value": "Ubuntu12", 
            "key": "os"
        }, 
        {
            "value": "Perf", 
            "key": "role"
        }, 
        {
            "value": {
                "value": "simple", 
                "key": "mode"
            }, 
            "key": "0"
        }, 
        {
            "value": {
                "value": 1.0, 
                "key": "scale"
            }, 
            "key": "1"
        }, 
        {
            "value": {
                "value": "1000x1000", 
                "key": "viewport"
            }, 
            "key": "2"
        }, 
        {
            "value": "desk_yahooanswers.skp", 
            "key": "benchName"
        }, 
        {
            "value": "wall", 
            "key": "measurementType"
        }
    ], 
    "key":
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",

    "isTrybot": false
}

[kelvinly, 2014-06-20 15:31:36]

On 2014/06/20 15:28:33, kelvinly wrote:
> It works!
> Latest schema output (same parameters as last run)
> {
>     "gitNumber": -1, 
>     "gitHash": "001011010101", 
>     "timestamp": 1410065408, 
>     "value": 10.0809463, 
>     "buildNumber": -1, 
>     "params": [
>         {
>             "value": "x86_64", 
>             "key": "arch"
>         }, 
>         {
>             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
>             "key": "builderName"
>         }, 
>         {
>             "value": "Release", 
>             "key": "configuration"
>         }, 
>         {
>             "value": "GTX660", 
>             "key": "gpu"
>         }, 
>         {
>             "value": "ShuttleA", 
>             "key": "model"
>         }, 
>         {
>             "value": "Ubuntu12", 
>             "key": "os"
>         }, 
>         {
>             "value": "Perf", 
>             "key": "role"
>         }, 
>         {
>             "value": {
>                 "value": "simple", 
>                 "key": "mode"
>             }, 
>             "key": "0"
>         }, 
>         {
>             "value": {
>                 "value": 1.0, 
>                 "key": "scale"
>             }, 
>             "key": "1"
>         }, 
>         {
>             "value": {
>                 "value": "1000x1000", 
>                 "key": "viewport"
>             }, 
>             "key": "2"
>         }, 
>         {
>             "value": "desk_yahooanswers.skp", 
>             "key": "benchName"
>         }, 
>         {
>             "value": "wall", 
>             "key": "measurementType"
>         }
>     ], 
>     "key":
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> 
>     "isTrybot": false
> }

It doesn't work! So I didn't actually read the whole output before posting,
gonna have to fix those last couple values..

[kelvinly, 2014-06-20 17:39:06]

On 2014/06/20 15:31:36, kelvinly wrote:
> On 2014/06/20 15:28:33, kelvinly wrote:
> > It works!
> > Latest schema output (same parameters as last run)
> > {
> >     "gitNumber": -1, 
> >     "gitHash": "001011010101", 
> >     "timestamp": 1410065408, 
> >     "value": 10.0809463, 
> >     "buildNumber": -1, 
> >     "params": [
> >         {
> >             "value": "x86_64", 
> >             "key": "arch"
> >         }, 
> >         {
> >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> >             "key": "builderName"
> >         }, 
> >         {
> >             "value": "Release", 
> >             "key": "configuration"
> >         }, 
> >         {
> >             "value": "GTX660", 
> >             "key": "gpu"
> >         }, 
> >         {
> >             "value": "ShuttleA", 
> >             "key": "model"
> >         }, 
> >         {
> >             "value": "Ubuntu12", 
> >             "key": "os"
> >         }, 
> >         {
> >             "value": "Perf", 
> >             "key": "role"
> >         }, 
> >         {
> >             "value": {
> >                 "value": "simple", 
> >                 "key": "mode"
> >             }, 
> >             "key": "0"
> >         }, 
> >         {
> >             "value": {
> >                 "value": 1.0, 
> >                 "key": "scale"
> >             }, 
> >             "key": "1"
> >         }, 
> >         {
> >             "value": {
> >                 "value": "1000x1000", 
> >                 "key": "viewport"
> >             }, 
> >             "key": "2"
> >         }, 
> >         {
> >             "value": "desk_yahooanswers.skp", 
> >             "key": "benchName"
> >         }, 
> >         {
> >             "value": "wall", 
> >             "key": "measurementType"
> >         }
> >     ], 
> >     "key":
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > 
> >     "isTrybot": false
> > }
> 
> It doesn't work! So I didn't actually read the whole output before posting,
> gonna have to fix those last couple values..

Data point looks like this now:
{
    "gitNumber": -1, 
    "gitHash": "001011010101", 
    "timestamp": 1410065408, 
    "value": 10.1490013, 
    "buildNumber": -1, 
    "params": [
        {
            "value": "x86_64", 
            "key": "arch"
        }, 
        {
            "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
            "key": "builderName"
        }, 
        {
            "value": "Release", 
            "key": "configuration"
        }, 
        {
            "value": "GTX660", 
            "key": "gpu"
        }, 
        {
            "value": "ShuttleA", 
            "key": "model"
        }, 
        {
            "value": "Ubuntu12", 
            "key": "os"
        }, 
        {
            "value": "Perf", 
            "key": "role"
        }, 
        {
            "value": "simple", 
            "key": "mode"
        }, 
        {
            "value": 1.0, 
            "key": "scale"
        }, 
        {
            "value": "1000x1000", 
            "key": "viewport"
        }, 
        {
            "value": "desk_yahooanswers.skp", 
            "key": "benchName"
        }, 
        {
            "value": "wall", 
            "key": "measurementType"
        }
    ], 
    "key":
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",

    "isTrybot": false
}

[benchen, 2014-06-20 17:42:04]

On 2014/06/20 17:39:06, kelvinly wrote:
> On 2014/06/20 15:31:36, kelvinly wrote:
> > On 2014/06/20 15:28:33, kelvinly wrote:
> > > It works!
> > > Latest schema output (same parameters as last run)
> > > {
> > >     "gitNumber": -1, 
> > >     "gitHash": "001011010101", 
> > >     "timestamp": 1410065408, 
> > >     "value": 10.0809463, 
> > >     "buildNumber": -1, 
> > >     "params": [
> > >         {
> > >             "value": "x86_64", 
> > >             "key": "arch"
> > >         }, 
> > >         {
> > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> > >             "key": "builderName"
> > >         }, 
> > >         {
> > >             "value": "Release", 
> > >             "key": "configuration"
> > >         }, 
> > >         {
> > >             "value": "GTX660", 
> > >             "key": "gpu"
> > >         }, 
> > >         {
> > >             "value": "ShuttleA", 
> > >             "key": "model"
> > >         }, 
> > >         {
> > >             "value": "Ubuntu12", 
> > >             "key": "os"
> > >         }, 
> > >         {
> > >             "value": "Perf", 
> > >             "key": "role"
> > >         }, 
> > >         {
> > >             "value": {
> > >                 "value": "simple", 
> > >                 "key": "mode"
> > >             }, 
> > >             "key": "0"
> > >         }, 
> > >         {
> > >             "value": {
> > >                 "value": 1.0, 
> > >                 "key": "scale"
> > >             }, 
> > >             "key": "1"
> > >         }, 
> > >         {
> > >             "value": {
> > >                 "value": "1000x1000", 
> > >                 "key": "viewport"
> > >             }, 
> > >             "key": "2"
> > >         }, 
> > >         {
> > >             "value": "desk_yahooanswers.skp", 
> > >             "key": "benchName"
> > >         }, 
> > >         {
> > >             "value": "wall", 
> > >             "key": "measurementType"
> > >         }
> > >     ], 
> > >     "key":
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > 
> > >     "isTrybot": false
> > > }
> > 
> > It doesn't work! So I didn't actually read the whole output before posting,
> > gonna have to fix those last couple values..
> 
> Data point looks like this now:
> {
>     "gitNumber": -1, 
>     "gitHash": "001011010101", 
>     "timestamp": 1410065408, 
>     "value": 10.1490013, 
>     "buildNumber": -1, 
>     "params": [
>         {
>             "value": "x86_64", 
>             "key": "arch"
>         }, 
>         {
>             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
>             "key": "builderName"
>         }, 
>         {
>             "value": "Release", 
>             "key": "configuration"
>         }, 
>         {
>             "value": "GTX660", 
>             "key": "gpu"
>         }, 
>         {
>             "value": "ShuttleA", 
>             "key": "model"
>         }, 
>         {
>             "value": "Ubuntu12", 
>             "key": "os"
>         }, 
>         {
>             "value": "Perf", 
>             "key": "role"
>         }, 
>         {
>             "value": "simple", 
>             "key": "mode"
>         }, 
>         {
>             "value": 1.0, 
>             "key": "scale"
>         }, 
>         {
>             "value": "1000x1000", 
>             "key": "viewport"
>         }, 
>         {
>             "value": "desk_yahooanswers.skp", 
>             "key": "benchName"
>         }, 
>         {
>             "value": "wall", 
>             "key": "measurementType"
>         }
>     ], 
>     "key":
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> 
>     "isTrybot": false
> }

So what's the decision on viewport?

[kelvinly, 2014-06-20 17:45:57]

On 2014/06/20 17:42:04, benchen wrote:
> On 2014/06/20 17:39:06, kelvinly wrote:
> > On 2014/06/20 15:31:36, kelvinly wrote:
> > > On 2014/06/20 15:28:33, kelvinly wrote:
> > > > It works!
> > > > Latest schema output (same parameters as last run)
> > > > {
> > > >     "gitNumber": -1, 
> > > >     "gitHash": "001011010101", 
> > > >     "timestamp": 1410065408, 
> > > >     "value": 10.0809463, 
> > > >     "buildNumber": -1, 
> > > >     "params": [
> > > >         {
> > > >             "value": "x86_64", 
> > > >             "key": "arch"
> > > >         }, 
> > > >         {
> > > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> > > >             "key": "builderName"
> > > >         }, 
> > > >         {
> > > >             "value": "Release", 
> > > >             "key": "configuration"
> > > >         }, 
> > > >         {
> > > >             "value": "GTX660", 
> > > >             "key": "gpu"
> > > >         }, 
> > > >         {
> > > >             "value": "ShuttleA", 
> > > >             "key": "model"
> > > >         }, 
> > > >         {
> > > >             "value": "Ubuntu12", 
> > > >             "key": "os"
> > > >         }, 
> > > >         {
> > > >             "value": "Perf", 
> > > >             "key": "role"
> > > >         }, 
> > > >         {
> > > >             "value": {
> > > >                 "value": "simple", 
> > > >                 "key": "mode"
> > > >             }, 
> > > >             "key": "0"
> > > >         }, 
> > > >         {
> > > >             "value": {
> > > >                 "value": 1.0, 
> > > >                 "key": "scale"
> > > >             }, 
> > > >             "key": "1"
> > > >         }, 
> > > >         {
> > > >             "value": {
> > > >                 "value": "1000x1000", 
> > > >                 "key": "viewport"
> > > >             }, 
> > > >             "key": "2"
> > > >         }, 
> > > >         {
> > > >             "value": "desk_yahooanswers.skp", 
> > > >             "key": "benchName"
> > > >         }, 
> > > >         {
> > > >             "value": "wall", 
> > > >             "key": "measurementType"
> > > >         }
> > > >     ], 
> > > >     "key":
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > 
> > > >     "isTrybot": false
> > > > }
> > > 
> > > It doesn't work! So I didn't actually read the whole output before
posting,
> > > gonna have to fix those last couple values..
> > 
> > Data point looks like this now:
> > {
> >     "gitNumber": -1, 
> >     "gitHash": "001011010101", 
> >     "timestamp": 1410065408, 
> >     "value": 10.1490013, 
> >     "buildNumber": -1, 
> >     "params": [
> >         {
> >             "value": "x86_64", 
> >             "key": "arch"
> >         }, 
> >         {
> >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> >             "key": "builderName"
> >         }, 
> >         {
> >             "value": "Release", 
> >             "key": "configuration"
> >         }, 
> >         {
> >             "value": "GTX660", 
> >             "key": "gpu"
> >         }, 
> >         {
> >             "value": "ShuttleA", 
> >             "key": "model"
> >         }, 
> >         {
> >             "value": "Ubuntu12", 
> >             "key": "os"
> >         }, 
> >         {
> >             "value": "Perf", 
> >             "key": "role"
> >         }, 
> >         {
> >             "value": "simple", 
> >             "key": "mode"
> >         }, 
> >         {
> >             "value": 1.0, 
> >             "key": "scale"
> >         }, 
> >         {
> >             "value": "1000x1000", 
> >             "key": "viewport"
> >         }, 
> >         {
> >             "value": "desk_yahooanswers.skp", 
> >             "key": "benchName"
> >         }, 
> >         {
> >             "value": "wall", 
> >             "key": "measurementType"
> >         }
> >     ], 
> >     "key":
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > 
> >     "isTrybot": false
> > }
> 
> So what's the decision on viewport?

I'm not sure, actually. I have a feeling viewport/grid or whatever are actually
still in use, since originally we had ~900 and now we're down to ~200 with those
options removed from the frontend. Should I remove it anyways?

[benchen, 2014-06-20 17:53:11]

On 2014/06/20 17:45:57, kelvinly wrote:
> On 2014/06/20 17:42:04, benchen wrote:
> > On 2014/06/20 17:39:06, kelvinly wrote:
> > > On 2014/06/20 15:31:36, kelvinly wrote:
> > > > On 2014/06/20 15:28:33, kelvinly wrote:
> > > > > It works!
> > > > > Latest schema output (same parameters as last run)
> > > > > {
> > > > >     "gitNumber": -1, 
> > > > >     "gitHash": "001011010101", 
> > > > >     "timestamp": 1410065408, 
> > > > >     "value": 10.0809463, 
> > > > >     "buildNumber": -1, 
> > > > >     "params": [
> > > > >         {
> > > > >             "value": "x86_64", 
> > > > >             "key": "arch"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> > > > >             "key": "builderName"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Release", 
> > > > >             "key": "configuration"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "GTX660", 
> > > > >             "key": "gpu"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "ShuttleA", 
> > > > >             "key": "model"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Ubuntu12", 
> > > > >             "key": "os"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Perf", 
> > > > >             "key": "role"
> > > > >         }, 
> > > > >         {
> > > > >             "value": {
> > > > >                 "value": "simple", 
> > > > >                 "key": "mode"
> > > > >             }, 
> > > > >             "key": "0"
> > > > >         }, 
> > > > >         {
> > > > >             "value": {
> > > > >                 "value": 1.0, 
> > > > >                 "key": "scale"
> > > > >             }, 
> > > > >             "key": "1"
> > > > >         }, 
> > > > >         {
> > > > >             "value": {
> > > > >                 "value": "1000x1000", 
> > > > >                 "key": "viewport"
> > > > >             }, 
> > > > >             "key": "2"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "desk_yahooanswers.skp", 
> > > > >             "key": "benchName"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "wall", 
> > > > >             "key": "measurementType"
> > > > >         }
> > > > >     ], 
> > > > >     "key":
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > 
> > > > >     "isTrybot": false
> > > > > }
> > > > 
> > > > It doesn't work! So I didn't actually read the whole output before
> posting,
> > > > gonna have to fix those last couple values..
> > > 
> > > Data point looks like this now:
> > > {
> > >     "gitNumber": -1, 
> > >     "gitHash": "001011010101", 
> > >     "timestamp": 1410065408, 
> > >     "value": 10.1490013, 
> > >     "buildNumber": -1, 
> > >     "params": [
> > >         {
> > >             "value": "x86_64", 
> > >             "key": "arch"
> > >         }, 
> > >         {
> > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> > >             "key": "builderName"
> > >         }, 
> > >         {
> > >             "value": "Release", 
> > >             "key": "configuration"
> > >         }, 
> > >         {
> > >             "value": "GTX660", 
> > >             "key": "gpu"
> > >         }, 
> > >         {
> > >             "value": "ShuttleA", 
> > >             "key": "model"
> > >         }, 
> > >         {
> > >             "value": "Ubuntu12", 
> > >             "key": "os"
> > >         }, 
> > >         {
> > >             "value": "Perf", 
> > >             "key": "role"
> > >         }, 
> > >         {
> > >             "value": "simple", 
> > >             "key": "mode"
> > >         }, 
> > >         {
> > >             "value": 1.0, 
> > >             "key": "scale"
> > >         }, 
> > >         {
> > >             "value": "1000x1000", 
> > >             "key": "viewport"
> > >         }, 
> > >         {
> > >             "value": "desk_yahooanswers.skp", 
> > >             "key": "benchName"
> > >         }, 
> > >         {
> > >             "value": "wall", 
> > >             "key": "measurementType"
> > >         }
> > >     ], 
> > >     "key":
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > 
> > >     "isTrybot": false
> > > }
> > 
> > So what's the decision on viewport?
> 
> I'm not sure, actually. I have a feeling viewport/grid or whatever are
actually
> still in use, since originally we had ~900 and now we're down to ~200 with
those
> options removed from the frontend. Should I remove it anyways?

The reduction might be related to the removal of record* and the cpu timer.
It might be fine to keep them here and just filter from the UI, but that's
redundant for us now. I'll let Joe make the call..

[jcgregorio, 2014-06-23 00:52:48]

On 2014/06/20 17:53:11, benchen wrote:
> On 2014/06/20 17:45:57, kelvinly wrote:
> > On 2014/06/20 17:42:04, benchen wrote:
> > > On 2014/06/20 17:39:06, kelvinly wrote:
> > > > On 2014/06/20 15:31:36, kelvinly wrote:
> > > > > On 2014/06/20 15:28:33, kelvinly wrote:
> > > > > > It works!
> > > > > > Latest schema output (same parameters as last run)
> > > > > > {
> > > > > >     "gitNumber": -1, 
> > > > > >     "gitHash": "001011010101", 
> > > > > >     "timestamp": 1410065408, 
> > > > > >     "value": 10.0809463, 
> > > > > >     "buildNumber": -1, 
> > > > > >     "params": [
> > > > > >         {
> > > > > >             "value": "x86_64", 
> > > > > >             "key": "arch"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release",

> > > > > >             "key": "builderName"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Release", 
> > > > > >             "key": "configuration"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "GTX660", 
> > > > > >             "key": "gpu"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "ShuttleA", 
> > > > > >             "key": "model"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Ubuntu12", 
> > > > > >             "key": "os"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Perf", 
> > > > > >             "key": "role"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": {
> > > > > >                 "value": "simple", 
> > > > > >                 "key": "mode"
> > > > > >             }, 
> > > > > >             "key": "0"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": {
> > > > > >                 "value": 1.0, 
> > > > > >                 "key": "scale"
> > > > > >             }, 
> > > > > >             "key": "1"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": {
> > > > > >                 "value": "1000x1000", 
> > > > > >                 "key": "viewport"
> > > > > >             }, 
> > > > > >             "key": "2"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "desk_yahooanswers.skp", 
> > > > > >             "key": "benchName"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "wall", 
> > > > > >             "key": "measurementType"
> > > > > >         }
> > > > > >     ], 
> > > > > >     "key":
> > > > > >
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > > 
> > > > > >     "isTrybot": false
> > > > > > }
> > > > > 
> > > > > It doesn't work! So I didn't actually read the whole output before
> > posting,
> > > > > gonna have to fix those last couple values..
> > > > 
> > > > Data point looks like this now:
> > > > {
> > > >     "gitNumber": -1, 
> > > >     "gitHash": "001011010101", 
> > > >     "timestamp": 1410065408, 
> > > >     "value": 10.1490013, 
> > > >     "buildNumber": -1, 
> > > >     "params": [
> > > >         {
> > > >             "value": "x86_64", 
> > > >             "key": "arch"
> > > >         }, 
> > > >         {
> > > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> > > >             "key": "builderName"
> > > >         }, 
> > > >         {
> > > >             "value": "Release", 
> > > >             "key": "configuration"
> > > >         }, 
> > > >         {
> > > >             "value": "GTX660", 
> > > >             "key": "gpu"
> > > >         }, 
> > > >         {
> > > >             "value": "ShuttleA", 
> > > >             "key": "model"
> > > >         }, 
> > > >         {
> > > >             "value": "Ubuntu12", 
> > > >             "key": "os"
> > > >         }, 
> > > >         {
> > > >             "value": "Perf", 
> > > >             "key": "role"
> > > >         }, 
> > > >         {
> > > >             "value": "simple", 
> > > >             "key": "mode"
> > > >         }, 
> > > >         {
> > > >             "value": 1.0, 
> > > >             "key": "scale"
> > > >         }, 
> > > >         {
> > > >             "value": "1000x1000", 
> > > >             "key": "viewport"
> > > >         }, 
> > > >         {
> > > >             "value": "desk_yahooanswers.skp", 
> > > >             "key": "benchName"
> > > >         }, 
> > > >         {
> > > >             "value": "wall", 
> > > >             "key": "measurementType"
> > > >         }
> > > >     ], 
> > > >     "key":
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > 
> > > >     "isTrybot": false
> > > > }
> > > 
> > > So what's the decision on viewport?
> > 
> > I'm not sure, actually. I have a feeling viewport/grid or whatever are
> actually
> > still in use, since originally we had ~900 and now we're down to ~200 with
> those
> > options removed from the frontend. Should I remove it anyways?
> 
> The reduction might be related to the removal of record* and the cpu timer.
> It might be fine to keep them here and just filter from the UI, but that's
> redundant for us now. I'll let Joe make the call..

Per the meeting minutes, we can hide 1000x1000.

https://docs.google.com/a/google.com/document/d/1AQ1pNESQOFc15KQw8nJIo-f2h1pf...

Also, I will add basalomon and robertphilips to this review.

  -joe

[kelvinly, 2014-06-23 13:26:59]

On 2014/06/23 00:52:48, jcgregorio wrote:
> On 2014/06/20 17:53:11, benchen wrote:
> > On 2014/06/20 17:45:57, kelvinly wrote:
> > > On 2014/06/20 17:42:04, benchen wrote:
> > > > On 2014/06/20 17:39:06, kelvinly wrote:
> > > > > On 2014/06/20 15:31:36, kelvinly wrote:
> > > > > > On 2014/06/20 15:28:33, kelvinly wrote:
> > > > > > > It works!
> > > > > > > Latest schema output (same parameters as last run)
> > > > > > > {
> > > > > > >     "gitNumber": -1, 
> > > > > > >     "gitHash": "001011010101", 
> > > > > > >     "timestamp": 1410065408, 
> > > > > > >     "value": 10.0809463, 
> > > > > > >     "buildNumber": -1, 
> > > > > > >     "params": [
> > > > > > >         {
> > > > > > >             "value": "x86_64", 
> > > > > > >             "key": "arch"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value":
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release",
> 
> > > > > > >             "key": "builderName"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "Release", 
> > > > > > >             "key": "configuration"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "GTX660", 
> > > > > > >             "key": "gpu"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "ShuttleA", 
> > > > > > >             "key": "model"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "Ubuntu12", 
> > > > > > >             "key": "os"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "Perf", 
> > > > > > >             "key": "role"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": {
> > > > > > >                 "value": "simple", 
> > > > > > >                 "key": "mode"
> > > > > > >             }, 
> > > > > > >             "key": "0"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": {
> > > > > > >                 "value": 1.0, 
> > > > > > >                 "key": "scale"
> > > > > > >             }, 
> > > > > > >             "key": "1"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": {
> > > > > > >                 "value": "1000x1000", 
> > > > > > >                 "key": "viewport"
> > > > > > >             }, 
> > > > > > >             "key": "2"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "desk_yahooanswers.skp", 
> > > > > > >             "key": "benchName"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "wall", 
> > > > > > >             "key": "measurementType"
> > > > > > >         }
> > > > > > >     ], 
> > > > > > >     "key":
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > > > 
> > > > > > >     "isTrybot": false
> > > > > > > }
> > > > > > 
> > > > > > It doesn't work! So I didn't actually read the whole output before
> > > posting,
> > > > > > gonna have to fix those last couple values..
> > > > > 
> > > > > Data point looks like this now:
> > > > > {
> > > > >     "gitNumber": -1, 
> > > > >     "gitHash": "001011010101", 
> > > > >     "timestamp": 1410065408, 
> > > > >     "value": 10.1490013, 
> > > > >     "buildNumber": -1, 
> > > > >     "params": [
> > > > >         {
> > > > >             "value": "x86_64", 
> > > > >             "key": "arch"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
> > > > >             "key": "builderName"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Release", 
> > > > >             "key": "configuration"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "GTX660", 
> > > > >             "key": "gpu"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "ShuttleA", 
> > > > >             "key": "model"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Ubuntu12", 
> > > > >             "key": "os"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "Perf", 
> > > > >             "key": "role"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "simple", 
> > > > >             "key": "mode"
> > > > >         }, 
> > > > >         {
> > > > >             "value": 1.0, 
> > > > >             "key": "scale"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "1000x1000", 
> > > > >             "key": "viewport"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "desk_yahooanswers.skp", 
> > > > >             "key": "benchName"
> > > > >         }, 
> > > > >         {
> > > > >             "value": "wall", 
> > > > >             "key": "measurementType"
> > > > >         }
> > > > >     ], 
> > > > >     "key":
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > 
> > > > >     "isTrybot": false
> > > > > }
> > > > 
> > > > So what's the decision on viewport?
> > > 
> > > I'm not sure, actually. I have a feeling viewport/grid or whatever are
> > actually
> > > still in use, since originally we had ~900 and now we're down to ~200 with
> > those
> > > options removed from the frontend. Should I remove it anyways?
> > 
> > The reduction might be related to the removal of record* and the cpu timer.
> > It might be fine to keep them here and just filter from the UI, but that's
> > redundant for us now. I'll let Joe make the call..
> 
> Per the meeting minutes, we can hide 1000x1000.
> 
>
https://docs.google.com/a/google.com/document/d/1AQ1pNESQOFc15KQw8nJIo-f2h1pf...
> 
> Also, I will add basalomon and robertphilips to this review.
> 
>   -joe

Done. Any word on whether we should be using the key-value schema?

[jcgregorio, 2014-06-23 13:30:08]

On 2014/06/23 13:26:59, kelvinly wrote:
> On 2014/06/23 00:52:48, jcgregorio wrote:
> > On 2014/06/20 17:53:11, benchen wrote:
> > > On 2014/06/20 17:45:57, kelvinly wrote:
> > > > On 2014/06/20 17:42:04, benchen wrote:
> > > > > On 2014/06/20 17:39:06, kelvinly wrote:
> > > > > > On 2014/06/20 15:31:36, kelvinly wrote:
> > > > > > > On 2014/06/20 15:28:33, kelvinly wrote:
> > > > > > > > It works!
> > > > > > > > Latest schema output (same parameters as last run)
> > > > > > > > {
> > > > > > > >     "gitNumber": -1, 
> > > > > > > >     "gitHash": "001011010101", 
> > > > > > > >     "timestamp": 1410065408, 
> > > > > > > >     "value": 10.0809463, 
> > > > > > > >     "buildNumber": -1, 
> > > > > > > >     "params": [
> > > > > > > >         {
> > > > > > > >             "value": "x86_64", 
> > > > > > > >             "key": "arch"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value":
> "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release",
> > 
> > > > > > > >             "key": "builderName"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "Release", 
> > > > > > > >             "key": "configuration"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "GTX660", 
> > > > > > > >             "key": "gpu"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "ShuttleA", 
> > > > > > > >             "key": "model"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "Ubuntu12", 
> > > > > > > >             "key": "os"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "Perf", 
> > > > > > > >             "key": "role"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": {
> > > > > > > >                 "value": "simple", 
> > > > > > > >                 "key": "mode"
> > > > > > > >             }, 
> > > > > > > >             "key": "0"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": {
> > > > > > > >                 "value": 1.0, 
> > > > > > > >                 "key": "scale"
> > > > > > > >             }, 
> > > > > > > >             "key": "1"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": {
> > > > > > > >                 "value": "1000x1000", 
> > > > > > > >                 "key": "viewport"
> > > > > > > >             }, 
> > > > > > > >             "key": "2"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "desk_yahooanswers.skp", 
> > > > > > > >             "key": "benchName"
> > > > > > > >         }, 
> > > > > > > >         {
> > > > > > > >             "value": "wall", 
> > > > > > > >             "key": "measurementType"
> > > > > > > >         }
> > > > > > > >     ], 
> > > > > > > >     "key":
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > > > > 
> > > > > > > >     "isTrybot": false
> > > > > > > > }
> > > > > > > 
> > > > > > > It doesn't work! So I didn't actually read the whole output before
> > > > posting,
> > > > > > > gonna have to fix those last couple values..
> > > > > > 
> > > > > > Data point looks like this now:
> > > > > > {
> > > > > >     "gitNumber": -1, 
> > > > > >     "gitHash": "001011010101", 
> > > > > >     "timestamp": 1410065408, 
> > > > > >     "value": 10.1490013, 
> > > > > >     "buildNumber": -1, 
> > > > > >     "params": [
> > > > > >         {
> > > > > >             "value": "x86_64", 
> > > > > >             "key": "arch"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release",

> > > > > >             "key": "builderName"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Release", 
> > > > > >             "key": "configuration"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "GTX660", 
> > > > > >             "key": "gpu"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "ShuttleA", 
> > > > > >             "key": "model"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Ubuntu12", 
> > > > > >             "key": "os"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "Perf", 
> > > > > >             "key": "role"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "simple", 
> > > > > >             "key": "mode"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": 1.0, 
> > > > > >             "key": "scale"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "1000x1000", 
> > > > > >             "key": "viewport"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "desk_yahooanswers.skp", 
> > > > > >             "key": "benchName"
> > > > > >         }, 
> > > > > >         {
> > > > > >             "value": "wall", 
> > > > > >             "key": "measurementType"
> > > > > >         }
> > > > > >     ], 
> > > > > >     "key":
> > > > > >
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > > 
> > > > > >     "isTrybot": false
> > > > > > }
> > > > > 
> > > > > So what's the decision on viewport?
> > > > 
> > > > I'm not sure, actually. I have a feeling viewport/grid or whatever are
> > > actually
> > > > still in use, since originally we had ~900 and now we're down to ~200
with
> > > those
> > > > options removed from the frontend. Should I remove it anyways?
> > > 
> > > The reduction might be related to the removal of record* and the cpu
timer.
> > > It might be fine to keep them here and just filter from the UI, but that's
> > > redundant for us now. I'll let Joe make the call..
> > 
> > Per the meeting minutes, we can hide 1000x1000.
> > 
> >
>
https://docs.google.com/a/google.com/document/d/1AQ1pNESQOFc15KQw8nJIo-f2h1pf...
> > 
> > Also, I will add basalomon and robertphilips to this review.
> > 
> >   -joe
> 
> Done. Any word on whether we should be using the key-value schema?

I fiddled a little with that and it made querying "weakly impossible", which
means I couldn't figure out how to do it, not that it is actually impossible, so
let's stick to the explicit, non-key-value schema.

  -joe

[kelvinly, 2014-06-23 13:52:22]

On 2014/06/23 13:30:08, jcgregorio wrote:
> On 2014/06/23 13:26:59, kelvinly wrote:
> > On 2014/06/23 00:52:48, jcgregorio wrote:
> > > On 2014/06/20 17:53:11, benchen wrote:
> > > > On 2014/06/20 17:45:57, kelvinly wrote:
> > > > > On 2014/06/20 17:42:04, benchen wrote:
> > > > > > On 2014/06/20 17:39:06, kelvinly wrote:
> > > > > > > On 2014/06/20 15:31:36, kelvinly wrote:
> > > > > > > > On 2014/06/20 15:28:33, kelvinly wrote:
> > > > > > > > > It works!
> > > > > > > > > Latest schema output (same parameters as last run)
> > > > > > > > > {
> > > > > > > > >     "gitNumber": -1, 
> > > > > > > > >     "gitHash": "001011010101", 
> > > > > > > > >     "timestamp": 1410065408, 
> > > > > > > > >     "value": 10.0809463, 
> > > > > > > > >     "buildNumber": -1, 
> > > > > > > > >     "params": [
> > > > > > > > >         {
> > > > > > > > >             "value": "x86_64", 
> > > > > > > > >             "key": "arch"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value":
> > "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release",
> > > 
> > > > > > > > >             "key": "builderName"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "Release", 
> > > > > > > > >             "key": "configuration"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "GTX660", 
> > > > > > > > >             "key": "gpu"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "ShuttleA", 
> > > > > > > > >             "key": "model"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "Ubuntu12", 
> > > > > > > > >             "key": "os"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "Perf", 
> > > > > > > > >             "key": "role"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": {
> > > > > > > > >                 "value": "simple", 
> > > > > > > > >                 "key": "mode"
> > > > > > > > >             }, 
> > > > > > > > >             "key": "0"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": {
> > > > > > > > >                 "value": 1.0, 
> > > > > > > > >                 "key": "scale"
> > > > > > > > >             }, 
> > > > > > > > >             "key": "1"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": {
> > > > > > > > >                 "value": "1000x1000", 
> > > > > > > > >                 "key": "viewport"
> > > > > > > > >             }, 
> > > > > > > > >             "key": "2"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "desk_yahooanswers.skp", 
> > > > > > > > >             "key": "benchName"
> > > > > > > > >         }, 
> > > > > > > > >         {
> > > > > > > > >             "value": "wall", 
> > > > > > > > >             "key": "measurementType"
> > > > > > > > >         }
> > > > > > > > >     ], 
> > > > > > > > >     "key":
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > > > > > 
> > > > > > > > >     "isTrybot": false
> > > > > > > > > }
> > > > > > > > 
> > > > > > > > It doesn't work! So I didn't actually read the whole output
before
> > > > > posting,
> > > > > > > > gonna have to fix those last couple values..
> > > > > > > 
> > > > > > > Data point looks like this now:
> > > > > > > {
> > > > > > >     "gitNumber": -1, 
> > > > > > >     "gitHash": "001011010101", 
> > > > > > >     "timestamp": 1410065408, 
> > > > > > >     "value": 10.1490013, 
> > > > > > >     "buildNumber": -1, 
> > > > > > >     "params": [
> > > > > > >         {
> > > > > > >             "value": "x86_64", 
> > > > > > >             "key": "arch"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value":
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release",
> 
> > > > > > >             "key": "builderName"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "Release", 
> > > > > > >             "key": "configuration"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "GTX660", 
> > > > > > >             "key": "gpu"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "ShuttleA", 
> > > > > > >             "key": "model"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "Ubuntu12", 
> > > > > > >             "key": "os"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "Perf", 
> > > > > > >             "key": "role"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "simple", 
> > > > > > >             "key": "mode"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": 1.0, 
> > > > > > >             "key": "scale"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "1000x1000", 
> > > > > > >             "key": "viewport"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "desk_yahooanswers.skp", 
> > > > > > >             "key": "benchName"
> > > > > > >         }, 
> > > > > > >         {
> > > > > > >             "value": "wall", 
> > > > > > >             "key": "measurementType"
> > > > > > >         }
> > > > > > >     ], 
> > > > > > >     "key":
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_desk_yahooanswers.skp_simple_viewport_1000x1000_wall",
> > > > > > > 
> > > > > > >     "isTrybot": false
> > > > > > > }
> > > > > > 
> > > > > > So what's the decision on viewport?
> > > > > 
> > > > > I'm not sure, actually. I have a feeling viewport/grid or whatever are
> > > > actually
> > > > > still in use, since originally we had ~900 and now we're down to ~200
> with
> > > > those
> > > > > options removed from the frontend. Should I remove it anyways?
> > > > 
> > > > The reduction might be related to the removal of record* and the cpu
> timer.
> > > > It might be fine to keep them here and just filter from the UI, but
that's
> > > > redundant for us now. I'll let Joe make the call..
> > > 
> > > Per the meeting minutes, we can hide 1000x1000.
> > > 
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1AQ1pNESQOFc15KQw8nJIo-f2h1pf...
> > > 
> > > Also, I will add basalomon and robertphilips to this review.
> > > 
> > >   -joe
> > 
> > Done. Any word on whether we should be using the key-value schema?
> 
> I fiddled a little with that and it made querying "weakly impossible", which
> means I couldn't figure out how to do it, not that it is actually impossible,
so
> let's stick to the explicit, non-key-value schema.
> 
>   -joe

Sounds good! Reverted schema back to the first one then, data looks like:
{
    "buildNumber": -1, 
    "gitHash": "001011010101", 
    "gitNumber": -1, 
    "isTrybot": false, 
    "key":
"Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release_tabl_ukwsj.skp_simple_viewport_1000x1000_wall",

    "params": {
        "arch": "x86_64", 
        "benchName": "tabl_ukwsj.skp", 
        "builderName": "Perf-Ubuntu12-ShuttleA-GTX660-x86_64-Release", 
        "configuration": "Release", 
        "gpu": "GTX660", 
        "measurementType": "wall", 
        "mode": "simple", 
        "model": "ShuttleA", 
        "os": "Ubuntu12", 
        "role": "Perf", 
        "scale": 1.0
    }, 
    "timestamp": 1410065408, 
    "value": 114.0425362
}

[jcgregorio, 2014-06-23 19:02:45]

https://codereview.chromium.org/329993008/diff/100001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureRenderer.h...
tools/PictureRenderer.h:338: // ??? This is appropriate behavior?
Don't commit until this gets clarified.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:23: using namespace sk_tools;
I'm leery of blanket using statements like this, does this need to be done?

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:180: const char builderName[],
Align with first param decl.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:185: : fStream(filename),
I believe this should be:

: fStream(filename)
, fParams()
, fConfigString() {

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:188: const char keys[6][32] = {
Why not const char *keys[6] = {"role", ...

Also, since this is const you can move it out of the function.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:190: const int numKeys = 6;
Is 6 just the size of keys? I'm pretty sure we have a macro for that.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:227: // Same here
As the code changes "Same here" may make less sense, please make this a comment
that can stand on its own.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:263: // Because there's no sort I'll just remove
the smallest
qsort?

[kelvinly, 2014-06-23 20:32:46]

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:23: using namespace sk_tools;
On 2014/06/23 19:02:44, jcgregorio wrote:
> I'm leery of blanket using statements like this, does this need to be done?

Done.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:180: const char builderName[],
On 2014/06/23 19:02:44, jcgregorio wrote:
> Align with first param decl.

Done.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:185: : fStream(filename),
On 2014/06/23 19:02:44, jcgregorio wrote:
> I believe this should be:
> 
> : fStream(filename)
> , fParams()
> , fConfigString() {

Won't compile the other way

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:188: const char keys[6][32] = {
On 2014/06/23 19:02:44, jcgregorio wrote:
> Why not const char *keys[6] = {"role", ...
> 
> Also, since this is const you can move it out of the function.

Moved this to a function in ResultsWriter.cpp, to be shared across both result
writers

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:190: const int numKeys = 6;
On 2014/06/23 19:02:44, jcgregorio wrote:
> Is 6 just the size of keys? I'm pretty sure we have a macro for that.

Um, would you happen to know it off the top of your head? Do you guys have docs
somewhere? I've been grepping through the source and relying on the doxygen
pages

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:227: // Same here
On 2014/06/23 19:02:44, jcgregorio wrote:
> As the code changes "Same here" may make less sense, please make this a
comment
> that can stand on its own.

Done.

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:263: // Because there's no sort I'll just remove
the smallest
On 2014/06/23 19:02:44, jcgregorio wrote:
> qsort?

Can you get the raw array out of an SkTArray?

[jcgregorio, 2014-06-23 20:40:44]

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:185: : fStream(filename),
Sorry, wasn't commenting on the order, just the formatting. See the Skia coding
style for "constructor initializers"

https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...



On 2014/06/23 20:32:46, kelvinly wrote:
> On 2014/06/23 19:02:44, jcgregorio wrote:
> > I believe this should be:
> > 
> > : fStream(filename)
> > , fParams()
> > , fConfigString() {
> 
> Won't compile the other way

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:190: const int numKeys = 6;
Good question, I usually rely on grepping the source and doxygen also. Let's see
what Brian or Robert say.

On 2014/06/23 20:32:46, kelvinly wrote:
> On 2014/06/23 19:02:44, jcgregorio wrote:
> > Is 6 just the size of keys? I'm pretty sure we have a macro for that.
> 
> Um, would you happen to know it off the top of your head? Do you guys have
docs
> somewhere? I've been grepping through the source and relying on the doxygen
> pages

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:263: // Because there's no sort I'll just remove
the smallest
begin()?

On 2014/06/23 20:32:46, kelvinly wrote:
> On 2014/06/23 19:02:44, jcgregorio wrote:
> > qsort?
> 
> Can you get the raw array out of an SkTArray?

[kelvinly, 2014-06-23 20:49:27]

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:185: : fStream(filename),
On 2014/06/23 20:40:44, jcgregorio wrote:
> Sorry, wasn't commenting on the order, just the formatting. See the Skia
coding
> style for "constructor initializers"
> 
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> 
> 
> 
> On 2014/06/23 20:32:46, kelvinly wrote:
> > On 2014/06/23 19:02:44, jcgregorio wrote:
> > > I believe this should be:
> > > 
> > > : fStream(filename)
> > > , fParams()
> > > , fConfigString() {
> > 
> > Won't compile the other way
> 

Ah, my bad, fixed

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:263: // Because there's no sort I'll just remove
the smallest
On 2014/06/23 20:40:44, jcgregorio wrote:
> begin()?
> 
> On 2014/06/23 20:32:46, kelvinly wrote:
> > On 2014/06/23 19:02:44, jcgregorio wrote:
> > > qsort?
> > 
> > Can you get the raw array out of an SkTArray?
> 

I don't think we have a begin():
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTArray.h?sp...

[jcgregorio, 2014-06-23 20:52:59]

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:263: // Because there's no sort I'll just remove
the smallest
Don't reference code.google.com, that's not canonical anymore, use
skia.googlesource.com or github:

 https://github.com/google/skia/blob/master/include/core/SkTArray.h#L273

On 2014/06/23 20:49:27, kelvinly wrote:
> On 2014/06/23 20:40:44, jcgregorio wrote:
> > begin()?
> > 
> > On 2014/06/23 20:32:46, kelvinly wrote:
> > > On 2014/06/23 19:02:44, jcgregorio wrote:
> > > > qsort?
> > > 
> > > Can you get the raw array out of an SkTArray?
> > 
> 
> I don't think we have a begin():
>
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTArray.h?sp...

[kelvinly, 2014-06-23 21:10:48]

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/100001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:263: // Because there's no sort I'll just remove
the smallest
On 2014/06/23 20:52:59, jcgregorio wrote:
> Don't reference http://code.google.com, that's not canonical anymore, use
> http://skia.googlesource.com or github:
> 
>  https://github.com/google/skia/blob/master/include/core/SkTArray.h#L273
> 
> On 2014/06/23 20:49:27, kelvinly wrote:
> > On 2014/06/23 20:40:44, jcgregorio wrote:
> > > begin()?
> > > 
> > > On 2014/06/23 20:32:46, kelvinly wrote:
> > > > On 2014/06/23 19:02:44, jcgregorio wrote:
> > > > > qsort?
> > > > 
> > > > Can you get the raw array out of an SkTArray?
> > > 
> > 
> > I don't think we have a begin():
> >
>
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTArray.h?sp...
> 

Fixed

[robertphillips, 2014-06-24 20:49:52]

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp
File bench/ResultsWriter.cpp (right):

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:33: Json::Value makeBuilderJSON(SkString buildername) {
kNumKeys ?
kKeys ?
This should probably be:
static const char* kKeys[] = { "role" ... "configuration" };

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:38: 
space after if ?

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:45: builderData["builderName"] = buildername.c_str();
space after if ?

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:47: SkString extras;
space after for ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
tools/PictureRenderer.h:299: 
can this be const ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
tools/PictureRenderer.h:304: result["scale"] = 1.0f;
put SK_Scalar1 on left hand side ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
tools/PictureRenderer.h:322: case kGPU_DeviceType:
0 != fSampleCount ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:116: }
OVERRIDE here ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:117: virtual void
logRenderer(sk_tools::PictureRenderer* renderer) {
fCurrentLine ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:143: BenchLogger* fLogger;
fCurrentLine ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:196: }
OVERRIDE here ?

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:201: // Apparently tiles aren't used, so tileMeta
is empty
OVERRIDE here ? 
Don't need ';' here

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:203: // Flags aren't used, so addTileFlag is empty
Same here

https://codereview.chromium.org/329993008/diff/180001/tools/bench_pictures_ma...
File tools/bench_pictures_main.cpp (right):

https://codereview.chromium.org/329993008/diff/180001/tools/bench_pictures_ma...
tools/bench_pictures_main.cpp:431: if (FLAGS_jsonLog.count() == 1) {
one line ?

[kelvinly, 2014-06-24 22:25:20]

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp
File bench/ResultsWriter.cpp (right):

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:33: Json::Value makeBuilderJSON(SkString buildername) {
On 2014/06/24 20:49:51, robertphillips wrote:
> kNumKeys ?
> kKeys ?
> This should probably be:
> static const char* kKeys[] = { "role" ... "configuration" };

Done.

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:38: 
On 2014/06/24 20:49:51, robertphillips wrote:
> space after if ?

Done.

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:45: builderData["builderName"] = buildername.c_str();
On 2014/06/24 20:49:51, robertphillips wrote:
> space after if ?

Done.

https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:47: SkString extras;
On 2014/06/24 20:49:51, robertphillips wrote:
> space after for ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
tools/PictureRenderer.h:299: 
On 2014/06/24 20:49:51, robertphillips wrote:
> can this be const ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
tools/PictureRenderer.h:304: result["scale"] = 1.0f;
On 2014/06/24 20:49:51, robertphillips wrote:
> put SK_Scalar1 on left hand side ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
tools/PictureRenderer.h:322: case kGPU_DeviceType:
On 2014/06/24 20:49:51, robertphillips wrote:
> 0 != fSampleCount ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:116: }
On 2014/06/24 20:49:52, robertphillips wrote:
> OVERRIDE here ?

Ah, sorry, didn't know that was a thing. Fixed

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:117: virtual void
logRenderer(sk_tools::PictureRenderer* renderer) {
On 2014/06/24 20:49:51, robertphillips wrote:
> fCurrentLine ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:143: BenchLogger* fLogger;
On 2014/06/24 20:49:52, robertphillips wrote:
> fCurrentLine ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:196: }
On 2014/06/24 20:49:51, robertphillips wrote:
> OVERRIDE here ?

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:201: // Apparently tiles aren't used, so tileMeta
is empty
On 2014/06/24 20:49:51, robertphillips wrote:
> OVERRIDE here ? 
> Don't need ';' here

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:203: // Flags aren't used, so addTileFlag is empty
On 2014/06/24 20:49:52, robertphillips wrote:
> Same here

Done.

https://codereview.chromium.org/329993008/diff/180001/tools/bench_pictures_ma...
File tools/bench_pictures_main.cpp (right):

https://codereview.chromium.org/329993008/diff/180001/tools/bench_pictures_ma...
tools/bench_pictures_main.cpp:431: if (FLAGS_jsonLog.count() == 1) {
On 2014/06/24 20:49:52, robertphillips wrote:
> one line ?

Done.

[kelvinly, 2014-06-24 22:26:35]

On 2014/06/24 22:25:20, kelvinly wrote:
> https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp
> File bench/ResultsWriter.cpp (right):
> 
>
https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
> bench/ResultsWriter.cpp:33: Json::Value makeBuilderJSON(SkString buildername)
{
> On 2014/06/24 20:49:51, robertphillips wrote:
> > kNumKeys ?
> > kKeys ?
> > This should probably be:
> > static const char* kKeys[] = { "role" ... "configuration" };
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
> bench/ResultsWriter.cpp:38: 
> On 2014/06/24 20:49:51, robertphillips wrote:
> > space after if ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
> bench/ResultsWriter.cpp:45: builderData["builderName"] = buildername.c_str();
> On 2014/06/24 20:49:51, robertphillips wrote:
> > space after if ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/bench/ResultsWriter.cpp...
> bench/ResultsWriter.cpp:47: SkString extras;
> On 2014/06/24 20:49:51, robertphillips wrote:
> > space after for ?
> 
> Done.
> 
> https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h
> File tools/PictureRenderer.h (right):
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
> tools/PictureRenderer.h:299: 
> On 2014/06/24 20:49:51, robertphillips wrote:
> > can this be const ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
> tools/PictureRenderer.h:304: result["scale"] = 1.0f;
> On 2014/06/24 20:49:51, robertphillips wrote:
> > put SK_Scalar1 on left hand side ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureRenderer.h...
> tools/PictureRenderer.h:322: case kGPU_DeviceType:
> On 2014/06/24 20:49:51, robertphillips wrote:
> > 0 != fSampleCount ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> File tools/PictureResultsWriter.h (right):
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> tools/PictureResultsWriter.h:116: }
> On 2014/06/24 20:49:52, robertphillips wrote:
> > OVERRIDE here ?
> 
> Ah, sorry, didn't know that was a thing. Fixed
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> tools/PictureResultsWriter.h:117: virtual void
> logRenderer(sk_tools::PictureRenderer* renderer) {
> On 2014/06/24 20:49:51, robertphillips wrote:
> > fCurrentLine ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> tools/PictureResultsWriter.h:143: BenchLogger* fLogger;
> On 2014/06/24 20:49:52, robertphillips wrote:
> > fCurrentLine ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> tools/PictureResultsWriter.h:196: }
> On 2014/06/24 20:49:51, robertphillips wrote:
> > OVERRIDE here ?
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> tools/PictureResultsWriter.h:201: // Apparently tiles aren't used, so tileMeta
> is empty
> On 2014/06/24 20:49:51, robertphillips wrote:
> > OVERRIDE here ? 
> > Don't need ';' here
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/PictureResultsWri...
> tools/PictureResultsWriter.h:203: // Flags aren't used, so addTileFlag is
empty
> On 2014/06/24 20:49:52, robertphillips wrote:
> > Same here
> 
> Done.
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/bench_pictures_ma...
> File tools/bench_pictures_main.cpp (right):
> 
>
https://codereview.chromium.org/329993008/diff/180001/tools/bench_pictures_ma...
> tools/bench_pictures_main.cpp:431: if (FLAGS_jsonLog.count() == 1) {
> On 2014/06/24 20:49:52, robertphillips wrote:
> > one line ?
> 
> Done.

Ah, sorry, forgot to change a comment. I couldn't change GetJSONConfig to const
because it meant setting all the getInternalConfigName to const, which I
apparently am not capable of doing correctly

[robertphillips, 2014-06-26 16:17:33]

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp
File bench/ResultsWriter.cpp (right):

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:32: 
SkString buildername -> const SkString& builderName ?

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:34: static const int kNumKeys = 6;
6 -> kNumKeys here ?

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:38: 
if (!builderName.isEmpty()) { ?

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:41: SkStrSplit(buildername.c_str(), "-", &splitBuilder);
SkASSERT(splitBuilder.count() >= kNumKeys); ?
++i ?

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:47: SkString extras;
++i ?

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.h#n...
bench/ResultsWriter.h:104: Json::Value* SkFindNamedNode(Json::Value* root, const
char name[]);
makeBuilderJSON -> SkMakeBuilderJSON ?

https://codereview.chromium.org/329993008/diff/220001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/220001/tools/PictureRenderer.h...
tools/PictureRenderer.h:337: case kAngle_DeviceType:
Seems fine to me

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:84: int numInnerLoops = 1) {
This SK_OVERRIDE is misplaced!

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:183: : fStream(filename)
Don't need to explicitly call default ctor here (for either fParams or
fConfigString)

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:230: // Not including skpSize because that's
deprecated?
this->makeKey ?

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:235: for(Json::ValueIterator vals = val.begin();
vals != val.end();
can this fit on the prior line ?

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:239: qsort(static_cast<void*>(times.begin()),
times.count(),
CompareDoubles ?

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:259: }
make this const ?

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:270: 
line up the variable names ?

[kelvinly, 2014-06-26 17:18:10]

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp
File bench/ResultsWriter.cpp (right):

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:32: 
On 2014/06/26 16:17:32, robertphillips wrote:
> SkString buildername -> const SkString& builderName ?

Done.

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:34: static const int kNumKeys = 6;
On 2014/06/26 16:17:32, robertphillips wrote:
> 6 -> kNumKeys here ?

Done.

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:38: 
On 2014/06/26 16:17:32, robertphillips wrote:
> if (!builderName.isEmpty()) { ?

Done.

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:41: SkStrSplit(buildername.c_str(), "-", &splitBuilder);
On 2014/06/26 16:17:32, robertphillips wrote:
> SkASSERT(splitBuilder.count() >= kNumKeys); ?
> ++i ?

Done.

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.cpp...
bench/ResultsWriter.cpp:47: SkString extras;
On 2014/06/26 16:17:32, robertphillips wrote:
> ++i ?

Done.

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/220001/bench/ResultsWriter.h#n...
bench/ResultsWriter.h:104: Json::Value* SkFindNamedNode(Json::Value* root, const
char name[]);
On 2014/06/26 16:17:32, robertphillips wrote:
> makeBuilderJSON -> SkMakeBuilderJSON ?

Done.

https://codereview.chromium.org/329993008/diff/220001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/220001/tools/PictureRenderer.h...
tools/PictureRenderer.h:337: case kAngle_DeviceType:
On 2014/06/26 16:17:32, robertphillips wrote:
> Seems fine to me

Hmm, it's been so long I can't remember what I thought was wrong with this, so
I'll just remove the comment and probably nothing will break

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:84: int numInnerLoops = 1) {
On 2014/06/26 16:17:33, robertphillips wrote:
> This SK_OVERRIDE is misplaced!

Haha, fixed. I'm confused why that didn't emit some kind of error..

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:183: : fStream(filename)
On 2014/06/26 16:17:33, robertphillips wrote:
> Don't need to explicitly call default ctor here (for either fParams or
> fConfigString)

Done.

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:230: // Not including skpSize because that's
deprecated?
On 2014/06/26 16:17:33, robertphillips wrote:
> this->makeKey ?

Done.

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:235: for(Json::ValueIterator vals = val.begin();
vals != val.end();
On 2014/06/26 16:17:33, robertphillips wrote:
> can this fit on the prior line ?

~4 characters too long

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:239: qsort(static_cast<void*>(times.begin()),
times.count(),
On 2014/06/26 16:17:33, robertphillips wrote:
> CompareDoubles ?

Done.

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:259: }
On 2014/06/26 16:17:33, robertphillips wrote:
> make this const ?

Done.

https://codereview.chromium.org/329993008/diff/220001/tools/PictureResultsWri...
tools/PictureResultsWriter.h:270: 
On 2014/06/26 16:17:33, robertphillips wrote:
> line up the variable names ?

Done.

[robertphillips, 2014-06-26 17:26:08]

C++ 1gtm

[jcgregorio, 2014-06-26 17:31:41]

On 2014/06/26 17:26:08, robertphillips wrote:
> C++ 1gtm

LGTM

[benchen, 2014-06-26 17:39:51]

lgtm, one line to remove.

https://codereview.chromium.org/329993008/diff/240001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/240001/tools/PictureRenderer.h...
tools/PictureRenderer.h:351: // results["dunno"] = fDrawFiltersConfig.c_str();
This needs to be removed?

[kelvinly, 2014-06-26 17:58:48]

https://codereview.chromium.org/329993008/diff/240001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.chromium.org/329993008/diff/240001/tools/PictureRenderer.h...
tools/PictureRenderer.h:351: // results["dunno"] = fDrawFiltersConfig.c_str();
On 2014/06/26 17:39:51, benchen wrote:
> This needs to be removed?

Done.

[rmistry, 2014-06-26 18:09:38]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-06-26 18:10:32]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kelvinly@google.com/329993008/280001

[commit-bot: I haz the power, 2014-06-26 18:26:49]

Change committed as 4d1a364e391a8fd79c9e133d27957761d35068d0