Major bench refactoring.

bench/benchmain.cpp

 
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
-
-#include "BenchTimer.h"
-
 #if SK_SUPPORT_GPU
 #include "GrContext.h"
 #include "GrContextFactory.h"
-#include "gl/GrGLDefines.h"
 #include "GrRenderTarget.h"
 #include "SkGpuDevice.h"
+#include "gl/GrGLDefines.h"
 #else
 class GrContext;
 #endif // SK_SUPPORT_GPU
 
+#include "BenchTimer.h"
 #include "SkBenchLogger.h"
 #include "SkBenchmark.h"
 #include "SkBitmapDevice.h"
 #include "SkCanvas.h"
 #include "SkCommandLineFlags.h"
 #include "SkDeferredCanvas.h"
-#include "SkColorPriv.h"
 #include "SkGraphics.h"
 #include "SkImageEncoder.h"
-#include "SkNWayCanvas.h"
+#include "SkOSFile.h"
 #include "SkPicture.h"
 #include "SkString.h"
-#include "SkTArray.h"
-#include "TimerData.h"

[epoger, 2013/09/04 14:36:40]

Now that benchmain doesn't use TimerData.h anymore, does it mean that we can
delete TimerData.h and TimerData.cpp?

(For that matter, why not update the code in TimerData.h rather than replacing
TimerData::TimerFlags with TimerTypes within this file?  It would be easier to
see what is changing that way, I think.)

[mtklein, 2013/09/04 19:06:51]

