Adding support for parsing extended gpu config parameters in nanobench.

bench/nanobench.cpp

Index: bench/nanobench.cpp
diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index 7efd858c5cd5a8a0e761c399a839fa6964526e2b..070bcc6845a6fc4073842f2d4cb7655d2bf5e4c2 100644
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -185,7 +185,7 @@ struct GPUTarget : public Target {
         }
         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;
     }
@@ -411,14 +411,56 @@ static bool is_gpu_config_allowed(const char* name, GrContextFactory::GLContextT
 #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,

-            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) {
@@ -432,7 +474,7 @@ static void create_configs(SkTDArray<Config>* configs) {
         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,                                             \
@@ -440,7 +482,7 @@ static void create_configs(SkTDArray<Config>* configs) {
                 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) {
@@ -472,7 +514,7 @@ static void create_configs(SkTDArray<Config>* configs) {
         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.

 }
@@ -1090,8 +1132,9 @@ int nanobench_main() {
                  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();
@@ -1105,7 +1148,7 @@ int nanobench_main() {
             continue;
         }
 
-        if (!configs.isEmpty()) {
+        if (!configs.empty()) {
             log->bench(bench->getUniqueName(), bench->getSize().fX, bench->getSize().fY);
             bench->delayedSetup();
         }
@@ -1117,7 +1160,7 @@ int nanobench_main() {
 
             // 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);