Change JSON output of nanobench.

bench/nanobench.cpp

Index: bench/nanobench.cpp
diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index 445370741665cca3dd64c0c8cd6b41b266fe95b5..f7e75145b7b1ce1a392d0fac700ac99207f46a24 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,35 @@ 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) {
+    if (!FLAGS_gpu) {
+        return;
+    }
+
+    // Can't get a SkGLContextHelper until we've created a context of that type.
+    gGrFactory.get(GrContextFactory::kNative_GLContextType);

[bsalomon, 2014/07/17 13:35:04]

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.

[jcgregorio, 2014/07/17 19:13:22]

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:

+    SkGLContextHelper* ctx = gGrFactory.getGLContext(GrContextFactory::kNative_GLContextType);
+    if (NULL != ctx) {
+        const GLubyte* version;
+        SK_GL_RET(*ctx, version, GetString(GR_GL_VERSION));
+        log->option("GL_VERSION", (const char*)(version));
+
+        SK_GL_RET(*ctx, version, GetString(GR_GL_RENDERER));
+        log->option("GL_RENDERER", (const char*) version);
+
+        SK_GL_RET(*ctx, version, GetString(GR_GL_VENDOR));
+        log->option("GL_VENDOR", (const char*) version);
+
+        SK_GL_RET(*ctx, version, GetString(GR_GL_SHADING_LANGUAGE_VERSION));
+        log->option("GL_SHADING_LANGUAGE_VERSION", (const char*) version);
+    }
+    gGrFactory.destroyContexts();
+}
+#endif
+
 int tool_main(int argc, char** argv);
 int tool_main(int argc, char** argv) {
     SetupCrashHandler();
@@ -279,14 +304,27 @@ 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() ? "" : FLAGS_gitHash[0];

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

"" -> "unknown-revision" perhaps?

[jcgregorio, 2014/07/17 19:13:22]

Done.

+        json.reset(SkNEW(NanoJSONResultsWriter(FLAGS_outResultsFile[0], gitHash)));
         log.add(json.get());
     }
     CallEnd<MultiResultsWriter> ender(log);

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

One of log.end() or this line can go away right?  I'm happy either way.

[jcgregorio, 2014/07/17 19:13:22]

Done.

+
+    if (1 == FLAGS_key.count() % 2) {
+        SkDebugf("ERROR: --key must be passed with an even number of arguments.\n");

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

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.

[jcgregorio, 2014/07/17 19:13:22]

Acknowledged.

+        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);
 
+#if SK_SUPPORT_GPU
+    fill_gpu_options(&log);
+#endif
+
     const double overhead = estimate_timer_overhead();
     SkAutoTMalloc<double> samples(FLAGS_samples);
 
@@ -305,7 +343,7 @@ 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);
+        bool loggedBench = false;
 
         SkTDArray<Target*> targets;
         create_targets(bench.get(), &targets);

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

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]

Done.

@@ -328,6 +366,10 @@ int tool_main(int argc, char** argv) {
                          bench->getName(), config, humanize(overhead).c_str());
                 continue;
             }
+            if (!loggedBench) {
+                log.bench(bench->getName(), bench->getSize().fX, bench->getSize().fY);
+                loggedBench = true;
+            }
 
             Stats stats(samples.get(), FLAGS_samples);
             log.config(config);
@@ -375,6 +417,7 @@ int tool_main(int argc, char** argv) {
         }
     #endif
     }
+    log.end();
 
     return 0;
 }