Add JSON output option to bench.

Add JSON output option to bench.

A new command-line flag --outResultsFile takes the filename to write the JSON into.

The human readable output is the same as before with one exception. Previously DEBUG would be printed if in debug mode, or nothing would be printed if in release mode. Now this is reported as a named option:

  build=DEBUG

or

  build=RELEASE

Committed: http://code.google.com/p/skia/source/detail?r=12465

[jcgregorio, 2013-11-22 19:08:35]

Review carefully, it's been a dog-year since I've written C++ :-)

Also, I was on the fence about breaking out the new classes into a new file(s)
vs keeping them in benchmain.cpp, please advise.

[bsalomon, 2013-11-22 20:36:30]

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:33: #ifdef SK_BUILD_FOR_WIN
We have this same #ifdef stuff in several places, maybe we should have a header
somewhere that is just this block?

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:77: virtual void bench(const char name[], int32_t x, int32_t
y) = 0;
maybe some short comments about how the caller uses these functions? It's not
totally clear what a valid call sequence is.

Would it make sense for the base class to have some asserts that calls are
correctly matched? (e.g. bench()/endBench())

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:86: class LoggerResultsWriter : public ResultsWriter {
I don't know much about the logger but I'm wondering if it's functionality
should be subsumed by this class.

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:858: SkDELETE(jsonWriter);
I don't think you need the NULL check.

[jcgregorio, 2013-11-25 14:39:16]

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:33: #ifdef SK_BUILD_FOR_WIN
On 2013/11/22 20:36:30, bsalomon wrote:
> We have this same #ifdef stuff in several places, maybe we should have a
header
> somewhere that is just this block?

Sure, how about include/utils/SkJSONCPP.h?

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:77: virtual void bench(const char name[], int32_t x, int32_t
y) = 0;
On 2013/11/22 20:36:30, bsalomon wrote:
> maybe some short comments about how the caller uses these functions? It's not
> totally clear what a valid call sequence is.
> 
> Would it make sense for the base class to have some asserts that calls are
> correctly matched? (e.g. bench()/endBench())

Done. Also, when writing the comments I realized I could drop the endBench()
call and make the interface a little simpler.

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:86: class LoggerResultsWriter : public ResultsWriter {
The current logger also takes care of logging errors, which this interface
doesn't currently handle. Should I add handling for errors and completely
subsume the old interface?

On 2013/11/22 20:36:30, bsalomon wrote:
> I don't know much about the logger but I'm wondering if it's functionality
> should be subsumed by this class.

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:858: SkDELETE(jsonWriter);
On 2013/11/22 20:36:30, bsalomon wrote:
> I don't think you need the NULL check.

Done.

[bsalomon, 2013-11-25 15:28:32]

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:33: #ifdef SK_BUILD_FOR_WIN
On 2013/11/25 14:39:16, jcgregorio wrote:
> On 2013/11/22 20:36:30, bsalomon wrote:
> > We have this same #ifdef stuff in several places, maybe we should have a
> header
> > somewhere that is just this block?
> 
> Sure, how about include/utils/SkJSONCPP.h?

SGTM

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:86: class LoggerResultsWriter : public ResultsWriter {
On 2013/11/25 14:39:16, jcgregorio wrote:
> The current logger also takes care of logging errors, which this interface
> doesn't currently handle. Should I add handling for errors and completely
> subsume the old interface?
> 
> On 2013/11/22 20:36:30, bsalomon wrote:
> > I don't know much about the logger but I'm wondering if it's functionality
> > should be subsumed by this class.
> 
I don't know. I'm not really familiar with this. If no one else chimes in, then
I'd say ignore my comment. :)

[djsollen, 2013-11-25 15:35:46]

SkJSONCPP.h works for me

https://codereview.chromium.org/83863002/diff/60001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/60001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:70: // Base class for writing out the bench results.
can we move these 3 classes to their own header?

[jcgregorio, 2013-11-25 16:06:47]

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:33: #ifdef SK_BUILD_FOR_WIN
On 2013/11/25 15:28:33, bsalomon wrote:
> On 2013/11/25 14:39:16, jcgregorio wrote:
> > On 2013/11/22 20:36:30, bsalomon wrote:
> > > We have this same #ifdef stuff in several places, maybe we should have a
> > header
> > > somewhere that is just this block?
> > 
> > Sure, how about include/utils/SkJSONCPP.h?
> 
> SGTM

Done.

https://codereview.chromium.org/83863002/diff/10003/bench/benchmain.cpp#newco...
bench/benchmain.cpp:86: class LoggerResultsWriter : public ResultsWriter {
On 2013/11/25 15:28:33, bsalomon wrote:
> On 2013/11/25 14:39:16, jcgregorio wrote:
> > The current logger also takes care of logging errors, which this interface
> > doesn't currently handle. Should I add handling for errors and completely
> > subsume the old interface?
> > 
> > On 2013/11/22 20:36:30, bsalomon wrote:
> > > I don't know much about the logger but I'm wondering if it's functionality
> > > should be subsumed by this class.
> > 
> I don't know. I'm not really familiar with this. If no one else chimes in,
then
> I'd say ignore my comment. :)

OK, ignoring for now.

https://codereview.chromium.org/83863002/diff/60001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/60001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:70: // Base class for writing out the bench results.
On 2013/11/25 15:35:46, djsollen wrote:
> can we move these 3 classes to their own header?

Done.

[djsollen, 2013-11-25 18:09:59]

lgtm with 2 nits

https://codereview.chromium.org/83863002/diff/100001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/100001/bench/benchmain.cpp#newc...
bench/benchmain.cpp:309: JSONResultsWriter* jsonWriter = NULL;
use the SkAutoTDelete template and you won't need the SkDelete at the bottom of
the file.

https://codereview.chromium.org/83863002/diff/100001/bench/benchmain.cpp#newc...
bench/benchmain.cpp:688: writer.end();
could you have some sort of stack wrapper that would call end() for you
automatically?  This is just a nice to have and not a requirement for this CL.

[jcgregorio, 2013-11-25 19:03:57]

https://codereview.chromium.org/83863002/diff/100001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/100001/bench/benchmain.cpp#newc...
bench/benchmain.cpp:309: JSONResultsWriter* jsonWriter = NULL;
On 2013/11/25 18:09:59, djsollen wrote:
> use the SkAutoTDelete template and you won't need the SkDelete at the bottom
of
> the file.

Done.

https://codereview.chromium.org/83863002/diff/100001/bench/benchmain.cpp#newc...
bench/benchmain.cpp:688: writer.end();
On 2013/11/25 18:09:59, djsollen wrote:
> could you have some sort of stack wrapper that would call end() for you
> automatically?  This is just a nice to have and not a requirement for this CL.

Done. See CallEnd<> above.

[djsollen, 2013-11-25 20:29:28]

https://codereview.chromium.org/83863002/diff/150001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/150001/bench/benchmain.cpp#newc...
bench/benchmain.cpp:700: writer.end();
don't need this anymore.

[jcgregorio, 2013-11-25 20:57:42]

https://codereview.chromium.org/83863002/diff/150001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/83863002/diff/150001/bench/benchmain.cpp#newc...
bench/benchmain.cpp:700: writer.end();
On 2013/11/25 20:29:28, djsollen wrote:
> don't need this anymore.

Done.

[djsollen, 2013-11-25 21:06:16]

lgtm

[jcgregorio, 2013-11-26 16:30:07]

On 2013/11/25 21:06:16, djsollen wrote:
> lgtm

Moved CallEnd into ResultsWriter.h per offline conversation.

[commit-bot: I haz the power, 2013-12-02 13:48:31]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jcgregorio@google.com/83863002/190001

[commit-bot: I haz the power, 2013-12-02 13:48:35]

Presubmit check for 83863002-190001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[bsalomon, 2013-12-02 19:49:35]

lgtm

[commit-bot: I haz the power, 2013-12-03 04:46:22]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jcgregorio@google.com/83863002/210001

[jcgregorio, 2013-12-03 14:21:10]

https://codereview.chromium.org/83863002/diff/230001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/83863002/diff/230001/bench/ResultsWriter.h#ne...
bench/ResultsWriter.h:63: "\nrunning bench [%3d %3d] %40s", x, y, name));
%040s produces 0-padded strings on a Mac:

http://108.170.217.252:10117/builders/Perf-Mac10.8-MacMini4.1-GeForce320M-x86...

[commit-bot: I haz the power, 2013-12-03 15:52:40]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jcgregorio@google.com/83863002/230001

[commit-bot: I haz the power, 2013-12-03 18:16:51]

Change committed as 12465