Change JSON output of nanobench.

Change JSON output of nanobench.

We're moving away from BigQuery for storing results so the output doens't have to conform to BQ requirements, which allows simplifying the format. Also stop parsing the filename for information and pass in buildbot parameters explicitly.

Adds the following flags to nanobench:

  --key
  --gitHash

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/bf5e5237b8f5dc9288d72e90864d6ba8d4bfbb72

[jcgregorio, 2014-07-16 19:11:29]

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:274: // Need help, how do I get the GL version, vendor,
renderer, etc?
Open question here, how do I do this?

[mtklein, 2014-07-16 19:24:03]

+Brian

If anyone knows the answer to those GL questions, Brian will.

[mtklein, 2014-07-16 19:26:51]

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:329: bool loggedBench = false;
Why this change?

[bsalomon, 2014-07-16 19:36:29]

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:274: // Need help, how do I get the GL version, vendor,
renderer, etc?
On 2014/07/16 19:11:29, jcgregorio wrote:
> Open question here, how do I do this?

You want to tunnel the SkGLContextHelper here (available via
GrContextFactory::getGLContext())

Then you can call:

GLubyte* version;
SK_GL_RET(ctx, version, GetString(GR_GL_VERSION));
log->option("GL_VERSION", (const char*) version);

Same for GL_RENDERER, GL_VENDOR, and maybe GL_SHADING_LANGUAGE_VERSION.

[jcgregorio, 2014-07-17 13:11:51]

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:274: // Need help, how do I get the GL version, vendor,
renderer, etc?
Thanks!

On 2014/07/16 19:36:29, bsalomon wrote:
> On 2014/07/16 19:11:29, jcgregorio wrote:
> > Open question here, how do I do this?
> 
> You want to tunnel the SkGLContextHelper here (available via
> GrContextFactory::getGLContext())
> 
> Then you can call:
> 
> GLubyte* version;
> SK_GL_RET(ctx, version, GetString(GR_GL_VERSION));
> log->option("GL_VERSION", (const char*) version);
> 
> Same for GL_RENDERER, GL_VENDOR, and maybe GL_SHADING_LANGUAGE_VERSION.

https://codereview.chromium.org/392393002/diff/60001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:329: bool loggedBench = false;
On 2014/07/16 19:26:51, mtklein wrote:
> Why this change?

So that we only add info to the JSON file if we actually run a test. The way it
was before there would be an entry in the JSON for every possible test, even if
nothing was run, so they would point to empty JSON objects.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:255: #if defined(SK_BUILD_FOR_WIN32)
Are there any other values locked up in #defines that would be useful to mirror
into the perf data?

[bsalomon, 2014-07-17 13:35:05]

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:275:
gGrFactory.get(GrContextFactory::kNative_GLContextType);
We don't always want to use the kNative context. We will be running nanobench
with different contexts and we'll want the context info for each. You should
assume that each config we run *may* be using a different context.

Also if we're going to create and destroy the contexts here we don't really need
to use the global GrContextFactory, you can just put one on the stack. It will
destroy its contexts in its destructor.

