Make nanobench setup configs outside of loop over benchmarks

bench/nanobench.cpp

Index: bench/nanobench.cpp
diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index 0af05527f12d70f96115af7604bb24936a365067..d2880c91545650596e24049bc72fb6336b331fc4 100644
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -153,6 +153,7 @@ static int gpu_bench(SkGLContextHelper* gl,
                      Benchmark* bench,
                      SkCanvas* canvas,
                      double* samples) {
+    gl->makeCurrent();
     // Make sure we're done with whatever came before.
     SK_GL(*gl, Finish());
 
@@ -200,71 +201,134 @@ static SkString to_lower(const char* str) {
     return lower;
 }
 
-struct Target {
-    const char* config;
+struct Config {
+    const char* name;
     Benchmark::Backend backend;
+    SkColorType color;
+    SkAlphaType alpha;
+    int samples;
+#if SK_SUPPORT_GPU
+    GrContextFactory::GLContextType ctxType;
+#endif
+};
+
+struct Target {
+    const Config* config;

[mtklein, 2014/07/22 17:44:19]

// unowned

or perhaps this?

struct Target {
  explicit Target(const Config& config) : config(config) {}

  const Config& config;
  SkAutoTDelete<SkSurface> surface;
...
}

[bsalomon, 2014/07/22 18:10:14]

Done, but makes a copy of config (seems harmless).

     SkAutoTDelete<SkSurface> surface;
 #if SK_SUPPORT_GPU
     SkGLContextHelper* gl;
 #endif
 };
 
-// If bench is enabled for backend/config, returns a Target* for them, otherwise NULL.
-static Target* is_enabled(Benchmark* bench, Benchmark::Backend backend, const char* config) {
-    if (!bench->isSuitableFor(backend)) {
-        return NULL;
-    }
-
+static bool is_cpu_config_allowed(const char* name) {
     for (int i = 0; i < FLAGS_config.count(); i++) {
-        if (to_lower(FLAGS_config[i]).equals(config)) {
-            Target* target = new Target;
-            target->config  = config;
-            target->backend = backend;
-            return target;
+        if (to_lower(FLAGS_config[i]).equals(name)) {
+            return true;
         }
     }
-    return NULL;
+    return false;
 }
 
-// Append all targets that are suitable for bench.
-static void create_targets(Benchmark* bench, SkTDArray<Target*>* targets) {
-    const int w = bench->getSize().fX,
-              h = bench->getSize().fY;
-    const SkImageInfo _8888 = { w, h, kN32_SkColorType,     kPremul_SkAlphaType },
-                       _565 = { w, h, kRGB_565_SkColorType, kOpaque_SkAlphaType };
+#if SK_SUPPORT_GPU
+static bool is_gpu_config_allowed(const char* name, GrContextFactory::GLContextType ctxType,
+                                    int sampleCnt) {

[mtklein, 2014/07/22 17:44:19]

funky alignment here

[bsalomon, 2014/07/22 18:10:14]

Done.

+    if (!is_cpu_config_allowed(name)) {
+        return false;
+    }
+    if (const GrContext* ctx = gGrFactory.get(ctxType)) {
+        return sampleCnt <= ctx->getMaxSampleCount();
+    }
+    return false;
+}
+#endif
 
-    #define CPU_TARGET(config, backend, code)                              \
-        if (Target* t = is_enabled(bench, Benchmark::backend, #config)) {  \
-            t->surface.reset(code);                                        \
-            targets->push(t);                                              \
+// Append all configs that are enabled and supported.
+static void create_configs(SkTDArray<Config>* configs) {
+    #define CPU_CONFIG(name, backend, color, alpha)                          \
+        if (is_cpu_config_allowed(#name)) {                                  \
+            Config config = { #name, Benchmark::backend, color, alpha, 0 };  \
+            configs->push(config);                                           \
         }
+         
     if (FLAGS_cpu) {
-        CPU_TARGET(nonrendering, kNonRendering_Backend, NULL)
-        CPU_TARGET(8888, kRaster_Backend, SkSurface::NewRaster(_8888))
-        CPU_TARGET(565,  kRaster_Backend, SkSurface::NewRaster(_565))
+        CPU_CONFIG(nonrendering, kNonRendering_Backend, kUnknown_SkColorType, kUnpremul_SkAlphaType)
+        CPU_CONFIG(8888, kRaster_Backend, kN32_SkColorType, kPremul_SkAlphaType)
+        CPU_CONFIG(565, kRaster_Backend, kRGB_565_SkColorType, kOpaque_SkAlphaType)
     }
 
 #if SK_SUPPORT_GPU
-
-    #define GPU_TARGET(config, ctxType, info, samples)                                            \
-        if (Target* t = is_enabled(bench, Benchmark::kGPU_Backend, #config)) {                    \
-            t->surface.reset(SkSurface::NewRenderTarget(gGrFactory.get(ctxType), info, samples)); \
-            t->gl = gGrFactory.getGLContext(ctxType);                                             \
-            targets->push(t);                                                                     \
+    #define GPU_CONFIG(name, ctxType, samples)                                   \
+        if (is_gpu_config_allowed(#name, GrContextFactory::ctxType, samples)) {  \
+            Config config = {                                                    \
+                #name,                                                           \
+                Benchmark::kGPU_Backend,                                         \
+                kN32_SkColorType,                                                \
+                kPremul_SkAlphaType,                                             \
+                samples,                                                         \
+                GrContextFactory::ctxType };                                     \
+            configs->push(config);                                               \
         }
+         
     if (FLAGS_gpu) {
-        GPU_TARGET(gpu,      GrContextFactory::kNative_GLContextType, _8888, 0)
-        GPU_TARGET(msaa4,    GrContextFactory::kNative_GLContextType, _8888, 4)
-        GPU_TARGET(msaa16,   GrContextFactory::kNative_GLContextType, _8888, 16)
-        GPU_TARGET(nvprmsaa4,  GrContextFactory::kNVPR_GLContextType, _8888, 4)
-        GPU_TARGET(nvprmsaa16, GrContextFactory::kNVPR_GLContextType, _8888, 16)
-        GPU_TARGET(debug,     GrContextFactory::kDebug_GLContextType, _8888, 0)
-        GPU_TARGET(nullgpu,    GrContextFactory::kNull_GLContextType, _8888, 0)
-        #if SK_ANGLE
-            GPU_TARGET(angle, GrContextFactory::kANGLE_GLContextType, _8888, 0)
-        #endif
+        GPU_CONFIG(gpu, kNative_GLContextType, 0)
+        GPU_CONFIG(msaa4, kNative_GLContextType, 4)
+        GPU_CONFIG(msaa16, kNative_GLContextType, 16)
+        GPU_CONFIG(nvprmsaa4, kNVPR_GLContextType, 4)
+        GPU_CONFIG(nvprmsaa16, kNVPR_GLContextType, 16)
+        GPU_CONFIG(debug, kDebug_GLContextType, 0)
+        GPU_CONFIG(nullgpu, kNull_GLContextType, 0)
+    }
+#endif
+}
+
+// If bench is enabled for backend/config, returns a Target* for them, otherwise NULL.

[mtklein, 2014/07/22 17:44:19]

... is enabled for config, returns a Target* for it, otherwise NULL.

[bsalomon, 2014/07/22 18:10:14]

Done.

+static Target* is_enabled(Benchmark* bench, const Config* config) {

[mtklein, 2014/07/22 17:44:19]

const Config& ?

[bsalomon, 2014/07/22 18:10:14]

Done.

+    if (!bench->isSuitableFor(config->backend)) {
+        return NULL;
+    }
+
+    const int w = bench->getSize().fX,
+              h = bench->getSize().fY;
+
+    SkImageInfo info;
+    info.fAlphaType = config->alpha;
+    info.fColorType = config->color;
+    info.fWidth = w;

[mtklein, 2014/07/22 17:44:19]

just inline bench->getSize().fX and bench->getSize().fY here?

[bsalomon, 2014/07/22 18:10:14]

Done.

+    info.fHeight = h;
+
+    Target* target = new Target;
+    target->config  = config;
+
+    SkAutoTUnref<SkSurface> surface;

[mtklein, 2014/07/22 17:44:19]

Why go through the extra pointer?

if (Benchmark::kRaster_Backend == config->backend) {
  target->surface.reset(SkSurface::NewRaster(info));
}
...

if (!target->surface && Benchmark::kNonRenderingBackend != config->backend) {
  ...
}

[bsalomon, 2014/07/22 18:10:14]

Done.

+    if (Benchmark::kRaster_Backend == config->backend) {
+        surface.reset(SkSurface::NewRaster(info));
+    }
+#if SK_SUPPORT_GPU
+    else if (Benchmark::kGPU_Backend == config->backend) {
+        surface.reset(SkSurface::NewRenderTarget(gGrFactory.get(config->ctxType), info,
+                                                 config->samples));
+        target->gl = gGrFactory.getGLContext(config->ctxType);
     }
 #endif
+
+    if (Benchmark::kNonRendering_Backend != config->backend &&  !surface) {
+        delete target;
+        return NULL;
+    }
+
+    target->surface.reset(SkSafeRef(surface.get()));
+    return target;
+}
+
+// Creates targets for a benchmark and a set of configs.
+static void create_targets(SkTDArray<Target*>* targets, Benchmark* b,
+                           const SkTDArray<Config>& configs) {
+    for (int i = 0; i < configs.count(); ++i) {
+        if (Target* t = is_enabled(b, &configs[i])) {
+            targets->push(t);
+        }
+        
+    }
 }
 
 static void fill_static_options(ResultsWriter* log) {
@@ -342,6 +406,9 @@ int tool_main(int argc, char** argv) {
         SkDebugf("loops\tmin\tmedian\tmean\tmax\tstddev\tsamples\tconfig\tbench\n");
     }
 
+    SkTDArray<Config> configs;
+    create_configs(&configs);
+
     for (const BenchRegistry* r = BenchRegistry::Head(); r != NULL; r = r->next()) {
         SkAutoTDelete<Benchmark> bench(r->factory()(NULL));
         if (SkCommandLineFlags::ShouldSkip(FLAGS_match, bench->getName())) {
@@ -349,7 +416,7 @@ int tool_main(int argc, char** argv) {
         }
 
         SkTDArray<Target*> targets;
-        create_targets(bench.get(), &targets);
+        create_targets(&targets, bench.get(), configs);
 
         if (!targets.isEmpty()) {
             log.bench(bench->getName(), bench->getSize().fX, bench->getSize().fY);
@@ -357,11 +424,11 @@ int tool_main(int argc, char** argv) {
         }
         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;
+            const char* config = targets[j]->config->name;
 
             const int loops =
 #if SK_SUPPORT_GPU
-                Benchmark::kGPU_Backend == targets[j]->backend
+                Benchmark::kGPU_Backend == targets[j]->config->backend
                 ? gpu_bench(targets[j]->gl, bench.get(), canvas, samples.get())
                 :
 #endif
@@ -376,7 +443,7 @@ 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) {
+            if (Benchmark::kGPU_Backend == targets[j]->config->backend) {
                 fill_gpu_options(&log, targets[j]->gl);
             }
 #endif