Yeah, deleting TimerData is coming.  I haven't done it yet because
bench_pictures is still using it, and this seemed massive enough as it is.  My
plan is to delete TimerData after folding bench_pictures into bench.

 
-enum benchModes {
-    kNormal_benchModes,
-    kDeferred_benchModes,
-    kDeferredSilent_benchModes,
-    kRecord_benchModes,
-    kPictureRecord_benchModes
+enum BenchMode {
+    kNormal_BenchMode,
+    kDeferred_BenchMode,
+    kDeferredSilent_BenchMode,
+    kRecord_BenchMode,
+    kPictureRecord_BenchMode
 };
+const char* BenchMode_Name[] = { "normal", "deferred", "deferredSilent", "record", "picturerecord" };
 
-#ifdef SK_DEBUG
-static const bool kDebugOnly = true;
-#else
-static const bool kDebugOnly = false;
-#endif
+enum TimerTypes {
+    kWall_TimerType,
+    kTruncWall_TimerType,
+    kCpu_TimerType,
+    kTruncCpu_TimerType,
+    kGpu_TimerType,
+};
+const char TimerType_Short[] = { 'w', 'W', 'c', 'C', 'g' };
+const char* TimerType_Name[] = { "msecs", "Wmsecs", "cmsecs", "Cmsecs", "gmsecs" };
 
 ///////////////////////////////////////////////////////////////////////////////
 
 static void erase(SkBitmap& bm) {
     if (bm.config() == SkBitmap::kA8_Config) {
         bm.eraseColor(SK_ColorTRANSPARENT);
     } else {
         bm.eraseColor(SK_ColorWHITE);
     }
 }
 
-#if 0
-static bool equal(const SkBitmap& bm1, const SkBitmap& bm2) {
-    if (bm1.width() != bm2.width() ||
-        bm1.height() != bm2.height() ||
-        bm1.config() != bm2.config()) {
-        return false;
-    }
-
-    size_t pixelBytes = bm1.width() * bm1.bytesPerPixel();
-    for (int y = 0; y < bm1.height(); y++) {
-        if (memcmp(bm1.getAddr(0, y), bm2.getAddr(0, y), pixelBytes)) {
-            return false;
-        }
-    }
-    return true;
-}
-#endif
-
 class Iter {
 public:
-    Iter(void* param) {
-        fBench = BenchRegistry::Head();
-        fParam = param;
-    }
+    Iter() : fBench(BenchRegistry::Head()) {}
 
     SkBenchmark* next() {
         if (fBench) {
             BenchRegistry::Factory f = fBench->factory();
             fBench = fBench->next();
-            return f(fParam);
+            return f(NULL);
         }
         return NULL;
     }
 
 private:
     const BenchRegistry* fBench;
-    void* fParam;
 };
 
 class AutoPrePostDraw {
 public:
     AutoPrePostDraw(SkBenchmark* bench) : fBench(bench) {
         fBench->preDraw();
     }
     ~AutoPrePostDraw() {
         fBench->postDraw();
     }
@@ skipping 29 matching lines @@
         size_t size = copy.getSize() >> 2;
         SkPMColor* p = copy.getAddr32(0, 0);
         for (size_t i = 0; i < size; i++) {
             int c = (*p >> SK_A32_SHIFT) & 0xFF;
             c = 255 - c;
             c |= (c << 24) | (c << 16) | (c << 8);
             *p++ = c | (SK_A32_MASK << SK_A32_SHIFT);
         }
     }
 
-    SkString str;
-    make_filename(name, &str);
-    str.appendf("_%s.png", config);
-    str.prepend(dir);
-    ::remove(str.c_str());
-    SkImageEncoder::EncodeFile(str.c_str(), copy, SkImageEncoder::kPNG_Type,
-                               100);
+    SkString filename;
+    make_filename(name, &filename);
+    filename.appendf("_%s.png", config);
+    SkString path = SkOSPath::SkPathJoin(dir, filename.c_str());
+    ::remove(path.c_str());
+    SkImageEncoder::EncodeFile(path.c_str(), copy, SkImageEncoder::kPNG_Type, 100);
 }
 
 static void performClip(SkCanvas* canvas, int w, int h) {
     SkRect r;
 
     r.set(SkIntToScalar(10), SkIntToScalar(10),
           SkIntToScalar(w*2/3), SkIntToScalar(h*2/3));
     canvas->clipRect(r, SkRegion::kIntersect_Op);
 
     r.set(SkIntToScalar(w/3), SkIntToScalar(h/3),
@@ skipping 13 matching lines @@
 static void performScale(SkCanvas* canvas, int w, int h) {
     const SkScalar x = SkIntToScalar(w) / 2;
     const SkScalar y = SkIntToScalar(h) / 2;
 
     canvas->translate(x, y);
     // just enough so we can't take the sprite case
     canvas->scale(SK_Scalar1 * 99/100, SK_Scalar1 * 99/100);
     canvas->translate(-x, -y);
 }
 
-static bool parse_bool_arg(char * const* argv, char* const* stop, bool* var) {
-    if (argv < stop) {
-        *var = atoi(*argv) != 0;
-        return true;
-    }
-    return false;
-}
-
 enum Backend {
     kNonRendering_Backend,
     kRaster_Backend,
     kGPU_Backend,
     kPDF_Backend,
 };
 
 static SkBaseDevice* make_device(SkBitmap::Config config, const SkIPoint& size,
                                  Backend backend, int sampleCount, GrContext* context) {
     SkBaseDevice* device = NULL;
@@ skipping 25 matching lines @@
         case kPDF_Backend:
         default:
             SkDEBUGFAIL("unsupported");
     }
     return device;
 }
 
 #if SK_SUPPORT_GPU
 GrContextFactory gContextFactory;
 typedef GrContextFactory::GLContextType GLContextType;
-static const GLContextType kDontCareGLCtxType = GrContextFactory::kNative_GLContextType;
+static const GLContextType kNative = GrContextFactory::kNative_GLContextType;
+#if SK_ANGLE
+static const GLContextType kANGLE  = GrContextFactory::kANGLE_GLContextType;
+#else
+static const GLContextType kANGLE  = kNative;
+#endif
+static const GLContextType kDebug  = GrContextFactory::kDebug_GLContextType;
+static const GLContextType kNull   = GrContextFactory::kNull_GLContextType;
 #else
 typedef int GLContextType;
-static const GLContextType kDontCareGLCtxType = 0;
+static const GLContextType kNative = 0, kANGLE = 0, kDebug = 0, kNull = 0;
 #endif
 
-static const struct {
-    SkBitmap::Config    fConfig;
-    const char*         fName;
-    int                 fSampleCnt;
-    Backend             fBackend;
-    GLContextType       fContextType;
-    bool                fRunByDefault;
+#ifdef SK_DEBUG
+static const bool kIsDebug = true;
+#else
+static const bool kIsDebug = false;
+#endif
+
+static const struct Config {
+    SkBitmap::Config    config;
+    const char*         name;
+    int                 sampleCount;
+    Backend             backend;
+    GLContextType       contextType;
+    bool                runByDefault;
 } gConfigs[] = {
-    { SkBitmap::kNo_Config,         "NONRENDERING", 0, kNonRendering_Backend, kDontCareGLCtxType,                      true     },
-    { SkBitmap::kARGB_8888_Config,  "8888",         0, kRaster_Backend,       kDontCareGLCtxType,                      true     },
-    { SkBitmap::kRGB_565_Config,    "565",          0, kRaster_Backend,       kDontCareGLCtxType,                      true     },
+    { SkBitmap::kNo_Config,        "NONRENDERING", 0, kNonRendering_Backend, kNative, true},
+    { SkBitmap::kARGB_8888_Config, "8888",         0, kRaster_Backend,       kNative, true},
+    { SkBitmap::kRGB_565_Config,   "565",          0, kRaster_Backend,       kNative, true},
 #if SK_SUPPORT_GPU
-    { SkBitmap::kARGB_8888_Config,  "GPU",          0, kGPU_Backend,          GrContextFactory::kNative_GLContextType, true     },
-    { SkBitmap::kARGB_8888_Config,  "MSAA4",        4, kGPU_Backend,          GrContextFactory::kNative_GLContextType, false    },
-    { SkBitmap::kARGB_8888_Config,  "MSAA16",      16, kGPU_Backend,          GrContextFactory::kNative_GLContextType, false    },
+    { SkBitmap::kARGB_8888_Config, "GPU",          0, kGPU_Backend,          kNative, true},
+    { SkBitmap::kARGB_8888_Config, "MSAA4",        4, kGPU_Backend,          kNative, false},
+    { SkBitmap::kARGB_8888_Config, "MSAA16",      16, kGPU_Backend,          kNative, false},
 #if SK_ANGLE
-    { SkBitmap::kARGB_8888_Config,  "ANGLE",        0, kGPU_Backend,          GrContextFactory::kANGLE_GLContextType,  true     },
+    { SkBitmap::kARGB_8888_Config, "ANGLE",        0, kGPU_Backend,          kANGLE,  true},
 #endif // SK_ANGLE
-    { SkBitmap::kARGB_8888_Config,  "Debug",        0, kGPU_Backend,          GrContextFactory::kDebug_GLContextType,  kDebugOnly },
-    { SkBitmap::kARGB_8888_Config,  "NULLGPU",      0, kGPU_Backend,          GrContextFactory::kNull_GLContextType,   true     },
+    { SkBitmap::kARGB_8888_Config, "Debug",        0, kGPU_Backend,          kDebug,  kIsDebug},
+    { SkBitmap::kARGB_8888_Config, "NULLGPU",      0, kGPU_Backend,          kNull,   true},
 #endif // SK_SUPPORT_GPU
 };
 
-static int findConfig(const char config[]) {
-    for (size_t i = 0; i < SK_ARRAY_COUNT(gConfigs); i++) {
-        if (!strcmp(config, gConfigs[i].fName)) {
-            return i;
-        }
-    }
-    return -1;
-}
+DEFINE_string(outDir, "", "If given, image of each bench will be put in outDir.");

[epoger, 2013/09/04 14:36:40]

In future CLs (I'm not asking you to back out any changes here!) I would really
prefer for a massive set of modifications like this to be split into multiple
CLs.

For example, switching to SkCommandLineFlags would have been a great candidate
for a CL of its own.

Reasons for this:
1. Let's say there is some problem with this CL (unrelated to the switch to
SkCommandLineFlags) after we commit it, so we roll it out… that will cause us to
lose the switch to SkCommandLineFlags, along with the other change.
2. It's a lot easier to fully understand/review piecewise changes like that.

On the other hand, I know sometimes I start in on a CL and I find "one little
related thing" to fix along the way, and before you know it I have what would
ideally be 2 separate CLs in one giant CL.  My guess is that may be what
happened here.

Anyway, I wanted to make sure you knew my preference on that, and if anyone has
counterpoints to my above reasoning I would love to hear them.  I am prepared to
change my mind. :-)

[mtklein, 2013/09/04 19:06:51]

Yeah, this was a case where the code was such that I didn't trust myself to make
a correct change without converting to FLAGS_ first.  I don't mind splitting out
the flags into another CL if you like.

+DEFINE_string(timers, "cg", "Timers to display. "
+              "Options: w(all) W(all, truncated) c(pu) C(pu, truncated) g(pu)");
 
-static void help() {
-    SkString configsStr;
-    static const size_t kConfigCount = SK_ARRAY_COUNT(gConfigs);
-    for (size_t i = 0; i < kConfigCount; ++i) {
-        configsStr.appendf("%s%s", gConfigs[i].fName, ((i == kConfigCount - 1) ? "" : "|"));
-    }
+DEFINE_bool(rotate, false,  "Rotate canvas before bench run?");
+DEFINE_bool(scale,  false,  "Scale canvas before bench run?");
+DEFINE_bool(clip,   false,  "Clip canvas before bench run?");
 
-    SkDebugf("Usage: bench [-o outDir] [--repeat nr] [--logPerIter] "
-                          "[--timers [wcgWC]*] [--rotate]\n"
-             "    [--scale] [--clip] [--min] [--forceAA 1|0] [--forceFilter 1|0]\n"
-             "    [--forceDither 1|0] [--forceBlend 1|0]"
-#if SK_SUPPORT_GPU
-             " [--gpuCacheSize <bytes> <count>]"
-#endif
-             "\n"
-             "    [--strokeWidth width] [--match name]\n"
-             "    [--mode normal|deferred|deferredSilent|record|picturerecord]\n"
-             "    [--config ");
-    SkDebugf("%s]\n", configsStr.c_str());
-    SkDebugf("    [-Dfoo bar] [--logFile filename] [-h|--help]");
-    SkDebugf("\n\n");
-    SkDebugf("    -o outDir : Image of each bench will be put in outDir.\n");
-    SkDebugf("    --repeat nr : Each bench repeats for nr times.\n");
-    SkDebugf("    --logPerIter : "
-             "Log each repeat timer instead of mean, default is disabled.\n");
-    SkDebugf("    --timers [wcgWC]* : "
-             "Display wall, cpu, gpu, truncated wall or truncated cpu time for each bench.\n");
-    SkDebugf("    --rotate : Rotate before each bench runs.\n");
-    SkDebugf("    --scale : Scale before each bench runs.\n");
-    SkDebugf("    --clip : Clip before each bench runs.\n");
-    SkDebugf("    --min : Print the minimum times (instead of average).\n");
-    SkDebugf("    --forceAA 1|0 : "
-             "Enable/disable anti-aliased, default is enabled.\n");
-    SkDebugf("    --forceFilter 1|0 : "
-             "Enable/disable bitmap filtering, default is disabled.\n");
-    SkDebugf("    --forceDither 1|0 : "
-             "Enable/disable dithering, default is disabled.\n");
-    SkDebugf("    --forceBlend 1|0 : "
-             "Enable/disable dithering, default is disabled.\n");
-#if SK_SUPPORT_GPU
-    SkDebugf("    --gpuCacheSize <bytes> <count>: "
-             "limits gpu cache to  bytes size or object count.\n");
-    SkDebugf("      -1 for either value means use the default. 0 for either disables the cache.\n");
-#endif
-    SkDebugf("    --strokeWidth width : The width for path stroke.\n");
-    SkDebugf("    --match [~][^]substring[$] [...] of test name to run.\n"
-             "             Multiple matches may be separated by spaces.\n"
-             "             ~ causes a matching test to always be skipped\n"
-             "             ^ requires the start of the test to match\n"
-             "             $ requires the end of the test to match\n"
-             "             ^ and $ requires an exact match\n"
-             "             If a test does not match any list entry,\n"
-             "             it is skipped unless some list entry starts with ~\n");
-    SkDebugf("    --mode normal|deferred|deferredSilent|record|picturerecord :\n"
-             "             Run in the corresponding mode\n"
-             "                 normal, Use a normal canvas to draw to;\n"
-             "                 deferred, Use a deferrred canvas when drawing;\n"
-             "                 deferredSilent, deferred with silent playback;\n"
-             "                 record, Benchmark the time to record to an SkPicture;\n"
-             "                 picturerecord, Benchmark the time to do record from a \n"
-             "                                SkPicture to a SkPicture.\n");
-    SkDebugf("    --logFile filename : destination for writing log output, in addition to stdout.\n");
-    SkDebugf("    --config %s:\n", configsStr.c_str());
-    SkDebugf("             Run bench in corresponding config mode.\n");
-    SkDebugf("    -Dfoo bar : Add extra definition to bench.\n");
-    SkDebugf("    -h|--help : Show this help message.\n");
-}
+DEFINE_bool(forceAA,        true,     "Force anti-aliasing?");
+DEFINE_bool(forceFilter,    false,    "Force bitmap filtering?");
+DEFINE_string(forceDither, "default", "Force dithering: true, false, or default?");
+DEFINE_bool(forceBlend,     false,    "Force alpha blending?");
+
+DEFINE_int32(gpuCacheBytes, -1, "GPU cache size limit in bytes.  0 to disable cache.");
+DEFINE_int32(gpuCacheCount, -1, "GPU cache size limit in object count.  0 to disable cache.");
+
+DEFINE_string(match, "",  "[~][^]substring[$] [...] of test name to run.\n"
+                          "Multiple matches may be separated by spaces.\n"
+                          "~ causes a matching test to always be skipped\n"
+                          "^ requires the start of the test to match\n"
+                          "$ requires the end of the test to match\n"
+                          "^ and $ requires an exact match\n"
+                          "If a test does not match any list entry,\n"
+                          "it is skipped unless some list entry starts with ~\n");
+DEFINE_string(mode, "normal",
+             "normal:         draw to a normal canvas;\n"
+             "deferred:       draw to a deferred canvas;\n"
+             "deferredSilent: deferred with silent playback;\n"
+             "record:         draw to an SkPicture;\n"
+             "picturerecord:  draw from an SkPicture to an SkPicture.\n");
+DEFINE_string(config, "", "Run configs given.  If empty, runs the defaults set in gConfigs.");
+DEFINE_string(logFile, "", "Also write stdout here.");
+DEFINE_int32(benchMs, 20, "Target time in ms to run each benchmark config.");
+DEFINE_string(timeFormat, "%9.2f", "Format to print results, in milliseconds per 1000 loops.");
 
 int tool_main(int argc, char** argv);
 int tool_main(int argc, char** argv) {
 #if SK_ENABLE_INST_COUNT
     gPrintInstCount = true;
 #endif
     SkAutoGraphics ag;
+    SkCommandLineFlags::Parse(argc, argv);
 
-    SkTDict<const char*> defineDict(1024);
-    int repeatDraw = 1;
-
-    int forceAlpha = 0xFF;
-    bool forceAA = true;
-    bool forceFilter = false;
-    SkTriState::State forceDither = SkTriState::kDefault;
-
-    static const uint32_t kDefaultTimerTypes = TimerData::kCpu_Flag | TimerData::kGpu_Flag;
-    static const TimerData::Result kDefaultTimerResult = TimerData::kAvg_Result;
-    uint32_t timerTypes = kDefaultTimerTypes;
-    TimerData::Result timerResult = kDefaultTimerResult;
-
-    bool doScale = false;
-    bool doRotate = false;
-    bool doClip = false;
-    bool hasStrokeWidth = false;
-
-#if SK_SUPPORT_GPU
-    struct {
-        int     fBytes;
-        int     fCount;
-    } gpuCacheSize = { -1, -1 }; // -1s mean use the default
-#endif
-
-    float strokeWidth;
-    SkTDArray<const char*> fMatches;
-    benchModes benchMode = kNormal_benchModes;
-    SkString perIterTimeformat("%.2f");
-    SkString normalTimeFormat("%6.2f");
-
-    SkString outDir;
-    SkBitmap::Config outConfig = SkBitmap::kNo_Config;
-    const char* configName = "";
-    Backend backend = kRaster_Backend;  // for warning
-    int sampleCount = 0;
-    SkTDArray<int> configs;
-    bool userConfig = false;
+    // First, parse some flags.
 
     SkBenchLogger logger;
+    if (FLAGS_logFile.count()) {
+        logger.SetLogFile(FLAGS_logFile[0]);
+    }
 
-    char* const* stop = argv + argc;
-    for (++argv; argv < stop; ++argv) {
-        if (strcmp(*argv, "-o") == 0) {
-            argv++;
-            if (argv < stop && **argv) {
-                outDir.set(*argv);
-                if (outDir.c_str()[outDir.size() - 1] != '/') {
-                    outDir.append("/");
-                }
-            }
-        } else if (strcmp(*argv, "--repeat") == 0) {
-            argv++;
-            if (argv < stop) {
-                repeatDraw = atoi(*argv);
-                if (repeatDraw < 1) {
-                    repeatDraw = 1;
-                }
-            } else {
-                logger.logError("missing arg for --repeat\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--logPerIter") == 0) {
-            timerResult = TimerData::kPerIter_Result;
-        } else if (strcmp(*argv, "--timers") == 0) {
-            argv++;
-            if (argv < stop) {
-                timerTypes = 0;
-                for (char* t = *argv; *t; ++t) {
-                    switch (*t) {
-                    case 'w': timerTypes |= TimerData::kWall_Flag; break;
-                    case 'c': timerTypes |= TimerData::kCpu_Flag; break;
-                    case 'W': timerTypes |= TimerData::kTruncatedWall_Flag; break;
-                    case 'C': timerTypes |= TimerData::kTruncatedCpu_Flag; break;
-                    case 'g': timerTypes |= TimerData::kGpu_Flag; break;
-                    }
-                }
-            } else {
-                logger.logError("missing arg for --timers\n");
-                help();
-                return -1;
-            }
-        } else if (!strcmp(*argv, "--rotate")) {
-            doRotate = true;
-        } else if (!strcmp(*argv, "--scale")) {
-            doScale = true;
-        } else if (!strcmp(*argv, "--clip")) {
-            doClip = true;
-        } else if (!strcmp(*argv, "--min")) {
-            timerResult = TimerData::kMin_Result;
-        } else if (strcmp(*argv, "--forceAA") == 0) {
-            if (!parse_bool_arg(++argv, stop, &forceAA)) {
-                logger.logError("missing arg for --forceAA\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--forceFilter") == 0) {
-            if (!parse_bool_arg(++argv, stop, &forceFilter)) {
-                logger.logError("missing arg for --forceFilter\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--forceDither") == 0) {
-            bool tmp;
-            if (!parse_bool_arg(++argv, stop, &tmp)) {
-                logger.logError("missing arg for --forceDither\n");
-                help();
-                return -1;
-            }
-            forceDither = tmp ? SkTriState::kTrue : SkTriState::kFalse;
-        } else if (strcmp(*argv, "--forceBlend") == 0) {
-            bool wantAlpha = false;
-            if (!parse_bool_arg(++argv, stop, &wantAlpha)) {
-                logger.logError("missing arg for --forceBlend\n");
-                help();
-                return -1;
-            }
-            forceAlpha = wantAlpha ? 0x80 : 0xFF;
-#if SK_SUPPORT_GPU
-        } else if (strcmp(*argv, "--gpuCacheSize") == 0) {
-            if (stop - argv > 2) {
-                gpuCacheSize.fBytes = atoi(*++argv);
-                gpuCacheSize.fCount = atoi(*++argv);
-            } else {
-                SkDebugf("missing arg for --gpuCacheSize\n");
-                help();
-                return -1;
-            }
-#endif
-        } else if (strcmp(*argv, "--mode") == 0) {
-            argv++;
-            if (argv < stop) {
-                if (strcmp(*argv, "normal") == 0) {
-                    benchMode = kNormal_benchModes;
-                } else if (strcmp(*argv, "deferred") == 0) {
-                    benchMode = kDeferred_benchModes;
-                } else if (strcmp(*argv, "deferredSilent") == 0) {
-                    benchMode = kDeferredSilent_benchModes;
-                } else if (strcmp(*argv, "record") == 0) {
-                    benchMode = kRecord_benchModes;
-                } else if (strcmp(*argv, "picturerecord") == 0) {
-                    benchMode = kPictureRecord_benchModes;
-                } else {
-                    logger.logError("bad arg for --mode\n");
-                    help();
-                    return -1;
-                }
-            } else {
-                logger.logError("missing arg for --mode\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--strokeWidth") == 0) {
-            argv++;
-            if (argv < stop) {
-                const char *strokeWidthStr = *argv;
-                if (sscanf(strokeWidthStr, "%f", &strokeWidth) != 1) {
-                  logger.logError("bad arg for --strokeWidth\n");
-                  help();
-                  return -1;
-                }
-                hasStrokeWidth = true;
-            } else {
-                logger.logError("missing arg for --strokeWidth\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--match") == 0) {
-            argv++;
-            while (argv < stop && (*argv)[0] != '-') {
-                *fMatches.append() = *argv++;
-            }
-            argv--;
-            if (!fMatches.count()) {
-                logger.logError("missing arg for --match\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--config") == 0) {
-            argv++;
-            if (argv < stop) {
-                int index = findConfig(*argv);
-                if (index >= 0) {
-                    *configs.append() = index;
-                    userConfig = true;
-                } else {
-                    SkString str;
-                    str.printf("unrecognized config %s\n", *argv);
-                    logger.logError(str);
-                    help();
-                    return -1;
-                }
-            } else {
-                logger.logError("missing arg for --config\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--logFile") == 0) {
-            argv++;
-            if (argv < stop) {
-                if (!logger.SetLogFile(*argv)) {
-                    SkString str;
-                    str.printf("Could not open %s for writing.", *argv);
-                    logger.logError(str);
-                    return -1;
-                }
-            } else {
-                logger.logError("missing arg for --logFile\n");
-                help();
-                return -1;
-            }
-        } else if (strlen(*argv) > 2 && strncmp(*argv, "-D", 2) == 0) {
-            argv++;
-            if (argv < stop) {
-                defineDict.set(argv[-1] + 2, *argv);
-            } else {
-                logger.logError("incomplete '-Dfoo bar' definition\n");
-                help();
-                return -1;
-            }
-        } else if (strcmp(*argv, "--help") == 0 || strcmp(*argv, "-h") == 0) {
-            help();
-            return 0;
-        } else {
-            SkString str;
-            str.printf("unrecognized arg %s\n", *argv);
-            logger.logError(str);
-            help();
-            return -1;
+    const uint8_t alpha = FLAGS_forceBlend ? 0x80 : 0xFF;
+    SkTriState::State dither = SkTriState::kDefault;
+    for (size_t i = 0; i < 3; i++) {
+        if (strcmp(SkTriState::Name[i], FLAGS_forceDither[0]) == 0) {
+            dither = static_cast<SkTriState::State>(i);
         }
     }
-    if ((benchMode == kRecord_benchModes || benchMode == kPictureRecord_benchModes)
-            && !outDir.isEmpty()) {
-        logger.logError("'--mode record' and '--mode picturerecord' are not"
-                  " compatible with -o.\n");
-        return -1;
+
+    uint32_t timerTypes = 0;
+    for (const char* c = FLAGS_timers[0]; *c; ++c) {

[epoger, 2013/09/04 14:36:40]

I think code like this would be much easier to understand and reuse if it was
encapsulated within its own file, along the lines of
https://code.google.com/p/skia/source/browse/trunk/gm/gm_error.h .  Something
like this:

enum TimerTypes {
  kWall_TimerType,
  kTruncatedWall_TimerType,
  …
}

class TimerTypeSet {
  TimerTypeSet(const char *arrayOfShortNames);
  bool includes(TimerType t);
}

As the code stands now, the details of how to read/write the timerTypes bitfield
are entangled within the benchmain code at large.

[mtklein, 2013/09/04 19:06:51]

Agreed.  I took a closer look at this and realized that timers were way
overcomplicated.  I redid all the timer code to be considerably simpler, which
should clean up all of these problems.

+        for (size_t i = 0; i < SK_ARRAY_COUNT(TimerType_Short); i++) {
+            if (*c == TimerType_Short[i]) {
+                timerTypes |= (1<<i);
+            }
+        }
     }
-    if ((benchMode == kRecord_benchModes || benchMode == kPictureRecord_benchModes)) {
-        perIterTimeformat.set("%.4f");
-        normalTimeFormat.set("%6.4f");
+
+    BenchMode benchMode = kNormal_BenchMode;
+    for (size_t i = 0; i < SK_ARRAY_COUNT(BenchMode_Name); i++) {
+        if (strcmp(FLAGS_mode[0], BenchMode_Name[i]) == 0) {
+            benchMode = static_cast<BenchMode>(i);
+        }
     }
-    if (!userConfig) {
-        // if no config is specified by user, add the default configs
-        for (unsigned int i = 0; i < SK_ARRAY_COUNT(gConfigs); ++i) {
-            if (gConfigs[i].fRunByDefault) {
+
+    SkTDArray<int> configs;
+    // Try user-given configs first.
+    for (int i = 0; i < FLAGS_config.count(); i++) {
+        for (size_t j = 0; j < SK_ARRAY_COUNT(gConfigs); j++) {
+            if (0 == strcmp(FLAGS_config[i], gConfigs[j].name)) {
+                *configs.append() = j;
+            }
+        }
+    }
+    // If there weren't any, fill in with defaults.
+    if (configs.count() == 0) {
+        for (size_t i = 0; i < SK_ARRAY_COUNT(gConfigs); ++i) {
+            if (gConfigs[i].runByDefault) {
                 *configs.append() = i;
             }
         }
     }
-    if (kNormal_benchModes != benchMode) {
+    // Filter out things we can't run.
+    if (kNormal_BenchMode != benchMode) {
         // Non-rendering configs only run in normal mode
         for (int i = 0; i < configs.count(); ++i) {
-            int configIdx = configs[i];
-            if (kNonRendering_Backend == gConfigs[configIdx].fBackend) {
+            const Config& config = gConfigs[configs[i]];
+            if (kNonRendering_Backend == config.backend) {
                 configs.remove(i, 1);
                 --i;
             }
         }
     }
-
 #if SK_SUPPORT_GPU
     for (int i = 0; i < configs.count(); ++i) {
-        int configIdx = configs[i];
+        const Config& config = gConfigs[configs[i]];
 
-        if (kGPU_Backend == gConfigs[configIdx].fBackend && gConfigs[configIdx].fSampleCnt > 0) {
-            GrContext* context = gContextFactory.get(gConfigs[configIdx].fContextType);
+        if (kGPU_Backend == config.backend) {
+            GrContext* context = gContextFactory.get(config.contextType);
             if (NULL == context) {
                 SkString error;
                 error.printf("Error creating GrContext for config %s. Config will be skipped.\n",
-                             gConfigs[configIdx].fName);
-                logger.logError(error.c_str());
+                             config.name);
+                logger.logError(error);
                 configs.remove(i);
                 --i;
                 continue;
             }
-            if (gConfigs[configIdx].fSampleCnt > context->getMaxSampleCount()){
+            if (config.sampleCount > context->getMaxSampleCount()){
                 SkString error;
                 error.printf("Sample count (%d) for config %s is unsupported. "
                              "Config will be skipped.\n",
-                             gConfigs[configIdx].fSampleCnt, gConfigs[configIdx].fName);
-                logger.logError(error.c_str());
+                             config.sampleCount, config.name);
+                logger.logError(error);
                 configs.remove(i);
                 --i;
                 continue;
             }
         }
     }
 #endif
 
-    // report our current settings
-    {
-        SkString str;
-        const char* deferredMode = benchMode == kDeferred_benchModes ? "yes" :
-            (benchMode == kDeferredSilent_benchModes ? "silent" : "no");
-        str.printf("skia bench: alpha=0x%02X antialias=%d filter=%d "
-                   "deferred=%s logperiter=%d",
-                   forceAlpha, forceAA, forceFilter, deferredMode,
-                   TimerData::kPerIter_Result == timerResult);
-        str.appendf(" rotate=%d scale=%d clip=%d min=%d",
-                   doRotate, doScale, doClip, TimerData::kMin_Result == timerResult);
-        str.appendf(" record=%d picturerecord=%d",
-                    benchMode == kRecord_benchModes,
-                    benchMode == kPictureRecord_benchModes);
-        const char * ditherName;
-        switch (forceDither) {
-            case SkTriState::kDefault: ditherName = "default"; break;
-            case SkTriState::kTrue: ditherName = "true"; break;
-            case SkTriState::kFalse: ditherName = "false"; break;
-            default: ditherName = "<invalid>"; break;
-        }
-        str.appendf(" dither=%s", ditherName);
-
-        if (hasStrokeWidth) {
-            str.appendf(" strokeWidth=%f", strokeWidth);
-        } else {
-            str.append(" strokeWidth=none");
-        }
-
-#if defined(SK_SCALAR_IS_FLOAT)
-        str.append(" scalar=float");
-#elif defined(SK_SCALAR_IS_FIXED)
-        str.append(" scalar=fixed");
+    // All flags should be parsed now.  Report our settings.
+    SkString str("skia bench:");
+    str.appendf(" mode=%s", FLAGS_mode[0]);
+    str.appendf(" alpha=0x%02X antialias=%d filter=%d dither=%s",
+                alpha, FLAGS_forceAA, FLAGS_forceFilter, SkTriState::Name[dither]);
+    str.appendf(" rotate=%d scale=%d clip=%d", FLAGS_rotate, FLAGS_scale, FLAGS_clip);
+
+#if defined(SK_SCALAR_IS_FIXED)
+    str.append(" scalar=fixed");
+#else
+    str.append(" scalar=float");
 #endif
 
 #if defined(SK_BUILD_FOR_WIN32)
-        str.append(" system=WIN32");
+    str.append(" system=WIN32");
 #elif defined(SK_BUILD_FOR_MAC)
-        str.append(" system=MAC");
+    str.append(" system=MAC");
 #elif defined(SK_BUILD_FOR_ANDROID)
-        str.append(" system=ANDROID");
+    str.append(" system=ANDROID");
 #elif defined(SK_BUILD_FOR_UNIX)
-        str.append(" system=UNIX");
+    str.append(" system=UNIX");
 #else
-        str.append(" system=other");
+    str.append(" system=other");
 #endif
 
 #if defined(SK_DEBUG)
-        str.append(" DEBUG");
-#endif
-        str.append("\n");
-        logger.logProgress(str);
-    }
-
-    SkTArray<BenchTimer*> timers(SK_ARRAY_COUNT(gConfigs));
+    str.append(" DEBUG");
+#endif
+    str.append("\n");
+    logger.logProgress(str);
+
+
+    // Set texture cache limits and get timers suitable for each config.
+    SkTDArray<BenchTimer*> timers;
     for (size_t i = 0; i < SK_ARRAY_COUNT(gConfigs); ++i) {
 #if SK_SUPPORT_GPU
+        const Config& config = gConfigs[i];
         SkGLContextHelper* glCtx = NULL;
-        if (kGPU_Backend == gConfigs[i].fBackend) {
-            GrContext* context = gContextFactory.get(gConfigs[i].fContextType);
+        if (kGPU_Backend == config.backend) {
+            GrContext* context = gContextFactory.get(config.contextType);
             if (NULL != context) {
                 // Set the user specified cache limits if non-default.
                 size_t bytes;
                 int count;
                 context->getTextureCacheLimits(&count, &bytes);
-                if (-1 != gpuCacheSize.fBytes) {
-                    bytes = static_cast<size_t>(gpuCacheSize.fBytes);
-                }
-                if (-1 != gpuCacheSize.fCount) {
-                    count = gpuCacheSize.fCount;
+                if (-1 != FLAGS_gpuCacheBytes) {
+                    bytes = static_cast<size_t>(FLAGS_gpuCacheBytes);
+                }
+                if (-1 != FLAGS_gpuCacheCount) {
+                    count = FLAGS_gpuCacheCount;
                 }
                 context->setTextureCacheLimits(count, bytes);
             }
-            glCtx = gContextFactory.getGLContext(gConfigs[i].fContextType);
-        }
-        timers.push_back(SkNEW_ARGS(BenchTimer, (glCtx)));
+            glCtx = gContextFactory.getGLContext(config.contextType);
+        }
+        *timers.append() = SkNEW_ARGS(BenchTimer, (glCtx));
 #else
-        timers.push_back(SkNEW(BenchTimer));
-#endif
-    }
-
-    Iter iter(&defineDict);
+        *timers.append() = SkNEW(BenchTimer);
+#endif
+    }
+
+    // Find the longest name of the benches we're going to run to make the output pretty.
+    Iter names;
     SkBenchmark* bench;
+    int longestName = 0;
+    while ((bench = names.next()) != NULL) {
+        SkAutoTUnref<SkBenchmark> benchUnref(bench);
+        if (SkCommandLineFlags::ShouldSkip(FLAGS_match, bench->getName())) {
+            continue;
+        }
+        const int length = strlen(bench->getName());
+        longestName = length > longestName ? length : longestName;
+    }
+
+    // Run each bench in each configuration it supports and we asked for.

[epoger, 2013/09/04 14:36:40]

Where is (was) the outer repeat loop that you deleted in this CL?  I don't see
it... obscured by all the other changes, I'm afraid.

[mtklein, 2013/09/04 19:06:51]

Yeah, exactly.  Hard to read.  It was everything involving repeatDraw.

+    Iter iter;
     while ((bench = iter.next()) != NULL) {
         SkAutoTUnref<SkBenchmark> benchUnref(bench);
-
-        SkIPoint dim = bench->getSize();
-        if (dim.fX <= 0 || dim.fY <= 0) {
+        if (SkCommandLineFlags::ShouldSkip(FLAGS_match, bench->getName())) {
             continue;
         }
 
-        bench->setForceAlpha(forceAlpha);
-        bench->setForceAA(forceAA);
-        bench->setForceFilter(forceFilter);
-        bench->setDither(forceDither);
-        if (hasStrokeWidth) {
-            bench->setStrokeWidth(strokeWidth);
-        }
-
-        // only run benchmarks if their name contains matchStr
-        if (SkCommandLineFlags::ShouldSkip(fMatches, bench->getName())) {
-            continue;
-        }
-
-        bool loggedBenchStart = false;
-
+        bench->setForceAlpha(alpha);
+        bench->setForceAA(FLAGS_forceAA);
+        bench->setForceFilter(FLAGS_forceFilter);
+        bench->setDither(dither);
         AutoPrePostDraw appd(bench);
 
-        for (int x = 0; x < configs.count(); ++x) {
-            int configIndex = configs[x];
-
-            bool setupFailed = false;
-
-            if (kNonRendering_Backend == gConfigs[configIndex].fBackend) {
-                if (bench->isRendering()) {
-                    continue;
-                }
-            } else {
-                if (!bench->isRendering()) {
-                    continue;
-                }
-            }
-
-            outConfig = gConfigs[configIndex].fConfig;
-            configName = gConfigs[configIndex].fName;
-            backend = gConfigs[configIndex].fBackend;
-            sampleCount = gConfigs[configIndex].fSampleCnt;
+        bool loggedBenchName = false;
+        for (int i = 0; i < configs.count(); ++i) {
+            const int configIndex = configs[i];
+            const Config& config = gConfigs[configIndex];
+
+            if ((kNonRendering_Backend == config.backend) == bench->isRendering()) {
+                continue;
+            }
+
             GrContext* context = NULL;
-            BenchTimer* timer = timers[configIndex];
-
 #if SK_SUPPORT_GPU
             SkGLContextHelper* glContext = NULL;
-            if (kGPU_Backend == backend) {
-                context = gContextFactory.get(gConfigs[configIndex].fContextType);
+            if (kGPU_Backend == config.backend) {
+                context = gContextFactory.get(config.contextType);
                 if (NULL == context) {
                     continue;
                 }
-                glContext = gContextFactory.getGLContext(gConfigs[configIndex].fContextType);
-            }
-#endif
-            SkBaseDevice* device = NULL;
-            SkCanvas* canvas = NULL;
-            SkPicture pictureRecordFrom;
-            SkPicture pictureRecordTo;
-
-            if (kNonRendering_Backend != backend) {
-                device = make_device(outConfig, dim, backend, sampleCount, context);
-                if (NULL == device) {
+                glContext = gContextFactory.getGLContext(config.contextType);
+            }
+#endif
+            SkAutoTUnref<SkBaseDevice> device;
+            SkAutoTUnref<SkCanvas> canvas;
+            SkPicture recordFrom, recordTo;
+            const SkIPoint dim = bench->getSize();
+
+            const SkPicture::RecordingFlags kRecordFlags =
+                SkPicture::kUsePathBoundsForClip_RecordingFlag;
+
+            if (kNonRendering_Backend != config.backend) {
+                device.reset(make_device(config.config,
+                                         dim,
+                                         config.backend,
+                                         config.sampleCount,
+                                         context));
+                if (!device.get()) {
                     SkString error;
-                    error.printf("Device creation failure for config %s. Will skip.\n", configName);
-                    logger.logError(error.c_str());
-                    setupFailed = true;
+                    error.printf("Device creation failure for config %s. Will skip.\n", config.name);
+                    logger.logError(error);
+                    continue;
+                }
+
+                switch(benchMode) {
+                    case kDeferredSilent_BenchMode:
+                    case kDeferred_BenchMode:
+                        canvas.reset(SkDeferredCanvas::Create(device.get()));
+                        break;
+                    case kRecord_BenchMode:
+                        canvas.reset(SkRef(recordTo.beginRecording(dim.fX, dim.fY, kRecordFlags)));
+                        break;
+                    case kPictureRecord_BenchMode:
+                        bench->draw(recordFrom.beginRecording(dim.fX, dim.fY, kRecordFlags));
+                        recordFrom.endRecording();
+                        canvas.reset(SkRef(recordTo.beginRecording(dim.fX, dim.fY, kRecordFlags)));
+                        break;
+                    case kNormal_BenchMode:
+                        canvas.reset(new SkCanvas(device.get()));
+                        break;
+                    default:
+                        SkASSERT(false);
+                }
+            }
+
+            if (NULL != canvas) {
+                canvas->clear(SK_ColorWHITE);
+                if (FLAGS_clip)   {   performClip(canvas, dim.fX, dim.fY); }
+                if (FLAGS_scale)  {  performScale(canvas, dim.fX, dim.fY); }
+                if (FLAGS_rotate) { performRotate(canvas, dim.fX, dim.fY); }
+            }
+
+            if (!loggedBenchName) {
+                loggedBenchName = true;
+                SkString str;
+                str.printf("running bench [%3d %3d] %*s ",
+                           dim.fX, dim.fY, longestName, bench->getName());
+                logger.logProgress(str);
+            }
+
+            BenchTimer* const timer = timers[configIndex];
+            bench->setLoops(0);
+            do {
+                // Ramp up 1 -> 4 -> 16 -> ... -> ~1 billion.
+                const int loops = bench->getLoops();
+                if (loops >= (1<<30)) {

[epoger, 2013/09/04 14:36:40]

What's with the magic "30" here?  I take it you are trying to make sure we don't
hit MAX_INT?

[mtklein, 2013/09/04 19:06:51]

1<<30 is about a billion, which is a somewhat arbitrary cutoff for the number of
loops we allow to run.  We can make this a flag or a named constant if you want,
but I wouldn't bother until we find we need to change it.  1<<30 ought to be a
big enough upper limit to reach ~millisecond runtimes until we start getting
CPUs running at >=THz and the loop bodies get down into ~picoseconds.  In
practice, a couple million is fine for skia.

You're right, though, that like in most other cases involving overflow, overflow
would break things terribly.  This check does double duty.

[epoger, 2013/09/04 19:18:26]

That all sounds fine, please just paste your explanation into the code as a
comment.

[mtklein, 2013/09/04 19:25:15]

Done.

+                    SkString str;
+                    str.printf("Can't ramp %s to %dms.\n", bench->getName(), FLAGS_benchMs);
+                    logger.logError(str);
+                    break;
+                }
+                bench->setLoops(loops == 0 ? 1 : loops * 4);
+
+                if ((benchMode == kRecord_BenchMode || benchMode == kPictureRecord_BenchMode)) {
+                    // Clear the recorded commands so that they do not accumulate.
+                    canvas.reset(recordTo.beginRecording(dim.fX, dim.fY, kRecordFlags));
+                }
+
+                timer->start();
+                if (NULL != canvas) {
+                    canvas->save();
+                }
+                if (benchMode == kPictureRecord_BenchMode) {
+                    recordFrom.draw(canvas);
                 } else {
-                    switch(benchMode) {
-                        case kDeferredSilent_benchModes:
-                        case kDeferred_benchModes:
-                            canvas = SkDeferredCanvas::Create(device);
-                            break;
-                        case kRecord_benchModes:
-                            canvas = pictureRecordTo.beginRecording(dim.fX, dim.fY,
-                                SkPicture::kUsePathBoundsForClip_RecordingFlag);
-                            canvas->ref();
-                            break;
-                        case kPictureRecord_benchModes: {
-                            // This sets up picture-to-picture recording.
-                            // The C++ drawing calls for the benchmark are recorded into
-                            // pictureRecordFrom. As the benchmark, we will time how
-                            // long it takes to playback pictureRecordFrom into
-                            // pictureRecordTo.
-                            SkCanvas* tempCanvas = pictureRecordFrom.beginRecording(dim.fX, dim.fY,
-                                SkPicture::kUsePathBoundsForClip_RecordingFlag);
-                            bench->draw(tempCanvas);
-                            pictureRecordFrom.endRecording();
-                            canvas = pictureRecordTo.beginRecording(dim.fX, dim.fY,
-                                SkPicture::kUsePathBoundsForClip_RecordingFlag);
-                            canvas->ref();
-                            break;
-                        }
-                        case kNormal_benchModes:
-                            canvas = new SkCanvas(device);
-                            break;
-                        default:
-                            SkASSERT(0);
-                    }
-                    device->unref();
-                    canvas->clear(SK_ColorWHITE);
-                }
-            }
-            SkAutoUnref canvasUnref(canvas);
-            if (!setupFailed) {
+                    bench->draw(canvas);
+                }
+
+                if (kDeferredSilent_BenchMode == benchMode) {
+                    static_cast<SkDeferredCanvas*>(canvas.get())->silentFlush();
+                } else if (NULL != canvas) {
+                    canvas->flush();
+                }
+
                 if (NULL != canvas) {
-                    if (doClip) {
-                        performClip(canvas, dim.fX, dim.fY);
-                    }
-                    if (doScale) {
-                        performScale(canvas, dim.fX, dim.fY);
-                    }
-                    if (doRotate) {
-                        performRotate(canvas, dim.fX, dim.fY);
-                    }
-                }
-
-                if (!loggedBenchStart) {
-                    loggedBenchStart = true;
-                    SkString str;
-                    str.printf("running bench [%d %d] %28s", dim.fX, dim.fY, bench->getName());
-                    logger.logProgress(str);
-                }
-
-                // warm up caches if needed
-                if (repeatDraw > 1 && NULL != canvas) {
-#if SK_SUPPORT_GPU
-                    // purge the GPU resources to reduce variance
-                    if (NULL != context) {
-                        context->freeGpuResources();
-                    }
-#endif
-                    SkAutoCanvasRestore acr(canvas, true);
-                    if (benchMode == kPictureRecord_benchModes) {
-                        pictureRecordFrom.draw(canvas);
-                    } else {
-                        bench->draw(canvas);
-                    }
-
-                    if (kDeferredSilent_benchModes == benchMode) {
-                        static_cast<SkDeferredCanvas*>(canvas)->silentFlush();
-                    } else {
-                        canvas->flush();
-                    }
-#if SK_SUPPORT_GPU
-                    if (NULL != context) {
-                        context->flush();
-                        SK_GL(*glContext, Finish());
-                    }
-#endif
-                }
-
-                // record timer values for each repeat, and their sum
-                TimerData timerData(repeatDraw);
-                for (int i = 0; i < repeatDraw; i++) {
-                    if ((benchMode == kRecord_benchModes || benchMode == kPictureRecord_benchModes)) {
-                        // This will clear the recorded commands so that they do not
-                        // accumulate.
-                        canvas = pictureRecordTo.beginRecording(dim.fX, dim.fY,
-                            SkPicture::kUsePathBoundsForClip_RecordingFlag);
-                    }
-
-                    timer->start(bench->getDurationScale());
-                    if (NULL != canvas) {
-                        canvas->save();
-                    }
-                    if (benchMode == kPictureRecord_benchModes) {
-                        pictureRecordFrom.draw(canvas);
-                    } else {
-                        bench->draw(canvas);
-                    }
-
-                    if (kDeferredSilent_benchModes == benchMode) {
-                        static_cast<SkDeferredCanvas*>(canvas)->silentFlush();
-                    } else if (NULL != canvas) {
-                        canvas->flush();
-                    }
-
-                    if (NULL != canvas) {
-                        canvas->restore();
-                    }
-
-                    // stop the truncated timer after the last canvas call but
-                    // don't wait for all the GL calls to complete
-                    timer->truncatedEnd();
-#if SK_SUPPORT_GPU
-                    if (NULL != glContext) {
-                        context->flush();
-                        SK_GL(*glContext, Finish());
-                    }
-#endif
-                    // stop the inclusive and gpu timers once all the GL calls
-                    // have completed
-                    timer->end();
-
-                    SkAssertResult(timerData.appendTimes(timer));
-
-                }
-                if (repeatDraw > 1) {
-                    const char* timeFormat;
-                    if (TimerData::kPerIter_Result == timerResult) {
-                        timeFormat = perIterTimeformat.c_str();
-                    } else {
-                        timeFormat = normalTimeFormat.c_str();
-                    }
-                    uint32_t filteredTimerTypes = timerTypes;
-                    if (NULL == context) {
-                        filteredTimerTypes &= ~TimerData::kGpu_Flag;
-                    }
-                    SkString result = timerData.getResult(timeFormat,
-                                        timerResult,
-                                        configName,
-                                        filteredTimerTypes);
-                    logger.logProgress(result);
-                }
-                if (outDir.size() > 0 && kNonRendering_Backend != backend) {
-                    saveFile(bench->getName(), configName, outDir.c_str(),
-                             device->accessBitmap(false));
-                }
-            }
-        }
-        if (loggedBenchStart) {
-            logger.logProgress(SkString("\n"));
-        }
-    }
-#if SK_SUPPORT_GPU
-#if GR_CACHE_STATS
-    for (int i = 0; i <= GrContextFactory::kLastGLContextType; ++i) {
-        GrContextFactory::GLContextType ctxType = (GrContextFactory::GLContextType)i;
-        GrContext* context = gContextFactory.get(ctxType);
-        if (NULL != context) {
-            SkDebugf("Cache Stats for %s context:\n", GrContextFactory::GLContextTypeName(ctxType));
-            context->printCacheStats();
-            SkDebugf("\n");
-        }
-    }
-#endif
-    // Destroy the GrContext before the inst tracking printing at main() exit occurs.
+                    canvas->restore();
+                }
+
+
+                // Stop truncated timers before GL calls complete, and stop the full timers after.
+                timer->truncatedEnd();
+#if SK_SUPPORT_GPU
+                if (NULL != glContext) {
+                    context->flush();
+                    SK_GL(*glContext, Finish());
+                }
+#endif
+                timer->end();
+            } while (!kIsDebug && timer->fWall < FLAGS_benchMs);
+
+            if (kIsDebug) {
+                // Lets not mislead ourselves by looking at Debug build bench times!

[epoger, 2013/09/04 14:36:40]

I think it would be better to print a warning here, indicating why the bench
times are not reported.

[mtklein, 2013/09/04 19:06:51]

Added one up front.

+                continue;
+            }
+
+            // In same order as TimerTypes, multiplied to normalize times to ms per 1000 iterations.

[epoger, 2013/09/04 14:36:40]

Does something bad happen if the TimerTypes enum is reordered?  If so, is there
some way we can make sure they stay in sync?  (Or, figure out a way that it
doesn't matter.)

[mtklein, 2013/09/04 19:06:51]

now moot

+            const double normalize = 1000.0 / bench->getLoops();
+            const double times[] = {
+                normalize * timer->fWall,
+                normalize * timer->fTruncatedWall,
+                normalize * timer->fCpu,
+                normalize * timer->fTruncatedCpu,
+                normalize * timer->fGpu,
+            };
+
+            uint32_t filteredTimerTypes = timerTypes;
+            if (NULL == context) {
+                filteredTimerTypes &= ~(1<<kGpu_TimerType);

[epoger, 2013/09/04 14:36:40]

In my fantasy world, this becomes the easier-to-follow

filteredTimerTypeSet.remove(kGpu_TimerType)

[mtklein, 2013/09/04 19:06:51]

this turned out to be a complete no-op.  removed it

+
+            }
+
+            SkString result;
+            result.appendf("   %s:", config.name);
+            for (size_t i = 0; i < SK_ARRAY_COUNT(TimerType_Name); i++) {

[epoger, 2013/09/04 14:36:40]

In my fantasy world, this becomes:

  for (int typeInt = 0; typeInt <= kLast_TimerType; typeInt++) {
    TimerType thisType = static_cast<TimerType>(typeInt);
    if (filteredTimerTypeSet.includes(thisType)) {
      ...

Which is not necessarily more readable than what you have now.  Typecasting
provides some protection, but at a cost.

[mtklein, 2013/09/04 19:06:51]

Should be moot now.  Just out of curiosity, what cost are you referring to?

+                if (filteredTimerTypes & (1<<i) && times[i] > 0) {

[epoger, 2013/09/04 14:36:40]

Why don't we report this timer if times[i] == 0?

[mtklein, 2013/09/04 19:06:51]

I don't have a opinion on whether we want to print out 0 times.  They're mostly
uninsightful, like seeing 0 GPU time spent in the NULLGPU configuration... duh. 
bench at head does not print these zeroes.

The important part is to skip reporting timers which were never measured, which
come back from BenchTimer as negative values.

+                    result.appendf(" %s = ", TimerType_Name[i]);
+                    result.appendf(FLAGS_timeFormat[0], times[i]);
+                }
+            }
+            logger.logProgress(result);
+
+            if (FLAGS_outDir.count() && kNonRendering_Backend != config.backend) {
+                saveFile(bench->getName(),
+                         config.name,
+                         FLAGS_outDir[0],
+                         device->accessBitmap(false));
+            }
+        }
+        if (loggedBenchName) {
+            logger.logProgress("\n");
+        }
+    }
+    timers.deleteAll();
+#if SK_SUPPORT_GPU
     gContextFactory.destroyContexts();
 #endif
-    for (size_t i = 0; i < SK_ARRAY_COUNT(gConfigs); ++i) {
-        SkDELETE(timers[i]);
-    }
-
     return 0;
 }
 
 #if !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_NACL)
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif