Adding support for parsing extended gpu config parameters in nanobench.

bench/nanobench.cpp

 /*
  * Copyright 2014 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include <ctype.h>
 
 #include "nanobench.h"
@@ skipping 167 matching lines @@
                                                                        this->config.ctxOptions),
                                                          SkSurface::kNo_Budgeted, info,
                                                          this->config.samples, &props));
         this->gl = gGrFactory->getContextInfo(this->config.ctxType,
                                               this->config.ctxOptions).fGLContext;
         if (!this->surface.get()) {
             return false;
         }
         if (!this->gl->fenceSyncSupport()) {
             SkDebugf("WARNING: GL context for config \"%s\" does not support fence sync. "
-                     "Timings might not be accurate.\n", this->config.name);
+                     "Timings might not be accurate.\n", this->config.name.c_str());
         }
         return true;
     }
     void fillOptions(ResultsWriter* log) override {
         const GrGLubyte* version;
         SK_GL_RET(*this->gl, version, GetString(GR_GL_VERSION));
         log->configOption("GL_VERSION", (const char*)(version));
 
         SK_GL_RET(*this->gl, version, GetString(GR_GL_RENDERER));
         log->configOption("GL_RENDERER", (const char*) version);
@@ skipping 174 matching lines @@
 }
 
 static SkString to_lower(const char* str) {
     SkString lower(str);
     for (size_t i = 0; i < lower.size(); i++) {
         lower[i] = tolower(lower[i]);
     }
     return lower;
 }
 
 static bool is_cpu_config_allowed(const char* name) {

[Kimmo Kinnunen, 2016/01/20 18:25:55]

this would be
static bool is_cpu_config_allowed(const char* name, const SkCommandLineConfig*
config)

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

     for (int i = 0; i < FLAGS_config.count(); i++) {
         if (to_lower(FLAGS_config[i]).equals(name)) {
             return true;
         }
     }
     return false;
 }
 
 #if SK_SUPPORT_GPU
 static bool is_gpu_config_allowed(const char* name, GrContextFactory::GLContextType ctxType,

[Kimmo Kinnunen, 2016/01/20 18:25:56]

likewise here.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

                                   GrContextFactory::GLContextOptions ctxOptions,
                                   int sampleCnt) {
     if (!is_cpu_config_allowed(name)) {
         return false;
     }
     if (const GrContext* ctx = gGrFactory->get(ctxType, ctxOptions)) {
         return sampleCnt <= ctx->caps()->maxSampleCount();
     }
     return false;
 }
 #endif
 
 #if SK_SUPPORT_GPU
 #define kBogusGLContextType GrContextFactory::kNative_GLContextType
 #define kBogusGLContextOptions GrContextFactory::kNone_GLContextOptions
 #else
 #define kBogusGLContextType 0
 #define kBogusGLContextOptions 0
 #endif
 
+// create a config from a specific 'gpu(...)' parameter pack.

[Kimmo Kinnunen, 2016/01/20 18:25:55]

This comment does not add any information, if you change the name of the
function, and so you can remove the comment.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+static void create_commandline_config(SkTArray<Config, false>& nanoConfigs, const SkCommandLineConfig& conf) {

[Kimmo Kinnunen, 2016/01/20 18:25:55]

So this function could be called
static void create_gpu_config(const SkCommandLineGpuConfig* config, 
SkTArray<Config>* configs)

* the create_commandline_config is too vague for what the code actually does,
especially with other related functions being present
* don't use abbreviation conf. You save 2 letters.
* don't use arbitrary, unrelated prefix nano
* out parameters go last
* out parameters as pointers

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+    if (!FLAGS_gpu)
+        return;

[Kimmo Kinnunen, 2016/01/20 18:25:55]

this would be removed.
Instead, you actually need to guard all GPU-related lines with 
#if SK_SUPPORT_GPU
...
#endif

and you need to test the compile with: 
SKIA_OUT=out-no-gpu GYP_DEFINES=skia_gpu=0 ./gyp_skia && ninja -C
out-no-gpu/Debug most

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+
+    // only interested in gpu configs.
+    const auto* gpuConf = conf.asConfigGpu();

[Kimmo Kinnunen, 2016/01/20 18:25:55]

This would go to the parent function.

+    if (!gpuConf)
+        return;
+
+    // if tag is empty the config was not specified with parameters,
+    // i.e. it was just simple config.
+    const auto& tag = gpuConf->getTag();
+    if (tag.isEmpty())

[Kimmo Kinnunen, 2016/01/20 18:25:55]

This is wrong. The intention is that you check conf.asConfigGpu().

+        return;
+

[Kimmo Kinnunen, 2016/01/20 18:25:55]

you would need to call also the is_gpu_config_allowed.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+    Config target;

[Kimmo Kinnunen, 2016/01/20 18:25:55]

So here you handle all GPU related configs. Thus you need to remove the
redundant impl in (1).

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+    target.name       = tag;

[Kimmo Kinnunen, 2016/01/20 18:25:56]

Since the original used object initializer syntax, you could stay with that if
possible.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+    target.backend    = Benchmark::kGPU_Backend;
+    target.color      = kN32_SkColorType;
+    target.alpha      = kPremul_SkAlphaType;
+    target.ctxOptions = gpuConf->getUseNVPR() ? GrContextFactory::kEnableNVPR_GLContextOptions : 
+        GrContextFactory::kNone_GLContextOptions;    
+    target.samples    = gpuConf->getSamples();
+    target.ctxType    = gpuConf->getContextType();
+    target.useDFText  = false;
+    nanoConfigs.push_back(target);
+}
+
+
+
+// create configs from specific command line parameters
+// that were specified using --config 'gpu(foo=bar,...)' parameters

[Kimmo Kinnunen, 2016/01/20 18:25:55]

The comment doesn't add that much value. It's rather obvious what the function
does?
(Comments should start with capital letter, form normal english sentences and
end with period.)

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+static void gather_commandline_configs(SkTArray<Config, false>& nanoConfigs) {

[Kimmo Kinnunen, 2016/01/20 18:25:55]

So this could be called 
static void create_configs(SkTDArray<Config>* configs) 

This way it would not be confusing to assess the responsibilities of the new and
old functions.

This could go below the next function.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+    SkCommandLineConfigArray configs;
+    ParseConfigs(FLAGS_config, &configs);
+    for (int i=0; i<configs.count(); ++i) {

[Kimmo Kinnunen, 2016/01/20 18:25:55]

Here whitespace between operators.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+        const auto& config = *configs[i];

[Kimmo Kinnunen, 2016/01/20 18:25:55]

so here you should have code like
#if SK_SUPPORT_GPU
if (SkCommandLineGpuConfig* gpuConfig = configs[i]->asGpuConfig()) {
    create_gpu_config(gpuConfig, outConfigs);
} else 
#endif
{
    create_config(config[i], outConfigs);
}

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

+        create_commandline_config(nanoConfigs, config);
+    }
+}
+
 // Append all configs that are enabled and supported.
-static void create_configs(SkTDArray<Config>* configs) {
+static void create_configs(SkTArray<Config, false>* configs) {

[Kimmo Kinnunen, 2016/01/20 18:25:55]

This would be called:
static void create_config(const SkCommandLineConfig* config, SkTArray<Config>*
configs)

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

     #define CPU_CONFIG(name, backend, color, alpha)                        \
         if (is_cpu_config_allowed(#name)) {                                \

[Kimmo Kinnunen, 2016/01/20 18:25:56]

here you would have
if (is_cpu_config_allowed(#name,

[Kimmo Kinnunen, 2016/01/20 18:25:55]

so this would call 
if (is_cpu_config_allowed(#name, config)) 

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

-            Config config = { #name, Benchmark::backend, color, alpha, 0,  \
+            Config config = { SkString(#name), Benchmark::backend, color, alpha, 0,  \
                               kBogusGLContextType, kBogusGLContextOptions, \
                               false };                                     \
-            configs->push(config);                                         \
+            configs->push_back(config);                                         \

[Kimmo Kinnunen, 2016/01/20 18:25:55]

in the macro, you would add "return", so that the function handles one config
and returns.

         }
 
     if (FLAGS_cpu) {
         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_CONFIG(name, ctxType, ctxOptions, samples, useDFText)            \
         if (is_gpu_config_allowed(#name, GrContextFactory::ctxType,              \

[Kimmo Kinnunen, 2016/01/20 18:25:55]

(1) GPU configs are already handled in the function you added. So you need to
remove this hunk altogether.. 

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

                                   GrContextFactory::ctxOptions, samples)) {      \
             Config config = {                                                    \
-                #name,                                                           \
+                SkString(#name),                                                 \
                 Benchmark::kGPU_Backend,                                         \
                 kN32_SkColorType,                                                \
                 kPremul_SkAlphaType,                                             \
                 samples,                                                         \
                 GrContextFactory::ctxType,                                       \
                 GrContextFactory::ctxOptions,                                    \
                 useDFText };                                                     \
-            configs->push(config);                                               \
+            configs->push_back(config);                                               \

[Kimmo Kinnunen, 2016/01/20 18:30:46]

Also make sure that macro line continuations match consistently.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

         }
 
     if (FLAGS_gpu) {
         GPU_CONFIG(gpu, kNative_GLContextType, kNone_GLContextOptions, 0, false)
         GPU_CONFIG(msaa4, kNative_GLContextType, kNone_GLContextOptions, 4, false)
         GPU_CONFIG(msaa16, kNative_GLContextType, kNone_GLContextOptions, 16, false)
         GPU_CONFIG(nvprmsaa4, kNative_GLContextType, kEnableNVPR_GLContextOptions, 4, false)
         GPU_CONFIG(nvprmsaa16, kNative_GLContextType, kEnableNVPR_GLContextOptions, 16, false)
         GPU_CONFIG(gpudft, kNative_GLContextType, kNone_GLContextOptions, 0, true)
         GPU_CONFIG(debug, kDebug_GLContextType, kNone_GLContextOptions, 0, false)
@@ skipping 11 matching lines @@
         GPU_CONFIG(mesa, kMESA_GLContextType, kNone_GLContextOptions, 0, false)
 #endif
     }
 #endif
 
 #ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK
     if (is_cpu_config_allowed("hwui")) {
         Config config = { "hwui", Benchmark::kHWUI_Backend, kRGBA_8888_SkColorType,
                           kPremul_SkAlphaType, 0, kBogusGLContextType, kBogusGLContextOptions,
                           false };
-        configs->push(config);
+        configs->push_back(config);
     }
 #endif

[Kimmo Kinnunen, 2016/01/20 18:25:55]

Not sure about this, but here you could propose to add a useful warning about
not using the config.
You could use same kind message wording as in DM.cpp, to stay consistent with
it.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

 }
 
 // If bench is enabled for config, returns a Target* for it, otherwise nullptr.
 static Target* is_enabled(Benchmark* bench, const Config& config) {
     if (!bench->isSuitableFor(config.backend)) {
         return nullptr;
     }
 
     SkImageInfo info = SkImageInfo::Make(bench->getSize().fX, bench->getSize().fY,
                                          config.color, config.alpha);
@@ skipping 595 matching lines @@
     } else if (FLAGS_quiet) {
         SkDebugf("! -> high variance, ? -> moderate variance\n");
         SkDebugf("    micros   \tbench\n");
     } else if (FLAGS_ms) {
         SkDebugf("curr/maxrss\tloops\tmin\tmedian\tmean\tmax\tstddev\tsamples\tconfig\tbench\n");
     } else {
         SkDebugf("curr/maxrss\tloops\tmin\tmedian\tmean\tmax\tstddev\t%-*s\tconfig\tbench\n",
                  FLAGS_samples, "samples");
     }
 
-    SkTDArray<Config> configs;
+    SkTArray<Config, false> configs;

[Kimmo Kinnunen, 2016/01/20 18:25:55]

TArray MEM_COPY defaults to false, so it's maybe clearer to leave it out.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

     create_configs(&configs);
+    gather_commandline_configs(configs);

[Kimmo Kinnunen, 2016/01/20 18:25:55]

and since create_configs would be your toplevel iteration loop, you would only
need to call that here.

[Sami Väisänen, 2016/01/21 12:42:32]

Done.

 
     if (FLAGS_keepAlive) {
         start_keepalive();
     }
 
     int runs = 0;
     BenchmarkStream benchStream;
     while (Benchmark* b = benchStream.next()) {
         SkAutoTDelete<Benchmark> bench(b);
         if (SkCommandLineFlags::ShouldSkip(FLAGS_match, bench->getUniqueName())) {
             continue;
         }
 
-        if (!configs.isEmpty()) {
+        if (!configs.empty()) {
             log->bench(bench->getUniqueName(), bench->getSize().fX, bench->getSize().fY);
             bench->delayedSetup();
         }
         for (int i = 0; i < configs.count(); ++i) {
             Target* target = is_enabled(b, configs[i]);
             if (!target) {
                 continue;
             }
 
             // During HWUI output this canvas may be nullptr.
             SkCanvas* canvas = target->getCanvas();
-            const char* config = target->config.name;
+            const char* config = target->config.name.c_str();
 
             target->setup();
             bench->perCanvasPreDraw(canvas);
 
             int maxFrameLag;
             int loops = target->needsFrameTiming(&maxFrameLag)
                 ? setup_gpu_bench(target, bench.get(), maxFrameLag)
                 : setup_cpu_bench(overhead, target, bench.get());
 
             if (FLAGS_ms) {
@@ skipping 122 matching lines @@
 
     return 0;
 }
 
 #if !defined SK_BUILD_FOR_IOS
 int main(int argc, char** argv) {
     SkCommandLineFlags::Parse(argc, argv);
     return nanobench_main();
 }
 #endif