[mtklein, 2014-07-17 13:41:16]

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:254: static void fill_static_options(ResultsWriter* log) {
Let's merge fill_static_options and fill_gpu_options into just one fill_options
with a #if-#endif section embedded?

This is also starting to look like a piece of code that'd be useful for other
test programs.  Let's keep this chunk in mind as something to extract later?

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:255: #if defined(SK_BUILD_FOR_WIN32)
On 2014/07/17 13:11:51, jcgregorio wrote:
> Are there any other values locked up in #defines that would be useful to
mirror
> into the perf data?

We could probably also provide the CPU architecture (x86, x864-64, ARM v7, ARm
v8).

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:309: const char* gitHash = FLAGS_gitHash.isEmpty() ? "" :
FLAGS_gitHash[0];
"" -> "unknown-revision" perhaps?

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:313: CallEnd<MultiResultsWriter> ender(log);
One of log.end() or this line can go away right?  I'm happy either way.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:316: SkDebugf("ERROR: --key must be passed with an even
number of arguments.\n");
To be fair, you've written this in a way that we could downgrade this to 

"WARNING: last value of --key will be ignored"

But I do like fail fast.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:349: create_targets(bench.get(), &targets);
Ah, gotcha.  Let's do it here as

if (!targets.isEmpty()) {
  log.bench(...);
  bench->preDraw();
}
for (int j = 0; j < targets.count(); j++) {
  ...
}

or, just a little less desirably, as

for (int j = 0; j < targets.count(); j++) {
  if (0 == j) { ... }
  ...
}

Having loggedBench stand alone as a bool had me looking around for the
complicated logic that requires it to exist.  Seems like we can write the
condition more precisely without it.

[jcgregorio, 2014-07-17 19:13:23]

OK, sample of the new JSON output looks like this:

{
    "gitHash": "d1830323662ae8ae06908b97f15180fd25808894",
    "key": {
      "arch": "Arm7",
      "gpu": "SGX540",
      "os": "Android", 
      "model": "GalaxyNexus",
    },
    "options":{
        "system":"UNIX"
    },
    "results":{
        "DeferredSurfaceCopy_discardable_640_480":{
            "gpu":{
                "max_ms":7.9920480,
                "mean_ms":7.9920480,
                "median_ms":7.9920480,
                "min_ms":7.9920480,
                "options":{
                    "GL_RENDERER":"Quadro K600/PCIe/SSE2",
                    "GL_SHADING_LANGUAGE_VERSION":"4.40 NVIDIA via Cg compiler",
                    "GL_VENDOR":"NVIDIA Corporation",
                    "GL_VERSION":"4.4.0 NVIDIA 331.38"
                }
            },
            "nvprmsaa4":{
                "max_ms":16.7961230,
                "mean_ms":16.7961230,
                "median_ms":16.7961230,
                "min_ms":16.7961230,
                "options":{
                    "GL_RENDERER":"Quadro K600/PCIe/SSE2",
                    "GL_SHADING_LANGUAGE_VERSION":"4.40 NVIDIA via Cg compiler",
                    "GL_VENDOR":"NVIDIA Corporation",
                    "GL_VERSION":"4.4.0 NVIDIA 331.38"
                }
            }
        },
        ...

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:254: static void fill_static_options(ResultsWriter* log) {
Actually had to keep fill_gpu_options separate because it's now calling
log->configOption and not just log->option since the values may change per
config.

On 2014/07/17 13:41:16, mtklein wrote:
> Let's merge fill_static_options and fill_gpu_options into just one
fill_options
> with a #if-#endif section embedded?
> 
> This is also starting to look like a piece of code that'd be useful for other
> test programs.  Let's keep this chunk in mind as something to extract later?

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:255: #if defined(SK_BUILD_FOR_WIN32)
Those should come in via --key from the buildbot information, unless there's
finer grained values we can record here?

On 2014/07/17 13:41:16, mtklein wrote:
> On 2014/07/17 13:11:51, jcgregorio wrote:
> > Are there any other values locked up in #defines that would be useful to
> mirror
> > into the perf data?
> 
> We could probably also provide the CPU architecture (x86, x864-64, ARM v7, ARm
> v8).

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:275:
gGrFactory.get(GrContextFactory::kNative_GLContextType);
OK, changed it so that these are recorded per config, and use the
SkGLContextHelper that's in Tasks so I don't need to construct or destroy them.

On 2014/07/17 13:35:04, bsalomon wrote:
> We don't always want to use the kNative context. We will be running nanobench
> with different contexts and we'll want the context info for each. You should
> assume that each config we run *may* be using a different context.
> 
> Also if we're going to create and destroy the contexts here we don't really
need
> to use the global GrContextFactory, you can just put one on the stack. It will
> destroy its contexts in its destructor.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:309: const char* gitHash = FLAGS_gitHash.isEmpty() ? "" :
FLAGS_gitHash[0];
On 2014/07/17 13:41:16, mtklein wrote:
> "" -> "unknown-revision" perhaps?

Done.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:313: CallEnd<MultiResultsWriter> ender(log);
On 2014/07/17 13:41:16, mtklein wrote:
> One of log.end() or this line can go away right?  I'm happy either way.

Done.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:316: SkDebugf("ERROR: --key must be passed with an even
number of arguments.\n");
On 2014/07/17 13:41:16, mtklein wrote:
> To be fair, you've written this in a way that we could downgrade this to 
> 
> "WARNING: last value of --key will be ignored"
> 
> But I do like fail fast.

Acknowledged.

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:349: create_targets(bench.get(), &targets);
On 2014/07/17 13:41:16, mtklein wrote:
> Ah, gotcha.  Let's do it here as
> 
> if (!targets.isEmpty()) {
>   log.bench(...);
>   bench->preDraw();
> }
> for (int j = 0; j < targets.count(); j++) {
>   ...
> }
> 
> or, just a little less desirably, as
> 
> for (int j = 0; j < targets.count(); j++) {
>   if (0 == j) { ... }
>   ...
> }
> 
> Having loggedBench stand alone as a bool had me looking around for the
> complicated logic that requires it to exist.  Seems like we can write the
> condition more precisely without it.

Done.

[mtklein, 2014-07-17 19:21:36]

lgtm

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:255: #if defined(SK_BUILD_FOR_WIN32)
On 2014/07/17 19:13:22, jcgregorio wrote:
> Those should come in via --key from the buildbot information, unless there's
> finer grained values we can record here?
> 
> On 2014/07/17 13:41:16, mtklein wrote:
> > On 2014/07/17 13:11:51, jcgregorio wrote:
> > > Are there any other values locked up in #defines that would be useful to
> > mirror
> > > into the perf data?
> > 
> > We could probably also provide the CPU architecture (x86, x864-64, ARM v7,
ARm
> > v8).
> 

We could go arbitrarily fine... e.g., dump /proc/cpuinfo, but in practice we
don't really care any deeper than what comes from the buildbot.

Can't OS come from there too?

[jcgregorio, 2014-07-17 19:32:46]

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/392393002/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:255: #if defined(SK_BUILD_FOR_WIN32)
It does come from there, but it's not clean, it's the value in the buildbot
name, so not just MAC, but Mac10.6, Mac10.7 or Mac10.8. What we record here is
just MAC, so this will give us two ways to filter the data.

On 2014/07/17 19:21:36, mtklein wrote:
> On 2014/07/17 19:13:22, jcgregorio wrote:
> > Those should come in via --key from the buildbot information, unless there's
> > finer grained values we can record here?
> > 
> > On 2014/07/17 13:41:16, mtklein wrote:
> > > On 2014/07/17 13:11:51, jcgregorio wrote:
> > > > Are there any other values locked up in #defines that would be useful to
> > > mirror
> > > > into the perf data?
> > > 
> > > We could probably also provide the CPU architecture (x86, x864-64, ARM v7,
> ARm
> > > v8).
> > 
> 
> We could go arbitrarily fine... e.g., dump /proc/cpuinfo, but in practice we
> don't really care any deeper than what comes from the buildbot.
> 
> Can't OS come from there too?

[bsalomon, 2014-07-17 19:44:48]

lgtm

[jcgregorio, 2014-07-17 19:49:10]

The CQ bit was checked by jcgregorio@google.com

[commit-bot: I haz the power, 2014-07-17 19:50:15]

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

[commit-bot: I haz the power, 2014-07-17 19:50:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 19:50:20]

Failed to apply patch for bench/nanobench.cpp:
While running git apply --index -p1;
  error: patch failed: bench/nanobench.cpp:52
  error: bench/nanobench.cpp: patch does not apply

Patch:       bench/nanobench.cpp
Index: bench/nanobench.cpp
diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index
445370741665cca3dd64c0c8cd6b41b266fe95b5..2ede9008c4c44c268947afc2ff03afbc70595033
100644
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -21,6 +21,7 @@
 #include "SkSurface.h"
 
 #if SK_SUPPORT_GPU
+    #include "gl/GrGLDefines.h"
     #include "GrContextFactory.h"
     GrContextFactory gGrFactory;
 #endif
@@ -52,6 +53,8 @@ DEFINE_string(outResultsFile, "", "If given, write results
here as JSON.");
 DEFINE_bool(resetGpuContext, true, "Reset the GrContext before running each
bench.");
 DEFINE_int32(maxCalibrationAttempts, 3,
              "Try up to this many times to guess loops for a bench, or skip the
bench.");
+DEFINE_string(key, "", "Space-separated key/value pairs to add to JSON.");
+DEFINE_string(gitHash, "", "Git hash to add to JSON.");
 
 
 static SkString humanize(double ms) {
@@ -260,13 +263,25 @@ static void fill_static_options(ResultsWriter* log) {
 #else
     log->option("system", "other");
 #endif
-#if defined(SK_DEBUG)
-    log->option("build", "DEBUG");
-#else
-    log->option("build", "RELEASE");
-#endif
 }
 
+#if SK_SUPPORT_GPU
+static void fill_gpu_options(ResultsWriter* log, SkGLContextHelper* ctx) {
+    const GLubyte* version;
+    SK_GL_RET(*ctx, version, GetString(GR_GL_VERSION));
+    log->configOption("GL_VERSION", (const char*)(version));
+
+    SK_GL_RET(*ctx, version, GetString(GR_GL_RENDERER));
+    log->configOption("GL_RENDERER", (const char*) version);
+
+    SK_GL_RET(*ctx, version, GetString(GR_GL_VENDOR));
+    log->configOption("GL_VENDOR", (const char*) version);
+
+    SK_GL_RET(*ctx, version, GetString(GR_GL_SHADING_LANGUAGE_VERSION));
+    log->configOption("GL_SHADING_LANGUAGE_VERSION", (const char*) version);
+}
+#endif
+
 int tool_main(int argc, char** argv);
 int tool_main(int argc, char** argv) {
     SetupCrashHandler();
@@ -279,12 +294,21 @@ int tool_main(int argc, char** argv) {
     }
 
     MultiResultsWriter log;
-    SkAutoTDelete<JSONResultsWriter> json;
+    SkAutoTDelete<NanoJSONResultsWriter> json;
     if (!FLAGS_outResultsFile.isEmpty()) {
-        json.reset(SkNEW(JSONResultsWriter(FLAGS_outResultsFile[0])));
+        const char* gitHash = FLAGS_gitHash.isEmpty() ? "unknown-revision" :
FLAGS_gitHash[0];
+        json.reset(SkNEW(NanoJSONResultsWriter(FLAGS_outResultsFile[0],
gitHash)));
         log.add(json.get());
     }
     CallEnd<MultiResultsWriter> ender(log);
+
+    if (1 == FLAGS_key.count() % 2) {
+        SkDebugf("ERROR: --key must be passed with an even number of
arguments.\n");
+        return 1;
+    }
+    for (int i = 1; i < FLAGS_key.count(); i += 2) {
+        log.key(FLAGS_key[i-1], FLAGS_key[i]);
+    }
     fill_static_options(&log);
 
     const double overhead = estimate_timer_overhead();
@@ -305,12 +329,14 @@ int tool_main(int argc, char** argv) {
         if (SkCommandLineFlags::ShouldSkip(FLAGS_match, bench->getName())) {
             continue;
         }
-        log.bench(bench->getName(), bench->getSize().fX, bench->getSize().fY);
 
         SkTDArray<Target*> targets;
         create_targets(bench.get(), &targets);
 
-        bench->preDraw();
+        if (!targets.isEmpty()) {
+            log.bench(bench->getName(), bench->getSize().fX,
bench->getSize().fY);
+            bench->preDraw();
+        }
         for (int j = 0; j < targets.count(); j++) {
             SkCanvas* canvas = targets[j]->surface.get() ?
targets[j]->surface->getCanvas() : NULL;
             const char* config = targets[j]->config;
@@ -331,6 +357,11 @@ int tool_main(int argc, char** argv) {
 
             Stats stats(samples.get(), FLAGS_samples);
             log.config(config);
+#if SK_SUPPORT_GPU
+            if (Benchmark::kGPU_Backend == targets[j]->backend) {
+                fill_gpu_options(&log, targets[j]->gl);
+            }
+#endif
             log.timer("min_ms",    stats.min);
             log.timer("median_ms", stats.median);
             log.timer("mean_ms",   stats.mean);

[jcgregorio, 2014-07-17 19:55:41]

The CQ bit was checked by jcgregorio@google.com

[commit-bot: I haz the power, 2014-07-17 19:56:25]

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

[commit-bot: I haz the power, 2014-07-17 20:14:22]

Change committed as bf5e5237b8f5dc9288d72e90864d6ba8d4bfbb72